Roslyn analyzers and code fixes

[savpek]

Question 1:
Actually this version is incorrect, analyzer results should likely be returned via event emitter since they may take while. Currently they are returned via CodeCheckService result which causes that analyses doesn't surface if changes aren't made on target projects documents. Easy to see on startup where problems doesn't exist until you make first change. Its also means that if you write change that causes new issue, it is possible that you dont see warning about it before you write another change somewhere.

At first version there wasn't caching on codecheckservice and it calculalated analysis for every document request, it basically anhilated cpu on any bigger solution (surprise surprise).

I tried to change so that event emitter surfaces analyzer results but it seems there are parts missing, i checked how CSharpDiagnosticService makes same thing but there are only QuickFixes currently available on message. This probably means that i need to update consuming applications (like omnisharp-vscode) that they can use those analyzer and surface results correctly. This is where i need guidelines and help how this should be made since it its omnisharp public api.

Question 2:
Should i be concerned about used language? Currently all diagnostic types are loaded, however it is same thing on refactorings and code fixes.

TODO

• Implement global analyzers.
• Implement project level analyzers.
• Implement global code fixes.
• Implement project level code fixes.
• Basic support for RuleSets.

[savpek]

I started to lookup for omnisharp-vscode repository implementation about how these could be possible to surface to editor via events. I don't find /diagnostic endpoint from protocol.ts which i expected since that call starts existing CSharpDiagnosticService worker. Is DiagnosticsService (/diagnostic) something that vscode doesn't use at all? If so where it is used and what are responsibilities of diagnostics versus codechecks?

[rchande]

@savpek Can you help me understand what this change is trying to add? What's different between RoslynAnalyzerService and the existing CSharpDiagnosticService (https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticService.cs)? Thanks!

[savpek]

dotnet/vscode-csharp#43 issue behind change.

I think at later point CSharpDiagnosticService.cs and this should be merged together since they responsibility are largely same. However theres currently several issues that makes that change quite heavy to implement.

If analyzers are loaded to solution/projects and compilation is done with analyzers i think current version of CSharpDiagnostics should return analytic results (attleast with very small change if i am right). However OmnisharpWorkspace doesn't support analyzers and theres ongoing work to move to MSBuildWorkspace (am i right?) which makes writing that implementation at this point quite obsolete. It requires analyzer assembly loaders etc to be written etc...

This version compiles results on its own and uses minimal version of analyzer loading (directly loads them from extension assemblies), which isn't as fancy as full blown solution but is step to impelement this correctly. There also caps which i don't understand how analyzers should be loaded at the end to full blown workspace, but it's something i need to study. Basically next thing is to support project scoped analyzers which need to dig deeper at those and it may be smart to wait MsBuildWorkspace at that point.

Another issue with CSharpDiagnosticService was that it seems (as i stated at message before) that it seems that it isn't used with vscode at all. (never gets called, events never surface etc, actually vscode addon misses definition for /diagnostic). It may be used in some different environment like VIM but no'one hasn't answered for that yet. This is something i think requires updates from vscode extension, but i havent figured it out yet how it should be done.

So shortly: minimal version without breaking anything existing towards better solution and for this reason i think i shouldn't try to merge CSharpDiagnosticService and this at this point. Its something that is determined later on.

[rchande]

@jinujoseph @DustinCampbell Do we have any plans to expose Roslyn's diagnostics engine?

[david-driscoll]

the /diagnostic endpoint isn't used by VSCode yet, it's something I added for atom to consume at one point, and it actually flows into how LSP works really nicely. It just hasn't been adopted yet. :)

That also why it behaves as a toggle, you have to kick the /diagnostic endpoint for the service to turn on, otherwise it does nothing (as you observed).

[savpek]

@david-driscoll when LSP version will be released? I am just thinking should i implement surfacing via event emitter mechanisms at this point? 👍

[savpek]

Are there some actions for this? I could continue this at weekend if necessary, however i don't know answers for questions i asked like:

• What will be status of msbuildworkspace at upcoming months? Should i start implement that part to omnisharp workspace? (like loading analyzer refs).
• Is LSP coming soon? Should i move to use event emitter?

or is this ok as it is at this point and should be merged?

ping @david-driscoll @DustinCampbell

[DustinCampbell]

@savpek: WRT the MSBuildProjectLoader API work that I'm doing over in Roslyn won't be available for use in OmniSharp for quite awhile. Even if it's merged tomorrow (it won't be 😀), the API won't be in a release of Roslyn on NuGet for awhile.

Updating OmniSharp's MSBuild project system to add analyzer references to the OmniSharpWorkspace shouldn't be a big deal or much work. It's just a matter of someone having the know-how and the time to type the code and test it. If you're up for it, I'll cheer you on! 😀

Eventually, the MSBuild project system will mostly go away once the Roslyn MSBuildProjectLoader API handles all of the same scenarios that OmniSharp does. When it's ready, we'll use that to populate the OmniSharpWorkspace. But, it'll probably a few months until there'll be a NuGet package that OmniSharp can consume.

Does that answer your first question?

[savpek]

Yes thanks 😄 I'll check how to implement those hopefully this weekend.

[savpek]

@DustinCampbell since net core csproj format doesn't make difference between analyzer reference assemblies and other reference assemblies in file format level how i should check which ones are analyzer assemblies? Should i just loop metadata through for any matching analyzer classes or is there fancier (and more efficient) way to check which referenced packages are analyzers?

[mjsabby]

@savpek You'll have to open up the nuget package and find the "analyzers" folder structure. As far as I can tell that is the only information available in the nuget package telling you it is an analyzer.

I would love to see this supported in VS Code as well.

[DustinCampbell]

I was under the impression that analyzer references came through as AnalyzerReference items in MSBuild.

[savpek]

@RononDex can you test new version: https://ci.appveyor.com/project/david-driscoll/omnisharp-roslyn/builds/22722332/artifacts

[RononDex]

@savpek just tried the new build, the duplicate key errors are gone

However, I am still getting the NullReference exceptions. I think this is related to the new Roslyn analyzers version though since there are also some invalid cast exception inside the log from our self-written analyzers
roslyn log.txt

[savpek]

If those custom analyzers are installed from nuget packages you could try to remove references and try what it looks like.

If you remove analyzers you have to clean workspace with git clean -fdx (or something), otherwise it loads assemblies from disk event after removal after restart. Known defect but not in top priority since changing back and forth between those references are not that common 🙂

[savpek]

@rchande have you checked review fixes are they ok?

[komanton]

@savpek I have tried the last build. It works now. The problem with
SA1028 is fixed. Thanks.

[komanton]

@savpek In the latest build from time to time, I see such exception (I can not reproduce it step by step, for now):

[fail]: OmniSharp.Roslyn.CSharp.Services.Diagnostics.CSharpDiagnosticWorkerWithAnalyzers Analyzer worker failed: System.NullReferenceException: Object reference not set to an instance of an object. at OmniSharp.Roslyn.CSharp.Services.Diagnostics.CSharpDiagnosticWorkerWithAnalyzers.<>c__DisplayClass15_0.<Worker>b__0(DocumentId documentId) at System.Linq.Enumerable.WhereSelectArrayIterator2.MoveNext()
at System.Linq.Lookup2.Create[TSource](IEnumerable1 source, Func2 keySelector, Func2 elementSelector, IEqualityComparer1 comparer) at System.Linq.GroupedEnumerable3.GetEnumerator()
at System.Linq.Buffer1..ctor(IEnumerable1 source)
at System.Linq.Enumerable.ToArray[TSource](IEnumerable1 source) at System.Collections.Immutable.ImmutableArray.CreateRange[T](IEnumerable1 items)
at OmniSharp.Roslyn.CSharp.Services.Diagnostics.CSharpDiagnosticWorkerWithAnalyzers.d__15.MoveNext()`

[savpek]

Thanks @komanton for reporting, i think issue was bug when document was removed/moved/etc. It added removed document for analysis queue which caused null ref error since document doesn't exist in workspace any more. Fixed it on latest commit.

[bjorkstromm]

Not used?

[david-driscoll]

@savpek Thank you so much for your patience, understanding and most importantly your persistence on this issue! @filipw and @mholo65 have been reviewing this, and everything looks good assuming everything merges correctly. We will be squashing the commit once CI completes.

Also thanks to @heejaechang for your help in reviewing this pull request.

[david-driscoll]

Waiting on CI just to make sure there is nothing broken, but will squash this tonight.

n/a

[filipw]

🎉🎉🎉