Create an API layer for external process #637

MrLuje · 2023-12-17T21:07:56Z

~~## Warning, I branched from the NET 6 version, so #606 should be merged first (ignore the first 4 commits for the review)~~

WIP on #627, it covers the following points:

Create the lib layer (Add FSharpLint.Client project)
Expose a version method for the Client/Console flow
I guess it should also be enough to publish FSharpLint.Client nuget

Add --daemon flag to FSharpLint.Console
All the "tool location detection" and "process handling" is copied from Fantomas project and adapted, basically:

try to locate FSharpLint.Console as a Local DotnetTool from current file, then as a Global DotnetTool then in PATH
keep a cache of already running daemon (per version)
communicate between FSharpLint.Client and the FSharpLint.Console daemon through JsonRpc

~~I'll rebase everything once #606 is merged~~

knocte · 2023-12-19T09:41:17Z

Good work @MrLuje!

I'll rebase everything once #606 is merged

Thanks, I plan to re-review and/or merge that PR (or an equivalent) this week, bare with me.

MrLuje · 2023-12-27T12:36:11Z

Added an FSHARPLINT_SEARCH_PATH_OVERRIDE env var that will mostly be used for testing that can override search location (global tool & local tool) and a test to ensure FCS is not referenced by FSharpLint.Client

knocte · 2023-12-28T09:26:56Z

Hello @MrLuje sorry for the delay here. Today I merged the NET5->NET6 transition. Can you now rebase this PR?
Thanks!

MrLuje · 2023-12-28T18:31:03Z

@knocte done

src/FSharpLint.Client/LSPFSharpLintServiceTypes.fsi

tests/FSharpLint.Client.Tests/TestClient.fs

src/FSharpLint.Client/FSharpLintToolLocator.fs

src/FSharpLint.Client/LSPFSharpLintServiceTypes.fs

src/FSharpLint.Client/Contracts.fsi

src/FSharpLint.Client/LSPFSharpLintServiceTypes.fs

knocte · 2024-01-08T02:34:28Z

@MrLuje side-note, the conflict is actually caused because there is a change in the 1st commit of this PR that has some unnecessary whitespace noise hehe:

diff --git a/src/FSharpLint.Console/FSharpLint.Console.fsproj b/src/FSharpLint.Console/FSharpLint.Console.fsproj
index e70053133..f97b5e02b 100644
--- a/src/FSharpLint.Console/FSharpLint.Console.fsproj
+++ b/src/FSharpLint.Console/FSharpLint.Console.fsproj
@@ -1,9 +1,7 @@
-<Project Sdk="Microsoft.NET.Sdk">
-
+<Project Sdk="Microsoft.NET.Sdk">
   <PropertyGroup>
     <OutputType>Exe</OutputType>
     <TargetFrameworks>net6.0</TargetFrameworks>
-
     <Title>FSharpLint.Console</Title>
     <Description>Console application to run FSharpLint.</Description>
     <PackageTags>F#;fsharp;lint;FSharpLint;fslint;cli</PackageTags>
@@ -14,15 +12,15 @@
     <IsPackable>true</IsPackable>
     <RollForward>Major</RollForward>
   </PropertyGroup>
-
   <ItemGroup>
     <Compile Include="Output.fs" />
+    <Compile Include="Version.fs" />
+    <Compile Include="Daemon.fs" />
     <Compile Include="Program.fs" />
   </ItemGroup>
-
   <ItemGroup>
     <ProjectReference Include="..\FSharpLint.Core\FSharpLint.Core.fsproj" />
+    <ProjectReference Include="..\FSharpLint.Client\FSharpLint.Client.fsproj" />
   </ItemGroup>
-
   <Import Project="..\..\.paket\Paket.Restore.targets" />
-</Project>
+</Project>
\ No newline at end of file

knocte · 2024-01-15T07:25:42Z

@knocte done

Oh, interesting, CI failed on your fork (macOS job), but I can't see the log because I guess I don't have permissions. Can you share it?

MrLuje · 2024-01-15T20:03:46Z

@knocte done

Oh, interesting, CI failed on your fork (macOS job), but I can't see the log because I guess I don't have permissions. Can you share it?

Not a lot more information from my side (it timed out after 6h, maybe a runner issue ?)

No more issue after a retry

knocte · 2024-01-30T03:22:44Z

@MrLuje sure, I'll do it this week. BTW can you rebase the PR? looks like there is some conflict

Hey @MrLuje, sorry for our delayed action on this, I just pushed a commit that introduces a File type, similar to Folder.

Now, there are a couple of things to fix here:

Early on when you published this PR for the first time, I saw a potential improvement about some lines of code you wrote, but I realised that the "improvable code fragment" could actually be detected by FSharpLint itself, and in fact we had a rule in the works to detect this. This rule is now merged to master, so if you rebase this PR, you will see the offending code and CI will tell you what to change and how :)
After working on the File type explained above, I've spotted another piece of code that is a bit confusing because it seems to be conflating files with folders, I will go ahead now (after I post this comment) to the "Files changed" tab and point it out there with more context.

knocte · 2024-01-30T03:25:16Z

src/FSharpLint.Client/LSPFSharpLintService.fs

+    let handleFile filePath =
+        if not (isPathAbsolute filePath) then
+            Error FSharpLintServiceError.FilePathIsNotAbsolute
+        else match Folder.from filePath with


@MrLuje see the code here above? it is feeding a "filePath" to the Folder type. So is it a file or a folder? if it's a folder, it should be renamed from filePath to dirPath.

It's a folder (getting the folder of a file)

@MrLuje ok this is confusing, I thought the point of the from function was to convert a path(string) to a proper type. So, in my mind, Folder.from would map a (folderPath:string) to a (folder:Folder), and so that's why we included a File type equivalent to the Folder type that has a From method that works this way.

If we want to get the folder where a file lives, I think we should create an API that is more similar to the already existing BCL's System.IO API, to avoid confusions. So, in System.IO, the way to do this is convert the filePath:string to a FileInfo, and then accessing the .Directory property. So how about changing Folder's From method to receive a folderPath (not filePath; when given a filePath it should error saying that it is a file, not a folder). So if in this piece of code you want to get the folder where certain path lives, just create it via File.From and then let's create a Folder property in File type?

I fear that it might be weird from a discovery POV (signature is a Folder but no clue that you have to go through a File before)
What about if Folder.From can handle both a filePath and a folderPath ? or a FromFile / FromFolder ?

@MrLuje I like the FromFile/FromFolder idea, let's do it

Done
I missed it when changing to FileInfo but I don't need Path.GetFullPath anymore, FileInfo handles it properly

…ludes FCS)

Introduce File type that checks existence of file on creation and is used in FSharpLintExecutableFile instead of string path.

MrLuje · 2024-02-02T21:21:29Z

@MrLuje sure, I'll do it this week. BTW can you rebase the PR? looks like there is some conflict

Hey @MrLuje, sorry for our delayed action on this, I just pushed a commit that introduces a File type, similar to Folder.

Now, there are a couple of things to fix here:

Early on when you published this PR for the first time, I saw a potential improvement about some lines of code you wrote, but I realised that the "improvable code fragment" could actually be detected by FSharpLint itself, and in fact we had a rule in the works to detect this. This rule is now merged to master, so if you rebase this PR, you will see the offending code and CI will tell you what to change and how :)

After working on the File type explained above, I've spotted another piece of code that is a bit confusing because it seems to be conflating files with folders, I will go ahead now (after I post this comment) to the "Files changed" tab and point it out there with more context.

Got it, cool new rule :)

knocte · 2024-02-04T03:44:19Z

src/FSharpLint.Client/FSharpLintToolLocator.fs

+
+    match startProcess processStart with
+    | Ok daemonProcess ->
+        // fsharplint:disable-next-line RedundantNewKeyword


@MrLuje why did you disable this rule several times in this PR? did you find a bug?

Maybe ? It is disposable, so explicite new is recommanded (as a warning)
Without it, I have :

Usage of new keyword here is redundant.F# Linter FL0014

Without the new :

It is recommended that objects supporting the IDisposable interface are created using the syntax 'new Type(args)', rather than 'Type(args)' or 'Type' as a function value representing the constructor, to indicate that resources may be owned by the generated value F# Compiler 760

Maybe ? It is disposable, so explicite new is recommanded (as a warning) Without it, I have :

Dear @MrLuje,

Thank you for bringing this issue to our attention. I attempted to replicate the warning you reported by eliminating the following commented line:

// fsharplint:disable-next-line RedundantNewKeyword

However, I did not encounter any warnings.

For further details, please refer to the following branch and check the CI status:

https://github.com/Mersho/FSharpLint/commits/api_layer_version_WithoutDisableRule

@MrLuje hey MrLuje, Mershad is right, see his branch (he edited his commit to add the link) where he removed the 2 comments disabling the rule and CI is still green

Hum, probably my local setup then...
Just to make sure to understand properly, if I go to https://try.fsharp.org/ and type

module Tour.Functions open System.IO let s = StreamReader("")

I get the same warning :

It is recommended that objects supporting the IDisposable interface are created using the syntax 'new Type(args)', rather than 'Type(args)' or 'Type' as a function value representing the constructor, to indicate that resources may be owned by the generated value

Since warnings as errors is enabled in FSharpLint project, I expect the issue I locally have

FSharpLint/Directory.Build.props

Lines 3 to 4 in 08ceae7

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>

<MSBuildTreatWarningsAsErrors>true</MSBuildTreatWarningsAsErrors>

Am i missing something ?

@MrLuje not sure I understand what you're saying. You're bringing up a code snippet that uses a type which is IDisposable, therefore the new keyword is needed. That's why you get the warning (from F# compiler).

The rule RedundantNewKeyword is, though, for cases when you're using the new keyword in types that are not IDisposable. In those cases, using the keyword new is not needed. But if you do use it, you don't get a warning from the F# compiler telling you that you don't need it. That's why RedundantNewKeyword exists: to tell you this.

Did I miss something?

@MrLuje anyway the strange thing here is that you're saying that if you use the new keyword, then you get an FSharpLint suggestion, and this is what Mersho refuted above: by showing you a commit whose CI is not red (which means our SelfCheck target passes with no errors; that is, FSharpLint rules are not flagging anything). So please just cherry-pick Mersho's commit from his branch here and push it to your branch.

@MrLuje not sure I understand what you're saying. You're bringing up a code snippet that uses a type which is IDisposable, therefore the new keyword is needed. That's why you get the warning (from F# compiler).

The rule RedundantNewKeyword is, though, for cases when you're using the new keyword in types that are not IDisposable. In those cases, using the keyword new is not needed. But if you do use it, you don't get a warning from the F# compiler telling you that you don't need it. That's why RedundantNewKeyword exists: to tell you this.

Did I miss something?

Nope, totally agree with this analysis 👍
I noticed something else (and I may be missing something about how FSharpLint works) :

running analysis on the project or solution doesn't trigger it

$ dotnet src/FSharpLint.Console/bin/Release/net6.0/dotnet-fsharplint.dll lint --file-type project src/FSharpLint.Client/FSharpLint.Client.fsproj ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/obj/Debug/net6.0/.NETCoreApp,Version=v6.0.AssemblyAttributes.fs ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/obj/Debug/net6.0/FSharpLint.Client.AssemblyInfo.fs ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/Contracts.fsi ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/Contracts.fs ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/LSPFSharpLintServiceTypes.fsi ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/LSPFSharpLintServiceTypes.fs ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/FSharpLintToolLocator.fsi ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/FSharpLintToolLocator.fs ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/LSPFSharpLintService.fsi ========== ========== Finished: 0 warnings ========== ========== Linting /home/vince/src/github/mrluje/FSharpLint.worktrees/rw/make_it_all_6/src/FSharpLint.Client/LSPFSharpLintService.fs ========== ========== Finished: 0 warnings ========== ========== Summary: 0 warnings ==========

running it only on FSharpLintToolLocator.fs triggers it

$ dotnet src/FSharpLint.Console/bin/Release/net6.0/dotnet-fsharplint.dll lint src/FSharpLint.Client/FSharpLintToolLocator.fs ========== Linting src/FSharpLint.Client/FSharpLintToolLocator.fs ========== Usage of `new` keyword here is redundant. Error on line 226 starting at column 22 let handler = new HeaderDelimitedMessageHandler( ^ See https://fsprojects.github.io/FSharpLint/how-tos/rules/FL0014.html -------------------------------------------------------------------------------- `x = true` might be able to be refactored into `x`. Error on line 17 starting at column 53 if supportedRange.IsSatisfied(parsedVersion, includePrerelease = true) then ^ See https://fsprojects.github.io/FSharpLint/how-tos/rules/FL0065.html -------------------------------------------------------------------------------- ========== Finished: 2 warnings ========== ========== Summary: 2 warnings ==========

In any case, I agree with you there shouldn't be these rules disabled in the final code, I'm pushing Mersho's commit

MrLuje marked this pull request as ready for review December 27, 2023 16:10

MrLuje force-pushed the api_layer_version branch from 6365ff0 to b2c2b30 Compare December 28, 2023 18:29