FSharp.Analyzers.SDK icon indicating copy to clipboard operation
FSharp.Analyzers.SDK copied to clipboard

Visual Studio 2017/2019 support?

Open Thorium opened this issue 4 years ago • 11 comments

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?

Thorium avatar Jun 06 '20 09:06 Thorium

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 avatar Jun 06 '20 10:06 Krzysztof-Cieslak

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.

Krzysztof-Cieslak avatar Jun 06 '20 11:06 Krzysztof-Cieslak

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?

deviousasti avatar Aug 17 '20 19:08 deviousasti

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.

baronfel avatar Aug 17 '20 19:08 baronfel

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

deviousasti avatar Aug 18 '20 12:08 deviousasti

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 avatar Oct 26 '20 18:10 dsyme

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

dsyme avatar Oct 26 '20 18:10 dsyme

@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.

deviousasti avatar Oct 26 '20 19:10 deviousasti

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

Yes I've mentioned that in the RFC

dsyme avatar Oct 26 '20 19:10 dsyme

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?

Thorium avatar Oct 26 '20 19:10 Thorium

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

Krzysztof-Cieslak avatar Oct 26 '20 21:10 Krzysztof-Cieslak