[style-guide] Vanity alignment used when using long case in match block

[knocte]

Issue created from fantomas-online

Code

match foo with
| SomeVeryLongMatchCase(1234567890, 1234567890, 1234567890, 1234567890, 1234567890, 1234567890, 1234567890, 1234567890, 1234567890) ->
    bar()
| _ -> ()

Result

match foo with
| SomeVeryLongMatchCase (1234567890,
                         1234567890,
                         1234567890,
                         1234567890,
                         1234567890,
                         1234567890,
                         1234567890,
                         1234567890,
                         1234567890) -> bar ()
| _ -> ()

Expected Result

match foo with
| SomeVeryLongMatchCase
    (
        1234567890,
        1234567890,
        1234567890,
        1234567890,
        1234567890,
        1234567890,
        1234567890,
        1234567890,
        1234567890
    ) -> bar()
| _ -> ()

Extra information

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

Options

Fantomas 4.6 branch at 10/24/2021 17:25:41 - 3032b536608ba8f2b37cc261230747fdb06c73ed

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]

Hey, I know you mean well, but this is getting a bit tiresome.
Too often is the banner of "vanity alignment" is used to point of the stylistic shortcomings of Fantomas.
Though often these are valid, they really should be clarified first with the guide.
Not a single word of guidance is given for this case in the guide.
The expected result is again not validated nor discussed at all. It is just the first thing that someone came up with that does not raise warnings. I want to see the rationale behind this in a guide.

Looking at the guide, the word "vanity" isn't even in there anymore.
The process should really be as followed:

1. Open an issue in the F# docs and ask for guidance.
2. Discuss until everything is clear.
3. Open a PR in the docs repo to address the new information.
4. Wait until the new change in the guide is published.
5. Open a Fantomas issue and point to the specific example that is not being respected.

In this specific case, I believe a lot will depend on the positive impact of https://github.com/fsharp/fslang-design/blob/main/FSharp-6.0/FS-1108-undentation-frenzy.md.

So please, do the homework first before opening issues here. This really impacts a lot of codebases if we change this behaviour.
One general complaint people have is that each Fantomas release just always shuffles something around.
Where some people welcome this as an improvement, others really only see this as noise in their git history.

Thanks for understanding.

[hvester]

While I agree that changes to the default formatting should be carefully considered I would not like to see this kind of things added to the style guide. There are probably hundreds of rare formatting corner cases and if all of them are added to the style guide then it simply gets too long in my opinion. If it is too long it does not work as learning material about how F# code should be formatted.

[knocte]

If it is too long it does not work as learning material about how F# code should be formatted.

Thanks, that's what was exactly my thinking when filing all this bugs, given that vanity-alignment was already discouraged in the guide, as a general principle. Even if the word "vanity" is not used anymore, I think the principle has still remained, if not many code samples.

[nojaf]

probably hundreds of rare formatting corner cases

This totally isn't one, this involves SynPat.LongIdent which has different offset rules in F# 5 than expressions.
As F# 6 will deal with this differently it should totally be discussed in the style guide.

I would not like to see this kind of things added to the style guide

If Fantomas implements the style guide correctly, there is literally zero learning curve to it.
Heck, this is even mentioned in the guide itself.
And the only way to ensure all topics are covered is to have them in the style guide.

To reiterate the impact of this is way too large not to have been justified up front in the style guide.
And as mentioned in our contribution guidelines: style is not to be discussed here.

So either you open an issue in the MS docs repository or I close this one.

[hvester]

probably hundreds of rare formatting corner cases

This totally isn't one, this involves SynPat.LongIdent which has different offset rules in F# 5 than expressions. As F# 6 will deal with this differently it should totally be discussed in the style guide.

Thanks for the clarification.

I would not like to see this kind of things added to the style guide

If Fantomas implements the style guide correctly, there is literally zero learning curve to it. Heck, this is even mentioned in the guide itself. And the only way to ensure all topics are covered is to have them in the style guide.

I still think that the style guide should not become a Fantomas specification covering everything to the smallest detail. There are scenarios and reasons not to use Fantomas and thus having a simple and easy to read style guide is helpful. Though I also think that deciding about changes to the default formatting based only on a discussion in GitHub issue isn't necessarily the optimal way.

[knocte]

@dsyme do you mind if I ask you your opinion on this? Let me know if you agree with the expected result here. FYI my company has implemented a fix for this in our internal fantomas fork and this is the result as an example of the improvement applied in real code:

https://gist.github.com/knocte/5ccb3101f199f041fc6381f47d9eed65

As you can see, it not only fixes the vanity alignment, it also mitigates excessive horizontal deviation to the right side.

If this looks ok, I will create PR first for the style guide, and later I'll propose our patch as a PR.

[dsyme]

I would have thought the default result should at least align with the deafult formatting of the expression that corresponds to the patternm e.g.

SomeVeryLongMatchCase(
    1234567890, 
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890)

That may mean vanity alignment, which you won't enjoy, or else this:

match foo with
| SomeVeryLongMatchCase(
    1234567890, 
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890) ->
    bar()
| _ -> ()

[nojaf]

As of F# 6 this is valid as well:

match foo with
| SomeVeryLongMatchCase(
    1234567890, 
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890,
    1234567890
  ) ->
    bar()
| _ -> ()

The nice thing is that it would match the behaviour of regular function applications (example).

[dsyme]

Yes, that's what I meant actually, thanks

[knocte]

The nice thing is that it would match the behaviour of regular function applications

Actually, my initial proposal matches also the current behaviour of fantomas, but not for function applications but for functions that receive tuples (which matches more with what we're talking about here). Example here.

This solution would be compatible with older versions of F# (and in fact, my team cannot upgrade to v6.0 yet :( ).

[nojaf]

Interestingly enough, this is now also valid code:

module M =
    let longFunctionWithLongTupleParameter(
        aVeryLongParam: AVeryLongTypeThatYouNeedToUse,
        aSecondVeryLongParam: AVeryLongTypeThatYouNeedToUse,
        aThirdVeryLongParam: AVeryLongTypeThatYouNeedToUse
    ) =
        ()

I understand the need of being compatible with older versions yet at the same not make use of FS-1108 feels like a wasted opportunity as well.

The broader question would be: "What F# versions should the style guide support?".
I would need to think more about all this, to be honest.

[dsyme]

The broader question would be: "What F# versions should the style guide support?".

We should support the latest major version, and perhaps adding notes to the style guide for the previous major version when we remember to. The rest of the docs also do this

[knocte]

and perhaps adding notes to the style guide for the previous major version

TBH I don't mind if no notes are added about previous versions in the style guide, however, can we still have a flag for fantomas that tries to be more conservative to still support older versions? Something like --legacyLang... My team would be willing to support/maintain this flag.

[nojaf]

I don't think Fantomas should carry the burden of older versions. If you are using an older language version, you should use an older Fantomas version that is compatible.
It doesn't seem all that pragmatic to change a style twice (once for legacy and once for the current language version).

Any development on Fantomas side where F# 6 relaxations to the indentation rules are used should probably be part of the next major version.

PS: you don't necessarily need to target the .NET 6 runtime to use F# 6 features. And NETCore 3.1 LTS ends in December 2022 (source). So, I think we can be a little easy going here.

[knocte]

PS: you don't necessarily need to target the .NET 6 runtime to use F# 6 features.

It's not about that. F#6 hasn't landed in Debian yet.

[dsyme]

On the style-guide question - I will update the style guide to indicate that multi-line patterns should be formatted in the same say as multi-line applications, like this.

Does that seem reasonable?

It's possible that due to differences in indentation-aware processing there may be some cases where this is not technically possible - do we know of any with F# 6?

[nojaf]

Nothing comes to mind here, I believe this seems reasonable.

[nojaf]

Another interesting case here would be named fields in a pattern match.

Consider:

[<Test>]
let ``SynTypeDefn with member with set/get`` () =
    let parseResults = 
        getParseResults
            """
type A() =
member this.Z with set (_:int):unit = () and get():int = 1
"""

    match parseResults with
    | ParsedInput.ImplFile (ParsedImplFileInput (modules = [ SynModuleOrNamespace.SynModuleOrNamespace(decls = [
        SynModuleDecl.Types(
            typeDefns = [ SynTypeDefn(typeRepr = SynTypeDefnRepr.ObjectModel(members = [
                SynMemberDefn.ImplicitCtor _
                SynMemberDefn.GetSetMember(Some (SynBinding(headPat = SynPat.LongIdent(extraId = Some getIdent))),
                                           Some (SynBinding(headPat = SynPat.LongIdent(extraId = Some setIdent))),
                                           m,
                                           { WithKeyword = mWith
                                             GetKeyword = Some mGet
                                             AndKeyword = Some mAnd
                                             SetKeyword = Some mSet })
            ])) ]
        )
    ]) ])) ->
        Assert.AreEqual("get", getIdent.idText)
        Assert.AreEqual("set", setIdent.idText)
        assertRange (3, 18) (3, 22) mWith
        assertRange (3, 23) (3, 26) mSet
        assertRange (3, 23) (3, 26) setIdent.idRange
        assertRange (3, 45) (3, 48) mAnd
        assertRange (3, 49) (3, 52) mGet
        assertRange (3, 49) (3, 52) getIdent.idRange
        assertRange (3, 4) (3, 62) m
    | _ -> Assert.Fail "Could not get valid AST"

This isn't formatted all the nicely today by Fantomas. (sample)

Maybe the following makes sense:

match x with
| Y (z = patZ) -> ()

• Try all in one line, respect maximum line length.

Y (z = patZ)

• If there is only one named field, keep the field name on the same line, and indent the pattern on the next line.

Y (z =
    patZ
)

• If there are multiple named fields, indent and new line everything.

Y (
    z = patZ // patZ is short
    a =
        patA // patA is long
)

---

Another interesting question about the indentation, in general, is if it should be relative to the start of the pattern of the bar of the match clause.

match x with
| Y (
    ... // column is 4
 ) // notice that the closing `)` should at least be 1 position further than the `|`
| Z (
      ... // column is 6 because `Z` started at 2
 ) // notice that the closing `)` should at least be 1 position further than the `|`

Y respects the "indentation flow" (every line starts on a multitude of the indentation size), while Z does not.

[nojaf]

Hello @dsyme, I made a prototype of the ideas above at fsprojects/fantomas#2645.
I think it is all reasonable and I would be interested to hear your thoughts on the matter.

[dsyme]

Looking at the tests it looks great! :)

[nojaf]

@dsyme I added some more tests in fsprojects/fantomas@5a7b75f

It can grow a little in the wild: dotnet/fsharp@d048025
Although I'm not sure how much the average F# user will notice this.
Pattern matching the syntax tree does look less common to me.

[dsyme]

@nojaf Ouch, yes, that is long. Sigh. Still I think it's the right decision. People just have to use active patterns to control this complexity

[dsyme]

This cascading is really problematic isn't it. Hmmm..... https://github.com/dotnet/fsharp/commit/d0480259b450a9d8e398346a12e93be4961343d6#diff-6fb95f66781ae87d118ad76ae0d[…]88674bea9c7401fcb6282b6R125-R137

Likewise https://github.com/dotnet/fsharp/commit/d0480259b450a9d8e398346a12e93be4961343d6#diff-7d548bc33531ee5fc089fbefa82[…]ae741ac7e4a5fcc11780c87R240-R253

[nojaf]

@dsyme I still need to chew on this some more, as these multiline patterns are a fascinating space.

I stumbled upon this example this morning:

match x with
| [| Y(itemOne =
        OhSomeActivePatternThing(
            a, b
        )
    )
    S(
        one,
        two,
        three,
        four,
        five
    ) |] -> ()

Imagine that the max_line_length is short and Y, OhSomeActivePatternThing and S don't fit on a single line.

I like the proposed formatting here, as every indent is based on the |. Each line in the pattern is positioned with a multiline of the indent_size.
The drawback is that Y and S are not aligned inside the array.

You could move everything inside the array to the next line/indent:

match x with
| [|
    Y(itemOne =
        OhSomeActivePatternThing(
            a, b
        )
    )
    S(
        one,
        two,
        three,
        four,
        five
    )
  |] -> ()

---

On a side note, I do think patterns inside a match clause should have slightly different rules, as opposed to perfectly mimicking what would happen inside applications.

For example, if an array/list pattern is the sole child of a long ident application, using Stroustrup does make sense to keep it short:

match parseResults with
| ModuleName.PatternName([
    SomethingElse(
        a,
        // comment
        b,
        c
    ) ]) ->
    assertRange (2, 21) (2, 22) mSlash
    assertRange (2, 21) (2, 29) mTuple
| _ -> Assert.Fail $"Could not get valid AST, got {parseResults}"

The same goes for a single named pattern (ModuleName.PatternName(x = [).

Yet, I'm not convinced this should be the case for every single scenario where you have an array/list/record somewhere in a pattern.
For example, I would not bring any Stroustrup into:

match x with
| [
    [|
      Foo
      Bar
    |]
    [|
      Foo
      Barry
    |]
  ] ->
    ()

[nojaf]

@dsyme a real-world example.

Another thing to note is that I'm using the max_line_length as a threshold to deceive whether an array/list/record should be multiline. Previously, we were using the specific settings that are now in play for expressions.

[dsyme]

In the linked real-world example - everything looks ok. I get that it's not identical to expressions though I can't quite rationalize why.

For arrays:

You could move everything inside the array to the next line/indent:

This feels more natural to me.