-
Notifications
You must be signed in to change notification settings - Fork 1.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
How should we represent source-generators / analyzers that are off by default? #30872
Comments
I'll chime in with a gotcha around the approach that the ASP.NET generators currently use that @RussKie and I recently ran into, specifically when projects reference the ASP.NET target framework but don't reference the SDK. One example might be test projects that need to reference ASP.NET APIs without being ASP.NET apps themselves. In these cases, there is no way to disable the generator without duplicating the logic that is in the Web SDK in the project's targets. |
To me it seems like we'd probably want number 2 (having the compiler recognize that some generators are disabled by default and having a way to enable them). Otherwise we would only be able to enable and disable generators on a per-assembly basis, which seems rather limiting. |
Yeah, we can't have the disabling logic in specific SDKs. It needs to be in the same place that raises the generator - which today is the .NET SDK as it resolves ref-packs. Probably we'd put it as some knob on ResolveTargetingPackAssets.
Yeah, it could get limiting if we have a lot of them. I could also see us just say that generators with unique enabled states need to go in unique assemblies. We're pretty granular today. What say our compiler folks @jaredpar @chsienki @RikkiGibson? I'd also like some PMs to chime in here before we slide into building a new "AnalyzerReference" system. |
Think we need to distinguish the desired behavior here:
This is an important distinction because it limits the type of gestures that we can support. For example:
Pretty much all of these necessitate option (1) above cause by the time we can see these data points the decisions on what generators are available has already been made. As for the various options thrown out so far:
At some point we need to put a cap on how many DLLs we're passing to the compiler. ASP.NET basic project passes 300+ already in just references. The compiler is fast and we cache but at some point our speed gets limited by the time it takes to read things off of disk 😄 It also increases the complexity of our analyzer load strategy (more opportunities for DLLs to mix versions). Would be nice if we didn't have to be so granular here. |
How things get disabled is actually a bit more flexible than one might assume. The end result is that the generator decides not to do its thing - which can happen pretty late - exactly as late as it decides to do its thing today which can be influenced by attributes. I'm not saying this is "ideal" since it does involve some overhead for disabled generators, but then we have other overhead for untriggered analyzers, unused references, etc. Right now we have a lot of options and we're trying to get some help in deciding which is best.
Indeed, but if we start caring about that granularity and combining generators then that means we need to push the decision into the compiler or generator. We can meet to further discuss this. |
We met on this today. The decision was that initially we'll consolidate the analyzer removals in a common place in the SDK after it raises those items from the targeting packs. @captainsafia and @layomia -- can you collaborate on this and submit a PR to the SDK in preview 3 to do this? Probably it should occur here, but @dsplaisted can advise better: Line 379 in 8286b19
The properties we use to set these should have some consistent naming since those will be user visible (for the time being) and set by AOT tooling - @eerhardt can you weigh in on where to set such properties to enable the generators? @dsplaisted, per the meeting today you mentioned you'd like to sketch out what general SDK support for such functionality might look like so that we could remove those targets hacks and represent the disable loggers in the targeting packs. I'll leave this issue open to track that. |
For ASP.NET's The ConfigurationBinder source generator is a bit trickier though, since it will ship both in the ASP.NET targeting pack and in a NuGet package. The defaulting logic may need to be duplicated for that one. |
Where do we put other AOT-specific settings? Seems like the default is more about AOT + ASP.NET framework reference and less about the web-SDK. IOW: won't putting the default for EnableRequestDelegateGenerator make it a problem when someone has an app that doesn't reference the WebSDK and pulishes it for AOT? Same as @captainsafia's comment here #30872 (comment) |
What "AOT-specific settings" are you are referring to? For dotnet/runtime it is in the AOT .targets files: For trimming-specific-settings (which also get set when publishing AOT), they are in ILLink.targets: Neither of these places is good for "ASP.NET AOT-specific settings".
I'm not sure I totally get the difference. Where are we supposed to put ASP.NET's MSBuild defaulting logic, if not in the |
I'm not sure if this is what you're asking about, but the logic to disable source generators by default should go in Microsoft.NET.Sdk. That's so that (for example) a test project that references a web project won't have the source generators enabled that should be off by default. |
👍 as mentioned by @captainsafia in #30872 (comment). |
I've opened #31265 to consolidate where/how the removals happen. |
@captainsafia -- @layomia and I were looking at this yesterday and noticed that you moved the place where the generator is removed, but didn't move where We were thinking about user scenarios and it seems like this only gets set for project that both uses the WebSdk and sets PublishAot to true. Do we think that it's likely that folks would want this if they didn't set PublishAot to true, but were still part of an AOT'ed application -- for instance in a library that makes up that application? What about if the application wasn't a WebProject, but referenced web projects and got the FrameworkReference from a ProjectReference? |
Assuming that you're referring to the logic here: sdk/src/WebSdk/ProjectSystem/Targets/Microsoft.NET.Sdk.Web.ProjectSystem.targets Lines 54 to 60 in e025c37
We set it in the WebSDK to address the specific scenario of folks who are building web apps + have AoT enabled.
In this case, the user would have to set the
In this case, the generator would be off-by-default because of the logic that's been moved to the In summary, while in preview and while AoT work in underway, we only want to override the off-by-default behavior in very specific scenarios, where projects using the WebSDK where native AoT is enabled is one of them. For anything else, users will have to explicitly opt-in at the moment. |
I'm just wondering if users will expect that they need to manually toggle |
I think this falls into the same category as any trim/aot incompatible code. I could have a solution with an app project and a library project. I decide I want to set The same thing will happen for EnableRequestDelegateGenerator. If you move some |
When we first added source generators/analyzers to our shared frameworks in .NET 6.0 they were meant to be always on and used like "add-on" features. New API that was explicitly addressed through some user gesture - like applying an attribute.
Now in .NET 8.0 more generators are coming online that intend to apply some behavior to existing code with no source changes. The user's source itself does not opt-in to using the generator, but the generator will change the user's code in some way.
One example of this is: dotnet/runtime#44493
This is meant to automatically replace usage of API that is not linker safe with source generated code that is linker safe.
There may be more in the future if we come up with additional generators that are meant to change the user's code in some meaningful way.
In light of these, we should think about how we want to represent these generators and activate them / disable them.
Today the conventions for generators / analyzers in both nuget packages and the ref-packs are "always on". There is not a great convention for disabling generators passed to the compiler. See dotnet/roslyn#55518
One case where this is happening today is
sdk/src/WebSdk/Web/Sdk/Sdk.props
Lines 64 to 75 in a76a0cc
Where ASP.NET doesn't want the generator enabled by default and runs a target to remove it. This works, but requires targets be added to the SDK for each generator, along with separate handling in the nuget package (in cases of OOB generators).
Describe the solution you'd like
Some consistent way to represent and interact with optional generators. Ideally it shouldn't involve running targets.
Potential solutions:
etc
cc @eerhardt @chsienki @dsplaisted @davidfowl @captainsafia @layomia @terrajobst @stephentoub
The text was updated successfully, but these errors were encountered: