search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.56k stars 4.54k forks source link

Address diagnostic issues in runtime incremental source generators #92509

Open layomia opened 9 months ago

layomia commented 9 months ago

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: https://github.com/dotnet/roslyn/issues/68291

CC @eiriktsarpalis, @stephentoub

ghost commented 9 months ago

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 https://github.com/dotnet/runtime/pull/89587.
Author: layomia
Assignees: layomia
Labels: `area-Extensions-Configuration`, `source-generator`
Milestone: 8.0.0
stephentoub commented 9 months ago

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 commented 9 months ago

I think that bug is https://github.com/dotnet/roslyn/issues/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 commented 9 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

jaredpar commented 2 months ago

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

eerhardt commented 2 months ago

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

I think it's https://github.com/dotnet/roslyn/issues/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 https://github.com/dotnet/runtime/issues/100785. You can't suppress SYSLIB1100 and SYSLIB1101 in code. They can only be suppressed globally in the project.

stephentoub commented 2 months ago

I think it's https://github.com/dotnet/roslyn/issues/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 commented 2 months ago

Addressing https://github.com/dotnet/roslyn/issues/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.