Analyzer diagnostics are swallowed if first symbol location happened to be in generated code

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.

https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/

MIT License

18.95k stars 4.02k forks source link

Analyzer diagnostics are swallowed if first symbol location happened to be in generated code #69543

Open Youssef1313 opened 1 year ago

Youssef1313 commented 1 year ago

Most analyzers report diagnostics for the first symbol location. If the analyzer is configured to not report in generated code, then the behavior differs based on whether the first symbol location is in generated syntax tree or not.

This is not an ideal experience.

Suggested approaches:

Introduce a new property ISymbol.FirstNonGeneratedLocation or a GetFirstNonGeneratedLocation extension method that analyzer authors should use.
If the previous suggestion is rejected as a public API, then this API could be implemented as an internal helper method in dotnet/roslyn and dotnet/roslyn-analyzers and then update analyzers to use it. But what about third-party analyzers?
Find a different way to fix this?

Youssef1313 commented 1 year ago

For now, I'm going with an ugly workaround like this: https://github.com/unoplatform/uno/pull/13273

cc @mavasani

CyrusNajmabadi commented 1 year ago

Most analyzers report diagnostics for the first symbol location. If the analyzer is configured to not report in generated code, then the behavior differs based on whether the first symbol location is in generated syntax tree or not.

I would not expect the first symbol location to ever be in generated code. Specifically, generated code is syntax trees appended to a compilation. So symbols should always return locations from real files closer in their ordering list versus generated files.

@jasonmalinowski @chsienki to confirm. But i find this surprising.

Youssef1313 commented 1 year ago

@CyrusNajmabadi We persist generated code on disk. That is why.

CyrusNajmabadi commented 1 year ago

@CyrusNajmabadi We persist generated code on disk. That is why.

Who is 'we' here?

Youssef1313 commented 1 year ago

Sorry for the confusion. I mean at Uno Platform.

CyrusNajmabadi commented 1 year ago

Sorry for the confusion. I mean at Uno Platform.

If you're generating the code to disk, and you're now referencing those files. Then those aren't source-generator files. THey would be considered normal files. There's no way the compiler woudl know they were source-generated either to report in some other location.

Youssef1313 commented 1 year ago

They are marked as auto generated (with // <auto-generated> comment) because we don't want analyzers to report diagnostics in them, but we want analyzers to report diagnostics in the other partials :/

CyrusNajmabadi commented 1 year ago

but you're emitting to disk (and presumably manually updating them?). Just remove auto-generated, since it soundsl ike you do want to report issues. :)

Youssef1313 commented 1 year ago

IIRC it was previously not marked as auto-generated and I got some warnings that I didn't care about in these specific files. That was long time ago so I'll need to revisit this.

Another concern is that we have 6294 files. Not sure if enabling analyzers for them will have a performance impact, but worth trying.

Youssef1313 commented 1 year ago

@CyrusNajmabadi I tried that. It didn't work well. For the generated files that don't have a non-generated partial, we don't really need any diagnostics (there are currently a lot of them)