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

Function lambda with type annotation breaks code #2295

Closed
3 tasks done
abelbraaksma opened this issue Jun 9, 2022 · 5 comments · Fixed by #2558
Closed
3 tasks done

Function lambda with type annotation breaks code #2295

abelbraaksma opened this issue Jun 9, 2022 · 5 comments · Fixed by #2558

Comments

@abelbraaksma
Copy link
Member

Issue created from fantomas-online

Code

let f x y : int -> int = 
    fun _ -> x + y
    : int -> int

Result

let f x y : int -> int = fun _ -> x + y: int -> int

Problem description

I encountered this in a slightly more complex scenario, but this was the minimal repro I could come up with. If there's no type annotation on the function definition f, Fantomas will reformat and put the type annotation on the function, i.e.:

let f x y = 
    fun _ -> x + y
    : int -> int

correctly becomes:

let f x y : int -> int = fun _ -> x + y

However, if there's already a type annotation on the definition, and the lambda has a type annotation, the result of putting the whole thing on the same line breaks, as can be seen on top of this post.

Probably a niche scenario as clearly the type annotation is redundant, but still, running Fantomas shouldn't break code. Solution could be to put the lambda in parentheses, or to drop the type annotation (after all it is redundant, it's already there), or to not format it to one line.

Extra information

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

Options

Fantomas master branch at 2022-06-09T08:42:49Z - 25634b2

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?

Btw: I do know this, but for some reason, in VS 2022 at least, using .fantomasignore does not have any effect.

@abelbraaksma abelbraaksma changed the title Function lambda with type annotation breaks code<Insert meaningful title> Function lambda with type annotation breaks code Jun 9, 2022
@nojaf
Copy link
Contributor

nojaf commented Jun 9, 2022

Hello, thanks for reporting this. It is an edge case for sure, yet an interesting one on an AST level.

The additional : int -> int at the end, leads to the expression to be wrapped in a SynExpr.Typed.
You also get this node when you have a regular type annotation in a signature.

For example:

let x : int = 1

leads to:

SynBinding
 (None, Normal, false, false, [],
  PreXmlDoc ((5,0), FSharp.Compiler.Xml.XmlDocCollector),
  SynValData
    (None, SynValInfo ([], SynArgInfo ([], false, None)), None),
  Named (SynIdent (x, None), false, None, tmp.fsx (5,4--5,5)),
  Some
    (SynBindingReturnInfo
       (LongIdent (SynLongIdent ([int], [], [None])),
        tmp.fsx (5,8--5,11), [])),
  Typed
    (Const (Int32 1, tmp.fsx (5,14--5,15)),
     LongIdent (SynLongIdent ([int], [], [None])),
     tmp.fsx (5,14--5,15)), tmp.fsx (5,4--5,5),
  Yes tmp.fsx (5,0--5,15),
  { LetKeyword = Some tmp.fsx (5,0--5,3)
    EqualsRange = Some tmp.fsx (5,12--5,13) })

I always assumed this was duplicate information from the SynBinding that made things easier in other places from the compiler.

So, a bit to my surprise:

let x : int = 1 : int

gives

SynBinding
 (None, Normal, false, false, [],
  PreXmlDoc ((5,0), FSharp.Compiler.Xml.XmlDocCollector),
  SynValData
    (None, SynValInfo ([], SynArgInfo ([], false, None)), None),
  Named (SynIdent (x, None), false, None, tmp.fsx (5,4--5,5)),
  Some
    (SynBindingReturnInfo
       (LongIdent (SynLongIdent ([int], [], [None])),
        tmp.fsx (5,8--5,11), [])),
  Typed
    (Typed
       (Const (Int32 1, tmp.fsx (5,14--5,15)),
        LongIdent (SynLongIdent ([int], [], [None])),
        tmp.fsx (5,14--5,21)),
     LongIdent (SynLongIdent ([int], [], [None])),
     tmp.fsx (5,14--5,21)), tmp.fsx (5,4--5,5),
  Yes tmp.fsx (5,0--5,21),
  { LetKeyword = Some tmp.fsx (5,0--5,3)
    EqualsRange = Some tmp.fsx (5,12--5,13) })

Anyway, I would suggest detecting that you have a SynExpr.Typed with an inner expression SynExpr.MatchLambda, SynExpr.Match, SynExpr.MatchBang, SynExpr.IfThenElse and always add a newline to solve this.

| TypedExpr (Typed, e, t) ->
genExpr astContext e
+> sepColon
+> genType astContext false t

I wouldn't change it everywhere you find usage of the TypedExpr active pattern, for the reason above. Sometimes the SynExpr.Typed wasn't really there.

I hope this helps, are you interested in submitting a PR?

Btw: I do know this, but for some reason, in VS 2022 at least, using .fantomasignore does not have any effect.

Please open an issue for that here if your .fantomasignore isn't working from the command line as well or over at https://github.com/fsprojects/fantomas-for-vs/ if it is only in the editor.

@auduchinok
Copy link
Contributor

auduchinok commented Jun 9, 2022

@nojaf This should ideally be fixed in FCS tree (so it contains the actual tree structure without changing it), but until that is done you can try using this trick to recognize the wrapper expressions: https://github.com/JetBrains/resharper-fsharp/blob/b6faed78754be2001edf81878e2ca53bbbd19191/ReSharper.FSharp/src/FSharp.Psi.Features/src/Parsing/FSharpTreeBuilderBase.fs#L874-L881

@nojaf
Copy link
Contributor

nojaf commented Jun 9, 2022

@auduchinok I'd be in favour of that as well but changing

https://github.com/dotnet/fsharp/blob/db4bf11c5b28a7fb416e8e8a0d503742985da017/src/Compiler/SyntaxTree/SyntaxTreeOps.fs#L670-L680

feels like a landmine 😸.
Who knows how much code in the compiler/tooling will depend on this.

@auduchinok
Copy link
Contributor

Yes, it does 😅

I think that wrapping it like this could/should be done just before processing an expression in the type checker. That way it doesn't look that scary and would allow the tree to have the correct structure. 🙂

@nojaf
Copy link
Contributor

nojaf commented Jun 9, 2022

Right, that does seem more doable. A bit similar to the fix of dotnet/fsharp#13131, where the logic was essentially moved from the untyped tree to the typed tree. I might open an issue for this later on in the F# repo.

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