New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Roslyn analyzers and code fixes #1076

Open
wants to merge 186 commits into
base: master
from

Conversation

Projects
None yet
@savpek
Copy link

savpek commented Dec 26, 2017

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

This comment has been minimized.

Copy link

savpek commented Dec 27, 2017

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?


var compiled = await project.GetCompilationAsync();
var analysis = await compiled
.WithAnalyzers(this.providers.SelectMany(x => x.CodeDiagnosticAnalyzerProviders).ToImmutableArray())

This comment has been minimized.

@srivatsn

srivatsn Dec 28, 2017

Contributor

A call to WithAnalyzers creates a new compilation and so running the analyzers will have to rebind the entire solution - something it would need to do for a regular compilation.GetDiagnostics() as well. It might be better to compute the analysis results along with the regular compiler diagnostics by calling CompilationWithAnalyzers.GetAllDiagnosticsAsync(). That will ensure that the entire solution is bound only once for the diagnostics instead of twice.

@rchande

This comment has been minimized.

Copy link
Contributor

rchande commented Jan 3, 2018

@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

This comment has been minimized.

Copy link

savpek commented Jan 5, 2018

OmniSharp/omnisharp-vscode#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.

global.json Outdated
@@ -1,5 +1,5 @@
{
"sdk": {
"version": "2.0.0"

This comment has been minimized.

@savpek

savpek Jan 5, 2018

This change was accident, undoed on following commit.

@rchande

This comment has been minimized.

Copy link
Contributor

rchande commented Jan 8, 2018

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

@david-driscoll

This comment has been minimized.

Copy link
Member

david-driscoll commented Jan 8, 2018

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

This comment has been minimized.

Copy link

savpek commented Jan 10, 2018

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

savpek added some commits Jan 11, 2018

@savpek

This comment has been minimized.

Copy link

savpek commented Jan 26, 2018

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

This comment has been minimized.

Copy link
Contributor

DustinCampbell commented Jan 26, 2018

@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

This comment has been minimized.

Copy link

savpek commented Jan 26, 2018

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

@savpek

This comment has been minimized.

Copy link

savpek commented Feb 9, 2018

@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

This comment has been minimized.

Copy link

mjsabby commented Jul 10, 2018

@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

This comment has been minimized.

Copy link
Contributor

DustinCampbell commented Jul 16, 2018

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

savpek added some commits Jul 22, 2018

@savpek

This comment has been minimized.

Copy link

savpek commented Nov 28, 2018

@komanton in that initial phase where all projects are analyzed during startup to get initial known state. I am surpised about CPU and memory however. Maybe roslyn automatically does things concurrently. in background during analysis and that can cause cap cpu during that phase.

Because of current toggle flag i don't see it as showstopper for this PR but definetly thing that must be improved. Memory usage needs profiling, i don't have idea what can cause that amount appear to memory.

Initial idea to support large solutions with analyzers:

  • Only execute semantic diagnostics initially (basically same as version without analyzers).
  • Execute further analysis on demand after that.
    • Toggleable feature, reduces information (not solution wide analysis initially) but is usefull for large solutions.

However if there is way to write same in much more faster manner against roslyn then that idea is obsolete.

Do anyone know good OS project that have large code base and have modern enough base to work with omnisharp? I have some available from work but they are not very good for this purpose since it's easier if we have common ground to discuss and test problems with code base that is available for all parties.

@komanton

This comment has been minimized.

Copy link

komanton commented Dec 3, 2018

For now, I am using analyzers for several days and it works great. Only one thing which I noticed it is a delay 6-15 sec between applying a fix to source code and disappearing an error information about the error from "Problems" tab. I am still working on my huge solution.

I can not reproduce such delay into my temp project which is attached above see ConsoleApp3.zip. There are error information is disappeared immediately after the fix it in my temp project.

savpek added some commits Dec 4, 2018

@seesharper

This comment has been minimized.

Copy link
Contributor

seesharper commented Dec 10, 2018

@savpek

Do anyone know good OS project that have large code base and have modern enough base to work with omnisharp? I have some available from work but they are not very good for this purpose since it's easier if we have common ground to discuss and test problems with code base that is available for all parties.

You could use the AspNetCore repo for testing with a large code base. 👍
https://github.com/aspnet/AspNetCore

@rchande

This comment has been minimized.

Copy link
Contributor

rchande commented Dec 10, 2018

@savpek It looks like some cake tests are failing for your PR.

Show resolved Hide resolved tests/OmniSharp.Roslyn.CSharp.Tests/DiagnosticsFacts.cs Outdated
// Retry is required when build system greatly slows down some times, another issue is that
// it feels that some of update events from workspace get lost and it requires further investigation.
// If missing events are true then same issue will happen with LSP version too.
await RetryAssert.On<ContainsException>(async () =>

This comment has been minimized.

@rchande

rchande Dec 10, 2018

Contributor

What would be a better way to fix this? Is there a way to ask the diagnostics service when its queue is empty?

Put differently: when this starts failing in CI, what are we supposed to do as maintainers of the product? Increase the retry count and hope it's not concealing a bug?

Show resolved Hide resolved tests/OmniSharp.Roslyn.CSharp.Tests/CustomRoslynAnalyzerFacts.cs Outdated
Show resolved Hide resolved tests/OmniSharp.Roslyn.CSharp.Tests/AnalyzerWorkerQueueFacts.cs
Show resolved Hide resolved tests/OmniSharp.Cake.Tests/CodeCheckFacts.cs
Show resolved Hide resolved src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/ICsDiagnosticService.cs Outdated
Show resolved Hide resolved ...harp.Roslyn.CSharp/Workers/Diagnostics/CsharpDiagnosticWorkerComposer.cs
Show resolved Hide resolved src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs Outdated

savpek added some commits Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment