[Bug]: Chained item function isn't respected in item update statement

[ViktorHofer]

Issue Description

Apparently, msbuild ignores chained ->WithMetadataValue('...', '...') item functions in an item update statement.

Steps to Reproduce

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <KnownFrameworkReference Update="@(KnownFrameworkReference->WithMetadataValue('Identity', 'Microsoft.NETCore.App')->WithMetadataValue('TargetFramework', 'net8.0'))"
      RuntimePackRuntimeIdentifiers="XXX;%(RuntimePackRuntimeIdentifiers)" />
    <!-- Test with a separate item... -->
    <_asdf Include="@(KnownFrameworkReference->WithMetadataValue('Identity', 'Microsoft.NETCore.App')->WithMetadataValue('TargetFramework', 'net8.0'))" />
  </ItemGroup>

</Project>

dotnet build /bl
Open binary log and search for KnownFrameworkReference. Observe that the RuntimePackRuntimeIdentifier metdata is updated on all "Microsoft.NETCore.App" items, not just the one with the "TargetFramework=net8.0" metadata.

The same works with an Include statement. Search for _asdf and observe that only one item is listed.

Expected Behavior

Only one item should be updated instead of all the ones with Identity=Microsoft.NETCore.App.

Actual Behavior

All Identity="Microsoft.NETCore.App" KnownFrameworkReference items get updated, regardless of the TargetFramework metdata on the item.

Analysis

No response

Versions & Configurations

No response

[KalleOlaviNiemitalo]

Not sure that is a bug. Consider how it works with Remove:

<Project>
  <ItemGroup>
    <Person Include="Mary" Profession="Teacher" />
    <Person Include="Mary" Profession="Lawyer" />
    <Person Include="Ulrich" Profession="Lawyer" />
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '1'">
    <Person Remove="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" />
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '1.B'">
    <Disappear Include="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" />
    <Person Remove="@(Disappear)" />
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '2'">
    <Person Remove="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" MatchOnMetadata="Profession" />
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '3'">
    <Person Remove="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" MatchOnMetadata="Identity;Profession" />
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '4'">
    <Person Remove="Mary" Condition="%(Profession) == 'Lawyer'" />
  </ItemGroup>

  <Target Name="Show">
    <Message Importance="high" Text="Identity='%(Person.Identity)' Profession='%(Person.Profession)'" />
  </Target>
</Project>

MSBuild version 17.8.3+195e7f5a3 for .NET

dotnet msbuild msbuild9636.proj -nologo -p:Case=1
  Identity='Ulrich' Profession='Lawyer'

dotnet msbuild msbuild9636.proj -nologo -p:Case=1.B
  Identity='Ulrich' Profession='Lawyer'

dotnet msbuild msbuild9636.proj -nologo -p:Case=2
  Identity='Mary' Profession='Teacher'

dotnet msbuild msbuild9636.proj -nologo -p:Case=3
  Identity='Mary' Profession='Teacher'
  Identity='Ulrich' Profession='Lawyer'

dotnet msbuild msbuild9636.proj -nologo -p:Case=4
msbuild9636.proj(26,27): error MSB4191: The reference to custom metadata "Profession" at position 13 is not allowed in this condition "%(Profession) == 'Lawyer'".

Then compare that to Update:

<Project>

  <ItemGroup>
    <Person Include="Mary" Profession="Teacher" />
    <Person Include="Mary" Profession="Lawyer" />
    <Person Include="Ulrich" Profession="Lawyer" />
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '1'">
    <Person Update="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))">
      <Updated>true</Updated>
    </Person>
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '1.B'">
    <Disappear Include="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" />
    <Person Update="@(Disappear)">
      <Updated>true</Updated>
    </Person>
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '2'">
    <!-- According to docs, MatchOnMetadata only works with Remove, but let's try anyway.  -->
    <Person Update="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" MatchOnMetadata="Profession">
      <Updated>true</Updated>
    </Person>
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '3'">
    <!-- According to docs, MatchOnMetadata only works with Remove, but let's try anyway.  -->
    <Person Update="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" MatchOnMetadata="Identity;Profession">
      <Updated>true</Updated>
    </Person>
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '4'">
    <Person Update="Mary" Condition="%(Profession) == 'Lawyer'">
      <Updated>true</Updated>
    </Person>
  </ItemGroup>

  <ItemGroup Condition="$(Case) == '5'">
    <Person Update="Mary">
      <Updated Condition="%(Profession) == 'Lawyer'">true</Updated>
    </Person>
  </ItemGroup>

  <Target Name="Show">
    <Message Importance="high" Text="Identity='%(Person.Identity)' Profession='%(Person.Profession)' Updated='%(Person.Updated)'" />
  </Target>
</Project>

dotnet msbuild msbuild9636-update.proj -nologo -p:Case=1
  Identity='Mary' Profession='Teacher' Updated='true'
  Identity='Mary' Profession='Lawyer' Updated='true'
  Identity='Ulrich' Profession='Lawyer' Updated=''

dotnet msbuild msbuild9636-update.proj -nologo -p:Case=1.B
  Identity='Mary' Profession='Teacher' Updated='true'
  Identity='Mary' Profession='Lawyer' Updated='true'
  Identity='Ulrich' Profession='Lawyer' Updated=''

dotnet msbuild msbuild9636-update.proj -nologo -p:Case=2
  Identity='Mary' Profession='Teacher' Updated='true'
  Identity='Mary' Profession='Lawyer' Updated='true'
  Identity='Ulrich' Profession='Lawyer' Updated=''

dotnet msbuild msbuild9636-update.proj -nologo -p:Case=3
  Identity='Mary' Profession='Teacher' Updated='true'
  Identity='Mary' Profession='Lawyer' Updated='true'
  Identity='Ulrich' Profession='Lawyer' Updated=''

dotnet msbuild msbuild9636-update.proj -nologo -p:Case=4
msbuild9636-update.proj(37,27): error MSB4191: The reference to custom metadata "Profession" at position 13 is not allowed in this condition "%(Profession) == 'Lawyer'".

dotnet msbuild msbuild9636-update.proj -nologo -p:Case=5
  Identity='Mary' Profession='Teacher' Updated=''
  Identity='Mary' Profession='Lawyer' Updated='true'
  Identity='Ulrich' Profession='Lawyer' Updated=''

[ViktorHofer]

Doesn't that just express that item remove statements are affected as well? Compare this with an Include statement which works as expected:

<Project>

  <ItemGroup>
    <Person Include="Mary" Profession="Teacher" />
    <Person Include="Mary" Profession="Lawyer" />
    <Person Include="Ulrich" Profession="Lawyer" />
  </ItemGroup>

  <ItemGroup>
    <_asdf Include="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" />
  </ItemGroup>

</Project>

The chained item function shouldn't mutate (in an Update or Remove statement) the holder item until the full chain is evaluated.

[KalleOlaviNiemitalo]

I mean Remove and Update compare only the Identity of the items by default, regardless of whether the attribute value contains item functions or refers to an item group or is just a literal string. And for Remove, one can change that by specifying MatchOnMetadata.

I think the least surprising fix would be to make MatchOnMetadata work with Update too.

[ViktorHofer]

I mean Remove and Update compare only the Identity of the items by default, regardless of whether the attribute value contains item functions

And exactly that doesn't make any sense to me. I would expect the following to happen:

<Project>

  <ItemGroup>
    <Person Include="Mary" Profession="Teacher" />
    <Person Include="Mary" Profession="Lawyer" />
    <Person Include="Ulrich" Profession="Lawyer" />
  </ItemGroup>

  <ItemGroup>
    <Person Update="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))"
            IsUpdated="true" />
    <!-- Step 1: Evaluate the first part of the chained item function:
                 @(Person->WithMetadataValue('Identity', 'Mary')) and hold the result. -->
    <!-- Step 2: Evaluate the second part of the chained item function and pass the result
                 from step1 in as the input: @(...->WithMetadataValue('Profession', 'Lawyer'))
                 Again, hold the result. -->
    <!-- Step 3: Pass the result from step 2 into the <Person Update="..." /> update statement.
                 Add the IsUpdated="true" metadata to Lawyer Mary only. -->
  </ItemGroup>

</Project>

[KalleOlaviNiemitalo]

@ViktorHofer, what would you expect to happen in case 1.B?

  <ItemGroup>
    <Disappear Include="@(Person->WithMetadataValue('Identity', 'Mary')->WithMetadataValue('Profession', 'Lawyer'))" />
    <Person Update="@(Disappear)">
      <Updated>true</Updated>
    </Person>
  </ItemGroup>

[ViktorHofer]

Oh, now I know what you are getting to. Well when passing an item to an Update statement, I would expect that msbuild really only updates/removes the items that having matching metadata but it doesn't look like that's how msbuild syntax works.

[JaynieBai]

There is one example. Should we match this the item specification @(FromPerson) with metadata. If no, how could I know which function should match metadata or not.

<Project>
  <ItemGroup>
    <FromPerson Include="Mary" Profession="Teacher1" />
    <FromPerson Include="Mary" Profession="Lawyer1" />
    <FromPerson Include="Ulrich" Profession="Lawyer1" />
  </ItemGroup>
  <ItemGroup>
    <Person Include="Mary" Profession="Teacher" />
    <Person Include="Mary" Profession="Lawyer" />
    <Person Include="Ulrich" Profession="Lawyer" />
  </ItemGroup>
  <ItemGroup>
    <Person Update="@(FromPerson)" Updated="true"/>
  </ItemGroup>
</Project>

Related code as bellow. Currently, it only matches with Identity, not with metadata

msbuild/src/Build/Evaluation/LazyItemEvaluator.UpdateOperation.cs

Lines 106 to 149 in 0ef8a68

	private void SetMatchItemSpec()
	{
	if (ItemspecContainsASingleBareItemReference(_itemSpec, _itemElement.ItemType))
	{
	// Perf optimization: If the Update operation references itself (e.g. <I Update="@(I)"/>)
	// then all items are updated and matching is not necessary
	_matchItemSpec = (itemSpec, item) => new MatchResult(true, null);
	}
	else if (ItemSpecContainsItemReferences(_itemSpec)
	&& QualifiedMetadataReferencesExist(_metadata, out _needToExpandMetadataForEachItem)
	&& !Traits.Instance.EscapeHatches.DoNotExpandQualifiedMetadataInUpdateOperation)
	{
	var itemReferenceFragments = _itemSpec.Fragments.OfType<ItemSpec<P, I>.ItemExpressionFragment>().ToArray();
	var nonItemReferenceFragments = _itemSpec.Fragments.Where(f => !(f is ItemSpec<P, I>.ItemExpressionFragment)).ToArray();
	_matchItemSpec = (itemSpec, item) =>
	{
	var isMatch = nonItemReferenceFragments.Any(f => f.IsMatch(item.EvaluatedInclude));
	Dictionary<string, I> capturedItemsFromReferencedItemTypes = null;
	foreach (var itemReferenceFragment in itemReferenceFragments)
	{
	foreach (var referencedItem in itemReferenceFragment.ReferencedItems)
	{
	if (referencedItem.ItemAsValueFragment.IsMatch(item.EvaluatedInclude))
	{
	isMatch = true;
	capturedItemsFromReferencedItemTypes ??= new Dictionary<string, I>(StringComparer.OrdinalIgnoreCase);
	capturedItemsFromReferencedItemTypes[referencedItem.Item.Key] = referencedItem.Item;
	}
	}
	}
	return new MatchResult(isMatch, capturedItemsFromReferencedItemTypes);
	};
	}
	else
	{
	_matchItemSpec = (itemSpec, item) => new MatchResult(itemSpec.MatchesItem(item), null);
	}
	}