[style-guide] when expressions in match clauses

[nojaf]

Hello,

The current style guide does not give any guidance on where to put the when expression in match clauses. Some examples:

match x with
| Y y when isOdd y -> "y is odd"
| Z z -> z

In small expressions, it makes sense to put the when expr after the pattern. There is a bit of a problem when the when expr is getting large or contains multiple lines. Some samples

My personal view on this is if you have a multiline when expression, you should probably refactor it to a partial active pattern and encapsulate your logic there.

Regardless, what advice should be given when this problem arises? I'm interested in the position of the when keyword, newlines and/or indentations of the multiline expression and the position of the -> arrow.

Thoughts @dsyme and community?

[vzarytovskii]

My personal preference is to always (even if it's only one) have when on a separate line, I find it a bit easier to notice and read. I do agree though, that if it gets complex, we should encourage people to use AP.

[dsyme]

I'm fairly inconsistent about this and jazz format a lot. A random selection of examples from dotnet/fsharp

        | ForEachThen (isFromSource, firstSourcePat, firstSource, JoinOrGroupJoinOrZipClause(nm, secondSourcePat, secondSource, keySelectorsOpt, pat3opt, mOpCore), innerComp) 
            when 
               (let _firstSourceSimplePats, later1 = 
                    use _holder = TemporarilySuspendReportingTypecheckResultsToSink cenv.tcSink
                    ...) ->
            Some ...

and

        | ILMemberAccess.FamilyOrAssembly | ILMemberAccess.Assembly
            when cenv.hasInternalsVisibleToAttrib -> true

and

        | OptionalSequential (JoinOrGroupJoinOrZipClause (_, _, _, _, _, mClause), _) 
                      when firstTry = CompExprTranslationPass.Initial ->

and

                | Expr.App (Expr.Link {contents = Expr.App (Expr.Val (ValDeref v, _, _), _, tyargs, [], _) }, _, [], args, m)  
                     when localRep.IsValRepresentedAsMethod v && not (cenv.recUses.ContainsKey v) -> 

and

        match vspec.MemberInfo with
        | None when
            vspec.IsBaseVal ||
            vspec.IsMemberThisVal && vspec.LogicalName = "__" -> false
        | _ -> true

As you can see, almost every imaginable style

[dsyme]

Perhaps

1. Format on one line if possible
2. Format on two lines if possible with when at start of second line 4-spaces in
3. Otherwise format on multiple lines with when at start of second line 4-spaces in, and multi-line predicate starting on 3rd line 8-spaces in.

That would give:

I'm fairly inconsistent about this and jazz format a lot. A random selection of examples from dotnet/fsharp
```fsharp
        | ForEachThen (isFromSource, firstSourcePat, firstSource, JoinOrGroupJoinOrZipClause(nm, secondSourcePat, secondSource, keySelectorsOpt, pat3opt, mOpCore), innerComp) 
            when 
                (let _firstSourceSimplePats, later1 = 
                     use _holder = TemporarilySuspendReportingTypecheckResultsToSink cenv.tcSink
                     ...) ->
            Some ...

and

        | ILMemberAccess.FamilyOrAssembly | ILMemberAccess.Assembly
            when cenv.hasInternalsVisibleToAttrib -> true

and

        | OptionalSequential (JoinOrGroupJoinOrZipClause (_, _, _, _, _, mClause), _) 
            when firstTry = CompExprTranslationPass.Initial ->

and

                | Expr.App (Expr.Link {contents = Expr.App (Expr.Val (ValDeref v, _, _), _, tyargs, [], _) }, _, [], args, m)  
                     when localRep.IsValRepresentedAsMethod v && not (cenv.recUses.ContainsKey v) -> 

and

        match vspec.MemberInfo with
        | None
            when
                vspec.IsBaseVal ||
                vspec.IsMemberThisVal && vspec.LogicalName = "__" ->
            false
        | _ -> true

I think the expression on the right of the -> should always be on the next line for either (2) or (3)

[nojaf]

That sounds reasonable to me. I might prototype this in Fantomas to verify there are no edge cases. Something just comes to mind where the -> needed to be on the next line to remain valid. Maybe this case doesn't occur anymore since the relaxations of F# 6.

[dsyme]

Something just comes to mind where the -> needed to be on the next line to remain valid.

I'm not aware of any cases needing this. I checked this case where the expression after when is non-atomic (has no definite end token)

let f () =
   match 1 with
   | _
       when
           let x = 1
           x > x -> 3
   | _ -> 3

[nojaf]

I raise you:

let f () =
   match 1 with
   | _
       when
           match () with
           | _ -> true ->
       2
   | _ -> 3

This could be an acceptable limitation, covered by the style guide. I mean, people shouldn't write this kind of code.

[dsyme]

Yes, you're right, well-spotted :)

[dsyme]

Agreed this can be a known limitation

[auduchinok]

I find it more clear when when is on the pattern line, as it make it easier to see the condition:

| ILMemberAccess.FamilyOrAssembly | ILMemberAccess.Assembly when
    cenv.hasInternalsVisibleToAttrib -> true

It also goes much more inline with other parts of the language where we place the keyword on the preceding line, like:

while condition do
    expression

if condition then
    expression

match x with
| somePattern ->
    expression

and not like:

while condition
    do expression
       someOtherOne

while condition
    do
        expression

if condition
    then expression

match x with
| somePattern
    -> expression

It may be a problem when you want to place the right hand side expression on a new line, though, as it'll currently have the same indent as the when expression. I've seen a trick that further indents code in similar cases and found it working very nice with F# as well:

match f a b c with
| someLongPattern, andAnotherOne when
        let value = 1 + 2
        someCondition ->
    let a = someFunc someArg [] true
    anotherFunc a 123 false

If a style like this was automatically used by a formatter and by an IDE Enter typing assists, that could be a good option to consider.

[nojaf]

The reasoning to keep the when after the pattern makes sense to me. The double indent for the expression seems a bit foreign in comparison to the rest of the style guide.

match a with
| b when
    c ->
    d

If c is long, I'm not overly bothered that it aligns with d. For the people that are, again partial active pattern is your friend.

[dsyme]

For me the problem is that this leads to significant visual confusion between the expression on the right of the -> and the when expression:

| ILMemberAccess.FamilyOrAssembly | ILMemberAccess.Assembly when
    cenv.hasInternalsVisibleToAttrib ->
    a>b

compared to:

| ILMemberAccess.FamilyOrAssembly | ILMemberAccess.Assembly
    when cenv.hasInternalsVisibleToAttrib ->
    a>b

It only feels applicable if we decide we want small expressions on the right of the ->

[auduchinok]

It only feels applicable if we decide we want small expressions on the right of the ->

I would certainly vote for it! 🙂

[dsyme]

I looked at this again and am still inclined to follow the rules here: https://github.com/fsharp/fslang-design/issues/662#issuecomment-1129906464

There are real readability issues with the when at the end. In order to understand the role of the guard and target expressions easily and simply, you need to know they are guard and target expressions. That means seeing the when and the -> and having appropriate indentation to help

I'll leave this open to discuss further however as both @nojaf and @auduchinok have said "put the when at the end" :)

[nojaf]

"when" at the start does also make sense to me. The rules above work for me, I don't really have a strong opinion about any of this.

[Banashek]

In the cases provided earlier in the thread, I believe that when is being compared with then, do, and with.

I mentally bucket when into a different category of keywords than the latter above, since I frame it as a branching qualifier/indicator, rather than a divisor between a block-validation construct and the subsequent block.

I think about how in some other languages, they can omit keywords like then, do, and with (see: python), but wouldn't be able to get away with getting rid of when unless it was replaced with some other sort of symbol or logical identifier.