Duplicate PackageReference includes are handled inconsistently by all restores, leading to unnecessary full restores

[nkolev92]

Take a project such as the following:

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

  <PropertyGroup>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="NuGet.Protocol" Version="5.5.0" />
    <PackageReference Include="NuGet.Protocol" Version="5.6.0" />
  </ItemGroup>

</Project>

Note the duplicate PackageReference declaration. Digging further reveals that different project types and different tools handle it differently.

See the below table for details

Restore Flavor	PackageReference declaration
Commandline restore (all)	First
Static graph restore (all)	First
VS - SDK based projects	Last
VS - Legacy PR projects	First

This leads to issues as the ones described in dotnet/arcade#5550 (see below repro).

I think logically from an msbuild perspective, we'd expect that the last value is the one that's resolved. This needs investigated and ideally consolidated.
Note that consolidating VS - LEgacy PR projects might be impossible, due to the fact that it's largely in maintenance mode.

See repro below.

cc folks that might have some thoughts around:

@jeffkl @davkean @dsplaisted @zivkan

fyi @sharwell @garath

DuplicateIds.zip

WIP:
Design: https://github.com/NuGet/Home/compare/dev-nkolev92-duplicatesHandling
Implementation: https://github.com/NuGet/NuGet.Client/tree/dev-nkolev92-duplicatePRs

[dsplaisted]

Can we make it an error to have duplicated PackageReference items? This would break projects, but seems like saner behavior.

The SDK already checks for a variety of duplicate items (Compile, FrameworkReference, etc) and errors if there are duplicates.

[nkolev92]

The commandline scenarios would be easy.

In VS, nomination that'd be problematic. I don't think the project-system can/should surface this error, rather it should be NuGet. That means NuGet needs to receive all the data.

The IVsReferenceItem that represents a PR has a name, which suggests it should be unique.
However, IVsReferenceItems which is the collection of items doesn't really rely on the name as a unique identifier.

Thoughts @davkean?

I'd have to investigate legacy to understand what happens there.

[dsplaisted]

Can you generate the error in the NuGet targets? Then you would have access to all the MSBuild items.

[nkolev92]

That might work.

[davkean]

Make CollectPackageReferences produce an error, be sure to respect $(ContinueOnError) (https://github.com/dotnet/msbuild/blob/6f41a4fa3c6957d430783b7d31fe16e6a13b6e8e/src/Shared/XMakeAttributes.cs#L69) so we don't break the design-time build.

[davkean]

Note, we cannot pass through the entire list of items to NuGet ourselves - the entire surface area in CPS is based on dictionaries and not a fair representation of the "list" concept in MSBuild.

[nkolev92]

fyi @rconard @anangaur for awareness.

[nkolev92]

I have confirmed at least 2 other bugs to the same root cause.
I'd expect that at least 2 more might be hitting this.
no-op is very important for us, especially across same version of commandline + VS restore, bumping the pri per that.

[nkolev92]

A precedent very much exists in https://github.com/dotnet/sdk/blob/44f381b62d466565639d51847c9127afbe7062a9/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.targets#L284 :)

I think the biggest concern here is how happy customers will be to suddenly start getting this error. :)
This is a categorically good change though, the behavior across all tools should be consistent.

[drewnoakes]

Could this be introduced behind a warning wave?

[nkolev92]

Can you elaborate what you mean by a warning wave?

[drewnoakes]

Looks like it's called a Change Wave in the docs: https://docs.microsoft.com/en-us/visualstudio/msbuild/change-waves?view=vs-2022

[nkolev92]

Hey all,

Given that this can be a pretty impactful change, I have written up a proposal.
I would appreciate any reviews on it #11649.

The pull request for the proposal can be found at NuGet/NuGet.Client#4484.

[nkolev92]

tldr of the proposal:

When a NuGet item is seen as a duplicate, the first item is chosen and a warning is raised. NU1505 for PackageReference, NU1506 for PackageDownload, NU1507 for PackageVersion.
This warning supports NoWarn, TreatWarningsAsErrors, and it can be disabled altogether.

We're also supporting ContinueOnError for VS purposes.

[nkolev92]

Reviewed the proposal internally.
Goal is 17.3/6.0.400 for VS/.NET SDK respectively