Ensure metadata location when created from xml element with known location (#8599)

Fixes #8579

Context
MetadataElement of an ItemElement doesn't have line&col of Location set despite those are created from known xml.
This way errors referencing the Location from metadata would end up pointing to location (0, 0) in the file.

Reason
This happens only to XmlElementWithLocation (as opposed to XmlAttributeWithLocation) during creating XmlElementWithLocation from scratch (during parsing phase that doesn't have the reader with locations already available) from an existing XmlAttributeWithLocation (that has the proper location intitialized from Load phase). Due to the need to keep the constructors contract (we override existing XmlElement), we cannot pass the additional info in the constructor.

Changes Made
Making the Location property of XmlElementWithLocation writable - so that it can be properly rewritten if constructed from an existing node with available location info.

Testing
Hand testing plus applying the changfe to the other PR: #8581, solved the issue with (0, 0) warnings location

Note
This or #8581 PR (whichever is merged later) should remove the workaround https://github.com/dotnet/msbuild/pull/8581/files#diff-e289ba4ce7fa0e72cf63049cce595eafcad1e7b2034ccb3305cd0f06c2f822b8R196-R197

Author: JanKrivanek

src/Build.OM.UnitTests/Construction/ProjectItemElement_Tests.cs

@@ -82,6 +82,30 @@ public void ReadNoChildren(string project)
             Assert.Equal(0, Helpers.Count(item.Metadata));
         }
 
+        [Fact]
+        public void ReadMetadataLocationPreserved()
+        {
+            string project = """
+                <Project>
+                    <Target Name='t'>
+                        <ItemGroup>
+                            <i Include='i' MetadataA='123' MetadataB='xyz' />
+                        </ItemGroup>
+                    </Target>
+                </Project>
+                """;
+
+            ProjectItemElement item = GetItemFromContent(project);
+            Assert.Equal(2, item.Metadata.Count);
+            ProjectMetadataElement metadatum1 = item.Metadata.First();
+            ProjectMetadataElement metadatum2 = item.Metadata.Skip(1).First();
+
+            Assert.Equal(4, metadatum1.Location.Line);
+            Assert.Equal(4, metadatum2.Location.Line);
+            Assert.Equal(27, metadatum1.Location.Column);
+            Assert.Equal(43, metadatum2.Location.Column);
+        }
+
         /// <summary>
         /// Read item with no include
         /// </summary>

src/Build.UnitTests/BackEnd/MSBuild_Tests.cs

@@ -4,7 +4,6 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
-
 using Microsoft.Build.Evaluation;
 using Microsoft.Build.Execution;
 using Microsoft.Build.Framework;
@@ -824,7 +823,8 @@ public void ItemsRecursionWithinTarget()
                       </Target>
                     </Project>
                 """;
-            var projectFile = env.CreateFile("test.proj", ObjectModelHelpers.CleanupFileContents(projectContent));
+            string projFileName = "test.proj";
+            var projectFile = env.CreateFile(projFileName, ObjectModelHelpers.CleanupFileContents(projectContent));
 
             MockLogger logger = new MockLogger(_testOutput);
             ObjectModelHelpers.BuildTempProjectFileExpectSuccess(projectFile.Path, logger);
@@ -839,6 +839,10 @@ public void ItemsRecursionWithinTarget()
             logger.AssertLogContains(string.Format(ResourceUtilities.GetResourceString("ItemReferencingSelfInTarget"), "iin1", "Filename"));
             logger.AssertLogContains(string.Format(ResourceUtilities.GetResourceString("ItemReferencingSelfInTarget"), "iin1", "Extension"));
             logger.AssertMessageCount("MSB4120", 6);
+            // The location of the offending attribute (TargetPath) is transferred - for both metadatums (%(Filename) and %(Extension)) on correct locations in xml
+            logger.AssertMessageCount($"{projFileName}(4,34):", 2, false);
+            logger.AssertMessageCount($"{projFileName}(5,34):", 2, false);
+            logger.AssertMessageCount($"{projFileName}(6,34):", 2, false);
             Assert.Equal(0, logger.WarningCount);
             Assert.Equal(0, logger.ErrorCount);
         }

src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs

@@ -185,19 +185,16 @@ private void ExecuteAdd(ProjectItemGroupTaskItemInstance child, ItemBucket bucke
                 if (condition)
                 {
                     ExpanderOptions expanderOptions = ExpanderOptions.ExpandAll;
-                    ElementLocation location = metadataInstance.Location;
                     if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_6) &&
                         // If multiple buckets were expanded - we do not want to repeat same error for same metadatum on a same line
                         bucket.BucketSequenceNumber == 0 &&
                         // Referring to unqualified metadata of other item (transform) is fine.
                         child.Include.IndexOf("@(", StringComparison.Ordinal) == -1)
                     {
                         expanderOptions |= ExpanderOptions.LogOnItemMetadataSelfReference;
-                        // Temporary workaround of unavailability of full Location info on metadata: https://github.com/dotnet/msbuild/issues/8579
-                        location = child.Location;
                     }
 
-                    string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(metadataInstance.Value, expanderOptions, location, loggingContext);
+                    string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(metadataInstance.Value, expanderOptions, metadataInstance.Location, loggingContext);
 
                     // This both stores the metadata so we can add it to all the items we just created later, and 
                     // exposes this metadata to further metadata evaluations in subsequent loop iterations.

src/Build/Construction/ProjectMetadataElement.cs

@@ -100,13 +100,13 @@ public string Value
         /// Creates an unparented ProjectMetadataElement, wrapping an unparented XmlElement.
         /// Caller should then ensure the element is added to a parent.
         /// </summary>
-        internal static ProjectMetadataElement CreateDisconnected(string name, ProjectRootElement containingProject)
+        internal static ProjectMetadataElement CreateDisconnected(string name, ProjectRootElement containingProject, ElementLocation location = null)
         {
             XmlUtilities.VerifyThrowArgumentValidElementName(name);
             ErrorUtilities.VerifyThrowArgument(!FileUtilities.ItemSpecModifiers.IsItemSpecModifier(name), "ItemSpecModifierCannotBeCustomMetadata", name);
             ErrorUtilities.VerifyThrowInvalidOperation(!XMakeElements.ReservedItemNames.Contains(name), "CannotModifyReservedItemMetadata", name);
 
-            XmlElementWithLocation element = containingProject.CreateElement(name);
+            XmlElementWithLocation element = containingProject.CreateElement(name, location);
 
             return new ProjectMetadataElement(element, containingProject);
         }

src/Build/Construction/ProjectRootElement.cs

@@ -1327,13 +1327,22 @@ public ProjectMetadataElement CreateMetadataElement(string name)
         /// Caller must add it to the location of choice in the project.
         /// </summary>
         public ProjectMetadataElement CreateMetadataElement(string name, string unevaluatedValue)
+        {
+            return this.CreateMetadataElement(name, unevaluatedValue, null);
+        }
+
+        /// <summary>
+        /// Creates a metadata node.
+        /// Caller must add it to the location of choice in the project.
+        /// </summary>
+        public ProjectMetadataElement CreateMetadataElement(string name, string unevaluatedValue, ElementLocation location)
         {
             if (Link != null)
             {
                 return RootLink.CreateMetadataElement(name, unevaluatedValue);
             }
 
-            ProjectMetadataElement metadatum = ProjectMetadataElement.CreateDisconnected(name, this);
+            ProjectMetadataElement metadatum = ProjectMetadataElement.CreateDisconnected(name, this, location);
 
             metadatum.Value = unevaluatedValue;
 
@@ -1785,14 +1794,23 @@ internal static ProjectRootElement OpenProjectOrSolution(string fullPath, IDicti
             return projectRootElement;
         }
 
+        /// <summary>
+        /// Creates a metadata node.
+        /// Caller must add it to the location of choice in the project.
+        /// </summary>
+        internal ProjectMetadataElement CreateMetadataElement(XmlAttributeWithLocation attribute)
+        {
+            return CreateMetadataElement(attribute.Name, attribute.Value, attribute.Location);
+        }
+
         /// <summary>
         /// Creates a XmlElement with the specified name in the document
         /// containing this project.
         /// </summary>
-        internal XmlElementWithLocation CreateElement(string name)
+        internal XmlElementWithLocation CreateElement(string name, ElementLocation location = null)
         {
             ErrorUtilities.VerifyThrow(Link == null, "External project");
-            return (XmlElementWithLocation)XmlDocument.CreateElement(name, XmlNamespace);
+            return (XmlElementWithLocation)XmlDocument.CreateElement(name, XmlNamespace, location);
         }
 
         /// <summary>

src/Build/ElementLocation/XmlDocumentWithLocation.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.IO;
+using System.Threading;
 using System.Xml;
 using Microsoft.Build.Internal;
 using Microsoft.Build.Shared;
@@ -59,6 +60,13 @@ internal class XmlDocumentWithLocation : XmlDocument
         /// </summary>
         private bool? _loadAsReadOnly;
 
+        /// <summary>
+        /// Location of the element to be created via 'CreateElement' call. So that we can
+        ///  receive and use location from the caller up the stack even if we are being called via
+        /// <see cref="XmlDocument"/> internal methods.
+        /// </summary>
+        private readonly AsyncLocal<ElementLocation> _elementLocation = new AsyncLocal<ElementLocation>();
+
         /// <summary>
         /// Constructor
         /// </summary>
@@ -180,6 +188,31 @@ public override void Load(string fullPath)
             }
         }
 
+        /// <summary>
+        /// Called during parse, to add an element.
+        /// </summary>
+        /// <remarks>
+        /// We create our own kind of element, that we can give location information to.
+        /// In order to pass the location through the callchain, that contains XmlDocument function
+        ///  that then calls back to our XmlDocumentWithLocation (so we cannot use call stack via passing via parameters),
+        ///  we use async local field, that simulates variable on call stack.
+        /// </remarks>
+        internal XmlElement CreateElement(string localName, string namespaceURI, ElementLocation location)
+        {
+            if (location != null)
+            {
+                this._elementLocation.Value = location;
+            }
+            try
+            {
+                return CreateElement(localName, namespaceURI);
+            }
+            finally
+            {
+                this._elementLocation.Value = null;
+            }
+        }
+
         /// <summary>
         /// Called during load, to add an element.
         /// </summary>
@@ -192,6 +225,10 @@ public override XmlElement CreateElement(string prefix, string localName, string
             {
                 return new XmlElementWithLocation(prefix, localName, namespaceURI, this, _reader.LineNumber, _reader.LinePosition);
             }
+            else if (_elementLocation?.Value != null)
+            {
+                return new XmlElementWithLocation(prefix, localName, namespaceURI, this, _elementLocation.Value.Line, _elementLocation.Value.Column);
+            }
 
             // Must be a subsequent edit; we can't provide location information
             return new XmlElementWithLocation(prefix, localName, namespaceURI, this);

src/Build/Evaluation/ProjectParser.cs

@@ -324,7 +324,7 @@ private ProjectItemElement ParseProjectItemElement(XmlElementWithLocation elemen
                 }
                 else if (isValidMetadataNameInAttribute)
                 {
-                    ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute.Name, attribute.Value);
+                    ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute);
                     metadatum.ExpressedAsAttribute = true;
                     metadatum.Parent = item;
 
@@ -744,7 +744,7 @@ private ProjectItemDefinitionElement ParseProjectItemDefinitionXml(XmlElementWit
                 }
                 else if (isValidMetadataNameInAttribute)
                 {
-                    ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute.Name, attribute.Value);
+                    ProjectMetadataElement metadatum = _project.CreateMetadataElement(attribute);
                     metadatum.ExpressedAsAttribute = true;
                     metadatum.Parent = itemDefinition;
 

src/Shared/UnitTests/MockLogger.cs

@@ -282,8 +282,13 @@ internal void LoggerEventHandler(object sender, BuildEventArgs eventArgs)
                             bool logMessage = !(eventArgs is BuildFinishedEventArgs) || LogBuildFinished;
                             if (logMessage)
                             {
-                                _fullLog.AppendLine(eventArgs.Message);
-                                _testOutputHelper?.WriteLine(eventArgs.Message);
+                                string msg = eventArgs.Message;
+                                if (eventArgs is BuildMessageEventArgs m && m.LineNumber != 0)
+                                {
+                                    msg = $"{m.File}({m.LineNumber},{m.ColumnNumber}): {msg}";
+                                }
+                                _fullLog.AppendLine(msg);
+                                _testOutputHelper?.WriteLine(msg);
                             }
                             break;
                         }
@@ -496,9 +501,9 @@ internal void AssertLogDoesntContain(string contains)
         /// </summary>
         internal void AssertNoWarnings() => Assert.Equal(0, WarningCount);
 
-        internal void AssertMessageCount(string message, int expectedCount)
+        internal void AssertMessageCount(string message, int expectedCount, bool regexSearch = true)
         {
-            var matches = Regex.Matches(FullLog, message);
+            var matches = Regex.Matches(FullLog, regexSearch ? message : Regex.Escape(message));
             matches.Count.ShouldBe(expectedCount);
         }
     }

src/Shared/UnitTests/ObjectModelHelpers.cs

@@ -1155,6 +1155,11 @@ internal static string GetOSPlatformAsString()
         /// </summary>
         internal static int Count(IEnumerable enumerable)
         {
+            if (enumerable is ICollection c)
+            {
+                return c.Count;
+            }
+
             int i = 0;
             foreach (object _ in enumerable)
             {

src/Shared/XmlUtilities.cs

@@ -32,9 +32,8 @@ internal static XmlElementWithLocation RenameXmlElement(XmlElementWithLocation o
                 return oldElement;
             }
 
-            XmlElementWithLocation newElement = (xmlNamespace == null)
-                ? (XmlElementWithLocation)oldElement.OwnerDocument.CreateElement(newElementName)
-                : (XmlElementWithLocation)oldElement.OwnerDocument.CreateElement(newElementName, xmlNamespace);
+            XmlElementWithLocation newElement =
+                (XmlElementWithLocation)((XmlDocumentWithLocation)oldElement.OwnerDocument).CreateElement(newElementName, xmlNamespace ?? string.Empty, oldElement.Location);
 
             // Copy over all the attributes.
             foreach (XmlAttribute oldAttribute in oldElement.Attributes)