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.