Should Roslyn or SDK provide a mechanism to enable/disable some/all source generators?

[ericstj]

User's might need to disable a source generator for some reason (like a performance or compatibility issue). In general we should fix those and it shouldn't be a very common occurrence, however we may want to have a mechanism available as a mitigation.

In writing up the docs for https://github.com/dotnet/docs/pull/24896#discussion_r661102362 we came to the conclusion that allowing users to toggle all framework source generators/analyzers wasn't particularly useful, and might even be dangerous.

https://github.com/dotnet/sdk/issues/1212 means that analyzers/generators from packages can't easily be excluded either.

Should we have targets in the SDK/MSBuild that formalize a file-based exclusion mechanism? Should roslyn support some exclusion mechanism itself (through compiler parameters, editorconfig, etc)?

cc @jaredpar @chsienki @terrajobst @KathleenDollard @jmarolf

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ericstj]

@sharwell pointed me to how Razor does this https://github.com/dotnet/sdk/blob/2fea637fd7dcc56a8d29329dd0c28de4309bd5f4/src/RazorSdk/SourceGenerators/RazorSourceGenerator.RazorProviders.cs#L17 cc @captainsafia

That's pretty cool that we could use .editorconfig to make generator-specific settings. Perhaps one thing we could do here is make all generators read this and and define their own setting to enable/disable.

[captainsafia]

Yep -- we use the flag to disable the source generator under different scenarios, like in VS 2019 where the incremental source generator API is not supported.

Speaking of incremental source generators, there's still some work pending their to make it easier to disable a generator in the runtime. What we currently implement is a bit of a "hack" on the generator side.

cc: @chsienki

[ericstj]

One thought is that we could have a property or item like "DisabledSourceGenerators" and pass that in via https://github.com/dotnet/roslyn/blob/2946c619d91c7279d69d71bbb521ea4647e29fc1/docs/features/source-generators.cookbook.md#consume-msbuild-properties-and-metadata then have all generators honor that in their Initialize methods. Seems like it'd be better if roslyn provided something, that way we wouldn't rely on folks all having this common code.

[jcouv]

FYI @chsienki

[jmarolf]

I think the answer to this question is "Yes" just so that its easier for us to debug customers build issues.

[ericstj]

@maryamariyan and @sharwell were discussing this. We'd like to see something land for 6.0 if possible.

[maryamariyan]

@maryamariyan and @sharwell were discussing this. We'd like to see something land for 6.0 if possible.

We chatted about identifying a way to granularly disable source generators (VS2019 story for V1 source generator).

Key takeaways:

• Source generators can theoretically disable themselves in either of these two cases: (1) V2 Source generators or (2) V1 source generator which doesn't use a SyntaxReceiver.
• For V1 source generators with SyntaxReceiver, they have to be disabled in another way. Even if the generator is disabled the syntax receiver will run accounting for the slow part.
• For V2, we have other approaches to disabled (e.g. SuppressRazorSourceGenerator) but we may not need to consider disabling for V2 at all because there are no perf concerns there.

Suggested approach to disable for V1: msbuild logic

• For a given assembly that only contains the source generator, we make sure it recognizes a build property that when set, it removes it from source generators that can get used.

Where the fix would go:

• Our Microsoft.Extensions.Logging and System.Text.Json source generator are both shipped as part of nuget and part of ref pack. So whatever fix we have would need to be available for both.

Link below shows how analyzers from the ref pack are added to build:

https://github.com/dotnet/sdk/blob/05372f900f081f25ac618fe6ca50e42e703483e8/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FrameworkReferenceResolution.targets#L347-L380

We would need to have a target that includes them with condition or remove them once added using the new msbuild property we end up introducing.

cc: @eerhardt @ericstj

[maryamariyan]

@KathleenDollard @jaredpar @chsienki @terrajobst @jmarolf if you have thoughts against this proposed approach of using an msbuild property please let us know.

[jaredpar]

We chatted about identifying a way to granularly disable source generators (VS2019 story for V1 source generator).

I can't tell if this proposal is for the generators owned by us (essentially first party generators) or if it's meant as a more general proposal for all generators.

When you think about "all generators" there are other complications to consider. Mostly that customers can, and do, bundle multiple products into a single DLL. Namely a generator, analyzer and suppressor can all be in the same DLL. Hence the idea of a general mechanism being "remove the analyzer DLL from the build" is not going to work because removing a suppressor can introduce build breaks.

[jmarolf]

As we discussed in this comment thread https://github.com/dotnet/docs/pull/24896#discussion_r661102362 my position is we should have a SkipGenerator option that has an identical design to SkipAnalyzers from here. Generators are part of build correctness and I do not think any developer would have (or should be expected to have) the knowledge for how to disable them (much less selectively disable them).

No idea about the scheduling of this work but @jaredpar and @chsienki do you have any objections to this proposal?

[jaredpar]

My main objection to this is that it's possible this introduces build breaks. Consider that /skipAnalyzers used to skip both analyzers and suppressors. After massive amounts of feedback though we had to pull back and only skip analyzers. The reason being that many customers have warnings promoted to errors and were relying on suppressors to remove warnings. Hence by adding /skipAnalyzers new warnings popped up and that blocked the build.

The issue with /skipGenerators is that not all generators are optional like the JSON generator (allows for optimizations). Instead there are many, like the C# syntax generator, which are required for compilation to succeed. It's likely that many of them will end up falling into this bucket. Hence it seems this option is more likely than not going to cause compilation errors when invoked.

Think there needs to be some sort of balance here for this to work. Essentially being able to differentiate between required generators and optional generators. Otherwise this flag won't be useful in practice

[jmarolf]

@jaredpar I agree that this is not as essential as /skipAnalyzers. essentially this is a diagnostic tool for us to use. I wish this was just an msbuild concept but there is not way to distinguish at an MSBuild level what a generator is from item metadata.

Think there needs to be some sort of balance here for this to work. Essentially being able to differentiate between required generators and optional generators. Otherwise this flag won't be useful in practice

This is a good insight. The main reason to implement /skipAnalyzers was to automatically elide analyzers in places where they were not needed for build performance. Perhaps we have some MSBuild convention where generators that are not required are added to a particular item group and not passed to the compiler if a property is present

[maryamariyan]

Essentially being able to differentiate between required generators and optional generators. Otherwise this flag won't be useful in practice

Even for a required generator, if not used by a particular project, (e.g. STJ) they should be able to selectively disable if they want to. Not expecting every developer to deal with, but a help to mitigate things for troubleshooting if they want it disabled.

When you think about "all generators" there are other complications to consider. Mostly that customers can, and do, bundle multiple products into a single DLL. Namely a generator, analyzer and suppressor can all be in the same DLL. Hence the idea of a general mechanism being "remove the analyzer DLL from the build" is not going to work because removing a suppressor can introduce build breaks.

We have seen this request come up multiple times during the release, so would be good to address in some widely acceptable way. (Either by build level or compiler level), type level granularity could be something that we could perhaps do in the compiler level.

I can't tell if this proposal is for the generators owned by us (essentially first party generators) or if it's meant as a more general proposal for all generators.

We want it not just for generators owned by us.

[jaredpar]

even for a required generator, if not used by a particular project, (e.g. STJ) they should be able to selectively disable if they want to. Not expecting every developer to deal with, but a help to mitigate things for troubleshooting if they want it disabled.

There are mechanisms today for disabling generators, mostly just tweaking the $(Analyzers) item group. That can be used to remove any generator (or analyzer / suppressor). What is the motivation for adding a second switch here? Is it just that we want a switch to disable all generators period for the customer that never uses them?

We have seen this request come up multiple times during the release, so would be good to address in some widely acceptable way.

In several cases though had the request been granted it would've lead to build breaks 😦 That is the difficulty of a switch like this, it's one which is not necessarily just a force for good. Flipping it can create other problems.

If the idea is to have a switch which just completely disables generators, and only generators, and the idea is that it is for the customer that does not want generators to be running. Then that is a relatively straight forward item to consider.

At the same time it also goes against previous asks from the runtime team where they'd prefer we error the compilation if their generators did not run. 😄

[ericstj]

The premise of my original request was that asking folks to muck with Analyzers via a target might not be something we want to do if we have to do it very often. I think that's still up for debate. We have seen evidence so far that there are various reasons that folks want to do this and asks for a specific feature to enable/disable specific generators.

At the same time it also goes against previous asks from the runtime team where they'd prefer we error the compilation if their generators did not run

We asked for the ability to design a generator in such a way that when code is written that requires it, compile will fail if the generator is not present. This request is complementary, as it would let folks know that they needed the thing they disabled.

[maryamariyan]

cc @KathleenDollard @terrajobst the PMs to check if they agree this as being a valuable thing to spend time/try to resolve.

Today a developer could write targets if they need to disable generators during design time build. Something like:

<Project>
    <Target Name="DisableAnalyzers"
     BeforeTargets="CoreCompile"
     Condition="'$(DesignTimeBuild)' == 'true'">
        <ItemGroup>
            <Analyzer Remove="@(Analyzer)" Condition="'%(Filename)' == 'Microsoft.NET.Sdk.Razor.SourceGenerators'" />
            <Analyzer Remove="@(Analyzer)" Condition="'%(Filename)' == 'System.Text.Json.SourceGeneration'" />
           <Analyzer Remove="@(Analyzer)" Condition="'%(Filename)' == 'Microsoft.Extensions.Logging.Generators'" />
        </ItemGroup>
    </Target>
</Project>

If this is not a valid user experience, we should further investigate how and when to come up with a widely acceptable mechanism to disable generators.

[jaredpar]

If this is not a valid user experience, we should further investigate how and when to come up with a widely acceptable mechanism to disable generators.

There are essentially two disable paths to look at:

1. Individual generator disable
2. Disable all generators

For (1) users can opt out using effectively the work around you provided above. Personally I'd want to see some very strong user feedback before we designed /skipGenerators to effectively take an argument. The MSBuild syntax for that is going to only be marginally better than what you have above.

For (2) I think the design is essentially to mirror the /skipAnalyzers switch. Effectively add an MSBuild property called $(SkipGenerators) and translate that to the command line switch /skipGenerators. The compiler would then be responsible for ensuring generators don't run from the provided libraries but would run analyzers and suppressors.

[jmarolf]

I agree we should do (2) first and then wait for feedback / use cases to do (1)

[maryamariyan]

Seems like there is consensus on adding mechanism to disable all source generators, and to wait until there's further feedback for disabling individual generators.

[ericstj]

@jaredpar / @chsienki -- will this be part of the .net 6.0 / roslyn 4.0 release? I think that's what the milestone 17.0 means but I wanted to confirm. Thanks!

[jaredpar]

Yes VS 17.0 maps to .NET 6.0. The intent is for us to implement this during 17.0 Preview 5.

[jaredpar]

Note: at this point we're thinking of pushing this out to 17.1. It's just getting close to the lock down date and there are work arounds customers can do in 17.0 to remove generators using existing syntax. Hence we want to prioritize other generator bug fixes over this.

Still intend to do the work, just want to push it to 17.1 instead of 17.0.

[Eli-Black-Work]

I'd like to see a way to mark generators as required.

This would allow us to have variations of the /skipGenerators flag:

• /skipGenerators=optional
• /skipGenerators=required
• /skipGenerators=all

/skipGenerators could default to /skipGenerators=optional.

I think this would solve @jaredpar's concerns about builds being broken by disabling generators wholesale 🙂

[jaredpar]

I'd like to see a way to mark generators as required.

My mental model of required is a generator that must run else the code will not compile. For example any generator that fills out a partial member. An optional generator is one that just improves the performance of existing scenarios. When not run the code has acceptable fallbacks that allow it to remain functional.

The vast majority of generators fall into the required category. I can only think of 1-2 that actually fall into the optional category. As such I do not think we'd ever go down a path where we mark generators as required because that is the assumed default. Instead if we ever went down this path it would be to have generators be marked optional.

I think this would solve @jaredpar's concerns about builds being broken by disabling generators wholesale 🙂

I also want to note that it's been a year since this was filed and we have not found a concrete scenario for needing this flag. Yes we've had a few cases where generators have interferred with compilation: either via bad performance or crashing. In every case though we've been able to either correct the bug on our side or work with the generator author to correct it on their side.