Restore may write nulls to project.assets.json

[tmat]

NuGet Product Used

MSBuild.exe

Product Version

9.0.100-preview.5.24307.3

Worked before?

9.0.100-preview.3.24204.13

Impact

I'm unable to use this version

Repro Steps & Context

SDK task ResolvePackageAssets throws NRE while enumerating NuGet.ProjectModel.ProjectFileDependencyGroup.Dependencies, which returns null.

I'm guessing that's because the project.assets.json contains nulls in projectFileDependencyGroups, which it probably shouldn't. Somehow nuget restore managed to produce these:

"libraries": {},
  "projectFileDependencyGroups": {
    ".NETFramework,Version=v4.7.2": [
      null,
      null,
      null,
      null,
      null,
      null,
      "Microsoft.DotNet.Build.Tasks.VisualStudio >= 9.0.0-beta.24317.3",
      "Microsoft.Net.Compilers.Toolset >= 4.11.0-2.24270.4",
      "Microsoft.VisualStudioEng.MicroBuild.Plugins.SwixBuild >= 1.1.286"
    ]
  },

C:\Program Files\dotnet\sdk\9.0.100-preview.5.24307.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(266,5): error MSB4018: The "ResolvePackageAssets" task failed unexpectedly.
System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.NET.Build.Tasks.ProjectContext.<>c__DisplayClass31_0.<GetTopLevelDependencies>b__3(String projectFileDependency)
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   at System.Linq.Enumerable.WhereEnumerableIterator`1.MoveNext()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheWriter.ComputePackageExclusions()
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheWriter..ctor(ResolvePackageAssets task)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheReader.CreateReaderFromDisk(ResolvePackageAssets task, Byte[] settingsHash)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheReader..ctor(ResolvePackageAssets task)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.ReadItemGroups()
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.ExecuteCore()
   at Microsoft.NET.Build.Tasks.TaskBase.Execute()
   at Microsoft.Build.BackEnd.TaskExecutionHost.Execute()
   at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext() 
[D:\R4\src\VisualStudio\Setup\Roslyn.VisualStudio.Setup.csproj]

Verbose Logs

Can share full binlog. DM me on Teams.
Searching for null,:

[nkolev92]

Reproducible in VS 17.11, P1.
Debugging now.

[nkolev92]

The nominations in P1 seem to not be sending PackageVersion.

I'm guessing VS has plenty of NU1008 errors which are the real problem.

I'll debug further to figure out whether the bug is NuGet or project-system.

[nkolev92]

The nominations are missing the central package versions.
Not sure why that is.

Updating to P2 to try it out and see if it's fixed there.

[nkolev92]

VS 17.11, P2 has the same issue.

The data coming it at the NuGet entry point through https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.VisualStudio/SolutionRestoreManager/IVsTargetFrameworkInfo3.cs doesn't have the PackageVersion items.

Sometimes, in the past we've had issues where a target fails and not all the data gets populated in the nomination event.

How did we get from the null write here?

NuGet/NuGet.Client@2be8680 changed the return from empty string just the name to null so what ends up in the assets file is different, which leads to this weird error.
I think we should probably fix that, since it's hiding the real issue (NU1008 error).

[nkolev92]

cc @tmeschter, @drewnoakes, @zivkan

I thought that the root cause might be a failing CollectCentralPackageVersions, but that wasn't the case. It seems like the collect target isn't running in VS to begin with. Not sure why that might be.

[drewnoakes]

Could this be related to dotnet/sdk#39316?

[nkolev92]

I'd expect dotnet/sdk#39316 is a symptom of this issue yes.

I think there's 2 sides to it.

1. We need to fix NuGet writing "null" values.
2. We need safer handling on the SDK side, since I think not having a version for a package is a possibility with CPM.

[nkolev92]

There's 17.10 reports for this as well: https://developercommunity.visualstudio.com/t/Misleading-error-message-when-an-entry-i/10686346.

I think we need to start making the "safety" changes because without those it can lead to many errors, making it challenging to recognize the first issue.

[tmat]

I'd expect dotnet/sdk#39316 is a symptom of this issue yes.

I think there's 2 sides to it.

1. We need to fix NuGet writing "null" values.
2. We need safer handling on the SDK side, since I think not having a version for a package is a possibility with CPM.

Agreed. Some hardening of these code paths would be great. E.g. using C# nullable attributes might have prevented the bug.

[nkolev92]

NuGet side fix: NuGet/NuGet.Client#5900

[nkolev92]

Regarding #2, the .NET SDK's code is probably just fine.
They're using nuget apis and just iterating over a collection. Technically a collection could contain null, so maybe you check that, but that might be excessive.

One discrepancy I did notice is that STJ & NJ parsing behave differently.
STJ ends up adding nulls in the collection, while NJ doesn't.
I think it might be worth it to align that behavior. I'll investigate that.