No error for Item update in target

[dasMulli]

Ran into this while creating a workaround to patch an existing target (https://github.com/dotnet/cli/issues/6397#issuecomment-347989664) and naively using the item update syntax inside a target to update the link metadata of an item.
However, using the update syntax inside a target seems to update the metadata of all items, which is unexpected.

Steps to reproduce

Project file

<Project DefaultTargets="PrintResults">
  <ItemGroup>
    <SomeStaticItem Include="Item1" SomeMeta="MetaVal1" />
    <SomeStaticItem Include="Item2" SomeMeta="MetaVal2" />
    <SomeStaticItem Include="Item3" SomeMeta="MetaVal3" />

    <SomeStaticItem Update="Item2" SomeMeta="ChangedMetaVal2" />
  </ItemGroup>

  <Target Name="CreateRuntimeUpdatedItems">
    <ItemGroup>
      <SomeRuntimeItem Include="@(SomeStaticItem)" />
      <SomeRuntimeItem Update="Item2" SomeMeta="ChangedMetaVal2" />
    </ItemGroup>
  </Target>

  <Target Name="PrintResults" DependsOnTargets="CreateRuntimeUpdatedItems">
    <Message Importance="high" Text="static item: %(SomeStaticItem.Identity): SomeMeta=%(SomeStaticItem.SomeMeta)" />
    <Message Importance="high" Text="runtiime item:%(SomeRuntimeItem.Identity): SomeMeta=%(SomeRuntimeItem.SomeMeta)" />
  </Target>
</Project>

Command line

dotnet msbuild

Expected behavior

Error for Update syntax not being allowed inside targets

or

  static item: Item1: SomeMeta=MetaVal1
  static item: Item2: SomeMeta=ChangedMetaVal2
  static item: Item3: SomeMeta=MetaVal3
  runtiime item:Item1: SomeMeta=MetaVal1
  runtiime item:Item2: SomeMeta=ChangedMetaVal2
  runtiime item:Item3: SomeMeta=MetaVal3

Actual behavior

  static item: Item1: SomeMeta=MetaVal1
  static item: Item2: SomeMeta=ChangedMetaVal2
  static item: Item3: SomeMeta=MetaVal3
  runtiime item:Item1: SomeMeta=ChangedMetaVal2
  runtiime item:Item2: SomeMeta=ChangedMetaVal2
  runtiime item:Item3: SomeMeta=ChangedMetaVal2

Environment data

dotnet msbuild /version output: tested on 15.5.179.9764 and 15.6.12.27473 .

OS info:

If applicable, version of the tool that invokes MSBuild (Visual Studio, dotnet CLI, etc): macOS 10.13.2, dotnet cli

[rainersigwald]

Looks like a duplicate of #1618, but that should have been fixed long ago (by throwing an error on the not-actually-supported Update-inside-a-target syntax).

If you change your target to use syntax like #1618 (comment), it should work.

I repro the problem with 15.1.548.43366, so it looks like the error never worked :(

[dasMulli]

yep the old syntax works perfectly, I assumed that since there was no Update metadata added it actually tried to update (which is exactly what's discussed on the linked issue).

[dasMulli]

(updated the issue a bit to focus on the error)

[dasMulli]

@rainersigwald I'm afraid adding the error now would break at least
https://github.com/dotnet/sdk/blob/cb4cf799e0afd5a6c8e0630638f550b95e3eb35e/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ConflictResolution.targets#L47
https://github.com/dotnet/sdk/blob/cb4cf799e0afd5a6c8e0630638f550b95e3eb35e/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets#L450-L453
https://github.com/dotnet/sdk/blob/cb4cf799e0afd5a6c8e0630638f550b95e3eb35e/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.PackageDependencyResolution.targets#L578-L580

[rainersigwald]

@dasMulli Yup, looks like it. Unfortunately, that means we can't do this in a minor update as it'd be a breaking change.

[tmat]

@rainersigwald We should fix this in Dev16.

Bugs like these make msbuild very hard to use. If you want to avoid breaking changes please consider introducing some compat switch that can be used to opt-in new behavior.

[nguerrera]

it looks like the error never worked :(

Was it ever implemented? AFAICT, #1618 was closed because it was included in #1124 (another bug), not because it was fixed.

[nguerrera]

Maybe we should consider making it actually work as one expects rather than erroring in Dev16? The examples shown above so far would not break. I am nervous as always about breaking changes, so we should search for more data on whether there are cases of an update on anything but the entire set, that are actually depending on their bug.

[davidfowl]

@rainersigwald can we do something in the .NET 6 timeframe?

[clairernovotny]

Looks like we missed 6.0 GA but can this be a candidate for a toolset update in a band release?

[rainersigwald]

@clairernovotny the nature of the breaking change makes that not seem like a great idea to me. We've found instances of this in basically all .NET repos as well as user projects. It's not clear to me that the benefit is worth the change, since it could be a subtle silent behavior change to presumably-working targets (after the authors worked around the bad behavior somehow to get it to meet their needs).

[KirillOsenkov]

I don't know what we need to do about this bug, but we need to do something.

[dasMulli]

How about emitting a warning here? "Warning: 'Update' for item updates within targets does not have any effect and all items will be changed. See ..."

This communicates to the author that the project file has some possibly unexpected behavior while not immediately breaking all builds (except for those with warnings as errors).

[rainersigwald]

We consider adding a warning a breaking change, based on painful experience.

The best solution here is going to be a warning that users can opt into, which is why this is marked in the "warning waves" tag, which will be rolled up in the analyzers work that's underway (this makes sense for the early set #9630 IMO).

[Nirmal4G]

Instead of going the roundabout way, why not make the Update attribute work? At this point, it would be better than adding a warning when people expect it to work!

[KirillOsenkov]

or make it work properly after users opt in

[JustinSchneiderPBI]

If you do support an opt-in behavior change, also support an opt-in warning (maybe a common msbuild errors analyzer, since we've found a number of ways in msbuild to shoot yourself in the foot) so all instances can be identified and validated for the opt-in behavior change.

@rainersigwald, sounds like even nowarn options are insufficient to consider automatic new warnings a non-breaking change... leaves me curious and sounds like a painful development impediment.

[rainersigwald]

If you do support an opt-in behavior change, also support an opt-in warning (maybe a common msbuild errors analyzer, since we've found a number of ways in msbuild to shoot yourself in the foot) so all instances can be identified and validated for the opt-in behavior change.

@rainersigwald, sounds like even nowarn options are insufficient to consider automatic new warnings a non-breaking change... leaves me curious and sounds like a painful development impediment.

Indeed it is painful! That's why we're investing a lot right now on a path forward which we're calling analyzers and tracking with https://github.com/dotnet/msbuild/issues?q=is%3Aissue+is%3Aopen+label%3A%22Feature%3A+Warning+Waves%22+.

[rainersigwald]

Instead of going the roundabout way, why not make the Update attribute work? At this point, it would be better than adding a warning when people expect it to work!

or make it work properly after users opt in

It would have to be opt-in since it's a confusing breaking behavior change otherwise. I'm not super opposed to that we'd need the warning when not opted in anyway, so let's get that then reevaluate.

[mmarinchenko]

This issue reminds me of #8527. The MSB4120 warning was added then (#8581). And later its priority was lowered (#9228).

[KirillOsenkov]

I'm more than ever convinced now that we need a mode where item updates in targets work as expected

[cliffchapmanrbx]

Stepped on this one this week because the sentence in the docs is very subtly different than the three previous ones. A reading comprehension failure, of course, just one that caused us a bit of an issue. Our specific use case was adding <Pack>false</Pack> to Content items added from Grpc.Core as it incorrectly added its DLLs to the contentFiles directory of some of our NuGet packages.