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

Cons pattern in let bindings is converted to invalid code #1996

Closed
1 of 3 tasks
clkbug opened this issue Dec 18, 2021 · 4 comments · Fixed by #2891
Closed
1 of 3 tasks

Cons pattern in let bindings is converted to invalid code #1996

clkbug opened this issue Dec 18, 2021 · 4 comments · Fixed by #2891
Labels
bug (soundness) good first issue Long hanging fruit: easy issue to get your feet wet!

Comments

@clkbug
Copy link
Contributor

clkbug commented Dec 18, 2021

Issue created from fantomas-online

Code

let x::y = [0]

Result

let (::) x, y = [ 0 ]

Problem description

After formatting, fantomas output the following error:

The following exception occurs while formatting stdin: Fantomas.FormatConfig+FormatException: Formatted content is not valid F# code
   at Fantomas.Extras.FakeHelpers.formatContentInternalAsync@95-5.Invoke(Boolean _arg2) in C:\Users\fverdonck\Projects\fantomas\src\Fantomas.Extras\FakeHelpers.fs:line 96
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, FSharpFunc`2 userCode, b result1) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 404
   at Fantomas.CodeFormatterImpl.isValidFSharpCode@377-7.Invoke(AsyncActivation`1 ctxt) in C:\Users\fverdonck\Projects\fantomas\src\Fantomas\CodeFormatterImpl.fs:line 377
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 104
--- End of stack trace from previous location ---
   at Microsoft.FSharp.Control.AsyncResult`1.Commit() in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 337
   at Microsoft.FSharp.Control.AsyncPrimitives.RunSynchronouslyInCurrentThread[a](CancellationToken cancellationToken, FSharpAsync`1 computation) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 870   
   at Microsoft.FSharp.Control.AsyncPrimitives.RunSynchronously[T](CancellationToken cancellationToken, FSharpAsync`1 computation, FSharpOption`1 timeout) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 878
   at Microsoft.FSharp.Control.FSharpAsync.RunSynchronously[T](FSharpAsync`1 computation, FSharpOption`1 timeout, FSharpOption`1 cancellationToken) in D:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 1142
   at Program.processSourceString(Boolean isFsiFile, String s, FSharpChoice`2 tw, FormatConfig config) in C:\Users\fverdonck\Projects\fantomas\src\Fantomas.CoreGlobalTool\Program.fs:line 114
   at Program.stringToFile@346(Boolean force, Boolean profile, String s, String outFile, FormatConfig config) in C:\Users\fverdonck\Projects\fantomas\src\Fantomas.CoreGlobalTool\Program.fs:line 358
Formatted content is not valid F# code

Extra information

  • The formatted result breaks by code.
  • The formatted result gives compiler warnings.
  • I or my company would be willing to help fix this.

Options

Fantomas 4.6 branch at 12/04/2021 18:09:39 - 9e35634

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?

@nojaf
Copy link
Contributor

nojaf commented Dec 18, 2021

Hello @clkbug, thank you for reporting this issue.
This is an interesting case from an AST point of view.

@dsyme I believe this is a case where the PreIdent would be useful.
If you have:

let x::y = [0]
let op_ColonColon x y = [0]

leads to

LongIdent (LongIdentWithDots ([op_ColonColon], []), None, None, ... )
LongIdent (LongIdentWithDots ([op_ColonColon], []), None, None, ... )

So, inside the pattern node, having the original syntax would be beneficial.
And when we detect the :: we should treat it as a special case in Fantomas.

@auduchinok
Copy link
Contributor

auduchinok commented Dec 20, 2021

It'd likely be easier to work with :: if it was a separate pattern case in FCS SyntaxTree, but it would probably require a lot of changes across the compiler.

@clkbug
Copy link
Contributor Author

clkbug commented Jun 2, 2023

This problem no longer seems to occur, doesn't it?

@nojaf
Copy link
Contributor

nojaf commented Jun 2, 2023

No, the AST got updated and SynPat.ListCons got introduced to represent this.

To close this issue properly, it would be great to include a regression test. That way, we can make sure that the changes made to the AST and the new SynPat.ListCons don't cause any unexpected issues or bugs.

If you're up for it, would you be interested in sending a PR (pull request)? It would be awesome to have your contribution to this fix!

@nojaf nojaf added the good first issue Long hanging fruit: easy issue to get your feet wet! label Jun 2, 2023
@nojaf nojaf linked a pull request Jun 2, 2023 that will close this issue
nojaf added a commit that referenced this issue Jun 2, 2023
* Add regression test for cons pattern in let binding (#1996)

* Correct test.

* Add changelog entry

---------

Co-authored-by: nojaf <florian.verdonck@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug (soundness) good first issue Long hanging fruit: easy issue to get your feet wet!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants