Re-enable FSharpLint integration

[baronfel]

We should work to update our FSharpLint dependency and verify the integration still works.

• Uncomment in paket.dependencies and paket.references files
• Uncomment methods in FsAutoComplete.Lsp.fs, Commands.fs, and project files as necessary
• Uncomment the fsharplint tests group in the tests' Program.fs
• See what breaks

[Numpsy]

So, what's the current situation with FSharpLint integration (besides FSharpLint itself apparently having not been updated past .NET 5) ?

[baronfel]

It's currently disabled from an IDE integration perspective because the API of the compiler libraries it was built against and the version of those compiler libraries we use have drifted rather significantly. This is a consequence of the lack of API Binary Compatibility in the compiler libraries.

Fantomas has the same problem, but that tool has circumvented the problem by creating an API surface that doesn't directly expose the compiler services, instead delegating to an external process. This model could work with fsharplint as well.

[TheAngryByrd]

Yeah having a layer like Fantomas does currently was my thoughts as well but I don’t have the capacity to maintain another big repo like that.

[Numpsy]

Thanks for the update. Alas, I don't have enough to to do anything more that the odd fix here and there either

[MrLuje]

@baronfel, I'd like to start working on FSharpLint LSP layer.

Previous implementation was using lintParsedSource method with an already built AST & FSharpCheckFileResults
val lintParsedSource : optionalParams:OptionalLintParameters -> parsedFileInfo:ParsedFileInformation -> LintResult

I suppose we could switch to the lintFile method which only take the path to the target file and then use a custom model to avoid using FSC results.
val lintFile : optionalParams:OptionalLintParameters -> filePath:string -> LintResult

The downside is that the file will be parsed at least twice (by FSCA & by FSharpLint); is this fine or is there a better way ?

[baronfel]

This is basically the trade-off that we've made with Fantomas already - luckily parsing files is typically very fast. Typechecking is the really expensive thing, does FSharpLint do that as well?

[TheAngryByrd]

does FSharpLint do that as well?

From what I remember yeah it does. We could share an rsp file so it doesn't need to do additional cracking but unless we're on the same version of FCS, we're not gonna be able to share the typecheck results.

[MrLuje]

So, I made a rough implementation between FSharpLint (PR) and FSAC (branch).
Is there an easy way to measure the impact of the linting process for a given set of files ? I enabled FSharp.fsac.netCoreDllPath settings and played around FSAC repo but I found there are too many things running at the same time to properly measure it.

Another question, I think the previous implementation enabled linting based on 2 criteria :

• FSharpConfig.Linter config being enabled
• a file called fsharplint.json exists somewhere in the workspace; do we still want this condition ? (since FSharpLint embeds a default configuration)

[baronfel]

Re: measuring impact, the 'simplest' way would be to log a message with the elapsed duration in the linting method that's actually calling the FSharpLint service. Since this call happens out-of-proc, users shouldn't see much impact on their IDE operations - just make sure that your service can handle cancellation since if a user makes changes to a file we should cancel in-progress operations and start new ones. You can also create a System.Diagnostics.Activity and then your operation will appear in OTel traces when that mechanism is hooked up for perf analysis.

Regarding enablement, I think we can ensure that the config.Linter preference is defaulted to true, then potentially key off of a few additional criteria:

• does the fsharplint service exist? (meaning is the tool in the local .net tool config, or is it installed globally?)
• is there an explicit configuration present?
  if either of those is true then linting should proceed. At least that's my first thoughts.

It's great to see this progressing!

[MrLuje]

I made a few tests (I took FSAC project as test repo):

• viewing/editing a file takes around 100ms for the parsing/linting part (except for the 1st request that takes a few seconds to load everything)
• it gets pretty nasty when you save a file and it sends hundreds of lintrequest at the same time; linting takes from 5s to 20s per file (with critical path at 45s for the whole analysis)

gist with traces

I used the following vscode configuration during the test (anything that I should change ?)

		"FSharp.fsac.netCoreDllPath": "~/src/FsAutoComplete/bin/Release/net6.0/fsautocomplete.dll",
		"FSharp.fsac.attachDebugger": false,
		"FSharp.codeLenses.references.enabled": false,
		"FSharp.codeLenses.signature.enabled": false,
		"FSharp.lineLens.enabled": "never",
		"FSharp.trace.server": "messages",
		"Fsharp.fsac.gc.noAffinitize": true,
		"FSharp.openTelemetry.enabled": true,
		"FSharp.fullNameExternalAutocomplete": false,
		"FSharp.externalAutocomplete": true,
		"FSharp.linter": true,

I still have some fixes to make; right know several lint daemons are created instead of a single one for the solution but I don't think it will dramatically change the timings.

[TheAngryByrd]

it gets pretty nasty when you save a file and it sends hundreds of lintrequest at the same time; linting takes from 5s to 20s per file (with critical path at 45s for the whole analysis)

Yeah this is because we type check all dependent files on save as changes to a File higher up can cascade breaking changes. We don't have a different configurable strategy. Some people have asked in the past for typechecking only dependent open files or not type checking dependent files at all, but at the cost of being incorrect about any errors in dependent files. It's also runs them in parallel based on cpu cores which isn't configurable either.

The main question in my mind would be, if someone updates a file, do we need to run linting on dependent files? For instance, making a change to File001.fs might make cascading changes to File004.fs and File010.fs in terms of signature changes, would there be cases were linting would be beneficial to run on File004.fs and File010.fs. In theory, probably yes, but I'm not familiar enough with the current lint rules that practically it matters.

Either way, we should probably have some type of strategies on save that:

• Lint all dependent files
• Lint only open dependent files
• Lint only the current file
• Put items in some type of queue that only requests X lints a time.

[laurentpayot]

I spent hours trying to make FSharpLint work in Ionide. Then I saw this thread 🤦‍♂️
Any news?

[Numpsy]

There's some work going on at fsprojects/FSharpLint#637