dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.12k stars 4.04k forks source link

Source generator does not produce the same result when compiled with dotnet or Visual Studio 2022 #71538

Closed dlemstra closed 10 months ago

dlemstra commented 10 months ago

Description

In one of my open source projects I am using the Color class of System.Drawing to generate code with IIncrementalGenerator. But the code that is being generated is different when using dotnet build or Visual Studio 2022 to build the project. The source generator is targeting netstandard20 so this means that the Color class should not contain the static RebeccaPurple property. But for some reason the source generator thinks this colors exists when building with dotnet build and it does the correct thing when building with Visual Studio 2022. I would expect them to have the same compiled output.

Reproduction Steps

The issue can be reproduced with the code of this project: https://github.com/dlemstra/source-generator-bug. And the difference can be seen in this pipeline run: https://github.com/dlemstra/source-generator-bug/actions/runs/7447335448.

Expected behavior

The output of the test project should be:

ColorGenerator.HasColorWithName returns true for Red

Actual behavior

But the output of the project is.

ColorGenerator.HasColorWithName returns true for Red
ColorGenerator.HasColorWithName returns true for RebeccaPurple

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

CyrusNajmabadi commented 10 months ago

The source generator is targeting netstandard20

This isn't relevant. That's what the code of the source-generator targets. What the generator then produces is compiled however the rest of the user's code is compiled.

So this is by design :)

sharwell commented 10 months ago

@CyrusNajmabadi in this case the issue is slightly different, although the resolution is still the same. The source generator is using reflection to probe the type system at runtime while the source generator is executing.

For @dlemstra , the solution here is to not use reflection to access members of Color. The following code isn't going to do what a user expects: https://github.com/dlemstra/source-generator-bug/blob/42d1cdc24d2b8d1fa8f08c2cb334c0547ab7af30/source-generator/ColorGenerator.cs#L62-L68

dlemstra commented 10 months ago

Can you explain in more detail why this doesn't work? Or how I could use a source generator to generate a copy of the System.Drawing.Color properties?

sharwell commented 10 months ago

Can you explain in more detail why this doesn't work?

When you compile code for netstandard2.0, a specific set of properties is available because they are defined in a netstandard.dll assembly that is passed to the compiler. At runtime, a different (newer) assembly is provided which is known to contain at least the items that were visible to the compiler, but might contain more members as well.

@terrajobst do we have any public documentation explaining how a property can be omitted from the reference assemblies used by a build, but then still be present at runtime for reflection?

Or how I could use a source generator to generate a copy of the System.Drawing.Color properties?

If you want to make the generator behave consistently, you can hard-code the known list of colors, or use the set available through IncrementalGeneratorInitializationContext.CompilationProvider and/or IncrementalGeneratorInitializationContext.MetadataReferencesProvider.

dlemstra commented 10 months ago

Thanks for the explanation and using those properties of IncrementalGeneratorInitializationContext sound like a better way to do this. Might be a good idea to document somewhere that the runtime used by the source generator can be different when using dotnet or VisualStudio. But I also understand that this really is an edge case.

sharwell commented 10 months ago

Might be a good idea to document somewhere that the runtime used by the source generator can be different when using dotnet or VisualStudio.

New documentation for this scenario will be included once https://github.com/dotnet/roslyn-analyzers/pull/7116 is merged. I'm not sure if there are other locations where it is also documented.

terrajobst commented 10 months ago

As Cyrus said, the generator, which is part of the compiler, is targeting .NET Standard. However, there is no such thing as a .NET Standard runtime, you will always run on a specific .NET runtime that implements .NET Standard. Implementing the standard means having to support all of its APIs. However, a .NET runtime can also offer more APIs. This can be additional assemblies, additional types, or even additional members on types that are part of the .NET Standard. If you use runtime reflection, you'll see which APIs the .NET runtime exposes which is generally a superset of what .NET Standard has. In other words, using reflection in a .NET Standard library isn't artificially limited to what is available in .NET Standard only, you'll see the true runtime representation, which will depend on the consumer of your library and this will typically vary based on which runtime happens to execute your code.

This is also why you see different results in Visual Studio vs dotnet build: Visual Studio is running on .NET Framework, dotnet build runs on .NET Core (~simplified, as both have a way to run things out of proc which might use different runtimes, but that's not relevant here)

In your source generator you're using reflection to determine which colors are available:

    private static bool HasColorWithName(string name)
    {
        var type = typeof(Color);
        return type
            .GetProperties(BindingFlags.Public | BindingFlags.Static)
            .Any(property => property.PropertyType == type && property.Name == name);
    }

You shouldn't do that as that confuses what the compiler is running on vs. what the user is compiling for. For example, the C# compiler may run on .NET Framework but compile code for .NET Core. Instead, you should use the Roslyn API to resolve the ITypeSymbol for System.Drawing.Color and then ask which members it exposes. This ensures you see the types throught the lens of the user's code.

terrajobst commented 10 months ago

@sharwell have we considered having an analyzer for source generators that calls out reflection APIs? Yes, in principle someone might do something smart with reflection APIs, but I'd claim that 99% of reflection usages inside of a source generator are probably a bug.

dlemstra commented 10 months ago

I think that is a fair point @terrajobst. I did not realize I could also use the roslyn api to resolve the ITypeSymbol to get all the available Color values. Having an analyzer that prevents people (including me) from making this mistake sounds like a good plan. The error it reports should then point to a document that explain why you should not do that and link to example of what someone should do instead.

terrajobst commented 10 months ago

The error it reports should then point to a document that explain why you should not do that and link to example of what someone should do instead.

Agreed

jaredpar commented 10 months ago

@sharwell have we considered having an analyzer for source generators that calls out reflection APIs

There is an analyzer for analyzers / generators that has a banned API list.

https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/Core/AnalyzerBannedSymbols.txt

At the moment that list is composed of APIs that either fundamentally:

  1. Break build correctness
  2. Break the rules of analyzers (no IO, no assembly loading, etc ...)

Reflection is a bit of a middle ground. It's not fundamentally wrong. Using it over a DLL that was an input to the compiler would be permissible (please use the symbol API for that though). It's not great but it's not fundamentally wrong. Reflecting into DLLs that are in the runtime process though is also not 100% wrong but I'd be highly suspicious of such code. I wouldn't be opposed to putting some of the reflection entry points into that list but it does represent a bit of a change.