I'm still trying to wrap my head around the original issue and the fix, to ensure I can't foresee unintended consequences from side effects. But if I understand correctly, say I have Contoso.Utilities as a transitive package, if I clone the repo and add Contoso.Utilities.csproj to my solution, then the build will use that project's dll? I can make changes to Contoso.Utilities and test with my app, without neeing to publish packages or changing dependency versions?

Here's my example project as a zip: tfmnugetoverlap.zip. Do a dotnet restore tfmnugetoverlap\main\main.csproj to repro the restore failure.

I'm really not a fan of optional types in public APIs. I prefer a new overload, which would also prevent a breaking ABI change. A new overload would also keep the convention of keeping the CancellationToken as the last parameter.

Agreed. Since I opened the PR I also explored another solution that would do the following instead:

The general problem is that the PackageSpecReferenceDependencyProvider which gets added to the restore context isn't aware of target frameworks / inner builds:

Because of that, the individual TFM restore operations consider all project dependencies, even the ones that conditionally are excluded for the current framework. I experimented with updating PackageSpecReferenceDependencyProvider to encode which projects apply to which framework. The challenging part is calculating the TFM aware dependency tree though. NuGet / msbuild creates a dgspec.json file which lists all the dependencies but the TFM selection doesn't happen until NuGet actually resolves the dependency. So building that dependency tree upfront might cause unintended side-effects which I'm not too excited to make given that this code base is unfamiliar to me.

So all in all, I guess the current approach (passing the actual dependencies in to the resolver logic vs updating the PackageSpecReferenceDependencyProvider to be TFM aware and only return the matching dependencies) is fine.

Thanks for the contribution @ViktorHofer! I know this must have been a lot of work.

The general problem is that the PackageSpecReferenceDependencyProvider which gets added to the restore context isn't aware of target frameworks / inner builds:

Can you elaborate on this?
It has methods that take a framework, so it can do the proper compat selection.

For example, I recently did work there: df7d995.

The packagespecreferenceprovider should be called in the context of calling a dependency provider.
When the package spec reference provider is asked for certain dependencies it is asked with a framework: https://github.com/NuGet/NuGet.Client/blob/40648c3cb3de37cf62b194accdc8e7117b36245f/src/NuGet.Core/NuGet.DependencyResolver.Core/ResolverUtility.cs#L161C38-L161C59.

You are probably onto something, but with the reasoning given here, I can't confirm it's the right thing yet.

When a transitive package dependency has the same identity as a direct project dependency that is differenced by a different inner build (TFM), the dependency is incorrectly resolved as a project.

What does same identity mean for you here? Same ID? Same ID/Version?

fwiw, I started writing a test so I can debug the behavior.
Here's the code.

[Fact] public async Task ExecuteAsync_WithConditionalProjectAndPackageReferences() {     // Arrange     using var context = new SourceCacheContext();     using var pathContext = new SimpleTestPathContext();     // Add nuget.org for the extra package.     pathContext.Settings.AddSource("nuget.org", "https://api.nuget.org/v3/index.json");      var mainProject = "main";     var mainProjectPath = Path.Combine(pathContext.SolutionRoot, mainProject);      var childProject = "System.Numerics.Vectors";     var childProjectPath = Path.Combine(pathContext.SolutionRoot, childProject);      var mainProjectJson = @"     {       ""version"": ""1.0.0"",       ""frameworks"": {         ""netstandard2.0"": {             ""imports"": [                         ""net461"",                         ""net462"",                         ""net47"",                         ""net471"",                         ""net472"",                         ""net48"",                         ""net481""                       ],             ""assetTargetFallback"": true,             ""dependencies"": {                 ""System.Memory"": {                               ""target"": ""Package"",                               ""version"": ""[4.5.5, )""                             },                   ""NETStandard.Library"": {                               ""suppressParent"": ""All"",                               ""target"": ""Package"",                               ""version"": ""[2.0.3, )"",                               ""autoReferenced"": true                             }             }         },         ""net8.0"": {             ""imports"": [                         ""net461"",                         ""net462"",                         ""net47"",                         ""net471"",                         ""net472"",                         ""net48"",                         ""net481""                       ],             ""assetTargetFallback"": true,             ""dependencies"": {             }         }       }     }";     PackageSpec systemNumericsVectorPackageSpec = ProjectTestHelpers.GetPackageSpec(childProject, pathContext.SolutionRoot, "net8.0",         useAssetTargetFallback: true, "net461\",\"net462\",\"net47\",\"net471\",\"net472\",\"net48\",\"net481");     PackageSpec mainPackageSpec = ProjectTestHelpers.GetPackageSpecWithProjectNameAndSpec(mainProject, pathContext.SolutionRoot, mainProjectJson);     var settings = Settings.LoadDefaultSettings(pathContext.SolutionRoot);     mainPackageSpec.RestoreMetadata.ConfigFilePaths = settings.GetConfigFilePaths();     mainPackageSpec.RestoreMetadata.Sources = SettingsUtility.GetEnabledSources(settings).ToList();     mainPackageSpec.RestoreMetadata.FallbackFolders = SettingsUtility.GetFallbackPackageFolders(settings).ToList();     mainPackageSpec.RestoreMetadata.PackagesPath = SettingsUtility.GetGlobalPackagesFolder(settings);      mainPackageSpec.RestoreMetadata.TargetFrameworks.Single(e => e.FrameworkName.Equals(NuGetFramework.Parse("net8.0")))         .ProjectReferences.Add(new ProjectRestoreReference()     {         ProjectUniqueName = systemNumericsVectorPackageSpec.RestoreMetadata.ProjectUniqueName,         ProjectPath = systemNumericsVectorPackageSpec.RestoreMetadata.ProjectPath,     });      var logger = new TestLogger();     var request = ProjectTestHelpers.CreateRestoreRequest(pathContext, logger, mainPackageSpec, systemNumericsVectorPackageSpec);      var restoreCommand = new RestoreCommand(request);     RestoreResult result = await restoreCommand.ExecuteAsync();     await result.CommitAsync(logger, CancellationToken.None);      result.Success.Should().BeTrue(because: logger.ShowMessages()); }

I used test\NuGet.Core.Tests\NuGet.Commands.Test\RestoreCommandTests\RestoreCommandTests.cs as the class

I took a stab fixing the PackageSpecReferenceDependencyProvider. It basically gets asked for a library, but it ignores the version. If the version matches, then I think the project should be preferred over the package.
See equivalent for packages:

I'm not really sure if direct refs are the ones that should be checked when including a project in the graph. I think if you have an extra layer of dependencies, then you'd likely have issues, like for example, if System.Numerics vector was a transitive project reference.

My changes are in https://github.com/NuGet/NuGet.Client/tree/dev-nkolev92-10368, here's what happens when I run dotnet list package

C:\Code\Temp\tfmnugetoverlap [main]> dotnet list .\main\main.csproj package --include-transitive
Project 'main' has the following package references
   [netstandard2.0]:
   Top-level Package             Requested   Resolved
   > NETStandard.Library   (A)   [2.0.3, )   2.0.3
   > System.Memory               4.5.5       4.5.5

   Transitive Package                            Resolved
   > Microsoft.NETCore.Platforms                 1.1.0
   > System.Buffers                              4.5.1
   > System.Numerics.Vectors                     4.4.0
   > System.Runtime.CompilerServices.Unsafe      4.5.3

   [net8.0]:
   Transitive Package             Resolved
   > System.Numerics.Vectors      1.0.0

(A) : Auto-referenced package.

Now the trick is that this is a very tricky change. I'm not really sure it's the right thing just yet.

I'll let the CI run all the tests and revisit after at again when I have more time.

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

I'm on holiday now and will respond when I'm back. Thanks for taking a look so far.

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

Met with @nkolev92 offline and talked about different solutions to this problem. I like @nkolev92's proposal which avoids adding new public API and meets our requirements. We identified that there's still a problem in regards to choosing P2P over PackageReferences but we believe this is a super corner case that people shouldn't hit today.

Closing this PR as he will open a new one with the test and the fix.