Home: 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 https://github.com/dotnet/arcade/issues/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

[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.

[dsplaisted]

To mitigate the impact of breaking people, I'd suggest a flag they can set to revert to the old behavior (or perhaps to revert to consistent "choose last" or "choose first" behavior across the four different restore types).

[marcin-krystianc]

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.

I lost in the past quite a chunk of time trying to investigate issues caused by duplicated PackageReference elements.
There are plenty of reasons why you can end-up having duplicates which are hard to spot:

• package references defined via Directory.Build.props
• package references implicitly defined in the SDK (e.g. FSharp.Core in *.fsproj files)
• package references defined by imported *.props files

Silently accepting these duplicates leads to unnecessary confusion. E.g. I can go to my *.fsproj project file and update the FSharp.Core version but it will have no effect on the version that is actually used (FSharp.Core is by default defined by the sdk unless DisableImplicitFSharpCoreReference is specified). Inexperienced developers don't even know how to debug such issues so it can make them feel helpless.

[davkean]

I've also spent a bunch diagnosing duplicate package references too. A warning would be extremely helpful.