[Bug]: Static Graph Restore failed with a NullReferenceException when a non-SDK-style project refs an SDK-style project with SetTargetFramework

[dfederm]

NuGet Product Used

MSBuild.exe

Product Version

msbuild 17

Worked before?

No

Impact

It's more difficult to complete my work

Repro Steps & Context

Dep\Dep.csproj:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFrameworks>net472;net5.0</TargetFrameworks>
  </PropertyGroup>
</Project>

Broken\Broken.csproj:

<Project>
  <Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\Microsoft.Common.props" />
  <PropertyGroup>
    <TargetFrameworkVersion>v4.7.2</TargetFrameworkVersion>
  </PropertyGroup>
  <ItemGroup>
    <ProjectReference Include="..\Dep\Dep.csproj">
      <SetTargetFramework>TargetFramework=net472</SetTargetFramework>
    </ProjectReference>
  </ItemGroup>
  <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
</Project>

Do a static graph restore on Broken\Broken.csproj. The output looks like:

Microsoft (R) Build Engine version 17.2.0-dev-22166-01+719247ede for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
C:\Program Files\Microsoft Visual Studio\2022\IntPreview\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error : Object reference not set to an instance of an object. [C:\Code\tmp\GraphRestoreRepro\Broken\Broken.csproj]

Build FAILED.

C:\Program Files\Microsoft Visual Studio\2022\IntPreview\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error : Object reference not set to an instance of an object. [C:\Code\tmp\GraphRestoreRepro\Broken\Broken.csproj]
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:06.70

The NRE specifically is in MSBuildStaticGraphRestore.GetPackageSpec. The caller iterates each ProjectWithInnerNodes and calls GetPackageSpec(project.OuterProject), but project.OuterProject is null for the Dep\Dep.csproj entry.

If any of the following is changed, it works:

1. Non-static graph restore
2. Remove the SetTargetFramework metadata from Broken\Broken.csproj
3. Migrate Broken\Broken.csproj to SDK-style

Verbose Logs

No response

[nkolev92]

Note that standard restore doesn't really support SetTargetFramework either.

NuGet chooses the best framework of the available ones.

[AArnott]

NuGet chooses the best framework of the available ones.

What does that mean? NuGet doesn't choose the target framework at all across a P2P that explicitly sets SetTargetFramework, I thought. We set it when NuGet isn't present to make the decision, at least. I expected (perhaps in ignorance) that the metadata would carry weight even on nuget-aware projects as well.

[jeffkl]

My initial investigation found some interesting results. As David noted, removing SetTargetFramework works and migrating to SDK-style projects works.

This is because SDK-style projects are treated as having an inner and outer node:

https://github.com/dotnet/msbuild/blob/46adf8052f15b8dd618e9bd46d226267105de0c8/src/Build/Graph/ProjectInterpretation.cs#L67-L82

For SDK-style projects, the SetTargetFramework applies but later on the TargetFramework global property is removed because project graph knows that this project is SDK-style. If the project is legacy, project graph treats its references as legacy and will only do a single evaluation of them. This results in the follow evaluations:

Legacy with SetTargetFramework -> SDK-style (net472, net5.0)

Project	Global Properties
Legacy
referenced SDK-style	TargetFramework=net472

Legacy without SetTargetFramework -> SDK-style

Project	Global Properties
Legacy
referenced SDK-style
referenced SDK-style	TargetFramework=net472
referenced SDK-style	TargetFramework=net5.0

SDK-style -> SDK-style

Project	Global Properties
SDK-style
referenced SDK-style
referenced SDK-style	TargetFramework=net472
referenced SDK-style	TargetFramework=net5.0

For builds, its okay if the graph is incomplete since all of the projects that will actually be built are there. However, for restore we need every project and all inner nodes representing their target frameworks.

We could fix the exception here but we'd probably need to fail restore saying that the project graph is incomplete. For now the workaround of removing SetTargetFramework seems like the best option.

[jeffkl]

What does that mean? NuGet doesn't choose the target framework at all across a P2P that explicitly sets SetTargetFramework, I thought. We set it when NuGet isn't present to make the decision, at least. I expected (perhaps in ignorance) that the metadata would carry weight even on nuget-aware projects as well.

During restore, SetTargetFramework is completely disregarded because NuGet needs a full representation of all projects and all target frameworks, not just the ones being built. It then does a complete restore for each project while the build might be more scoped and only build certain target frameworks. So at evaluation time, NuGet gets all projects, and at build time MSBuild only builds projects based on stuff like SetTargetFramework.

[AArnott]

So at evaluation time, NuGet gets all projects, and at build time MSBuild only builds projects based on stuff like SetTargetFramework.

How can we accommodate both? Is there some Condition we should add to the SetTargetFramework metadata so that nuget restore ignores it but build uses it? Or if as it sounds, NuGet already ignores it during restore, why is restore failing?

[jeffkl]

How can we accommodate both? Is there some Condition we should add to the SetTargetFramework metadata so that nuget restore ignores it but build uses it? Or if as it sounds, NuGet already ignores it during restore, why is restore failing?

Static graph-based restore is expecting to get a graph back from MSBuild that represents every outer and inner evaluation. However, you've discovered a bug/feature where if you use SetTargetFramework with a legacy project referencing an SDK-style project, the graph is incomplete. NuGet then reads the graph and can't find the outer project evaluation representing the project and its target frameworks and unfortunately at the moment it throws an exception rather than logging an error.

We think the solution is to have two modes for static graph. One mode tells MSBuild to get all project edges that represent the complete graph and another mode (which is the current one) that only gets project edges that are to be built. This second mode leaves out some outer project evaluations that NuGet needs but would potentially cause MSBuild to build more than it needed to. But a complete graph is needed for restore.