Support check-style flag on fantomas-tool

[lpedrosa]

Summary

Add CLI support for check-style flag much like other code formatters out there. This would allow fantomas to run in a CI environment and check if the codebase is consistently formatted.

Description

In a team setting, it is desirable to have a consistent codebase. Many other languages have formatters available, here are some examples:

• gofmt
• rustfmt
• black

All of these formatters have a way to specify that we are only interested in understanding if any of the files scanned requires formatting:

• rustfmt and black have a --check flag, which exits with a code other than 0, if any of the files requires formatting.
• gofmt has an -l flag, which outputs only the filenames of the files that require formatting.

Both these approaches can be used in a CI pipeline step, in order to keep formatting consistency in a codebase.

I believe the FakeHelpers module provides a similar task if you are using FAKE.

Would it be possible to bring some of this logic to the CLI?

Proposal

Consider a simple program with a folder structure:

MySolution.sln
src/
    MyProgram/
        Helpers.fs
        Program.fs
        MyProgram.fsproj
        

Assuming we forgot to run fantomas on our project, when we run it with the --check flag:

$ dotnet fantomas ./src/MyProgram/ --recurse --check
Files that need formatting:
src/MyProgram/Helpers.fs
src/MyProgram/Program.fs

The exit code could then be checked by any CI scripts and fail the build

$ echo $?
1

I am available to help with this, if you are interested.

[nojaf]

Hello @lpedrosa, this sounds like a good idea but I'm not sure I want this in Fantomas just yet.
Maybe in the future when Fantomas is more stable, some projects now still have lots of issues after formatting. People might have false positives because the code doesn't format on their local machine anyway.

I would also like to see this in practice first to get a good feel of how this would work out in on a real project.

So my suggestion here it the following:
Could you create a proof of concept in a FAKE script? I would like to try this first in my own projects.
I believe all the building blocks to create something are there and the code for the POC could later maybe end up in Fantomas itself.

I hope you can understand my reasoning here, let me know what you think.

[lpedrosa]

That makes sense, thanks for the detailed explanation!

Could you create a proof of concept in a FAKE script?

I am not very familiar with FAKE but it's on my list of things to pick up, since most of the F# projects seem to use it. Just to clarify some things before I begin:

• When you mean a POC using FAKE, do you mean a new build task that behaves just like what was described above i.e. outputting the files that require formatting?
• Do you want me to add some sample CI code on how one could use this, or we should just keep it simple for now?

[nojaf]

With POC in FAKE I mean create a FAKE target that does the format check you have in mind.

See https://github.com/fsprojects/fantomas/blob/master/fake-sample/script.fsx for example.
It would be nice if you could create a target like Target.create "CheckCodeFormat" (fun _ -> ()) .
Do the validation inside that function and throw an error if some code is not formatted.

Build agents can then call dotnet fake run nameofscript.fsx -t CheckCodeFormat.
If you have a sample somewhere with the FAKE code that should suffice.

[lpedrosa]

Hey @nojaf, I apologise for the delay.

Here an example project: https://github.com/lpedrosa/fantomas-check

Most of the logic is in the build.fsx. I have also added a dockerfile in, since I wanted to check the task output on Linux (I'm currently using a windows machine).

Let me know what you think.

[nojaf]

Hey @lpedrosa , this looks like a nice sample. Thanks! I'll try it out in my own projects later this week.
Maybe it is also an option to do a similar check with the last write timestamp of the file.
I'm not entirely sure but I think there is some internal check and Fantomas will not write the file to disk if the formatted content is the same.

[nojaf]

@lpedrosa I've added the sample you provide to one of my projects, https://github.com/nojaf/trivia-tool/blob/master/build.fsx#L70

I suggest that I play around with it for some time and see how it goes.

[lpedrosa]

@lpedrosa I've added the sample you provide to one of my projects, https://github.com/nojaf/trivia-tool/blob/master/build.fsx#L70

Hi @nojaf, I will have a look!

Maybe it is also an option to do a similar check with the last write timestamp of the file.

I'm not sure I follow what you meant here. Are you suggesting to do this check before calling the CheckCodeFormat task?

[nojaf]

Hey @lpedrosa , I was thinking of replacing CodeFormatter.FormatDocumentAsync by formatFileAsync and do some sort of check based on doing some sort of check based on the timestamp to quickly find out whether any file did change.

The replacing part still seems like a good idea, the timestamp check thing not.

[lpedrosa]

I think I got what you are saying. If I am not mistaken, the processSourceString function in the fantomas cli tool is not checking if the formatted content is valid fsharp code.

In that case, you are right pointing out that the FakeHelpers.formatFileAsync is a better alternative since:

• it gives clients, the ability to understand if a file was Formatted, it remained Unchanged or if it contains an Error
• it doesn't overwrite the original file on error, since it uses a temp file

Is that a good analysis of your proposal?

[lpedrosa]

I was also wondering: is there a way to make the abstraction returned by the formatFilesAsync become a disposable resource? People using this function might forget to clean the temp files it creates.

The formatCode task and the processSourceString function could then consume the result by doing something like:

use! results = files |> formatFileAsync config
// rest of the function
// ...
// temp files generated would be cleaned when the function scope finishes

[nojaf]

@lpedrosa it would be great if we check that the formatted output is valid AST. I'm in favour of replacing the current cli to have this behaviour. That might resolve my initial concert that Fantomas doesn't always product valid AST after format.
So in short, the --checked should not throw if the output of Fantomas isn't valid AST or throws an exception.

To move forward:

• Replace the current CodeFormatter.FormatDocumentAsync by formatFileAsync
• Modifiy formatFileAsync to clean up the temp files
• Add --check as cli command
• Add a FakeHelper to expose the same functionality as --check in the cli.
• Try and write some unit tests for the new FakeHelper
• Update documentation & samples

@lpedrosa do you want to take the lead on this in a PR? I'm happy to help out where needed.

[lpedrosa]

Sound great, I'll keep you posted with any questions I might have. One final question:

So in short, the --checked should not throw if the output of Fantomas isn't valid AST or throws an exception.

Do you mean that if the --checked flag encounters an invalid AST or an exception, while checking the format, it should make sure that it wraps that up in a nice error message to be displayed to the user, rather than a stack trace?

[nojaf]

Yes, so in case the user tries --checked with a valid F# file and after formatting, Fantomas messed something up which causes the formatted output not to be valid anymore. If that scenario happens a friendly message should be shown inviting the user to submit a bug via the online tool.

I would still return exitcode 0 if this happens as this isn't the user's fault.

[VivekRajagopal]

Thank you for looking into this. I am investigating adding a pre-commit git hook to an F# project, and catch if files are not properly formatted, without actually modifying the file. This proposal would solve that issue, unless there is a current workaround.

[nojaf]

Hey @VivekRajagopal , if you want a solution today you can check out @lpedrosa his sample FAKE script. I'm using it myself here.

Otherwise you'll need to wait until we have an implementation for --checked 😉.

[nojaf]

@lpedrosa you can test this with https://www.nuget.org/packages/Fantomas/3.3.0-beta-002

[lpedrosa]

I've tested it out the CLI. I only noticed now that I missed something on the return values, which will not match the docs or the usage string:

Don't format files, just check if they have changed. Exits with 0 if it's formatted correctly, with 1 if some files need formatting and 99 if there was an internal error

Right now it's always exiting with 99 if the files have no error but need formatting. Should be a question of swapping it around and fixing the tests. Do you want me to do it @nojaf (in another PR) or do you want to use your magical "master push powers 🎩"?

I have also created a branch on my fantomas-check project, so I could test the new API, but I'm getting a weird MissingMethodException:

 System.MissingMethodException: Method not found: 'FSharp.Compiler.SourceCodeServices.FSharpChecker FSharp.Compiler.SourceCodeServices.FSharpChecker.Create(Microsoft.FSharp.Core.FSharpOption`1<Int32>, Microsoft.FSharp.Core.FSharpOption`1<Boolean>, Microsoft.FSharp.Core.FSharpOption`1<Boolean>, Microsoft.FSharp.Core.FSharpOption`1<Resolver>, Microsoft.FSharp.Core.FSharpOption`1<Microsoft.FSharp.Core.FSharpFunc`2<System.Tuple`2<System.String,System.DateTime>,Microsoft.FSharp.Core.FSharpOption`1<System.Tuple`3<System.Object,IntPtr,Int32>>>>, Microsoft.FSharp.Core.FSharpOption`1<Boolean>, Microsoft.FSharp.Core.FSharpOption`1<Boolean>)'.
   at [email protected](Unit unitVar)
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
--- End of stack trace from previous location where exception was thrown ---
   at System.Lazy`1.CreateValue()
   at [email protected](Unit unitVar)
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvoke[T,TResult](AsyncActivation`1 ctxt, TResult result1, FSharpFunc`2 part2) in E:\A\_work\130\s\src\fsharp\FSharp.Core\async.fs:line 398
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in E:\A\_work\130\s\src\fsharp\FSharp.Core\async.fs:line 109

Can you double check if that's happening with you @nojaf? Could be something with my lock files.

[jindraivanek]

I'm getting a weird MissingMethodException

This is caused by FSharp.Compiler.Service version mismatch. You need to pin version of FSharp.Compiler.Service to version that Fantomas is using (34.0.1 in this case).

[lpedrosa]

You need to pin version of FSharp.Compiler.Service to version that Fantomas is using (34.0.1 in this case).

That seemed to fix it, when I tried locally.

It does seem to resolve to 34.1 on the lock file, example when we don't pin the version.

Is this a common gotcha with how fake dependencies are pulled by paket? Ideally, the user wouldn't need to know about the compiler service at all.

[nojaf]

Closing this issue as the original functionality was implemented.
Keeping #686 to discuss the pinned FCS version problem.