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

Crash when formatting with config file #824

Closed
knocte opened this issue May 11, 2020 · 7 comments · Fixed by #1183
Closed

Crash when formatting with config file #824

knocte opened this issue May 11, 2020 · 7 comments · Fixed by #1183

Comments

@knocte
Copy link
Contributor

knocte commented May 11, 2020

PLEASE USE THE ONLINE TOOL TO REPORT BUGS!!

Sorry, I don't reproduce this problem with the online tool.

Repro code:

namespace GWallet.Backend.Tests

module Foo =

    let someFunc someArg =
        let someRetVal = someArg + 1
        someRetVal


    let someOtherFunc () =
        let var1withAVeryLongLongLongLongLongLongName, var2withAVeryLongLongLongLongLongLongName =
            someFunc 1, someFunc 2

        ()

Fantomas config file:

{
  "PageWidth":80
}

Call fantomas with:

fantomas --config ./fantomas-config.json Repro.fs

Results:

The following exception occurs while formatting stdin: Fantomas.FormatConfig+FormatException: Formatted content is not valid F# code
   at Fantomas.FakeHelpers.formatContentAsync@53-3.Invoke(Boolean _arg2) in /home/runner/work/fantomas/fantomas/src/Fantomas/FakeHelpers.fs:line 54
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, FSharpFunc`2 userCode, b result1) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 417
   at Fantomas.CodeFormatterImpl.isValidFSharpCode@377-7.Invoke(AsyncActivation`1 ctxt) in /home/runner/work/fantomas/fantomas/src/Fantomas/CodeFormatterImpl.fs:line 377
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 109
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.FSharp.Control.AsyncResult`1.Commit() in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 350
   at Microsoft.FSharp.Control.AsyncPrimitives.RunSynchronouslyInCurrentThread[a](CancellationToken cancellationToken, FSharpAsync`1 computation) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 882
   at Microsoft.FSharp.Control.AsyncPrimitives.RunSynchronously[T](CancellationToken cancellationToken, FSharpAsync`1 computation, FSharpOption`1 timeout) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 890
   at Microsoft.FSharp.Control.FSharpAsync.RunSynchronously[T](FSharpAsync`1 computation, FSharpOption`1 timeout, FSharpOption`1 cancellationToken) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 1154
   at Program.processSourceString(Boolean isFsiFile, String s, FSharpChoice`2 tw, FormatConfig config) in /home/runner/work/fantomas/fantomas/src/Fantomas.CoreGlobalTool/Program.fs:line 87
   at Program.stringToFile@273(Boolean force, Boolean profile, String s, String outFile, FormatConfig config) in /home/runner/work/fantomas/fantomas/src/Fantomas.CoreGlobalTool/Program.fs:line 281
@Bobface
Copy link
Contributor

Bobface commented May 12, 2020

@nojaf The reason for the error is a formatting error. This is the result of the formatting of the supplied source:

namespace GWallet.Backend.Tests

module Foo =

    let someFunc someArg =
        let someRetVal = someArg + 1
        someRetVal


    let someOtherFunc () =
        let var1withAVeryLongLongLongLongLongLongName,
            var2withAVeryLongLongLongLongLongLongName
            =
            someFunc 1, someFunc 2

        ()

The two long variables being split into two lines seems to be invalid F# - I tried to compile it and got an error. Moving both variable names into the same line makes it compile.

I would begin working on this issue by making sure that multiple variable names are always kept on the same line, including the = sign. Let me know what you think about that.

@nojaf
Copy link
Contributor

nojaf commented May 12, 2020

@Bobface what happens if you add ( ) instead?

    let someOtherFunc () =
        let (var1withAVeryLongLongLongLongLongLongName,
             var2withAVeryLongLongLongLongLongLongName)
            =
            someFunc 1, someFunc 2

        ()

@Bobface
Copy link
Contributor

Bobface commented May 12, 2020

@nojaf That compiles fine. Is this your preferred way?

@knocte
Copy link
Contributor Author

knocte commented May 12, 2020

Mmm, but if fantomas does this, it would be the first case I know that it adds parenthesis when they were not there already, which to me doesn't sound as the best approach.

@nojaf
Copy link
Contributor

nojaf commented May 12, 2020

Fantomas does this in other places as well.
Example: lazy keyword.

This is a trade-off situation. Putting it all on the same line will lead to someone opening an issue "hey the page width is not respected" or "in-consist style when tuple destructering is involved".
Having the extra parenthesis while they were not there in the first place, leads to your remark @knocte.

I would go for the parenthesis approach here.

@knocte
Copy link
Contributor Author

knocte commented May 12, 2020

I would go for the parenthesis approach here

This would make it even harder to come up with a solution for #684 :(

@nojaf
Copy link
Contributor

nojaf commented May 12, 2020

Not necessarily, the case where we add parenthesis is clearly defined and scoped to the area to tuple destructuring in let bindings. So I don't see an immediate correlation to #684 here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants