Issue with analyzers: Custom analyzer Puma.Security.Rules doesn't show up

[savpek]

Based on https://github.com/OmniSharp/omnisharp-vscode/issues/43#issuecomment-481442970

With minimal repro analyzer-doesnt-show-up-puma-issue.zip

Expected:

But:

It seems something filter them out, ruleset suppress them or assembly doesn't get loaded at all. There are no errors on logs when this happens.

[savpek]

Figured it out, need to write tests etc so not gonna probably happen today. But soon ™️

[savpek]

This is much trickier case, if that change is applied analysis takes over 10x more juice from CPU.

From documentation Depending on analyzers' behavior, returned diagnostics can have locations outside the tree, and some diagnostics that would be reported for the tree by an analysis of the complete compilation can be absent. (https://docs.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.diagnostics.compilationwithanalyzers.getanalyzersemanticdiagnosticsasync?view=roslyn-dotnet)

What are those behaviors that can cause absent? I have no idea. Something in puma analyzers trigger that behavior it seems. That not seems to be very common however, for example built in analyzers seems to return identical results with single file filtered results vs full project analysis.

Based on comment https://github.com/dotnet/roslyn/issues/9522#issuecomment-195106431 GetAnalyzerSyntaxDiagnostics + GetAnalyzerSemanticDiagnostics it seems that they can replace GetAnalyzerDiagnosticsAsync/GetAllDiagnosticsAsync. But it seems not to be case here.

I think project scoped analysis without file filtering can be enabled (opt in likely since it is much more expensive to execute). But without foreground/background processing it will cause delay for feedback after typing which is bit anonying

That foreground/background prosessing is needed anyway to really support full solution analysis with updates (change interface A manually, there should be errors all around but currently nothing understands which files must be reanalyzed) + it gives better responsivenes during load up of larger projects. But that processing model is large change for analysis routines, i will likely create initial PR at following weeks but it will take a while before it can be merged.

Foreground: What use is currently writing editing, mostly the current file. Immediatly analyzed, fast feedback. Short throtling. Background: What are interesting things in wider scope but not time critical, like security analysis in puma case. Delayed feedback, longer throttling to use less resources.

@filipw @rchande do you know anything about those differences between analysis methods? I could write to roslyn issues ask for more information.

[ejohn20]

@savpek Thank you for the feedback. This makes sense, as we are using full compilation analysis to analyze code blocks and reduce false positives. @meadisu27 thoughts on this?

[meadisu27]

@ejohn20, makes sense as well. Puma makes use of compilation analysis actions so the GetAnalyzerSyntaxDiagnostics + GetAnalyzerSemanticDiagnostics would likely not pick up any warnings.

@savpek - an opt-in would be a acceptable option (kind of like Visual Studio's Full solution analysis checkbox). However, you are correct that it would cause a delay in feedback which of course is not-so-acceptable. We experience the same in Visual Studio.

Puma's analyzer could be changed to no longer make use of compilation analysis actions and strictly use semantic/syntax actions. However, we make use of the compilation actions in order to capture enough state about the code to reduce false positives. Alternatively, if Puma knew ahead of time whether full compilation analysis is opted in to, it could make the appropriate decision of how to register the analyzers (i.e. more performant, more false positives vs less performant, less false positives)

I'd be curious if the operation related actions(i.e. OperationBlockStartAction/EndAction, etc) would cause the same problem as it seems the Roslyn team is using that strategy for some of the analyzers they have been authoring lately, DoNotPassLiteralsAsLocalizedParameters.