Skip to content
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

Address diagnostic issues in runtime incremental source generators #92509

Open
layomia opened this issue Sep 22, 2023 · 26 comments
Open

Address diagnostic issues in runtime incremental source generators #92509

layomia opened this issue Sep 22, 2023 · 26 comments
Labels
area-Extensions-Configuration blocked Issue/PR is blocked on something - see comments source-generator Indicates an issue with a source generator feature
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented Sep 22, 2023

When testing the configuration binder source generator we discovered that the way the runtime source generators emit diagnostics is not suppressible in source. This is a regression from previous releases (see attached sample) however we've yet to scenario where a user would have a pragma suppression for one of these existing diagnostics - most are errors or informational.

See attached project: generatorWarnings.zip

We also discovered that even in previous releases, when a warning is praga suppressed the IDE analyzer flags the suppression as unnecessary.

We also discovered that in 8.0, the diagnostics (at least in Regex generator) no longer seem to honor .editorconfig severity setting.

Related: dotnet/roslyn#68291

CC @eiriktsarpalis, @stephentoub

@layomia layomia added area-Extensions-Configuration source-generator Indicates an issue with a source generator feature labels Sep 22, 2023
@layomia layomia added this to the 8.0.0 milestone Sep 22, 2023
@layomia layomia self-assigned this Sep 22, 2023
@ghost
Copy link

ghost commented Sep 22, 2023

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

Issue Details

Placeholder to address related feedback in #89587.

Author: layomia
Assignees: layomia
Labels:

area-Extensions-Configuration, source-generator

Milestone: 8.0.0

@stephentoub
Copy link
Member

I'd mentioned this to @sharwell last week, and he wasn't sure why the diagnostics we're outputting in this way wouldn't be suppressible with a pragma. He suggested it might be a Roslyn bug.

@ericstj
Copy link
Member

ericstj commented Oct 3, 2023

I think that bug is dotnet/roslyn#68291 based on my discussions with @eiriktsarpalis. I think there's some concern that there might not be a safe way for incremental generators to emit diagnostics at all.

@ericstj ericstj modified the milestones: 8.0.0, 9.0.0 Oct 10, 2023
@ericstj
Copy link
Member

ericstj commented Oct 10, 2023

We understand that this change is a regression from 7.0 for some generators, but we haven't been able to identify a case where a user would have suppressed a diagnostic in source.

@eiriktsarpalis Still need to prototype a solution to this. Depending on what that looks like we could service it if someone is blocked by this.

Workaround: NoWarn at the project level.

@eiriktsarpalis
Copy link
Member

It seems that something deeper is at play here. I tried fixing the issue by removing the trimming logic for Location instances but it appears that this still doesn't fix pragma suppressions. At this point I'm not sure what could be causing this but I'll try creating a minimal reproduction using the Roslyn APIs.

@eiriktsarpalis
Copy link
Member

Actually, disregard the above. The unit test I added is actually incorrect because it relied on the assumption that suppressing the issue in source would make it go away from the list of diagnostics reported by the GeneratorDriver. The suppression does actually work if I try to test it manually in the IDE.

@CodeBlanch
Copy link
Contributor

but we haven't been able to identify a case where a user would have suppressed a diagnostic in source.

Just FYI we ran into this here: https://github.com/open-telemetry/opentelemetry-dotnet/pull/5520/files#r1556221048

@eerhardt
Copy link
Member

Do we know who owns the next "action" here? Is this a Roslyn issue? Or a problem with our source generators? It would be really great if this could be addressed in .NET 9.

This issue really affects the dev experience using the Configuration Binder source generator. There are so many places where libraries use Options classes that have "extra properties" on them that aren't expected to be configured using the configuration, but instead set through code. In every instance of that, we need to globally suppress SYSLIB1100, SYSLIB1101 in the .csproj, but that means we won't ever see these warnings again in the project. So if we add a new Options class and configure it, we won't know if there are properties that don't work with the ConfigBinder.

cc @jasonmalinowski (who is assigned the linked Roslyn issue)

@eiriktsarpalis
Copy link
Member

Do we know who owns the next "action" here? Is this a Roslyn issue? Or a problem with our source generators? It would be really great if this could be addressed in .NET 9.

The issue fundamentally is that diagnostic instances pointing to SimpleLocation are not suppressible, it only works if they are made to point to SourceLocation instances. The problem is that our source generators are intentionally converting SourceLocation instances to equivalent SimpleLocation values since the former encapsulates the full Compilation object, making it unsuitable for incremental caching. I think this needs to be addressed at the Roslyn/source gen API level.

@eerhardt
Copy link
Member

eerhardt commented May 1, 2024

@jaredpar - is this on the radar of getting addressed in Roslyn in the .NET 9 timeframe?

@jaredpar
Copy link
Member

jaredpar commented May 1, 2024

@eerhardt is there a roslyn issue you're referring to?

@eerhardt
Copy link
Member

eerhardt commented May 1, 2024

@eerhardt is there a roslyn issue you're referring to?

I think it's dotnet/roslyn#68291 based on the above discussion. But @eiriktsarpalis and/or @ericstj would need to confirm.

The uber scenario I'm interested in having work is described in #100785. You can't suppress SYSLIB1100 and SYSLIB1101 in code. They can only be suppressed globally in the project.

@stephentoub
Copy link
Member

stephentoub commented May 1, 2024

I think it's dotnet/roslyn#68291 based on the above discussion

Yes.

Effectively, with incremental source generators as they exist today, you can choose to either:
a) have it actually be incremental (only recomputing stuff when the relevant code changes), or
b) have suppressible diagnostics via pragmas
but you can't have both: if a diagnostic includes the real location information, then it breaks incrementality, and if the diagnostic doesn't include the real location information, then it's not suppressible via pragmas.

The feedback thus far has then been "well, source generators probably shouldn't be raising diagnostics... duplicate the relevant checking into separate analyzers".

@eiriktsarpalis
Copy link
Member

Addressing dotnet/roslyn#68291 would fix Diagnostic equality for the purposes of incremental compilation, but it wouldn't address incremental Diagnostic values encapsulating SyntaxTree instances.

Taking a step back, it would be worth pointing out that the issue stems from the fact that diagnostics in generators can only be reported in the final SourceProductionContext phase. This creates the necessity for any conforming source generator to incorporate the diagnostics that it does report into the incremental model -- but today this isn't possible without hacks because the Diagnostic type isn't amenable to incremental caching.

@ericstj
Copy link
Member

ericstj commented Aug 8, 2024

@eiriktsarpalis - do you think we can do something in 9.0 to improve this? Do we have what we need from Roslyn?

@eiriktsarpalis
Copy link
Member

We're still blocked on account of

  1. Diagnostic instances not being amenable to incremental caching and
  2. Diagnostics pointing to SimpleLocation instances not being suppressible by pragmas.

Addressing either of the two issues would unblock this issue.

@eiriktsarpalis eiriktsarpalis added the blocked Issue/PR is blocked on something - see comments label Aug 8, 2024
@eiriktsarpalis eiriktsarpalis removed this from the 9.0.0 milestone Aug 8, 2024
@eiriktsarpalis eiriktsarpalis added this to the 10.0.0 milestone Aug 8, 2024
@eerhardt
Copy link
Member

eerhardt commented Aug 8, 2024

@jaredpar - is this something that can get into the .NET 10 plan early? This is a pretty impactful issue with warnings coming from source generators. You can't suppress the warnings in a constrained location. You can only suppress the warning globally in a project.

@jaredpar
Copy link
Member

jaredpar commented Aug 8, 2024

@eerhardt I'm not entirely sure what the problem is at this point. The discussion is about ability to suppress but references this issue in roslyn dotnet/roslyn#68291. That issue is effectively "By Design".

Think a concrete example would help here.

@eerhardt
Copy link
Member

eerhardt commented Aug 8, 2024

Think a concrete example would help here.

The uber scenario I'm interested in having work is described in #100785. You can't suppress SYSLIB1100 and SYSLIB1101 in code. They can only be suppressed globally in the project.

@eiriktsarpalis
Copy link
Member

@jaredpar here's an overview of the situation. TL;DR it's a product of three separate issues:

  1. Incremental source generators can only emit diagnostics in the final stage of their pipeline, via the
    SourceProductionContext type. This implies that any diagnostic must be incorporated into the incremental model used by the generator so that it can be fed to the SourceProductionContext. It further implies that Diagnostics objects must have structural equality semantics.

  2. We can't cache Diagnostic objects in incremental values because:

    1. their equality implementation is broken and
    2. they transitively hold a reference to the Compilation via the SourceLocation object that they typically encapsulate.

    We work around this issue by storing diagnostic data in an intermediate record that has the expected equality semantics and also trims any references to the compilation object. The final diagnostic object is rehydrated once the SourceProductionContext is available.

  3. This issue; Diagnostics that point to Locations created via the Location.Create method are not pragma-suppressible.

This is a problem that every well-behaved incremental source generator is going to stumble into, and authors are forced to trade off between incremental performance and emitting warnings that aren't locally suppressible.

@CyrusNajmabadi
Copy link
Member

We work around this issue by storing diagnostic data in an intermediate record that has the expected equality semantics and also trims any references to the compilation object. The final diagnostic object is rehydrated once the SourceProductionContext is available.

Fwiw, this is the right way to do things.

Note: if I had my way, diagnostics would not be possible in generators at all. Definitionally, they are never incremental as any edit invalidates them.

It is much better on every metric to just use an analyzer for diagnostics. There, being incremental does not matter, and no semantic operations are gated on this code executing (like with generators).

The best design is to separate out concerns and leave generators for generating and analyzers for analyzing.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 11, 2024

Definitionally, they are never incremental as any edit invalidates them.

Let me see if I can try to unpack that statement. A Diagnostic object is essentially a triple consisting of a DiagnosticDescriptor, a Location and an array of arguments. There is nothing inherently non-incremental about the first and final components, so I presume the problem sits with Location which is not incremental because it needs to be pointing to the current syntax tree. By trimming away the syntax tree from our incremental diagnostic model we lose the ability to locally suppress it.

One possible solution might be to use the current Compilation to recover the SourceLocation corresponding to the Location stored in the incremental model, however SourceProductionContext doesn't expose any information beyond what is stored in the incremental values.

It is much better on every metric to just use an analyzer for diagnostics.

That might be good advice when building a new generator, but I don't think it's workable for the ones we (or customers) have already shipped. In my view we should try to come up with a mitigation and document guidance to users (potentially even deprecating SourceProductionContext.ReportDiagnostic?).

@CyrusNajmabadi
Copy link
Member

which is not incremental because it needs to be pointing to the current syntax tree

That's one part. The other is simply that a diagnostic literally says it is at a particular location. So practically 50% of edits changes that location. This is not what you want with incremental. You want like 99.99% of edits to have no effect.

If 50% of edits break incrementality, you might as well not be incremental.

That might be good advice when building a new generator, but I don't think it's workable for the ones we (or customers) have already shipped

I'm not sure why. We shipped all these systems knowing that we had no idea the best way to do things, and that w would revise later. I had meetings with runtime directly on that issue. We decided it was better to ship, then find and fix things, than to try to solve the problem up front.

But we've learned in several places that the patterns we hoped would work don't actually work :-(

potentially even deprecating SourceProductionContext.ReportDiagnostic

That would be my strong preference. We can probably align with the deprecation and removal of support for v1 generators.

@eiriktsarpalis
Copy link
Member

The other is simply that a diagnostic literally says it is at a particular location. So practically 50% of edits changes that location. This is not what you want with incremental. You want like 99.99% of edits to have no effect.

If a particular edit changes the location of a diagnostic, then presumably the effect of updating the diagnostic is desirable. Conversely, if you have a well-behaved incremental model that incorporates location/diagnostic data, then producing an equal model must imply that the previously issued set of diagnostics is still applicable. We have written a number of unit tests validating the behaviour of incremental models in the presence of diagnostics.

We shipped all these systems knowing that we had no idea the best way to do things, and that w would revise later. I had meetings with runtime directly on that issue. We decided it was better to ship, then find and fix things, than to try to solve the problem up front.

While I agree with the approach in principle, a rewrite of our source generators is going to be expensive and risky, so I would think this should be kept as a move of last resort and provided that the stated goals justify the costs. For the case of diagnostics, my impression is that it's a 95% solved problem (using the techniques described earlier) so coming up with a smart (quoted or unquoted) mitigation to this current issue is something we should want to consider.

@CyrusNajmabadi
Copy link
Member

If a particular edit changes the location of a diagnostic, then presumably the effect of updating the diagnostic is desirable.

No. The desire of incremental generators is to take 99.99% of edits down to no ops. That's exactly why diagnostics are a problem. They encode positional information (something practically no other generators need), which effectively makes them non incremental, defeating the purpose.

Just don't do diagnostics here. We have the right system for them already. Just use that. :-)

a rewrite of our source generators is going to be expensive and risky

Yes. That was definitely discussed in the meetings. That's the cost of running ahead fast on tech that was definitely still being baked :-)

I'm just telling you my recommendations. It's up to you what you want to end up doing.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 12, 2024

If a particular edit changes the location of a diagnostic, then presumably the effect of updating the diagnostic is desirable.

No. The desire of incremental generators is to take 99.99% of edits down to no ops. That's exactly why diagnostics are a problem. They encode positional information (something practically no other generators need), which effectively makes them non incremental, defeating the purpose.

Point taken, but you'd be surprised how many positional attributes of source code impact generation and this is especially true for generators acting on type graphs. While the presence of diagnostics did turn out to be the primary contributor to bad SG performance in STJ, this was only because we were naively storing Diagnostics directly on the incremental model. Once we switched to using the intermediate equatable representation it's become an issue of least concern.

It's up to you what you want to end up doing.

Unless you can recommend a way in which we could look up a SourceLocation from the SourceProductionContext, I don't think there is really much we can do on our side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Extensions-Configuration blocked Issue/PR is blocked on something - see comments source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

No branches or pull requests

8 participants