Allow Remove item operation to match on specified metadata instead of just item identity

src/Build.OM.UnitTests/Definition/ProjectItem_Tests.cs

@@ -2173,6 +2173,49 @@ public void RemoveWithItemReferenceOnMatchingMetadata()
             items.ElementAt(2).GetMetadataValue("M2").ShouldBe("d");
         }
 
+        [Fact]
+        public void RemoveWithItemReferenceOnMatchingMetadataReferences()
+        {
+            string content = 
+                @"<Project>
+                    <PropertyGroup>
+                        <p0>v0</p0>
+                    </PropertyGroup>
+                    <ItemGroup>
+                        <Meta2 Include='M2'/>
+                        <Meta2 Include='g'/>
+
+                        <I1 Include='a1' v0='1' M2='a'/>
+                        <I1 Include='b1' v0='2' M2='x'/>
+                        <I1 Include='c1' v0='3' M2='y'/>
+                        <I1 Include='d1' v0='4' M2='b'/>
+
+                        <I2 Include='a2' v0='x' m2='c'/>
+                        <I2 Include='b2' v0='2' m2='x'/>
+                        <I2 Include='c2' v0='3' m2='Y'/>
+                        <I2 Include='d2' v0='y' m2='d'/>
+
+                        <I2 Remove='@(I1)' MatchOnMetadata='$(Meta1);@(Meta2)' />
+                    </ItemGroup>
+                </Project>";
+
+            using (var env = TestEnvironment.Create())
+            {
+                var project = ObjectModelHelpers.CreateInMemoryProject(env.CreateProjectCollection().Collection, content, null, null);
+
+                var items = project.ItemsIgnoringCondition.Where(i => i.ItemType.Equals("I2"));
+
+                items.Select(i => i.EvaluatedInclude).ShouldBe(new[] { "a2", "c2", "d2" });
+
+                items.ElementAt(0).GetMetadataValue("v0").ShouldBe("x");
+                items.ElementAt(0).GetMetadataValue("M2").ShouldBe("c");
+                items.ElementAt(1).GetMetadataValue("v0").ShouldBe("3");
+                items.ElementAt(1).GetMetadataValue("M2").ShouldBe("Y");
+                items.ElementAt(2).GetMetadataValue("v0").ShouldBe("y");
+                items.ElementAt(2).GetMetadataValue("M2").ShouldBe("d");
+            }
+        }
+
         [Fact]
         public void KeepWithItemReferenceOnNonmatchingMetadata()
         {
@@ -2225,7 +2268,7 @@ public void FailWithMultipleItemReferenceOnMatchingMetadata()
 
                 <I3 Remove='@(I1);@(I2)' MatchOnMetadata='M1;m2' />");
 
-            Assert.ThrowsAny<Exception>(() => ObjectModelHelpers.CreateInMemoryProject(content));
+            Assert.ThrowsAny<InvalidProjectFileException>(() => ObjectModelHelpers.CreateInMemoryProject(content));
         }
 
         [Fact]
@@ -2244,7 +2287,7 @@ public void FailWithMetadataItemReferenceOnMatchingMetadata()
 
                 <I2 Remove='%(I1.M1)' MatchOnMetadata='M1;m2' />");
 
-            Assert.ThrowsAny<Exception>(() => ObjectModelHelpers.CreateInMemoryProject(content));
+            Assert.ThrowsAny<InvalidProjectFileException>(() => ObjectModelHelpers.CreateInMemoryProject(content));
         }
 
         [Fact]

src/Build.UnitTests/BackEnd/IntrinsicTask_Tests.cs

@@ -1749,6 +1749,50 @@ public void RemoveWithItemReferenceOnMatchingMetadata()
             items.ElementAt(2).GetMetadataValue("M2").ShouldBe("d");
         }
 
+        [Fact]
+        public void RemoveWithItemReferenceOnMatchingMetadataReferences()
+        {
+            // <PropertyGroup>
+            //     <p0>v0</p0>
+            // </PropertyGroup>
+            string content = ObjectModelHelpers.CleanupFileContents(
+                @"<Project ToolsVersion='msbuilddefaulttoolsversion' xmlns='msbuildnamespace'>
+                    <Target Name='t'>
+                        <ItemGroup>
+                            <Meta2 Include='M2'/>
+                            <Meta2 Include='g'/>
+
+                            <I1 Include='a1' v0='1' M2='a'/>
+                            <I1 Include='b1' v0='2' M2='x'/>
+                            <I1 Include='c1' v0='3' M2='y'/>
+                            <I1 Include='d1' v0='4' M2='b'/>
+
+                            <I2 Include='a2' v0='x' m2='c'/>
+                            <I2 Include='b2' v0='2' m2='x'/>
+                            <I2 Include='c2' v0='3' m2='Y'/>
+                            <I2 Include='d2' v0='y' m2='d'/>
+
+                            <I2 Remove='@(I1)' MatchOnMetadata='$(p0);@(Meta2)' />
+                        </ItemGroup>
+                    </Target></Project>");
+
+            IntrinsicTask task = CreateIntrinsicTask(content);
+            PropertyDictionary<ProjectPropertyInstance> properties = GeneratePropertyGroup();
+            Lookup lookup = LookupHelpers.CreateLookup(properties);
+            ExecuteTask(task, lookup);
+
+            var items = lookup.GetItems("I2");
+
+            items.Select(i => i.EvaluatedInclude).ShouldBe(new[] { "a2", "c2", "d2" });
+
+            items.ElementAt(0).GetMetadataValue("v0").ShouldBe("x");
+            items.ElementAt(0).GetMetadataValue("M2").ShouldBe("c");
+            items.ElementAt(1).GetMetadataValue("v0").ShouldBe("3");
+            items.ElementAt(1).GetMetadataValue("M2").ShouldBe("Y");
+            items.ElementAt(2).GetMetadataValue("v0").ShouldBe("y");
+            items.ElementAt(2).GetMetadataValue("M2").ShouldBe("d");
+        }
+
         [Fact]
         public void KeepWithItemReferenceOnNonmatchingMetadata()
         {
@@ -1814,7 +1858,7 @@ public void FailWithMultipleItemReferenceOnMatchingMetadata()
                     </Target></Project>");
             IntrinsicTask task = CreateIntrinsicTask(content);
             Lookup lookup = LookupHelpers.CreateEmptyLookup();
-            Assert.ThrowsAny<InternalErrorException>(() => ExecuteTask(task, lookup));
+            Assert.ThrowsAny<InvalidProjectFileException>(() => ExecuteTask(task, lookup));
         }
 
         [Fact]
@@ -1839,7 +1883,7 @@ public void FailWithMetadataItemReferenceOnMatchingMetadata()
                     </Target></Project>");
             IntrinsicTask task = CreateIntrinsicTask(content);
             Lookup lookup = LookupHelpers.CreateEmptyLookup();
-            Assert.ThrowsAny<InternalErrorException>(() => ExecuteTask(task, lookup));
+            Assert.ThrowsAny<InvalidProjectFileException>(() => ExecuteTask(task, lookup));
         }
 
         [Fact]

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

@@ -272,28 +272,21 @@ private List<ProjectItemInstance> FindItemsUsingMatchOnMetadata(
 
             var itemSpec = new ItemSpec<ProjectPropertyInstance, ProjectItemInstance>(child.Remove, bucket.Expander, child.RemoveLocation, Project.Directory, true);
 
-            // todo create OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem resource
-            ErrorUtilities.VerifyThrowInvalidOperation(
+            ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(
                 itemSpec.Fragments.Count == 1
                 && itemSpec.Fragments.First() is ItemSpec<ProjectPropertyInstance, ProjectItemInstance>.ItemExpressionFragment,
+                new BuildEventFileInfo(string.Empty),
                 "OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem",
                 child.RemoveLocation,
                 child.Remove);
 
-            var itemFragment = itemSpec.Fragments.First() as ItemSpec<ProjectPropertyInstance, ProjectItemInstance>.ItemExpressionFragment;
-
             var itemsToRemove = new List<ProjectItemInstance>();
 
             foreach (var item in items)
             {
-                foreach (var referencedItem in itemFragment.ReferencedItems)
+                if (itemSpec.MatchesItemOnMetadata(item, matchOnMetadata))
                 {
-                    if (matchOnMetadata.All(
-                            m =>
-                                item.GetMetadataValue(m).Equals(referencedItem.Item.GetMetadataValue(m)) && !item.GetMetadataValue(m).Equals(string.Empty)))
-                    {
-                        itemsToRemove.Add(item);
-                    }
+                    itemsToRemove.Add(item);
                 }
             }
 

src/Build/Evaluation/ItemSpec.cs

@@ -87,12 +87,13 @@ public override bool IsMatch(string itemToMatch)
                 return ReferencedItems.Any(v => v.ItemAsValueFragment.IsMatch(itemToMatch));
             }
 
-            public override bool IsMatchOnMetadata(IItem itemToMatch, ImmutableList<string> metadata)
+            public override bool IsMatchOnMetadata(IItem item, IEnumerable<string> metadata)
             {
                 foreach (var referencedItem in ReferencedItems)
                 {
-                    if (metadata.All(m =>
-                            itemToMatch.GetMetadataValue(m).Equals(referencedItem.Item.GetMetadataValue(m)) && !referencedItem.Item.GetMetadataValue(m).Equals(string.Empty)))
+                    var nonEmptyMetadata = metadata.Where(m => !item.GetMetadataValue(m).Equals(string.Empty) || !referencedItem.Item.GetMetadataValue(m).Equals(string.Empty));
+                    if (nonEmptyMetadata.Count() != 0 && nonEmptyMetadata.All(
+                            m => item.GetMetadataValue(m).Equals(referencedItem.Item.GetMetadataValue(m))))
                     {
                         return true;
                     }
@@ -313,7 +314,7 @@ public bool MatchesItem(I item)
         /// <param name="item">The item to attempt to find a match for based on matching metadata</param>
         /// <param name="metadata">Names of metadata to look for matches for</param>
         /// <returns></returns>
-        public bool MatchesItemOnMetadata(IItem item, ImmutableList<string> metadata)
+        public bool MatchesItemOnMetadata(IItem item, IEnumerable<string> metadata)
         {
             foreach (var fragment in Fragments)
             {
@@ -454,7 +455,7 @@ public virtual bool IsMatch(string itemToMatch)
             return FileMatcher.IsMatch(itemToMatch);
         }
 
-        public virtual bool IsMatchOnMetadata(IItem itemToMatch, ImmutableList<string> metadata)
+        public virtual bool IsMatchOnMetadata(IItem itemToMatch, IEnumerable<string> metadata)
         {
             return false;
         }

src/Build/Evaluation/LazyItemEvaluator.RemoveOperation.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using Microsoft.Build.Construction;
+using Microsoft.Build.Execution;
 using Microsoft.Build.Shared;
 using System.Collections.Immutable;
 using System.Linq;
@@ -14,24 +15,27 @@ class RemoveOperation : LazyItemOperation
         {
             readonly ImmutableList<string> _matchOnMetadata;
 
-            public RemoveOperation(OperationBuilder builder, LazyItemEvaluator<P, I, M, D> lazyEvaluator)
+            public RemoveOperation(RemoveOperationBuilder builder, LazyItemEvaluator<P, I, M, D> lazyEvaluator)
                 : base(builder, lazyEvaluator)
             {
-                _matchOnMetadata = builder.ItemElement.MatchOnMetadata.Equals(string.Empty) ? null : builder.ItemElement.MatchOnMetadata.Split(';').ToImmutableList();
+                _matchOnMetadata = builder.MatchOnMetadata.ToImmutable();
             }
 
             // todo port the self referencing matching optimization (e.g. <I Remove="@(I)">) from Update to Remove as well. Ideally make one mechanism for both. https://github.com/Microsoft/msbuild/issues/2314
             // todo Perf: do not match against the globs: https://github.com/Microsoft/msbuild/issues/2329
             protected override ImmutableList<I> SelectItems(ImmutableList<ItemData>.Builder listBuilder, ImmutableHashSet<string> globsToIgnore)
             {
-                if (_matchOnMetadata != null && (_itemSpec.Fragments.Count != 1 || _itemSpec.ItemSpecString.Contains('%')))
-                {
-                    throw new InternalErrorException("Only a single item reference allowed when removing with metadata match");
-                }
+                ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(
+                    _matchOnMetadata.IsEmpty || (!_matchOnMetadata.IsEmpty
+                    && _itemSpec.Fragments.Count == 1
+                    && _itemSpec.Fragments.First() is ItemSpec<ProjectProperty, ProjectItem>.ItemExpressionFragment),
+                    new BuildEventFileInfo(string.Empty),
+                    "OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem");
+
                 var items = ImmutableHashSet.CreateBuilder<I>();
                 foreach (ItemData item in listBuilder)
                 {
-                    if ((_matchOnMetadata == null && _itemSpec.MatchesItem(item.Item)) || (_matchOnMetadata != null && _itemSpec.MatchesItemOnMetadata(item.Item, _matchOnMetadata)))
+                    if ((_matchOnMetadata.IsEmpty && _itemSpec.MatchesItem(item.Item)) || (!_matchOnMetadata.IsEmpty && _itemSpec.MatchesItemOnMetadata(item.Item, _matchOnMetadata)))
                         items.Add(item.Item);
                 }
 
@@ -64,5 +68,13 @@ public ImmutableHashSet<string>.Builder GetRemovedGlobs()
                 return builder;
             }
         }
+        class RemoveOperationBuilder : OperationBuilder
+        {
+            public ImmutableList<string>.Builder MatchOnMetadata { get; } = ImmutableList.CreateBuilder<string>();
+
+            public RemoveOperationBuilder(ProjectItemElement itemElement, bool conditionResult) : base(itemElement, conditionResult)
+            {
+            }
+        }
     }
 }

src/Build/Evaluation/LazyItemEvaluator.cs

@@ -516,10 +516,38 @@ private IncludeOperation BuildIncludeOperation(string rootDirectory, ProjectItem
 
         private RemoveOperation BuildRemoveOperation(string rootDirectory, ProjectItemElement itemElement, bool conditionResult)
         {
-            OperationBuilder operationBuilder = new OperationBuilder(itemElement, conditionResult);
+            RemoveOperationBuilder operationBuilder = new RemoveOperationBuilder(itemElement, conditionResult);
 
             ProcessItemSpec(rootDirectory, itemElement.Remove, itemElement.RemoveLocation, operationBuilder);
 
+            // Process MatchOnMetadata
+            if (itemElement.MatchOnMetadata.Length > 0)
+            {
+                string evaluatedmatchOnMetadata = _expander.ExpandIntoStringLeaveEscaped(itemElement.MatchOnMetadata, ExpanderOptions.ExpandProperties, itemElement.MatchOnMetadataLocation);
+
+                if (evaluatedmatchOnMetadata.Length > 0)
+                {
+                    var matchOnMetadataSplits = ExpressionShredder.SplitSemiColonSeparatedList(evaluatedmatchOnMetadata);
+
+                    ImmutableList<string>.Builder metadataPatterns = ImmutableList.CreateBuilder<string>();
+                    foreach (var matchOnMetadataSplit in matchOnMetadataSplits)
+                    {
+                        metadataPatterns.Add(matchOnMetadataSplit);
+                        AddItemReferences(matchOnMetadataSplit, operationBuilder, itemElement.MatchOnMetadataLocation);
+                    }
+
+                    if (metadataPatterns != null)
+                    {
+                        foreach (string metadata in metadataPatterns)
+                        {
+                            string metadataExpanded = _expander.ExpandIntoStringLeaveEscaped(metadata, ExpanderOptions.ExpandPropertiesAndItems, itemElement.MatchOnMetadataLocation);
+                            var metadataSplits = ExpressionShredder.SplitSemiColonSeparatedList(metadataExpanded);
+                            operationBuilder.MatchOnMetadata.AddRange(metadataSplits);
+                        }
+                    }
+                }
+            }
+
             return new RemoveOperation(operationBuilder, this);
         }
 
@@ -578,7 +606,7 @@ private void ProcessMetadataElements(ProjectItemElement itemElement, OperationBu
             }
         }
 
-        private void AddItemReferences(string expression, IncludeOperationBuilder operationBuilder, IElementLocation elementLocation)
+        private void AddItemReferences(string expression, OperationBuilder operationBuilder, IElementLocation elementLocation)
         {
             if (expression.Length == 0)
             {

src/Build/Resources/Strings.resx

@@ -1504,6 +1504,9 @@ Utilization:          {0} Average Utilization: {1:###.0}</value>
   <data name="OM_MatchingProjectAlreadyInCollection">
     <value>An equivalent project (a project with the same global properties and tools version) is already present in the project collection, with the path "{0}". To load an equivalent into this project collection, unload this project first.</value>
   </data>
+  <data name="OM_MatchOnMetadataIsRestrictedToOnlyOneReferencedItem">
+    <value>Removing with MatchOnMetadata only supports one referenced item.</value>
+  </data>
   <data name="OM_ProjectWasNotLoaded">
     <value>The project provided was not loaded in the collection.</value>
   </data>