ionide / FSharp.Analyzers.SDK

Library for building custom analyzers for F# / FSAC
http://ionide.io/FSharp.Analyzers.SDK/
MIT License
74 stars 22 forks source link

Visual Studio 2017/2019 support? #23

Open Thorium opened 4 years ago

Thorium commented 4 years ago

I know this project is now under Ionide, but there has been questions to support the old Visual Studio. It wouldn't be so hard to do by combining code in this SDK and use this to do rest: http://www.fssnip.net/7SQ/title/Visual-Studio-2017-extension-Displaying-Light-Bulb-Suggestions-

But source-code-wise, would you accept PR containing dependences to Microsoft.VisualStudio.*.dll? Or what should be done, a new project with paket-references to files here?

Krzysztof-Cieslak commented 4 years ago

I don't know much about VS extensibility but... TBF, I wouldn't recommend doing this unless:

  1. You can access the VS Project System and get info about F# projects (FSharpProjectOptions) from it
  2. You can access instance of FSharpChecker that's used by F# editor support. (this is something old power tools couldn't do, and I assume this haven't really changed)

In Ionide/FSAC, because we control everything we can easily pass FSharpProjectOptions and FSharpChecker instance we use in there.

In the command-line tool, we just create own FSharpChecker and we we use ProjectSystem library to parse projects. Theoretically, you could do the same in VS extension... but it would potentially have huge performance implications not only on this feature but on whole VS experience - especially running 2 different project system that both will call MsBuild has potential to cause a lot of problems. But also trying to run 2 FSharpChecker instances in a single process won't work well - underlying reactor queue is singleton and 2 different sources of calls will probably cause a lot of issues.

Krzysztof-Cieslak commented 4 years ago

As mentioned by Phillip during F# Conf Q&A - hopefully, in the long term this will be solved by VS adapting FsAutoComplete as a base for F# editor tooling support.

deviousasti commented 4 years ago

It wouldn't be so hard to do by combining code in this SDK and use this to do rest: http://www.fssnip.net/7SQ/title/Visual-Studio-2017-extension-Displaying-Light-Bulb-Suggestions-

Suggestions API isn't too bad, but the tagging, error list and updates, it turns out are a bit more involved that that.

As @Krzysztof-Cieslak said, the biggest hurdle to perf right now is that VF# uses its own checker, and not FCS. Although double-checking, we can get sort-of-okay perf by hosting FCS in VS now.

Even if VS adapts FCS, there would need to be VSIX infrastructure for this. Internally VS uses the Roslyn analyzers SDK, using the Microsoft.CodeAnalysis.ExternalAccess.FSharp package (which isn't available to the public). We could implement analyzers as exporting IFSharpDocumentDiagnosticAnalyzer, which like the FSharp.Analyzers.SDK takes in a context and returns a set of Diagnostic.

We'd get text tagging, warnings in the error list, code fix suggestions etc., for free. But alas, it's not public, nor is it likely to be.

To start with, taking a perf hit is probably better than nothing. (I guess?)

How does Ionide currently 'discover' the various analyzers referenced by the project? I was planning to integrate support for the analyzers SDK into https://github.com/deviousasti/fsharp-linting-for-vs/

Any suggestions a TODO list to integrate this? Do we refactor FSharpLint to use the analyzers SDK?

baronfel commented 4 years ago

How does Ionide currently 'discover' the various analyzers?

FSAC (which Ionide uses) has a package reference to the FSharp.Analyzers.SDK.Client in this repo, then calls this member to do the actual scanning/registration at startup and when the configuration changes.

Then, as files are checked in the background, there's an event inside FSAC that's fired and the configured analyzers are run on the file using this member.

deviousasti commented 4 years ago

Thank you for the prompt response. I think Step 1 would be to either refactor FSharpLint to the analyzers SDK, or create a facade.

dsyme commented 4 years ago

I've started an RFC discussing how to get F# analyzers incorporated into FSHarp.Compiler.Service, i.e. available in all F# tooling

https://github.com/fsharp/fslang-design/blob/master/tooling/FST-1033-analyzers.md

The main issue is binary compatibility for the information being drawn from the FCS API, so analyzers can be binary compatible components.

dsyme commented 4 years ago

See also https://github.com/ionide/FSharp.Analyzers.SDK/issues/28

deviousasti commented 4 years ago

@dsyme Can we have fixes as lazy (similar to FSharpLint)?

type Message =
      { ...
      Range: Range.range
      Fixes: lazy Fix list }

Fixes are only needed when a user opens a CodeFix suggestion.

dsyme commented 4 years ago

@dsyme Can we have fixes as lazy (similar to FSharpLint)?

Yes I've mentioned that in the RFC

Thorium commented 4 years ago

I find the Fix type of FromText : string; ToText : string is a bit hard to populate in real life.

I found FSharpExpr my analyzer was looking for, and I'd like to replace...wait, I don't know the original text FromText, only FSharpExpr. And how would I construct something with correct identifier to have the ToText?

Maybe provide a DU: Either the strings, or something more AST-like?

Krzysztof-Cieslak commented 4 years ago

Yes, fixes shouldn't be provided as string - in current implementation it's the result of me not having enough time to implement AST based version rather than design choice