-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should Roslyn or SDK provide a mechanism to enable/disable some/all source generators? #55518
Comments
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. |
This property was a somewhat arbitrary hack off of all framework source generators / analyzers. If we do something like this, it would make more sense to have it be more targeted. https://github.com/dotnet/sdk/issues/19501
@sharwell pointed me to how Razor does this https://github.com/dotnet/sdk/blob/2fea637fd7dcc56a8d29329dd0c28de4309bd5f4/src/RazorSdk/SourceGenerators/RazorSourceGenerator.RazorProviders.cs#L17 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. |
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 |
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. |
FYI @chsienki |
I think the answer to this question is "Yes" just so that its easier for us to debug customers build issues. |
@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:
Suggested approach to disable for V1: msbuild logic
Where the fix would go:
Link below shows how analyzers from the ref pack are added to build: 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. |
@KathleenDollard @jaredpar @chsienki @terrajobst @jmarolf if you have thoughts against this proposed approach of using an msbuild property please let us know. |
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. |
As we discussed in this comment thread dotnet/docs#24896 (comment) my position is we should have a No idea about the scheduling of this work but @jaredpar and @chsienki do you have any objections to this proposal? |
My main objection to this is that it's possible this introduces build breaks. Consider that The issue with 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 |
@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.
This is a good insight. The main reason to implement |
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.
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.
We want it not just for generators owned by us. |
There are mechanisms today for disabling generators, mostly just tweaking the
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. 😄 |
The premise of my original request was that asking folks to muck with
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. |
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. |
There are essentially two disable paths to look at:
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 For (2) I think the design is essentially to mirror the |
I agree we should do (2) first and then wait for feedback / use cases to do (1) |
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. |
Yes VS 17.0 maps to .NET 6.0. The intent is for us to implement this during 17.0 Preview 5. |
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. |
I'd like to see a way to mark generators as This would allow us to have variations of the
I think this would solve @jaredpar's concerns about builds being broken by disabling generators wholesale 🙂 |
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 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 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. |
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 dotnet/docs#24896 (comment) we came to the conclusion that allowing users to toggle all framework source generators/analyzers wasn't particularly useful, and might even be dangerous.
dotnet/sdk#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
The text was updated successfully, but these errors were encountered: