Strange behavior with OCaml operators

abelbraaksma commented 2 years ago

Code

module X =
    let (land) x y = x + y
    let foo x y = x land y

Result

Note: this result is fine, using the online version, but it also shows the following message, but without the details, so I don't know if any formatting actually took place:

module X =
    let (land) x y = x + y
    let foo x y = x land y

The result I get locally, with a slightly older version of Fantomas, is:

module X =
    let ``land`` x y = x + y
    let foo x y = x ``land`` y

Problem description

The land etc as an operator was supposed to be deprecated by the F# compiler, but I could still create these without setting any local compiler flags, yet I saw my code auto-formatted as the second example above, which obviously won't compile (I have format-on-save on in VS 2022).

I'd totally understand if this is a non-issue, as it is supposed to be a deprecated construct. I just like to understand the CompilerMessage from the online Fantomas and whether constructs like above are indeed ignored by Fantomas, or treated as what they are: operators.

Extra information

[x] The formatted result breaks my code: partially, it's unclear what happened in the online version
[x] Fantomas online throws errors, or resurfaces compiler errors, unclear
[x] The formatted result gives compiler ~warnings~ errors.
[x] I or my company would be willing to help fix this.

Options

Fantomas master branch at 2022-07-19T14:35:52Z - 0fe6785076e045f28e4c88e6a57dd09b649ce671

Default Fantomas configuration

_{Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file? yes, I know ;)}

nojaf commented 2 years ago

Hello,

When Fantomas parses the initial code, it won't process if there are any errors. land and will yield a warning but we continue:

https://github.com/fsprojects/fantomas/blob/0fe6785076e045f28e4c88e6a57dd09b649ce671/src/Fantomas.Core/CodeFormatterImpl.fs#L15-L55

After formatting the code, we validate the result in:

https://github.com/fsprojects/fantomas/blob/0fe6785076e045f28e4c88e6a57dd09b649ce671/src/Fantomas.Core/Validation.fs#L1-L53

I believe currently that land warning is not added to safeToIgnoreWarnings. From Fantomas' point of view, this is rather irrelevant, as that problem will have been there, to begin with.

The Exception of type 'FSharp.Compiler.DiagnosticsLogger+UserCompilerMessage' was thrown. part comes from https://github.com/fsprojects/fantomas/blob/0fe6785076e045f28e4c88e6a57dd09b649ce671/src/Fantomas.FCS/Parse.fs#L1048-L1052

Where we are currently missing | :? UserCompilerMessage as ucm -> Some ucm.range, ucm.message, Some ucm.number to properly process the information.

And potentially we could add This construct is for ML compatibility. In previous versions of F# 'land' was a reserved keyword but the use of this keyword is now deprecated. You can disable this warning by using '--mlcompatibility' or '--nowarn:62'. to the set of safeToIgnoreWarnings.

Do note that fantomas-tool does not filter the diagnostics in https://github.com/fsprojects/fantomas-tools/blob/16f04a5ff9646af112c81a00e2841212da7b9b27/src/server/FantomasOnlinePreview/FormatCode.fs#L32-L62.

Lastly, previous versions of Fantomas (4.x and lower) used to check if the identifier needs backticks, for performance reasons we no longer have this check. Identifiers are all over the place, so being able to trust the source code has really improved the speed.

We only add backticks when the range and length of the identifier differ exactly 4 units.

https://github.com/fsprojects/fantomas/blob/0fe6785076e045f28e4c88e6a57dd09b649ce671/src/Fantomas.Core/CodePrinter.fs#L417-L427

If you are interested, I would accept a PR for some of the pointers above.

abelbraaksma commented 2 years ago

@nojaf, thanks for the insights! For some reason, FCS does not give me warnings about the land etc operators. Though I wouldn't dream of using them anyway, allegedly F# 6 added deprecation warnings and I was testing that out, which led to code being formatted as above, hence the report.

So this is basically already fixed in Fantomas 5, per your new way of testing for `` (double backticks) in code: basically leaving them in place when they're there. That only leaves Fantomas erroring out on the warning for FS0062 (which, again, I didn't get locally for some reason).

fsprojects / fantomas