Log item self-expansion (#8581)

Fixes #8527

Context
Self referencing item metadata in an item definition within the target leads to possible unintended expansion (and cross-applying of pre-existing item instances).

This behavior is not a bug - it's the implication of the target batching feature - but might be confusing. So detection and warning is added here

Changes Made
A self reference (qualified or unqualified) of metadata within the item defeiniton (which is within a target) is detected and high importance message is issued.

Testing
Tests added for a warning case and for a non-warning case (self-referencing metadata outside of target context)

Doc
MicrosoftDocs/visualstudio-docs-pr#11034

Possible impact
It's unfortunately hard to obtain data on prevalence of this pattern due to limitations of available code searches (github search doesn't support '@' sign and no escaping option; SourceGraph, grep.app doesn't support regex lookaheads). So I cannot quantify (I'll try again later on).
But there are some sparse evidence of usage:

https://github.com/JeremyAnsel/JeremyAnsel.HLSL.Targets/blob/master/JeremyAnsel.HLSL.Targets/JeremyAnsel.HLSL.Targets/JeremyAnsel.HLSL.Targets.csproj#L49
https://github.com/neuecc/MessagePack-CSharp/blob/master/src/MessagePack.MSBuild.Tasks/MessagePack.MSBuild.Tasks.csproj#L35
https://github.com/dotnet/aspnetcore/blob/main/eng/AfterSigning.targets#L22
all of those seem to be doing something else then intended (the variable part of path is empty, so it gets just the base directory), but they do not break.

tl;dr;: unless we are able to better quantify, we might resort to demote the warning to a message Demoted to Message severity

Author: JanKrivanek

documentation/wiki/ChangeWaves.md

@@ -29,6 +29,7 @@ A wave of features is set to "rotate out" (i.e. become standard functionality) t
 - [Log an error when no provided search path for an import exists](https://github.com/dotnet/msbuild/pull/8095)
 - [Log assembly loads](https://github.com/dotnet/msbuild/pull/8316)
 - [AnyHaveMetadataValue returns false when passed an empty list](https://github.com/dotnet/msbuild/pull/8603)
+- [Log item self-expansion](https://github.com/dotnet/msbuild/pull/8581)
 
 ### 17.4
 - [Respect deps.json when loading assemblies](https://github.com/dotnet/msbuild/pull/7520)

src/Build.UnitTests/BackEnd/MSBuild_Tests.cs

@@ -772,6 +772,116 @@ public void ItemsIncludeExcludePathsCombinations()
             }
         }
 
+        /// <summary>
+        /// Referring to an item outside of target leads to 'naturally expected' reference to the item being processed.
+        ///  No expansion occurs.
+        /// </summary>
+        [Fact]
+        public void ItemsRecursionOutsideTarget()
+        {
+            using TestEnvironment env = TestEnvironment.Create();
+            string projectContent = """
+                    <Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'>
+                     <ItemGroup>
+                        <iout1 Include='a/b.foo' TargetPath='%(Filename)%(Extension)' />
+                        <iout1 Include='c/d.foo' TargetPath='%(Filename)%(Extension)' />
+                        <iout1 Include='g/h.foo' TargetPath='%(Filename)%(Extension)' />
+                      </ItemGroup>
+                      <Target Name='a'>
+                        <Message Text="iout1=[@(iout1)]" Importance='High' />
+                        <Message Text="iout1-target-paths=[@(iout1->'%(TargetPath)')]" Importance='High' />
+                      </Target>
+                    </Project>
+                """;
+            var projectFile = env.CreateFile("test.proj", ObjectModelHelpers.CleanupFileContents(projectContent));
+
+            MockLogger logger = new MockLogger(_testOutput);
+            ObjectModelHelpers.BuildTempProjectFileExpectSuccess(projectFile.Path, logger);
+
+            _testOutput.WriteLine(logger.FullLog);
+
+            logger.AssertLogContains("iout1=[a/b.foo;c/d.foo;g/h.foo]");
+            logger.AssertLogContains("iout1-target-paths=[b.foo;d.foo;h.foo]");
+        }
+
+        /// <summary>
+        /// Referring to an item within target leads to item expansion which might be unintended behavior - hence warning.
+        /// </summary>
+        [Fact]
+        public void ItemsRecursionWithinTarget()
+        {
+            using TestEnvironment env = TestEnvironment.Create();
+            string projectContent = """
+                    <Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'>
+                      <Target Name='a'>
+                        <ItemGroup>
+                          <iin1 Include='a/b.foo' TargetPath='%(Filename)%(Extension)' />
+                          <iin1 Include='c/d.foo' TargetPath='%(Filename)%(Extension)' />
+                          <iin1 Include='g/h.foo' TargetPath='%(Filename)%(Extension)' />
+                        </ItemGroup>
+                        <Message Text="iin1=[@(iin1)]" Importance='High' />
+                        <Message Text="iin1-target-paths=[@(iin1->'%(TargetPath)')]" Importance='High' />
+                      </Target>
+                    </Project>
+                """;
+            var projectFile = env.CreateFile("test.proj", ObjectModelHelpers.CleanupFileContents(projectContent));
+
+            MockLogger logger = new MockLogger(_testOutput);
+            ObjectModelHelpers.BuildTempProjectFileExpectSuccess(projectFile.Path, logger);
+
+            _testOutput.WriteLine(logger.FullLog);
+
+            logger.AssertLogDoesntContain("iin1=[a/b.foo;c/d.foo;g/h.foo]");
+            logger.AssertLogDoesntContain("iin1-target-paths=[b.foo;d.foo;h.foo]");
+            logger.AssertLogContains("iin1=[a/b.foo;c/d.foo;g/h.foo;g/h.foo]");
+            logger.AssertLogContains("iin1-target-paths=[;b.foo;b.foo;d.foo]");
+
+            logger.AssertLogContains(string.Format(ResourceUtilities.GetResourceString("ItemReferencingSelfInTarget"), "iin1", "Filename"));
+            logger.AssertLogContains(string.Format(ResourceUtilities.GetResourceString("ItemReferencingSelfInTarget"), "iin1", "Extension"));
+            logger.AssertMessageCount("MSB4120", 6);
+            Assert.Equal(0, logger.WarningCount);
+            Assert.Equal(0, logger.ErrorCount);
+        }
+
+        /// <summary>
+        /// Referring to an unrelated item within target leads to expected expansion.
+        /// </summary>
+        [Fact]
+        public void UnrelatedItemsRecursionWithinTarget()
+        {
+            using TestEnvironment env = TestEnvironment.Create();
+            string projectContent = """
+                    <Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'>
+                      <ItemGroup>
+                        <iout1 Include='a/b.foo'/>
+                        <iout1 Include='c/d.foo'/>
+                        <iout1 Include='g/h.foo'/>
+                      </ItemGroup>
+
+                      <Target Name='a'>
+                        <ItemGroup>
+                          <iin1 Include='@(iout1)' TargetPath='%(Filename)%(Extension)' />
+                        </ItemGroup>
+                        <Message Text="iin1=[@(iin1)]" Importance='High' />
+                        <Message Text="iin1-target-paths=[@(iin1->'%(TargetPath)')]" Importance='High' />
+                      </Target>
+                    </Project>
+                """;
+            var projectFile = env.CreateFile("test.proj", ObjectModelHelpers.CleanupFileContents(projectContent));
+
+            MockLogger logger = new MockLogger(_testOutput);
+            ObjectModelHelpers.BuildTempProjectFileExpectSuccess(projectFile.Path, logger);
+
+            _testOutput.WriteLine(logger.FullLog);
+
+            logger.AssertLogContains("iin1=[a/b.foo;c/d.foo;g/h.foo]");
+            logger.AssertLogContains("iin1-target-paths=[b.foo;d.foo;h.foo]");
+
+            logger.AssertLogDoesntContain("MSB4120");
+            Assert.Equal(0, logger.WarningCount);
+            Assert.Equal(0, logger.ErrorCount);
+        }
+
         /// <summary>
         /// Check if passing different global properties via metadata works
         /// </summary>

src/Build/BackEnd/Components/Logging/LoggingContext.cs

@@ -128,6 +128,36 @@ internal void LogComment(MessageImportance importance, string messageResourceNam
             _loggingService.LogComment(_eventContext, importance, messageResourceName, messageArgs);
         }
 
+        /// <summary>
+        ///  Helper method to create a message build event from a string resource and some parameters
+        /// </summary>
+        /// <param name="importance">Importance level of the message</param>
+        /// <param name="file">The file in which the event occurred</param>
+        /// <param name="messageResourceName">string within the resource which indicates the format string to use</param>
+        /// <param name="messageArgs">string resource arguments</param>
+        internal void LogComment(MessageImportance importance, BuildEventFileInfo file, string messageResourceName, params object[] messageArgs)
+        {
+            ErrorUtilities.VerifyThrow(_isValid, "must be valid");
+
+            _loggingService.LogBuildEvent(new BuildMessageEventArgs(
+                null,
+                null,
+                file.File,
+                file.Line,
+                file.Column,
+                file.EndLine,
+                file.EndColumn,
+                ResourceUtilities.GetResourceString(messageResourceName),
+                helpKeyword: null,
+                senderName: "MSBuild",
+                importance,
+                DateTime.UtcNow,
+                messageArgs)
+            {
+                BuildEventContext = _eventContext
+            });
+        }
+
         /// <summary>
         /// Helper method to create a message build event from a string
         /// </summary>

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

@@ -184,7 +184,20 @@ private void ExecuteAdd(ProjectItemGroupTaskItemInstance child, ItemBucket bucke
 
                 if (condition)
                 {
-                    string evaluatedValue = bucket.Expander.ExpandIntoStringLeaveEscaped(metadataInstance.Value, ExpanderOptions.ExpandAll, metadataInstance.Location, loggingContext);
+                    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);
 
                     // 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.
@@ -612,7 +625,7 @@ private List<ProjectItemInstance> FindItemsMatchingMetadataSpecification(
         /// 1. The metadata table created for the bucket, may be null.
         /// 2. The metadata table derived from the item definition group, may be null.
         /// </summary>
-        private class NestedMetadataTable : IMetadataTable
+        private class NestedMetadataTable : IMetadataTable, IItemTypeDefinition
         {
             /// <summary>
             /// The table for all metadata added during expansion
@@ -722,6 +735,8 @@ internal void SetValue(string name, string value)
             {
                 _addTable[name] = value;
             }
+
+            string IItemTypeDefinition.ItemType => _itemType;
         }
     }
 }

src/Build/Definition/ProjectItemDefinition.cs

@@ -26,7 +26,7 @@ namespace Microsoft.Build.Evaluation
     /// ProjectMetadataElement, and these can be added, removed, and modified.
     /// </remarks>
     [DebuggerDisplay("{_itemType} #Metadata={MetadataCount}")]
-    public class ProjectItemDefinition : IKeyed, IMetadataTable, IItemDefinition<ProjectMetadata>, IProjectMetadataParent
+    public class ProjectItemDefinition : IKeyed, IMetadataTable, IItemDefinition<ProjectMetadata>, IProjectMetadataParent, IItemTypeDefinition
     {
         /// <summary>
         /// Project that this item definition lives in.

src/Build/Evaluation/Expander.cs

@@ -88,6 +88,13 @@ internal enum ExpanderOptions
         /// </summary>
         Truncate = 0x40,
 
+        /// <summary>
+        /// Issues build message if item references unqualified or qualified metadata odf self - as this can lead to unintended expansion and
+        ///  cross-combination of other items.
+        /// More info: https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-batching#item-batching-on-self-referencing-metadata
+        /// </summary>
+        LogOnItemMetadataSelfReference = 0x80,
+
         /// <summary>
         /// Expand only properties and then item lists
         /// </summary>
@@ -441,7 +448,7 @@ internal string ExpandIntoStringLeaveEscaped(string expression, ExpanderOptions
 
             ErrorUtilities.VerifyThrowInternalNull(elementLocation, nameof(elementLocation));
 
-            string result = MetadataExpander.ExpandMetadataLeaveEscaped(expression, _metadata, options, elementLocation);
+            string result = MetadataExpander.ExpandMetadataLeaveEscaped(expression, _metadata, options, elementLocation, loggingContext);
             result = PropertyExpander<P>.ExpandPropertiesLeaveEscaped(result, _properties, options, elementLocation, _usedUninitializedProperties, _fileSystem, loggingContext);
             result = ItemExpander.ExpandItemVectorsIntoString<I>(this, result, _items, options, elementLocation);
             result = FileUtilities.MaybeAdjustFilePath(result);
@@ -871,8 +878,9 @@ private static class MetadataExpander
             /// <param name="metadata">The metadata to be expanded.</param>
             /// <param name="options">Used to specify what to expand.</param>
             /// <param name="elementLocation">The location information for error reporting purposes.</param>
+            /// <param name="loggingContext">The logging context for this operation.</param>
             /// <returns>The string with item metadata expanded in-place, escaped.</returns>
-            internal static string ExpandMetadataLeaveEscaped(string expression, IMetadataTable metadata, ExpanderOptions options, IElementLocation elementLocation)
+            internal static string ExpandMetadataLeaveEscaped(string expression, IMetadataTable metadata, ExpanderOptions options, IElementLocation elementLocation, LoggingContext loggingContext = null)
             {
                 try
                 {
@@ -896,7 +904,7 @@ internal static string ExpandMetadataLeaveEscaped(string expression, IMetadataTa
                     {
                         // if there are no item vectors in the string
                         // run a simpler Regex to find item metadata references
-                        MetadataMatchEvaluator matchEvaluator = new MetadataMatchEvaluator(metadata, options);
+                        MetadataMatchEvaluator matchEvaluator = new MetadataMatchEvaluator(metadata, options, elementLocation, loggingContext);
                         result = RegularExpressions.ItemMetadataPattern.Value.Replace(expression, new MatchEvaluator(matchEvaluator.ExpandSingleMetadata));
                     }
                     else
@@ -915,7 +923,7 @@ internal static string ExpandMetadataLeaveEscaped(string expression, IMetadataTa
                         using SpanBasedStringBuilder finalResultBuilder = Strings.GetSpanBasedStringBuilder();
 
                         int start = 0;
-                        MetadataMatchEvaluator matchEvaluator = new MetadataMatchEvaluator(metadata, options);
+                        MetadataMatchEvaluator matchEvaluator = new MetadataMatchEvaluator(metadata, options, elementLocation, loggingContext);
 
                         if (itemVectorExpressions != null)
                         {
@@ -993,13 +1001,23 @@ private class MetadataMatchEvaluator
                 /// </summary>
                 private ExpanderOptions _options;
 
+                private IElementLocation _elementLocation;
+
+                private LoggingContext _loggingContext;
+
                 /// <summary>
                 /// Constructor taking a source of metadata.
                 /// </summary>
-                internal MetadataMatchEvaluator(IMetadataTable metadata, ExpanderOptions options)
+                internal MetadataMatchEvaluator(
+                    IMetadataTable metadata,
+                    ExpanderOptions options,
+                    IElementLocation elementLocation,
+                    LoggingContext loggingContext)
                 {
                     _metadata = metadata;
-                    _options = options & (ExpanderOptions.ExpandMetadata | ExpanderOptions.Truncate);
+                    _options = options & (ExpanderOptions.ExpandMetadata | ExpanderOptions.Truncate | ExpanderOptions.LogOnItemMetadataSelfReference);
+                    _elementLocation = elementLocation;
+                    _loggingContext = loggingContext;
 
                     ErrorUtilities.VerifyThrow(options != ExpanderOptions.Invalid, "Must be expanding metadata of some kind");
                 }
@@ -1030,6 +1048,17 @@ internal string ExpandSingleMetadata(Match itemMetadataMatch)
                        (!isBuiltInMetadata && ((_options & ExpanderOptions.ExpandCustomMetadata) != 0)))
                     {
                         metadataValue = _metadata.GetEscapedValue(itemType, metadataName);
+
+                        if ((_options & ExpanderOptions.LogOnItemMetadataSelfReference) != 0 &&
+                            _loggingContext != null &&
+                            !string.IsNullOrEmpty(metadataName) &&
+                            _metadata is IItemTypeDefinition itemMetadata &&
+                            (string.IsNullOrEmpty(itemType) || string.Equals(itemType, itemMetadata.ItemType, StringComparison.Ordinal)))
+                        {
+                            _loggingContext.LogComment(MessageImportance.High, new BuildEventFileInfo(_elementLocation),
+                                "ItemReferencingSelfInTarget", itemMetadata.ItemType, metadataName);
+                        }
+
                         if (IsTruncationEnabled(_options) && metadataValue.Length > CharacterLimitPerExpansion)
                         {
                             metadataValue = metadataValue.Substring(0, CharacterLimitPerExpansion - 3) + "...";

src/Build/Evaluation/IItemTypeDefinition.cs

@@ -0,0 +1,12 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.Build.Evaluation;
+
+internal interface IItemTypeDefinition
+{
+    /// <summary>
+    /// The item type to which this metadata applies.
+    /// </summary>
+    string ItemType { get; }
+}

src/Build/Instance/ProjectItemDefinitionInstance.cs

@@ -20,7 +20,7 @@ namespace Microsoft.Build.Execution
     /// Immutable.
     /// </summary>
     [DebuggerDisplay("{_itemType} #Metadata={MetadataCount}")]
-    public class ProjectItemDefinitionInstance : IKeyed, IMetadataTable, IItemDefinition<ProjectMetadataInstance>, ITranslatable
+    public class ProjectItemDefinitionInstance : IKeyed, IMetadataTable, IItemDefinition<ProjectMetadataInstance>, ITranslatable, IItemTypeDefinition
     {
         /// <summary>
         /// Item type, for example "Compile", that this item definition applies to
@@ -235,5 +235,7 @@ internal static ProjectItemDefinitionInstance FactoryForDeserialization(ITransla
 
             return instance;
         }
+
+        string IItemTypeDefinition.ItemType => _itemType;
     }
 }

src/Build/Instance/ProjectItemInstance.cs

@@ -33,7 +33,8 @@ public class ProjectItemInstance :
         ITaskItem2,
         IMetadataTable,
         ITranslatable,
-        IMetadataContainer
+        IMetadataContainer,
+        IItemTypeDefinition
     {
         /// <summary>
         /// The project instance to which this item belongs.
@@ -2137,7 +2138,7 @@ public void SetMetadata(IEnumerable<Pair<ProjectMetadataElement, string>> metada
             /// Also, more importantly, because typically the same regular metadata values can be shared by many items,
             /// and keeping item-specific metadata out of it could allow it to be implemented as a copy-on-write table.
             /// </summary>
-            private class BuiltInMetadataTable : IMetadataTable
+            private class BuiltInMetadataTable : IMetadataTable, IItemTypeDefinition
             {
                 /// <summary>
                 /// Item type
@@ -2195,6 +2196,8 @@ public string GetEscapedValueIfPresent(string requiredItemType, string name)
 
                     return value;
                 }
+
+                string IItemTypeDefinition.ItemType => _itemType;
             }
         }
 

src/Build/Microsoft.Build.csproj

@@ -160,6 +160,7 @@
     <Compile Include="BackEnd\Components\SdkResolution\SdkResolverException.cs" />
     <Compile Include="BackEnd\Components\SdkResolution\TranslationHelpers.cs" />
     <Compile Include="FileSystem\*.cs" />
+    <Compile Include="Evaluation\IItemTypeDefinition.cs" />
     <Compile Include="Utilities\ReaderWriterLockSlimExtensions.cs" />
     <Compile Include="BackEnd\Node\ConsoleOutput.cs" />
     <Compile Include="BackEnd\Node\PartialBuildTelemetry.cs" />