Roslyn: provide generic text analyzers that can analyze any text file involved in running app

[heejaechang]

current analyzer design is code centric.

we support this scenario but not the other way around.

• analyzer check additional file, and tell users "you need to change code A to B since you have C in the additional file"

but not support scenarios such as

• analyzer check additional file, and tell users "you need to change string in additional file A to B since you have C in the code"

or

• analyzer check just additional file, and tell users "you need to change string in additional file A to B since it has C problem"

but we are getting customer feedback saying running app in reality involves more than code such as config files, resource files, designer files and etc. and they want to report issues on all these places not just on code.

we might need to think about adding general text analyzers at least in IDE layer (since it will definitely have all context, information needed).

Proposed features:

1. [Analysis] Passing down all the non-source files in the msbuild project file as additional files down to the compiler, not just the ones specifically tagged as additional file: This enables additional file analyzers on all projects without end user needing to explicitly change his project file to get this feature.
2. [Analysis] First class analysis support for additional files: XXXAnalysisContext.RegisterAdditionalFileAction(), which could be implemented similar to syntax tree actions in both CompilationWithAnalyzers and IDE layer.
3. [Code fixes] Support light bulb and code fixes for diagnostics in additional file. This should automatically add support for suppression and fix all occurrences.
4. [IDE presentation] Support squiggles and error list navigation on double click for additional file diagnostics.

[heejaechang]

related to this issues -

https://github.com/dotnet/roslyn/issues/1431
https://github.com/dotnet/roslyn/issues/6649

one of prototype that will at least let us report issues on additional files.
https://github.com/dotnet/roslyn/pull/8525

[heejaechang]

tagging @dotnet/roslyn-analysis

[heejaechang]

by generic text analyzers, I meant supporting

1. diagnostic presentation - squiggles, error list and etc
2. light bulb
3. when to engine analyze additional files
4. API such as RegisterXXX for those additional files.
5. support suppression and fix all.

[mavasani]

There is a workaround for analyzers wanting to report diagnostics on additional files - report diagnostics with ExternalFile location pointing to additional files that are passed into context Options. In fact, our DeclarePublicAPIAnalyzer reports diagnostics on additional files, and they do show up during command builds, and even in the error list. I think only missing pieces are IDE showing squiggles and code fixes for additional files.

[mavasani]

See here for additional file diagnostics reported by DeclarePublicAPIAnalyzer.

[heejaechang]

@mavasani it can't be shown in error list if it contains line/column. and even if it does, double click on the entry won't work.

[mavasani]

Agreed, I was just trying to state that compiler/analyzer driver doesn't need any new APIs. We need to support this feature in IDE presentation

[mavasani]

Only 1 and 2 above is needed

[heejaechang]

that just solves one usage case where user creates diagnostic from code, it doesn't work if user want to create diagnostic from additional files. not on additional files, but oriented from additional files since they don't have specific place to check additional files.

[mavasani]

User can report additional file diagnostics from CompilationAction or CompilationEndAction.

[heejaechang]

"Only 1 and 2 above is needed" this is not true. let's say I want to check web.config file, when should I check the config file? every time any of the RegisterXXX is called? StartCompilation, EndCompilation? if additional files are changed, do I need to wait until full compilation is analyzed to get diagnostics?

[heejaechang]

"User can report additional file diagnostics from CompilationAction or CompilationEndAction." that is just yet another workaround.

we should provide RegisterAdditionaFile just like RegisterSyntaxTree and RegisterSemanticModel so that user has explicit place to create diagnostics from additional files.

[mavasani]

when should I check the config file?

CompilationAction or CompilationEndAction

if additional files are changed, do I need to wait until full compilation is analyzed to get diagnostics

Yes, but that is true for any compilation diagnostic. Unless we want to consider additional files to be first class files for which we support local file only analysis. Note that we will also need public APIs on CompilationWithAnalyzers to fetch additional file diagnostics and add state tracking logic. IMO, its too much work for an uncommon scenario. We should just treat additional file diagnostics as any other compilation level diagnostics.

[heejaechang]

then it is not supporting generic text analyzer. we should do all those so that it is properly supported. otherwise, it is just yet another add something that so slow no body use thing.

[heejaechang]

by the way, if we believe it is uncommon scenario, we should just not do anything. doing thing just halfway to check checkbox doesn't actually make it better IMO.

[mavasani]

I think we need to first understand what is the real customer ask:

1. Provide some way of reporting diagnostics on additional files. Just like public API scenario, if additional file diagnostics normally involve full compilation wide analysis, reporting it as an compilation end action diagnostic will suffice. This feature currently exists.
2. Make additional files first class diagnostic analysis source - support for local + non-local diagnostics on additional files, local is needed because users believe they can produce valuable diagnostics by just analyzing the single additional file itself.

Once we decide on whether 1 is sufficient or 2 is necessary, we ought to implement IDE diagnostic presentation to make the story complete.

[heejaechang]

additional file doesn't need to analyzer only itself. that is one scenario, but other scenario is it poke around compilation given to figure out something.

[heejaechang]

forgot to add all suppression and fix all.

[heejaechang]

I saw both requests /1/ and /2/. usually people said they can accept /1/ (error list working) but would be nice if they can get /2/ (can analyzer any text file)

[mavasani]

FixAll should just work just like regular compilation end diagnostic with location.

Suppression would need lot more design as current approach is completely based on pragma/source attributes.

[heejaechang]

sure, do we already call fixer for diagnostics on additional files on fix all? I know fixer can change additional files but, that is from diagnostics on code not on additional files.

[heejaechang]

tagging @ljw1004 since he once asked about analyzing additional files and reporting diagnostics on them.

[mavasani]

sure, do we already call fixer for diagnostics on additional files on fix all

I meant it should just work once we support light bulb in additional files.

[heejaechang]

@mavasani oh, ya sure. I think everything happen on additional files should be same as on any other code files however we support/implement it.

[heejaechang]

@mavasani I am actually a bit against this "FixAll should just work just like regular compilation end diagnostic with location."

I would rather preper "FixAll should just work just like regular diagnostic with location."

why limit diagnostics on additional file to be limited to those not that useful compilation end diagnostics which is just slow?

[mavasani]

@heejaechang - I meant there is nothing special needed for FixAll here. Any work in the IDE presentation layer (lightbulb, error list, squiggles in additional file) should be done irrespective of whether it works as local file diagnostics or non-local compilation diagnostics. Only thing we need to decide is how important it is to support 1 versus 2 mentioned in https://github.com/dotnet/roslyn/issues/11097#issuecomment-217269846 - this will decide what additional work needs to go in compiler/analyzer driver/IDE diagnostic analysis engine.

[heejaechang]

I don't think support /1/ is any less work then /2/ though. there are a lot of thing to change just to support put things on non code file in IDE.

also, /1/ and /2/ are not tightly coupled work. /1/ and /2/ can be done independent to each other.

[mavasani]

I don't think support /1/ is any less work then /2/ though

It is not different in IDE layer, but it is lot more work + public APIs in compiler layer - hence needs design team approval first.

[heejaechang]

I agree it is more work, but not sure whether it more than what IDE needs to do support this. also, we can put this support on IDE only even though getting design team approval shouldn't be the reason we can't do this.

[ljw1004]

My scenario: I have already written my PlatformSpecificAnalyzer to check whether you use certain WinRT types in your UWP code, and report a squiggle on it.

I also want to write it to check whether you use those types in your XAML file, and report a squiggle on them too.

I am happy to write my own XAML parser, and my own binding logic for that XAML file.

But I need a way to trigger my analyzer to re-examine the XAML file upon appropriate events (e.g. if the XAML code editor buffer has changed), and I need to generate squiggles in the XAML file.

[ejohn20]

My scenario: I'm analyzing markup files (.cshtml, .aspx), JS, and configuration files for various bad practices / policy violations. There are a lot of things in these files that need analysis (insecure methods, vulnerable JS libs, insecure configuration) during development for immediate feedback to the dev team.

For now, a big improvement would be to enhance the IDE to allow build warnings with the file path and line number so the dev team can double click to browse to that file and line number.

Tagging / squiggles in the general text files would be a nice to have at some point as well.

[heejaechang]

@ejohn20 all those files you mentioned here are I believe part of C#/VB projects? I am asking since this doesn't make Roslyn analyzers to run on any projects. it is still files belong to C#/VB projects.

...

also, how do you decide when to re-analyze the cshtml, aspx, JS and etc files? at the compilation end action? is it okay for those files to be analyzed at the end?

[ejohn20]

@heejaechang correct, I'm mainly focused on web apps (MVC or Web Forms) written in C# / VB. Although, it could also apply to newer Node.js, AngularJS, etc. projects down the road.

Right now, I'm simply running the analysis and adding diagnostics using the context.RegisterCompilationAction event. It seems to work pretty well, executing shortly after saving changes to the file. Real time analysis is not as important to me in these files, as it is in a code file.

I'm ok with it running analysis on compilation end, unless you can think of a reason this might not work?

[heejaechang]

@ejohn20 no that will work. I asking to see whether you want live analysis like C#/VB or okay with current experience with little bit better error list support.

[mavasani]

Thanks everyone for the feedback.

To summarize, I think there are bunch of user requests here, and we need to discuss each of them at the design meeting, and prioritize and then tackle them. Proposed features:

1. [Analysis] Passing down all the non-source files in the msbuild project file as additional files down to the compiler, not just the ones specifically tagged as additional file. This enables additional file analyzers on all projects without end user needing to explicitly change his project file to get this feature.
2. [Analysis] First class analysis support for additional files: XXXAnalysisContext.RegisterAdditionalFileAction(), which could be implemented similar to syntax tree actions in both CompilationWithAnalyzers and IDE layer.
3. [Code fixes] Support light bulb and code fixes for diagnostics in additional file. This should automatically add support for suppression and fix all occurrences.
4. [IDE presentation] Support squiggles and error list navigation on double click for additional file diagnostics.

I have also added these to the first comment in this issue.

[ljw1004]

@mavasani I'd be interested in having the .csproj also be passed as an additional file.

(For an analyzer, I had to pick up one of the directives inside the .csproj, and use that to determine whether or not to show diagnostics within the C#/VB project. I implemented a terribly ugly workaround. It was actually technically _impossible_ to add the csproj as an additionalfile using the normal mechanism, for some msbuild reason that I can't remember.)

[KevinH-MS]

I also encountered this issue with an IIncrementalAnalyzer I'm writing. Modifying the .csproj for the projects being analyzed is not an option. I need to read <EmbeddedResource> files for the analysis I'm doing, but I only want to re-process these files when there's actually a change to one of them. Right now, I have to re-process the resource files (or at least check if their contents are different) each time I analyze. I would like for the Workspace to either:

a. Automatically track all files in the solution (even if they're non-code files).
or
b. Have a mode where if certain types of analyzers are installed, it will track all files.

[srivatsn]

If you were writing a regular analyzer with the resource as an additional file then you could use this