Ensure metadata location when created from xml element with known location

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)