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

Broken comma in match expression in parameter invocation #1869

Closed
2 of 3 tasks
piaste opened this issue Aug 16, 2021 · 3 comments · Fixed by #2565
Closed
2 of 3 tasks

Broken comma in match expression in parameter invocation #1869

piaste opened this issue Aug 16, 2021 · 3 comments · Fixed by #2565
Labels
bug (soundness) good first issue Long hanging fruit: easy issue to get your feet wet!

Comments

@piaste
Copy link
Contributor

piaste commented Aug 16, 2021

Issue created from fantomas-online

Code

module Utils

type U = A of int | B of string

type C() = 
    member this.M (a : int, b : string) = ()

let f () = 
    let u = A 0
    do C().M(
        match u with
             | A i -> i
             | B _ -> 0
             ,
        match u with
             | A _ -> ""
             | B s -> s
    )

Result

module Utils

type U =
    | A of int
    | B of string

type C() =
    member this.M(a: int, b: string) = ()

let f () =
    let u = A 0

    do
        C()
            .M(
                match u with
                | A i -> i
                | B _ -> 0,
                match u with
                | A _ -> ""
                | B s -> s
            )

Problem description

When using a match expression as a parameter argument, the comma that separates parameters can be misinterpreted by the F# compiler as a tuple separator.

Options for disambiguating include:

  • Wrapping the match expression in parenthesis
  • Adding a newline after the match expression (as shown in the pre-format 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 Master at 08/11/2021 17:06:14 - 383b729

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 Aug 21, 2021

This is quite the horrible code you have there. I would advise against passing match expressions as arguments.
That being said I would accept a PR that extends the check we have in

and genTuple astContext es =
let genShortExpr astContext e =
addParenForTupleWhen (genExpr astContext) e
let shortExpression =
col sepComma es (genShortExpr astContext)
let longExpression =
let containsLambdaOrMatchExpr =
es
|> List.pairwise
|> List.exists
(function
| SynExpr.Match _, _
| SynExpr.Lambda _, _
| InfixApp (_, _, _, SynExpr.Lambda _), _ -> true
| _ -> false)
let sep =
if containsLambdaOrMatchExpr then
(sepNln +> sepComma)
else
(sepComma +> sepNln)
let lastIndex = List.length es - 1
let genExpr astContext idx e =
match e with
| SynExpr.IfThenElse _ when (idx < lastIndex) ->
autoParenthesisIfExpressionExceedsPageWidth (genExpr astContext e)
| _ -> genExpr astContext e
coli sep es (genExpr astContext)
atCurrentColumn (expressionFitsOnRestOfLine shortExpression longExpression)

so that in this case, the comma is also flipped.

@wuzzeb
Copy link

wuzzeb commented Nov 22, 2021

I think I also hit this issue but with function matching.

Old Code

module Util

open Aether

let inline non<'a when 'a : equality> (def : 'a) : Lens<'a option, 'a> =
  ( function
      | Some(a) -> a
      | None -> def
  , fun a _ -> if a = def then None else Some(a)
  )

Note that Aether's lens is a tuple of getter and setter so are defining a tuple of functions.

Result of formatting

let inline non<'a when 'a: equality> (def: 'a) : Lens<'a option, 'a> =
  (function
  | Some (a) -> a
  | None -> def, (fun a _ -> if a = def then None else Some(a)))

With the formatting, the F# compiler complains error FS0002: This function takes too many arguments, or is used in a context where a function is not expected

I worked around it by putting parens around the getter function, so fantomas formats it to

let inline non<'a when 'a: equality> (def: 'a) : Lens<'a option, 'a> =
  ((function
   | Some (a) -> a
   | None -> def),
   (fun a _ -> if a = def then None else Some(a)))

Which compiles but is still somewhat confusing to read since you don't instantly notice the comma

@nojaf
Copy link
Contributor

nojaf commented Sep 19, 2022

I believe this issue is fixed.
A regression test could close this issue.

@nojaf nojaf added the good first issue Long hanging fruit: easy issue to get your feet wet! label Sep 19, 2022
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