Confusing symmetry between infix operators

[cmeeren]

Issue created from fantomas-online

Code

let shouldIncludeRelationship relName =
    req.Includes |> List.exists (fun path ->
      path.Length >= currentIncludePath.Length + 1
      && path |> List.take (currentIncludePath.Length + 1) = currentIncludePath @ [relName]
    )

Result

let shouldIncludeRelationship relName =
    req.Includes
    |> List.exists (fun path ->
        path.Length
        >= currentIncludePath.Length
        + 1
        && path
           |> List.take (currentIncludePath.Length + 1) = currentIncludePath @ [ relName ])

Problem description

The following:

path.Length
>= currentIncludePath.Length
+ 1

Should be on one line:

path.Length >= currentIncludePath.Length + 1

Also, this is weirdly formatted (line break and indentation):

&& path
   |> List.take (currentIncludePath.Length + 1) = currentIncludePath @ [ relName ])

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 07/20/2020 08:56:02 - d42a156

Name	Value
IndentSize	4
MaxLineLength	120
SemicolonAtEndOfLine	false
SpaceBeforeParameter	true
SpaceBeforeLowercaseInvocation	true
SpaceBeforeUppercaseInvocation	false
SpaceBeforeClassConstructor	false
SpaceBeforeMember	false
SpaceBeforeColon	false
SpaceAfterComma	true
SpaceBeforeSemicolon	false
SpaceAfterSemicolon	true
IndentOnTryWith	false
SpaceAroundDelimiter	true
MaxIfThenElseShortWidth	40
MaxInfixOperatorExpression	50
MaxRecordWidth	40
MaxArrayOrListWidth	40
MaxValueBindingWidth	40
MaxFunctionBindingWidth	40
MultilineBlockBracketsOnSameColumn	false
NewlineBetweenTypeDefinitionAndMembers	false
KeepIfThenInSameLine	false
MaxElmishWidth	40
SingleArgumentWebMode	false
AlignFunctionSignatureToIndentation	false
AlternativeLongMemberDefinitions	false
StrictMode	false

[knocte]

Should be on one line:

Why? Because compilation breaks?

I think you can workaround this by increasing MaxInfixOperatorExpression

[cmeeren]

Why? Because compilation breaks?

Sorry the short description. Compilation is fine, but I find

path.Length
>= currentIncludePath.Length
+ 1

to be very confusing to read. There shouldn't be visual symmetry between >= and +. The >= operator is the main thing going on here, with path.Length and currentIncludePath.Length + 1 being the two arguments. So if line breaks were really needed, it would be better with

```f#
path.Length
>= currentIncludePath.Length
   + 1

Ideally currentIncludePath.Length + 1 should be on one line here. I tried increasing MaxInfixOperatorExpression to 120 with no effect.

---

By the way, I misread the intentation here:

Also, this is weirdly formatted (line break and indentation):

&& path
   |> List.take (currentIncludePath.Length + 1) = currentIncludePath @ [ relName ])

I think the indentation here makes sense.

---

Sorry about the issue title, by the way 😅

[nojaf]

Hey @cmeeren

So

Should be on one line:

path.Length >= currentIncludePath.Length + 1

This is a bit point of view I'm afraid. From a logic perspective, I'd yes agreed. However, Fantomas has no idea what operators like >= and + mean, it only knows that there is an expression with multiple operators that is longer than the threshold @knocte mentioned.

There a few exceptions to this: newLineInfixOps and noBreakInfixOps .
In those cases, we always break or never break.

We could add >= and <= to those lists but that might still not give your the desired result.

Not sure what other options we have here.

[cmeeren]

This is a bit point of view I'm afraid. From a logic perspective, I'd yes agreed. However, Fantomas has no idea what operators like >= and + mean

(Emphasis mine) So it's not just a matter of taste, it's more an implementation challenge?

I'll think about this and get back to you.

[nojaf]

Indeed, Fantomas has no idea what your code does and if it even makes sense.
For example, we can format valid syntax where inside the project the code does not compile.
That is why it is very difficult to make better decisions here.

[cmeeren]

I think this may be solved by Fantomas knowing about operator precedence.

Quasi algorithm:

• If line is too long, find lowest precedence operator and break on that (break on all operators with the same precedence)
• If some of the broken lines are still too long, find the next lowest precedence operator(s) in those lines and break on those, but indent as demonstrated in this issue

Do this recursively, of course.

[nojaf]

So if we have a + b * c where whole expression is too long:

• We split a + b and c as * takes precedence.
• a + b is not too long for example
• the outcome would be:

a + b
* c

Like that?

[knocte]

If * takes precedence over + (which I guess it is the case for F#, to mimic math), then IMO the splitting should be done in the opposite way:

a +
b * c

EDIT: which is actually what @cmeeren was proposing as algorithm in his last comment

[nojaf]

@knocte yes indeed, my bad, thanks for the correction.
Wishful thinking there is something exposed in the F# compiler (FCS) that can help us here.

[cmeeren]

If not, perhaps the order of operators could be hardcoded for now.

[nojaf]

Hey, I'm trying to give this issue a shot by having a better way of splitting the infix operators.
I'm working something out along the lines of

type Associativity =
    | Right
    | Left
    | NonAssociative

type InfixOperatorMeta =
    { Priority: int
      Associativity: Associativity
      PreferOnNewline: bool }

// Not sure if a fixed map is workable
// https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/symbol-and-operator-reference/#operator-precedence

let operatorMeta =
    [ "+", { Priority = 1; Associativity = Left; PreferOnNewline = false }
      "-", { Priority = 1; Associativity = Left; PreferOnNewline = false }
      "*", { Priority = 2; Associativity = Left; PreferOnNewline = false }
      "|>", { Priority = 3; Associativity = Left; PreferOnNewline = false }
      "=", { Priority = 3; Associativity = Left; PreferOnNewline = false }
      "&&", { Priority = 4; Associativity = Left; PreferOnNewline = false } ]
    |> Map.ofList

And then in the CodePrinter you try and split on the lowest priority.
I've started with some easy samples to wrap my head around it:

[<Test>]
let ``simple math`` () =
    formatSourceString false """let myValue = a + b * c
"""   { config with MaxInfixOperatorExpression = 5 }
    |> prepend newline
    |> should equal """
let myValue =
    a +
    b * c
"""

[<Test>]
let ``simple math in one line`` () =
    formatSourceString false """let myValue = a + b * c
"""   { config with MaxInfixOperatorExpression = 50 }
    |> prepend newline
    |> should equal """
let myValue = a + b * c
"""

[<Test>]
let ``simple math reversed`` () =
    formatSourceString false """let myValue = a * b + c
"""   { config with MaxInfixOperatorExpression = 5 }
    |> prepend newline
    |> should equal """
let myValue =
    a * b
    + c
"""

[<Test>]
let ``multiple sum operators`` () =
    formatSourceString false """let myValue = a + b * c + d
"""  { config with MaxInfixOperatorExpression = 5 }
    |> prepend newline
    |> should equal """
let myValue =
    a +
    b * c +
    d
"""

Now the first thing I'm struggling with it deciding when an operator should be added before or after the expression.
How to be consistent here? Always operators at the back? I don't think that always work if you take:

let x =
    a * b +
    c

same problem for always at the front

let x =
    a
    + b * c

I'm open for suggestions on this.

[cmeeren]

Now the first thing I'm struggling with it deciding when an operator should be added before or after the expression.

IMHO code is easier to read if the operators are at the front. But in any case, if you treat all operators the same (not just arithmetic operators), you kinda have to, because no-one wants:

let exec f x =
  x |>
  f

The only sensible option here is:

let exec f x =
  x
  |> f

same problem for always at the front

let x =
    a
    + b * c

I don't see any problem with this. Of course, it's a bit artificial since the line is so short, but given MaxInfixOperatorExpression = 5 this is what I would expect and want.

[knocte]

IMO |> should be considered a special operator. That is, the way I see it, it should be formatted as:

let exec f x =
    x
    |> f

and

let x =
    a +
    b * c

[cmeeren]

I then suggest that it be made configurable whether to break with operators at the end or at the front.

[cmeeren]

FWIW, the reason I find front operators more readable is a combination of 1) operators we break at are more "important" for readability given their associativity in the hierarchy, and 2) placing them at the front makes them easier to see since a) they are all aligned, and b) you don't have to hunt for them far to the right if you have long lines.

[nojaf]

I then suggest that it be made configurable whether to break with operators at the end or at the front.

No, just no 🙃. Sorry, this is really unmaintainable to have a setting for each operator. Especially when users can have custom operators.

Maybe Fantomas can store an opinioned preferred location for some operators and have a default at the front.

But then again, I agreed with @cmeeren his last remarks.

[cmeeren]

No, just no 🙃. Sorry, this is really unmaintainable to have a setting for each operator. Especially when users can have custom operators.

I meant one setting for all operators. Completely agree that one setting per operator is nuts. 😛

But then again, I agreed with @cmeeren his last remarks.

👍

[nojaf]

@cmeeren I was able to resolve the original issue and published an alpha on NuGet. Could you let me know if anything weird popped up in other places.

[cmeeren]

Thanks! I'll let you know.