SourceGenerators in shared compilation can cause MissingMethodException

[hymccord]

Version Used:
Visual Studio v17.5p1
Roslyn in .NET 7.0.101 and in .NETFramework 4.7.2 (the one that ships with VS)

Steps to Reproduce:

> mkdir CTRepro
> cd CTRepro
> dotnet new console
> dotnet add package CommunityToolkit.Mvvm -v 8.1.0-preview2
> dotnet add package Microsoft.Windows.CsWin32 -v 0.2.164-beta

then add a new .cs file so the CommunityToolkit SourceGenerators will run

using CommunityToolkit.Mvvm.Input;

namespace CTRepro;

internal partial class Model
{
    [RelayCommand]
    private void Execute() { }
}

Then you can either build in VS or use msbuild -restore /t:Rebuild to trigger the warning

CSC : warning CS8785: Generator 'RelayCommandGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'MissingMethodException' with message 'Method not found: 'System.ReadOnlySpan`1<!0> System.Collections.Immutable.ImmutableArray`1.AsSpan()'.' [C:\Users\hank.mccord\source\repos\CTRepro\CTRepro.csproj]

Expected Behavior:

The generated code should include the ICommand that is generated

Actual Behavior:

Because the source generator encountered an error, the associated IL is missing entirely.
If you build with dotnet build, it works as expected.

Investigation/Cause

A couple things collide to cause this:

• Using msbuild or Visual Studio uses the full framework MSBuild (i.e. no ALCs) under a shared compilation context
• CsWin32 is shipping System.Memory in their analyzers directory
• VBCSCompiler eventually loads System.Memory in the LoadFrom context (even though System.Memory is in the probing path)

I hope this enough information. Someone who is very familiar with assembly loading process should get the gist of what's going on.
The .NET version of roslyn has the names of simple assemblies that will be provided in by the compiler but the .NETFramework has a completely different loading process since there are no ALCs.

Is this a concern to the Roslyn team or should the fix be CsWin32 package not shipping Roslyn provided dlls in their analyzer folder?

[Update(jcouv):] This issue is referenced by some skipped tests.

[jaredpar]

I'm not able to reproduce this behavior. I've followed the steps outlined and built with a couple of different versions of msbuild and can't seem to hit this. Do you have a binary log from the build you can share out? If so I should be able to spot the difference between our setups.

[jaredpar]

Never mind I can repro the problem now. The problem was I failed to save before trying to build 🤦

[jaredpar]

@AArnott, @RikkiGibson, @jjonescz FYI

I debugged into this a bit this morning and here is what is happening.

1. Microsoft.Windows.CSwin32 is including System.Memory in it's analyzer folder that means it is passed to the compiler via /analyzer
2. The compiler loads everything passed via /analyzer, without condition, into the LoadFrom context.
3. The result of 1 + 2 is that we end up with two copies of ReadOnlySpan<T> in the process: one in the default context and the other in the LoadFrom context.
4. There is only one copy of System.Collections.Immutable though, it exists in the default context and hence uses the ReadOnlySpan<T> from there.
5. When the CommunityToolkit.Mvvm calls ImmutableArray<T>.AsSpan it expects the return to be ReadOnlySpan<T> from the LoadFrom context (that is where it's dependency is loaded) but gets the one from the default context because that is where ImmutableArray<T> has loaded it and this leads to the exception

There are not a lot of great options for fixing this on the compiler side. After all the reference was passed via /analyzer hence we should be loading it. I understand though it gets a bit complicated in cases like this where the dependency conflicts with items we're deploying in the compiler.

One approach we could take is ignoring dependencies that have simple names that match compiler dlls. That heuristic worries me a bit though about what corners it could open up.

[RikkiGibson]

3. one in the default context and the other in the LoadFrom context.

I'm a little confused here. I thought when you build from VS, the compiler runs on netframework. That means there are no load contexts. Right?

It sounds like there's an inconsistency between the System.Collections.Immutable referenced by an analyzer and the one referenced by the compiler. The analyzer's version says that ReadOnlySpan comes from assembly A, while the compiler's version says ReadOnlySpan comes from assembly B. Right? We already have a warning for when an analyzer references a newer compiler version than the compiler actually hosting it. Can/should we somehow extend this warning to the analyzer's dependencies?

[Sergio0694]

Just double checking here - this is something that would either be fixed by Roslyn or by CsWinRT, but not something that's related to the MVVM Toolkit per se, right? I'm wrapping up for the 8.1 release so just want to make sure there isn't anything wrong on our part there that should be addressed. Am I right to consider this as just an external issue which just so happens to repro when using the MVVM Toolkit now? Also, thanks everyone for investigating this! 😄

[hymccord]

I'm a little confused here. I thought when you build from VS, the compiler runs on netframework. That means there are no load contexts. Right?

.NETFramework is the one with load contexts. The 'Default' context, 'LoadFrom' context, and the 'No' context (not relevant here).

Jared got the to same conclusion I saw. Types aren't equivalent across load contexts.

[hymccord]

One approach we could take is ignoring dependencies that have simple names that match compiler dlls. That heuristic worries me a bit though about what corners it could open up.

This is essentially what happens in the .NET version though, yes (seen here)? If it's a compiler provided dll, then it's loaded in the compiler's ALC. It's just that the .NETFramework version take a different approach.

[jaredpar]

I'm a little confused here. I thought when you build from VS, the compiler runs on netframework. That means there are no load contexts. Right?

There are no AssemblyLoadContexts but there are load contexts on .NET Core. The names make this confusing 😦

Essentially on .NET Framework there is an internal partitioning of assembly loads into different context within an AppDomain. It's similar to AssemblyLoadContext but not quite the same because it's not programmable, extensible, etc ... It's just implicit from how an assembly was loaded. This blog is my goto source for getting familiar with them

https://learn.microsoft.com/en-us/archive/blogs/suzcook/choosing-a-binding-context

It sounds like there's an inconsistency between the System.Collections.Immutable referenced by an analyzer and the one referenced by the compiler. The analyzer's version says that ReadOnlySpan comes from assembly A, while the compiler's version says ReadOnlySpan comes from assembly B. Right?

Not quite. The analyzer and compiler agree on System.Collections.Immutable but disagree on System.Memory even though they are referencing the same version. Essentially System.Memory is loaded twice: once in the compiler context and once in the analyzer context. That means there are two copies of ReadOnlySpan<T>.

Now when the analyzer goes to call AsSpan it is specifying a signature using the analyzer definition of ReadOnlySpan<T> while the ImmutableArray<T> uses the compiler definition of ReadOnlySpan<T>. These are different types as they come from different assembly (same assembly in a different context is a different assembly) and that leads to a MissingMethodException

This is essentially what happens in the .NET version though, yes (seen here)? If it's a compiler provided dll, then it's loaded in the compiler's ALC. It's just that the .NETFramework version take a different approach.

That is what we do with .NET core but I'm hopeful we can delete it there. There are some corner case issues with that approach that I think we could solve with a slightly different approach in AssemblyLoadContext

[RikkiGibson]

.NETFramework is the one with load contexts.

So you're saying .NET Framework has a concept of load contexts..for loading assemblies.. which is entirely unrelated to .NET Core's concept of assembly load contexts? 🙁

https://learn.microsoft.com/en-us/dotnet/framework/deployment/best-practices-for-assembly-loading

[RikkiGibson]

Now when the analyzer goes to call AsSpan it is specifying a signature using the analyzer definition of ReadOnlySpan<T> while the ImmutableArray<T> uses the compiler definition of ReadOnlySpan<T>.

OK. So the metadata signature for AsSpan() that the analyzer bound to at compile time is not meaningfully distinguishable from the one present at runtime. This is really just a case of us blindly loading everything, even assemblies whose simple names are "owned" by the compiler.

I would agree that the solution is probably to share the set of "compiler assembly simple names" on the Framework side, and when such assemblies are found in /analyzer to essentially ignore them and load the compiler's version instead.

I am also wondering, though, if it is essentially a bug that CsWin32 is shipping "compiler assemblies" when the compiler host is always going to provide the version that needs to actually be used in order for things to work. For example, should it be a warning at analyzer packaging time or similar if assemblies with certain simple names are found in the directory.

There are some corner case issues with that approach that I think we could solve with a slightly different approach in AssemblyLoadContext

I wasn't aware of this, and perhaps I should know more about this in order to understand whether the warning I suggested above would be appropriate/useful.

[jaredpar]

I would agree that the solution is probably to share the set of "compiler assembly simple names" on the Framework side, and when such assemblies are found in /analyzer to essentially ignore them and load the compiler's version instead.

The part I don't like about our .NET Core solution is the hard coding of names. Yes we have checks to make sure it's consistent but it still feels off a bit for me in ways that are hard to quantify.

What I was thinking about for .NET Framework was along these lines.
jaredpar@9d716e1

My first version had none of the checks for name or file location. I wanted it to be essentially if the host owns this name then always load from the host. That leads to lots of runtime exceptions though because we end up probing for every single assembly and that felt off. The check and then load feels a bit better.

I am also wondering, though, if it is essentially a bug that CsWin32 is shipping "compiler assemblies" when the compiler host is always going to provide the version that needs to actually be used in order for things to work.

Strictly speaking yes I consider it a bug but I'm also sympathetic to the push back of "okay, so tell me how to not ship the assembly". The problem I see generators like CSWin32 facing is they have some dependencies and those bring in dependencies transitively that are shared with the compiler. So to be compliant with the "don't ship compiler assemblies" idea these generators have to know what assemblies we ship, across all versions of the compiler, and prune them out. That's not realistic I think.