FIx build errors with live (SCI) runtime dependencies when targeting NetPrevious (net9.0) in source-build (VMR)

[ViktorHofer]

Unblocks dotnet/sdk#45427

Fixes the following two errors:

• src/Features/Core/Portable/ExternalAccess/Watch/Api/WatchHotReloadService.cs(217,13): error CS1503: Argument 4: cannot convert from 'System.Collections.Immutable.ImmutableHashSet<Microsoft.CodeAnalysis.Project>' to 'System.Collections.Generic.IReadOnlySet<Microsoft.CodeAnalysis.Project>' [src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj::TargetFramework=net9.0]
• src/roslyn/src/Features/Core/Portable/ExternalAccess/Watch/Api/WatchHotReloadService.cs(218,13): error CS1503: Argument 5: cannot convert from 'System.Collections.Immutable.ImmutableHashSet<Microsoft.CodeAnalysis.Project>' to 'System.Collections.Generic.IReadOnlySet<Microsoft.CodeAnalysis.Project>' [src/roslyn/src/Features/Core/Portable/Microsoft.CodeAnalysis.Features.csproj::TargetFramework=net9.0]

The underlying SDK being used is "10.0.100-alpha.1.24555.54"

[CyrusNajmabadi]

Why did this break? Why can't an immutable hash set be passed to a read only set?

[ViktorHofer]

Given that this only happens in the source-build leg in the VMR (which uses the latest compiler and latest dependencies) I double checked whether the SBRP's Microsoft.NETCore.App.Ref/9.0.0 ref/System.Collections.Immutable.dll assembly has a wrong definition for ImmutableHashSet<T>. It correctly imports IReadOnlySet<T>:

Not sure what's going on here.

[ViktorHofer]

Oh I see what's going on. This assembly reference is used:

/__w/1/vmr/src/roslyn/artifacts/sb/package-cache/system.collections.immutable/10.0.0-alpha.1.24617.1/lib/netstandard2.0/System.Collections.Immutable.dll

and so this branch is entered: https://github.com/dotnet/runtime/blob/237c7594967573e809db40b09dded0d4a6d98cec/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableHashSet_1.cs#L20

[ViktorHofer]

This is happening because of what was discussed in #76354. Roslyn builds the NetPrevious TFM in the product source-build but its dependencies only target NetCurrent and netstandard2.0. As NetPrevious can't use the NetCurrent SCI assembly, it gets the netstandard2.0 assembly in which ImmutableHashSet<T> doesn't inherit from IReadOnlySet<T>.

From what I see, this doesn't fail in the repo source-build leg because the runtime intermediate package isn't used and hence, dependencies don't get upgraded.

cc @jasonmalinowski @jaredpar @jjonescz

[jaredpar]

I think this change is digging the hole deeper. I think we need to go back to the previous fix and do as I suggested originally which is ahve that TFM be both NetCurrent and NetPrevious.

[jaredpar]

Changes of this nature can't be merged unless we do a full VS validation insertion. This is too core of a change to go through without one.

[ViktorHofer]

Have that TFM be both NetCurrent and NetPrevious.

I thought that's what we did? Maybe I misunderstood the original change. @jaredpar who would be a good person to drive that change? Btw how would that solve the above described issue?

[jaredpar]

I thought that's what we did?

No it was changed to be $(NetPrevious) only

https://github.com/dotnet/roslyn/pull/76354/files#diff-00f5693d71dbe7582970cb797b17a1d85497c08a3e2838c8ea7ce2026526b07aR58

Btw how would that solve the above described issue?

Once everything has a $(NetCurrent) target I would assume that fixes the fact it's falling back to a netstandard2.0 target.

[ViktorHofer]

Once everything has a $(NetCurrent) target I would assume that fixes the fact it's falling back to a netstandard2.0 target.

The issue here is that as long as you target NetPrevious in the source-build product, you will get the netstandard2.0 assemblies of runtime packages. Removing NetPrevious from the list should obviously fix the issues but I don't understand how consistently adding NetPrevious to all properties would fix this.

[jaredpar]

The issue here is that as long as you target NetPrevious in the source-build product, you will get the netstandard2.0 assemblies of runtime packages.

Not sure what to do here then. Roslyn has built NetPrevious and NetCurrent in our SB TFMs for quite some time. It sounds like source build has changed such that this is not valid anymore? If so we can adjust but very unlikely to happen anytime this year. Too big of a change.

[ViktorHofer]

Not sure what to do here then. Roslyn has built NetPrevious and NetCurrent in our SB TFMs for quite some time. It sounds like source build has changed such that this is not valid anymore?

Nothing changed outside of roslyn. Runtime packages always only had NetCurrent and netstandard2.0 assemblies in their source-built packages. The change that regressed this is e75664f. It wasn't caught in your repo source-build validation because you don't use the "live" runtime packages there. But it blocked the roslyn -> dotnet/sdk insertion as the source-build leg is failing: dotnet/sdk#44874. I meanwhile submitted a VMR patch from this PR to get the insertion unblocked.

[jaredpar]

@tmat, @jasonmalinowski you all will need to decide how to handle this. At the moment the workspace layer is using NetPrevious in source build. That will end up using netstandard2.0 versions of some libraries that have different API sets. The change e75664f is hitting one of these gaps.

Overall we may want to just delete NetPrevious from full source build. That's not happening until Jan though.