Add public support for third party Document and Project diagnostic sources

[mavasani]

Currently, we have an internal DocumentDiagnosticAnalyzer and ProjectDiagnosticAnalyzer types that enable internal clients to report document and project specific diagnostics.

These are not true diagnostic analyzers, but just need a way to easily use all the pieces of our diagnostic/fix system.

The correct design is for DocumentDiagnosticAnalyzer and ProjectDiagnosticAnalyzer to use the IIncrementalAnalyzer and IDiagnosticUpdateSource types and plug into the Solution crawler to report diagnostics.

Additionally, the analyze callbacks into these types should allow configuring whether or not they need to perform UI centric operations, so that they can be invoked from the VS UI thread.

We should eventually make these public so third party extensions can be written for such VS-specific diagnostic sources.

[mavasani]

Assigning to @JohnHamby as per his request.

[gfraiteur]

PostSharp is interested in this feature.

[mavasani]

@JohnHamby @heejaechang @srivatsn a design point here is do we de-duplicate diagnostics from such IDE-only diagnostic sources and the Build? In case of PostSharp, the diagnostics generated by the IDE Diagnostic source overlap those from the build - so they do need the build-intellisense de-duping. Additionally, they need a means to disable the diagnostic source at which point all diagnostics from the source should be cleared and diagnostics from build should show in the error list. Basically, we need some extra work in our build-intellisense de-duping layer to handle the new scenarios.

[heejaechang]

@gfraiteur how currently postsharp works? why do they need document/project analyzer? those are usually only for features that require VS specific services and never care to run on build or command line scenario. in other words, I would expect document/project analyzer is just used so that one can create fixer for diagnostics they generated for open files.

[mavasani]

@heejaechang PostSharp analyzer is a VSIX based analyzer, only for live analysis. They need some way to report diagnostics accessing VS Workspace services (such as options) - and because we don't have concept of VS-only analyzers that can access Workspace, it seems reasonable for them to implement their component as a 3rd party diagnostic source rather then an analyzer.

[heejaechang]

@mavasani if they become diagnostic source, framework only provide a way to report diagnostic. they have to iterate through solution themselves (without affecting IDE perf), no fixer support, no de-dup and etc.

I think opening that up is a bit very risk due to perf reasons. I would prefer opening up Document/Project Analyzer that only work on top of open files and we run them on UI thread.

[mavasani]

@heejaechang Agreed - I think exposing Document/Project Analyzer should also be just fine.

[gfraiteur]

@heejaechang As @mavasani said, our analyzer is a VS-only analyzer, deriving from DiagnosticAnalyzer and installed in our VSIX. We are just calling RegisterSemanticModelAction. At build time, we have a totally different stack, not based on Roslyn, and we emit diagnostics through MSBuild.

The only issue we're having is that enabling/disabling the VS-only analyzer requires a VS restart. We would prefer not to work at too low-level and not to care about performance issues.

[heejaechang]

@gfraiteur well, if you don't want to care about performance, you shouldn't participate in IDE. since ,unlike build, in IDE, everyone share resources and pain.

that being said, if you don't want to restart VS, don't use VSIX based approach, use NuGet based approach. by using VSIX, assumption is you need to restart VS if you add or remove VSIX. not something specific to analyzers, but that is the design of VSIX. if you don't want that behavior, you shouldn't use VSIX. and for those people, we provide NuGet based approach.

[gfraiteur]

@heejaechang I'm sorry, I didn't express myself accurately. We do care about performance a lot and that's why we would like to minimize the effort required to achieve high performance.

We have a large VSIX and we allow users to enable or disable individual features, including the analyzer. We would benefit from a VS feature where we could enable or disable our analyzer dynamically.

[heejaechang]

@gfraiteur right now, the way we provide to enable or disable analyzer is
either you remove VSIX or remove analyzer reference from projects (nuget)

or disable all rules belong to an analyzer (Ruleset)

leaving analyzers but disabling it is equal to disabling all rules that belong to the analyzer.

can you rather disable all rules that belong to the analyzer? @tmeschter @mavasani do we support changing ruleset rules programmatically?

[tmeschter]

@heejaechang @gfraiteur @mavasani We don't provide any interfaces for changing rule set files programatically, no.

That being said, rule set files are pretty simple XML. You can get the path to the active rule set file by accessing the "CodeAnalysisRuleSet" property of the project's active build configuration, update the file yourself, and VS will eventually pick up the changes and propagate them to the workspace and analyzers.

[gfraiteur]

In our model, enabling/disabling our analyzer is a UI feature, not a build feature, and therefore it should not be persisted in a source file.

Any way that would invalidate the current diagnostics and force a new analysis would make the trick.

[srivatsn]

That's a valid scenario. Since this is assigned to @JohnHamby - let's wait for John to come up with a API proposal and discuss that.

[Foroughi]

Any recent update for this feature ?! is there even a map to add this feature ?! should we still wait ?! that would be great if we have a workaround at least for specific scenarios under specific circumstances

[CyrusNajmabadi]

@Foroughi What are you trying to accomplish?

[heejaechang]

tagging @jinujoseph

this will require bunch of design decisions. behavior on OOP, scope of it running (whole solution or etc), MEF, VS API, Light bulb, Code Fix and etc.

[CyrusNajmabadi]

this will require bunch of design decisions.

Agreed. It' not clear to me if IIncrementalAnalyzer is even the right currency for a third party. It seems to leak out C#/VB concepts (like "SyntaxNode bodyOpt"). We might needed a cleaner and simpler 'core' api that anyone can use, and hten have IIncrementalAnalyzer be hte advanced API that C# and VB specifically use for optimization purposes.

--

That said, i want to know what the actual use case is here that customers are looking for this for.

[Foroughi]

@CyrusNajmabadi , I'm trying to create an analyzer to determine if a method is being used somewhere in the whole solution or not, any idea how to do this ?!

[CyrusNajmabadi]

1. Does this analyzer need to run inside VS?
2. In general, we don't have a story for analyzers that need to run solution-wide**. I would prefer to have an answer to that, versus a different Project/Document diagnostic sources.

** The issues here are really about perf and scale. Solutions can be huge, and it would be so simple for an analyzer to totally skew running times across the board here.

What do you do when you find out if the method is/isn't being used? What sort of end-to-end experience/feature are you trying to provide?

[Foroughi]

1- Its gonna be installed as a nugget package on our companies project to make sure developers are on the right path

in case analyzer finds an unused method the user will be informed by a warning to remove that method or maybe offers for a code fixer.

FYI it's just a simple scenario to answer this question: why Roslyn should support solution-wide (or even Project-wide). We have lots of false alarms on analyzers that needs a solution-wide scoped analyzer to be fixed

Should I inform our company that there won't be any update in future to fix this ?!! should we wait ?!

I think I need to mention this again that the nugget package is gonna be installed on just our companies solutions to keep the developers in the right path and also keep the projects clean as much as possible. this means we can follow any specific pattern of our solution. this may help you to suggest a workaround.

[CyrusNajmabadi]

Should I inform our company that there won't be any update in future to fix this ?!! should we wait ?!

"to fix this" is very vague. I think Roslyn should, at some point, have a mechanism for people who want to do solution-wide-analysis. However, i don't think that this issue shoudl be the issue tracking that. They are unrelated topics. This topic is "Add public support for third party Document and Project diagnostic sources". What you are asking for is something altogether different.

should we wait ?!

Have you considered writing command line tooling htat can use the Roslyn API to do this sort of highly expensive computation in a manner that won't impact the user whne using VS? You could do as much analysis as you wanted there (including whole solution analysis), and report the results for your devs to fix.

[Foroughi]

what I meant by "to fix this" was exactly what you mentioned (thanks for making me clear). there should be a mechanism to achieve solution-wide analyzers.

I'm working on a workaround which using System.IO to read the syntax tree of all the files and use them to make a solution-wide search. Obviously, I`ve faced the fact that difference files have different semantic models.

[CyrusNajmabadi]

I'm working on a workaround which using System.IO to read the syntax tree of all the files and use them to make a solution-wide search

The right way to do this would just be to use the roslyn Workspace API, which already knows how to read in solutions, and gives you a unified view over all of them. You could then directly access APIs we have like "FindAllReferences" to answer questions you have like:

in case analyzer finds an unused method the user will be informed by a warning to remove that method or maybe offers for a code fixer.

You could just use hte FindReferences API and see if you get 0 results back.

[Foroughi]

hold on. I think I'm missing something here. clarify something for me.

Can we have access to the current workspace or current solution (using Roslyn API) in analyzers, when we want to have NuGet packages as output (Not VSIX) ?!

[Foroughi]

SymbolFinder.FindReferencesAsync needs the current solution as its second parameter, but as far as I know, we don't have access to the current workspace in analyzer. that's the reason I start to work on that idea to use System.IO to read the files.

[CyrusNajmabadi]

I don't know enough to answer for certain. @jmarolf might know. Jon, are the Roslyn workspace APIs available for third parties to use through nuget?

[jmarolf]

@Foroughi

SymbolFinder.FindReferencesAsync needs the current solution as its second parameter, but as far as I know, we don't have access to the current workspace in analyzer. that's the reason I start to work on that idea to use System.IO to read the files.

VSIX analyzers are just Visual Studio extensions so they can do anything an extension can do, including importing the VisualStudioWorkspace (see here for how to do this).

Analysers that are imported using nuget packages do not have access to the workspace or any apis that the compiler does not have access to. The api as it is currently implemented only allows analyzers to know about things in the current compilation (a project in Visual Studio). If you call RegisterCompilationAction from within the Initalize method in your analyzer, you will be able to look at all the symbols within the compilation.

Why can't analyzers see the scope for an entire solution? The simplest answer is: because the compiler can't, and analyzers run inside the compiler. This is done so that analyzers can be run on continuous integration servers without Visual Studio installed. MSBuild reads the solution file and then invokes the compiler once for each project. The compiler is never aware of project dependencies and the compiler team does not want to be in that business, they are happy to leave it to MSBuild.

People have tried to work around this by loading their solution using MSBuildWorkspace and attempting to look at documents across projects that way. This will fail occasionally because MSBuildWorkspace is not thread-safe. It will also cause memory usage to skyrocket. People have tried to cache MSBuildWorkspace instances to partially resolve this problem but the cache needs to be invalidated every time a new compilation is created (essentially in the event of all but the most trivial changes). Basically, going down this path is rife with pain and is unsupported.

Enough people have asked for this feature that its something we think we need to do eventually. There is no reasonable way to accomplish it today unless you are willing to write a Visual Studio extension that imports the Visual Studio Workspace and attempts to run its own analysis engine.

If Visual Studio is not sufficient (this analysis must run on CI builds), I would write a command-line tool that uses MSBuildWorkspace to load your solution and do the checking.

[sharwell]

@Foroughi take a look at AnalyzerRunner for a command line tool that opens a solution in a workspace and runs code against it. You could build an arbitrarily complex analyzer tool using this approach.

[sharwell]

The AnalyzerRunner code is in #23087

[Foroughi]

@jmarolf

I would write a command-line tool that uses MSBuildWorkspace to load your solution and do the checking.

Can you explain it more, please ?! what does that have to do with nugget packages ?!

[Foroughi]

@sharwell thanks, let me have a look at this, but I'm not sure how a command line tool can help me on my nugget package?

[Foroughi]

Maybe I need to mention that the nugget package project is being based on .Net Standard 2 ,and as far as i found out we don't have access to MSBuildWorkspace

[sharwell]

@Foroughi That's correct. We are working on making MSBuildWorkspace available to .NET Standard (#21670), but for now it's limited to .NET Framework scenarios.

[Foroughi]

@sharwell that's a good news. by any chance do you know about the ETA?

[jmarolf]

@Foroughi why is your analyzer targeting netstandard2.0? which apis do you need that are not available under netstandard1.3? Currently deploying analyzers that target netstandard2.0 is rather error prone.

[DustinCampbell]

@sharwell: To be clear, that PR will not move MSBuildWorkspace to netstandard2.0. Also, I have some concerns with moving it to netstandard2.0 due to the fact that it is very challenging to set up MSBuild such that it can be used from a .NET Core application. Even then, it won't work with many projects.

[heejaechang]

@Foroughi currently, we don't expose Document/Project Analyzer worrying someone will use for something it is not designed to be used like solution wide analysis. and I don't think even if we expose those, we would allow solution wide analysis.

instead, we can have solution wide analysis designed explicitly. but currently no such plan I believe.

tagging @jinujoseph @DustinCampbell @Pilchie

...

I guess you want to use the associated services that comes with DiagnosticAnalyzer such as squiggle, error list, Lightbulb, code fix, ruleset, editorconfig or support for vsix/nuget but not DiagnosticAnalyzer itself since it doesn't provide enough scope for you.

but unfortunately, just giving you more info won't give you right experience. all those associated services are created with how DiagnosticAnalyzer works in mind.

for example, even after you can analyze whole solution, your code fix and Lightbulb won't work as fast/smooth as you hope since scope is too big. and in current design, such solution wide analysis, lightbulb, code fix will bring down whole system. since current system, does not distinguish slow, fast analysis, fixes. you get experience of the slowest analysis.

in other words, to support solution wide analysis, fix and etc, we need to design specifically for the scenario and characteristics that comes with it.

I am not saying we can't do it, but I am saying this bug was not about providing solution wide analysis.

[heejaechang]

if you only care about command line scenario. you can build tool yourself as others suggested, and analyze solution yourself in anyway you want to.

once you have MSWorkspace and Solution, you basically have all information you need.

DiagnosticAnalyzer is provided so that it can be connected to multiple host, services, but that also means it could have restriction you might not care (such as perf assumption, scope of analysis and etc)

it might be better for you to just create your own tool rather than workaround limitations we intentionally put to make sure VS experience is good.