Skip to content
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

Visual Studio 2017/2019 support? #23

Open
Thorium opened this issue Jun 6, 2020 · 11 comments
Open

Visual Studio 2017/2019 support? #23

Thorium opened this issue Jun 6, 2020 · 11 comments
Labels
question Further information is requested

Comments

@Thorium
Copy link
Contributor

Thorium commented Jun 6, 2020

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
Copy link
Member

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 Krzysztof-Cieslak added the question Further information is requested label Jun 6, 2020
@Krzysztof-Cieslak
Copy link
Member

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
Copy link

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
Copy link
Collaborator

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
Copy link

deviousasti commented Aug 18, 2020

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
Copy link

dsyme commented Oct 26, 2020

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
Copy link

dsyme commented Oct 26, 2020

See also #28

@deviousasti
Copy link

@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
Copy link

dsyme commented Oct 26, 2020

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

Yes I've mentioned that in the RFC

@Thorium
Copy link
Contributor Author

Thorium commented Oct 26, 2020

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants