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
19.01k stars 4.03k forks source link

RegisterCodeFixesAsync does not get called for fixable diagnostic id #57621

Closed hugener closed 2 years ago

hugener commented 2 years ago

Version Used: Visual Studio 16.11.5 Microsoft.CodeAnalysis 3.11.0

Steps to Reproduce: I have NuGet Package that supports a number of diagnostics and corresponding code fixes. For exactly one code fix RegisterCodeFixesAsync does not get called causing VS not to offer the fix. All other code fixes work.

A minimal solution is available here: Playground.zip

  1. The solution should report a SDU0009 diagnostic, pointing to the fact that the Expression record does not have "factory method". This works including red squiggly lines in VS. image
  2. It is supposed to also offer a code fix for this diagnostic, but fails to do so.

I have checked with the debugger that the CodeFixProvider provides fixable ids, including SDU0009. For other diagnostics this is followed by a call to RegisterCodeFixesAsync, where the CodeAction is registered. However this never happens for SDU0009.

The only difference I see to other diagnostics is that this one provides additional properties. However, temporarily removing these does not change anything.

Code reporting the diagnostic happens here

Code fix provider is here

Code fix is here

Expected Behavior: RegisterCodeFixesAsync called -> Code fix for SDU0009 offered

Actual Behavior: RegisterCodeFixesAsync not called -> No code fix is offered

mavasani commented 2 years ago

This is by design for the implementation used in the analyzer. Code fixes are only supported for local diagnostics, i.e. which are reported in the span of the original symbol being analyzed. So in your case, it will only trigger if the code below reports a diagnostic in one of the locations/declaring references of the passed in named type: https://github.com/sundews/Sundew.DiscriminatedUnions/blob/5d9be1765c1132fb98892bc73eda73135ac6dc45/Source/Sundew.DiscriminatedUnions.Analyzer/DiscriminatedUnionCaseAnalyzer.cs#L17

However, your code seems to be walking base types across the compilation and reporting diagnostics on them, which may be present anywhere in the entire project. These are non-local diagnostics, as we would need to analyze the whole project to correctly refresh them after every edit. Doing so would make Ctrl + Dot an extremely expensive operation, hence we don't support code fixes for such non-local diagnostics reported by an analyzer.

Youssef1313 commented 2 years ago

@mavasani Can this be easily caught by a new RSxxxx analyzer? I think this is something that @333fred encountered recently and it was non-trivial to investigate. Otherwise, I'd point to that in the analyzer tutorial in dotnet/docs.

mavasani commented 2 years ago

Can this be easily caught by a new RSxxxx analyzer?

I am not sure it can catch all cases, and I suspect it can also lead to false positives as dataflow might be involved.

I'd point to that in the analyzer tutorial in dotnet/docs

I think that is a much more reasonable way to go. It would be good to explain with an example how reporting a diagnostic outside the callback symbol/node leads to this scenario.

Youssef1313 commented 2 years ago

@mavasani I opened https://github.com/dotnet/docs/issues/27653.

hugener commented 2 years ago

which may be present anywhere in the entire project. These are non-local diagnostics, as we would need to analyze the whole project to correctly refresh them after every edit

Hi @mavasani Thanks for explaining the behavior. Yes, I am creating a closed inheritance hierarchy by checking that each concrete type that has a base class or interface (Marked with a special attribute), fulfills the following:

  1. Not defined in a different assembly
  2. Has a "factory method" for that type in the base class or interface.

That way I can be sure that all concrete types are known at compile-time (I can check the factory methods), while avoiding doing the analysis with CompilationEndAction which may be deactivated in VS.

The codefix is there to take away the burden of implementing factory methods.

It means that diagnostics on a base class or interface as you mentioned, are being reported while analyzing the derived type and as I understand it, this is what makes it non-local. In my first naïve implementation, I was trying to determine all derived types while analyzing the base class/interfaces, but this led to unusable VS performance (Scan all types in compilation), which is my I came up with this new approach.

As I report the diagnostics on the base class/interface while analyzing the derived classes, how come does tracking the diagnostic for updating the red squiggly lines, etc. not fall into the same category of an extremely expensive operation?

Would you consider some of the following as a workaround?

  1. Always reporting an Info diagnostics on the base class/interface and the execution of the codefix might or might not change anything.
  2. Reporting the Error diagnostics on the derived class and have the codefix change the base class/interface. Is that even possible (I guess I would have to be able to find the other document to edit)?
  3. Any other suggestions?

Finally, I have passing tests for the code fix here. Shouldn't these fail with an error that the code fix was attempted on a non-local diagnostic or should I create another issue for that?