Obsoletions with a DiagnosticId still get warnings in generated code

[eerhardt]

Description

This issue is tracking updating the System.Text.Json and ConfigurationBinder source generators to take advantage of https://github.com/dotnet/roslyn/issues/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:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.1" />
  </ItemGroup>

</Project>

using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;
using System.Text.Json.Serialization;

var builder = Host.CreateApplicationBuilder(args);

var c = new MyClass();
builder.Configuration.Bind(c);

[JsonSerializable(typeof(MyClass))]
public partial class JsonContext : JsonSerializerContext
{
}

public class MyClass
{
    [Obsolete("message", DiagnosticId = "EE01")]
    public string? Name { get; set; }
}

Expected behavior

I shouldn't get warnings that I can't take action against.

Actual behavior

I get the following warnings:

\ConsoleApp119\obj\Debug\net8.0\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(65,17): warning EE01: 'MyClass.Name' is obsolete: 'message' [C:\Users\eerhardt\source\repos\ConsoleApp119\ConsoleApp119\ConsoleApp119.csproj]
\ConsoleApp119\obj\Debug\net8.0\System.Text.Json.SourceGeneration\System.Text.Json.SourceGeneration.JsonSourceGenerator\JsonContext.MyClass.g.cs(53,36): warning EE01: 'MyClass.Name' is obsolete: 'message' [C:\Users\eerhardt\source\repos\ConsoleApp119\ConsoleApp119\ConsoleApp119.csproj]
\ConsoleApp119\obj\Debug\net8.0\System.Text.Json.SourceGeneration\System.Text.Json.SourceGeneration.JsonSourceGenerator\JsonContext.MyClass.g.cs(54,45): warning EE01: 'MyClass.Name' is obsolete: 'message' [C:\Users\eerhardt\source\repos\ConsoleApp119\ConsoleApp119\ConsoleApp119.csproj]
\ConsoleApp119\obj\Debug\net8.0\System.Text.Json.SourceGeneration\System.Text.Json.SourceGeneration.JsonSourceGenerator\JsonContext.MyClass.g.cs(80,43): warning EE01: 'MyClass.Name' is obsolete: 'message' [C:\Users\eerhardt\source\repos\ConsoleApp119\ConsoleApp119\ConsoleApp119.csproj]

Regression?

No response

Known Workarounds

globally <NoWarn> these warnings.

Configuration

No response

Other information

cc @ericstj @eiriktsarpalis @tarekgh

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-extensions-configuration See info in area-owners.md if you want to be subscribed.

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

[tarekgh]

@eiriktsarpalis I marked this for .NET 9.0. Let me know if you want to change that. Thanks!

[teo-tsirpanis]

Maybe Roslyn would need to add support for #pragma warning disable obsolete?

[eiriktsarpalis]

One possible approach that was considered in the past is exposing a string[] Suppressions { get; set; } property in the JsonSourceGenerationOptionsAttribute that gives users explicit control over what suppressions should be made.

[eerhardt]

exposing a string[] Suppressions { get; set; } property in the JsonSourceGenerationOptionsAttribute

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.

Maybe Roslyn would need to add support for #pragma warning disable obsolete?

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.

[RikkiGibson]

A pseudo-diagnostic-id similar to #pragma warning disable nullable might be the right thing here. Please file an issue on roslyn.

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";
    }
}

[RikkiGibson]

BTW, it's also possible to disable all warnings using #pragma warning disable. But, this might be considered undesirable if it hides bugs with the generated code itself.

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-extensions-configuration See info in area-owners.md if you want to be subscribed.

[dotnet-policy-service[bot]]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

[eerhardt]

This issue applies to both the Configuration Binder and the System Text Json source generators.

[tarekgh]

Should we log issue for Roslyn regarding that?

[teo-tsirpanis]

Each issue/PR should only have one label. It might belong to area-Meta, I'm letting you (plural) to decide.

Should we log issue for Roslyn regarding that?

I have opened dotnet/roslyn#73292.

[tarekgh]

Tagging it with area-Meta may result in it being overlooked by other area owners for tracking purposes.

[eiriktsarpalis]

BTW, it's also possible to disable all warnings using #pragma warning disable. But, this might be considered undesirable if it hides bugs with the generated code itself.

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.

[tarekgh]

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.

Can't the user utilize <NoWarn> in their project to disable any warning?

[jaredpar]

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.

They're not actionable by users, but they're indicators to the generators that they're using C# incorrectly.

[eerhardt]

Can't the user utilize NoWarn in their project to disable any warning?

I addressed this in the top comment:

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.

[eiriktsarpalis]

They're not actionable by users, but they're indicators to the generators that they're using C# incorrectly.

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.

[CyrusNajmabadi]

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.

[stephentoub]

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: "Something I've been thinking about recently is that source generators should be disabling warnings altogether in generated code," ?

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.

[jaredpar]

Why shouldn't the source generator be able to disable warnings on things it itself is generating?

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.

[eiriktsarpalis]

That seems like a recipe for letting bad code silently enter generators.

I think it's reasonable for warnings to be emitted for debug builds of the source generators. I don't see much of a point in production builds -- if emitting a warning helps uncover potential bugs, I think users should just be reporting the bugs themselves assuming they exist.

[jaredpar]

. I don't see much of a point in production builds -

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.

[RikkiGibson]

Is there a similar issue here when the user types include ExperimentalAttribute on members? SharpLab

[gregsdennis]

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.

[jaredpar]

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.

[RikkiGibson]

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.)

[jaredpar]

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 BinaryFormatter without a diagnostic. Are we okay with that use case?

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 BinaryFormatter. Genuinely on the fence about that.

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.

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 [*.Regex.g.cs] which possibly matches more than you intended.

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 user

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 #pragma work (likely more so for generated code because it can't be as easily tuned as hand written code). I'm certainly sympathetic to cases like this where getting it right for generators is burdensome and recognize we may need to find ways to strike a better balance.

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.

[CyrusNajmabadi]

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-

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.

[stephentoub]

At the same time it also means that generators can essentially use BinaryFormatter without a diagnostic.

That's already possible, no? The source generator would just emit pragma warning disable SYSLIB0011 today. Or it would use a mechanism to access it that the obsolete analyzer can't see, e.g. reflection.

[jaredpar]

The source generator would just emit pragma warning disable SYSLIB0011

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 #pragma warning disable obsolete it's not clear what was suppressed, just anything obsolete related. Can't make inferences as to whether say BinaryFormatter was used or not.

[stephentoub]

That would show up if you were auditing code, looking at a SARIF log, etc

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 disable obsolete the actual ids for anything it suppresses rather than a blanket log for the category? It could treat it as if the individual codes actually encountered were suppressed, which the dev can't necessarily do but the compiler can.

[jaredpar]

I'd expect such code auditing to not include Roslyn source generated code, since it's generally not merged into a repo

Agree that users won't reasonably audit this. Tooling though does perform these audits and they do consider generated code.

And what stops the C# compiler from logging for disable obsolete the actual ids for anything it suppresses rather than a blanket log for the category? It could treat it as if the individual codes actually encountered were suppressed, which the dev can't necessarily do but the compiler can.

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 #pragma was the right way I'd probably end up being fine with it.

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.

[eerhardt]

I agree we shouldn't disable all warnings in generated code. That seems going too far.

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?

The JSON source generator already decided it should disable any "obsolete" warnings inside the code it generates. That's why it adds a #pragma warning disable CS0612, CS0618 line to the top of the generated files:

https://github.com/dotnet/runtime/blob/6c3245ed44e3f205a244d990e50c9510f6e1e9a6/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs#L140-L141

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.

[tarekgh]

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.

[RikkiGibson]

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.

[terrajobst]

It seems there are a couple of approaches (in order of impact):

1. Ability to turn of all obsoletion warnings which the analyzer can emit
2. Roslyn configuration for suppressions that are only applied to generated code
3. Exposing configuration for source generators to suppress user supplied diagnostic IDs (would be custom for each generator)
4. Do nothing, i.e. the user has to suppress them globally.

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 (<NoWarnInGeneratedCode>SYSLIB001</<NoWarnInGeneratedCode>) or as a kind of pairing based on the name the generated syntax tree was given (if we really feel the need for more localization).

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.

[jaredpar]

Do nothing, i.e. the user has to suppress them globally.

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.

Roslyn configuration for suppressions that are only applied to generated code

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.

Ability to turn of all obsoletion warnings which the analyzer can emit

If we take this approach it will be the big hammer style. Basically disable all obsolete diagnostics including ones we'd probably want like BinaryFormatter. Don't see us doing a #pragma warning disable obsolete except ... style feature.

Personally, I think (1) makes the most sense and has the least amount of moving piece

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.

[terrajobst]

Does this mean we agree on the direction (i.e. support #pragma warning disable obsolete), that is, an explicit suppression for all obsoletions, akin to experimental and nullable?

[jaredpar]

I think it's the most promising path at this point.

[eiriktsarpalis]

Moving to 10.0.0, seems unlikely we'll get to a solution in 9.

[terrajobst]

@jaredpar @eiriktsarpalis should this issue be moved to dotnet/roslyn? Looks like we concluded this should be a compiler feature.

[RikkiGibson]

It feels reasonable to close this out and track with dotnet/roslyn#73292. (feel free to re-triage if you don't agree.)

[eerhardt]

Note that once https://github.com/dotnet/roslyn/issues/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 #pragma warning disable obsolete (or whatever is decided) to the generated code.

[terrajobst]

@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.

[eerhardt]

I've updated the top comment of this issue with:

This issue is tracking updating the System.Text.Json and ConfigurationBinder source generators to take advantage of https://github.com/dotnet/roslyn/issues/73292 when it is implemented.

[terrajobst]

Thanks man, appreciated!