-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Obsoletions with a DiagnosticId still get warnings in generated code #101738
Comments
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
Tagging subscribers to this area: @dotnet/area-extensions-configuration |
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
@eiriktsarpalis I marked this for .NET 9.0. Let me know if you want to change that. Thanks! |
Maybe Roslyn would need to add support for |
One possible approach that was considered in the past is exposing a |
One concern about that is we would need to do that for every source generator that gets/sets user defined properties. And also there are some source generators (like the ConfigurationBinder) that don't have an "Options" API where these things could be placed.
This seems like an interesting idea. A blanket "suppress obsoletions" statement. cc @RikkiGibson - who implemented Customizable Obsolete diagnostics (dotnet/roslyn#42518) With other Roslyn analyzers, you can opt-in/out of running the analyzer on generated code. But since this one is built into Roslyn itself, I'm not sure how to tell it stop. |
A pseudo-diagnostic-id similar to One rather ugly workaround here might be to put the usages of the "custom obsolete" members in an obsolete context, which does have the effect of blanket suppressing obsolete diagnostics. (seems like this also shows that compiler knows how to blanket suppress the diagnostics already.) SharpLab using System;
public class User
{
[Obsolete(DiagnosticId = "A")]
public string? Prop { get; set; }
}
[Obsolete]
public class Gen {
public void M(User user) {
user.Prop = "B";
}
} |
BTW, it's also possible to disable all warnings using |
Tagging subscribers to this area: @dotnet/area-extensions-configuration |
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
This issue applies to both the Configuration Binder and the System Text Json source generators. |
Should we log issue for Roslyn regarding that? |
Each issue/PR should only have one label. It might belong to
I have opened dotnet/roslyn#73292. |
Tagging it with |
Something I've been thinking about recently is that source generators should be disabling warnings altogether in generated code, since they're not actionable at all when surfaced to users. We could keep them on for debug builds of the source generator to catch bugs at development time. It could be said that this approach will result in bugs not being reported, but I would argue that it's not important unless the warning condition results in actual functional bugs that the users report to us. |
Can't the user utilize |
They're not actionable by users, but they're indicators to the generators that they're using C# incorrectly. |
I addressed this in the top comment:
|
The fact that they're not actionable makes it a pretty poor experience from an end-user perspective. Arguably it should only be a SG bug if the incorrect use of C# results in incorrect functional behaviour of the generated code. |
I def do not want generators to be able to suppress warnings automatically. If they're emitting something with a warning I want to know so I can assess the situation. |
You're referring to: Why shouldn't the source generator be able to disable warnings on things it itself is generating? Just as it can today with a pragma in the generated code. Lots of source generators, including ours, use #pragma warning disable in the generated code. |
I don't have any issues with generators suppressing specific warnings. I do have issues with a blanket "let's automatically suppress all warnings". That seems like a recipe for letting bad code silently enter generators. |
Consider this exact bug as an example of why we should be emitting in production builds. There is a behavior that the generator author is unaware of. Lacking warnings in production builds the author would never know about it. Yes in this case the response is "we could ignore that". That will not always be the case. |
Is there a similar issue here when the user types include |
I have worked for employers who have policies of treating warnings as errors. Warnings from generated code would absolutely break them, but they would still consider blanket-ignoring even specific warnings a bad idea because they don't want their devs writing code that causes warnings. I concur that automatically ignoring warnings from generated code is a good thing. The only alternative I can see is adding a project property that allows an opt-in or -out for warnings on generated code. |
Imagine the warning is coming from a security based analyzer. Automatically suppressing that just because it's generated code is almost certainly the wrong move and would result in uncomfortable conversations. |
I think warnings in the compiler are fairly keyed toward: if we are reporting this, something is almost certainly wrong, and the generator author should think about what's happening when those warnings occur. It's possible that suppression is the right thing to do, but not necessarily. Analyzer authors have the ability to configure whether the analyzer runs on generated code or not. And it seems to me like if the analyzer is indicating the intention to run on generated code, the generated code should respect that and not attempt to suppress diagnostics in it--at least, again, not without reasoning out why the suppression is the right thing to do. It also feels reasonable to make sure we have a story for users who get a diagnostic in generated code which they don't own and they need to get unblocked. Maybe we should make sure the user can write an editorconfig which refers to the path of the generated code (the generated file has a "path" according to the compiler even if it is not written to disk), and adjust behavior of diagnostics that way. Roslyn also has a concept of "diagnostic suppressors". While it might not be the cleanest, the JSON generator might be able to solve its problem by shipping a suppressor which notices that a diagnostic is for an obsolete/experimental/etc. property that the user is asking us to generate a serializer for, and suppresses the warning on it accordingly. (BTW, yet another possibility I didn't see mentioned: It might be possible that the right thing to do is actually to attribute the generated methods related to the obsolete/experimental/etc. type with the same obsolete attributes being used on the user members. This suppresses the warnings in the implementation of these methods and forwards the question of how to handle the obsolete-ness/experimental-ness to the user.) |
For obsolete / experimental specifically I was mulling over whether we should consider a named group suppression (as detailed in 73292). #pragma warning disable obsolete
#pragma warning disable experimental Given how configurable these types of warnings are I understand how that is an attractive idea. There is some symmetry with nullable in the idea there of just want to ignore warnings from this group. At the same time it also means that generators can essentially use The named group for nullable was done specifically because nullable warnings can't impact code semantics (it's part of the core design principals). Hence suppressing in bulk is not going to have adverse effects on the code. I'm unsure if obsolete falls into the same category because of scenarios like
Very much agree with this. It can be done today with .globalconfig but that's a bit clunky. Suppressions there apply to all source, not just generated one. Path based suppressions can be used in .editorconfig to suppress in generated files but it's imprecise. Have to use sections like
I continue to strongly disagree with this viewpoint. Source generators need to have the right tools in order to produce code that compiles cleanly. Just like with hand written code that is going to occasionally involve some amount of But I cannot get behind the idea of disabling all warnings in generated code: either implicitly or with a single switch. Warnings exist to alert users to potential issues in code. The design of the language is that such warnings need to be explicitly dealt with: either by fixing the code or by suppressing the warning. This is not just the feelings of the language, it's also part of our general security posture. Over the last few years we've taken several changes to the SARIF logger to better surface what diagnostics ran in a build, and which diagnostics were suppressed and how were they suppressed. This was at the request of various security teams to ensure they can better audit security analyzers in build: making sure they're not disabled entirely, individual suppressions are audited, etc ... Having the compiler silently suppress all warnings does not really fit into that model and I have a hard time seeing it being accepted. |
I strongly agree with this. Some analyzers already choose to disable them in generated code. Others continue to run. I think this should be an analyzer decision. |
That's already possible, no? The source generator would just emit |
Correct they can do that. But that is an explicit suppression. That would show up if you were auditing code, looking at a SARIF log, etc ... It gives organizations the capability to make policy decisions around that diagnostic. Where as with |
I'd expect such code auditing to not include Roslyn source generated code, since it's generally not merged into a repo. And what stops the C# compiler from logging for |
Agree that users won't reasonably audit this. Tooling though does perform these audits and they do consider generated code.
This is an approach we could take. There are some issues with SARIF we'd need to work out. It's a bit of a twist to the existing use cases: single item suppressing groups of warnings [note 1][nrt] . Should be fine though. Again though, for obsolete / experimental I'm on the fence with whether we should do this. Can also see arguments for this should be a warning. After all the user said "this obsolete" and then did an action where we generated code to use it. Was that intentional? What if it wasn't and now we're silently consuming a member they didn't want used. I can convince myself both ways on this one. If we ended up deciding as a group the That is not true for other classes of warnings. Bulk suppressing all warnings is not an approach I can see working. [nrt]: Yes there is already a group disable for NRT. As I mentioned earlier though these warnings never impact program semantics so it's never come up as a concern for auditing. |
I agree we shouldn't disable all warnings in generated code. That seems going too far.
The JSON source generator already decided it should disable any "obsolete" warnings inside the code it generates. That's why it adds a runtime/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs Lines 140 to 141 in 6c3245e
This issue is that some obsoletions use their own diagnostic ID, and this list can't be statically known ahead of time. The JSON source generator could inspect every property it is going to reference and keep a list of the obsoletion IDs and then suppress them (either at the top of the file, or around each usage), but that would need to be done for every source generator that needs this behavior. The logical reason why it is OK to suppress these warnings is because the source generator wants to match the reflection based logic. The reflection based logic happily uses Obsolete (and experimental) properties without warnings. The source generator should behave the same. |
Is it possible we can have a helper method which can get the instances of all obsoletion attribute references in the compilation and from these can get all diagnostics Ids? I don't think this needs to be checked for every property for such attribute. |
re: suppressing BinaryFormatter. It seems like if we suppressed these, the SARIF logs will still show that the diagnostic for using it is present but suppressed. Right? In this case, I think anyone auditing will have all the information they need about what is going on. re: a helper method to get obsolete attribute usages: I think enumerating the set of obsolete diagnostics that could occur, prior to generating source, is going to be more computationally expensive than introducing the ability to blanket-suppress all obsolete diagnostics when they occur, because it may force us to bind attributes which we otherwise wouldn't have bound in the "primordial compilation". It also might be tricky to do correctly in all scenarios. I paged this area in a little more and recalled that all obsolete diagnostics with a custom ID have a well-known entry in CustomTags. I think that writing a DiagnosticSuppressor which suppresses all obsolete diagnostics would be very simple using this. |
It seems there are a couple of approaches (in order of impact):
Personally, I think (1) makes the most sense and has the least amount of moving pieces. @jaredpar's argument makes sense, but the problem I see is that in practice many developers will simply choose to ignore them anyway. And if their only recourse is to turn it off globally, well, that's what they are going to do. At least that's our experience and originally prompted custom diagnostic IDs to begin with. Personally, I'd rather a source generator turns off all obsoletion warnings for its generated code than developers suppressing specific obsoletion diagnostics globally, just to suppress in generated code. However, if we truly believe the developer should issue the suppressions, not the generator, then I think we'd want a generalized framework to let the user pass them in. This can either be a global context ( I don't like the idea of handling this in each and every source generator, either by detecting obsoletions and adding suppression or by configuration. Both feel error prone and/or complicate the developer experience around the feature that uses the generator. And I really don't like the last recourse (4) because it undermines the goal of making the developer aware when they use obsolete members in code they write. |
I don't really see this as a long term option. Yes today the only way to control diagnostics in generated code is globally through a .globalconfig file. That is not a good answer. The general mentality of the compiler is that if there is a warning in generated code that is likely a bug the generator needs to address and over the long term users shouldn't be suppressing these warnings. In the short term while the generator is working on a fix it would be nice if the diagnostic could be suppressed for the generated code only. @chsienki was going to open an issue for us to track potentially adding that in the future.
If we took action here it would not be to suppress diagnostics only in generated code. It would be geared towards letting diagnostics be suppressed in the generated code for a specific generator. Suppressing in all code leads to cases where you turn off a warning in Generator A, but then Generator B then silently introduces the same bug.
If we take this approach it will be the big hammer style. Basically disable all obsolete diagnostics including ones we'd probably want like
It unfortunately has a number of moving pieces as well. Designing the SARIF angle will be interesting and likely a non-trivial amount of work. |
Does this mean we agree on the direction (i.e. support |
I think it's the most promising path at this point. |
Moving to 10.0.0, seems unlikely we'll get to a solution in 9. |
@jaredpar @eiriktsarpalis should this issue be moved to dotnet/roslyn? Looks like we concluded this should be a compiler feature. |
It feels reasonable to close this out and track with dotnet/roslyn#73292. (feel free to re-triage if you don't agree.) |
Note that once dotnet/roslyn#73292 is implemented, we will still need to make the small change in the JSON and Config Binder source generators to take advantage of the new feature - by adding |
@eerhardt presumably you're suggesting to use this issue to track the needed changes for that? If so, it would be good to update the title / description to reflect this. |
I've updated the top comment of this issue with:
|
Thanks man, appreciated! |
Description
This issue is tracking updating the System.Text.Json and ConfigurationBinder source generators to take advantage of dotnet/roslyn#73292 when it is implemented.
Original Description
When the System.Text.Json and ConfigurationBinder source generators generate code against types that have properties with
[Obsolete("message", DiagnosticId = "ID01")]
attributes, the generated code is emitting warnings that I can't suppress and can't control. The only thing I can do is suppress the warning globally, which isn't ideal because I still want those warnings for my "hand written" code.Both of those generators already suppress "normal" obsoletions with
#pragma warning disable CS0612, CS0618
. But since these obsoletion warnings get their own DiagnosticID, those suppressions don't work.(Note the case I hit was with the Configuration Binder source generator against a class that had an
X509Certificate2
property - which has 3 properties with different DiagnosticIDs -SYSLIB0026;SYSLIB0027;SYSLIB0028
.)Reproduction Steps
dotnet build
the following project:Expected behavior
I shouldn't get warnings that I can't take action against.
Actual behavior
I get the following warnings:
Regression?
No response
Known Workarounds
globally
<NoWarn>
these warnings.Configuration
No response
Other information
cc @ericstj @eiriktsarpalis @tarekgh
The text was updated successfully, but these errors were encountered: