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

[style-guide] when expressions in match clauses #662

Open
nojaf opened this issue May 18, 2022 · 15 comments
Open

[style-guide] when expressions in match clauses #662

nojaf opened this issue May 18, 2022 · 15 comments

Comments

@nojaf
Copy link
Contributor

nojaf commented May 18, 2022

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
Copy link
Contributor

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
Copy link
Contributor

dsyme commented May 18, 2022

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
Copy link
Contributor

dsyme commented May 18, 2022

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)

@dsyme dsyme changed the title [style-guide] when epxressions in match clauses [style-guide] when expressions in match clauses May 18, 2022
@nojaf
Copy link
Contributor Author

nojaf commented May 18, 2022

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
Copy link
Contributor

dsyme commented May 18, 2022

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
Copy link
Contributor Author

nojaf commented May 18, 2022

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
Copy link
Contributor

dsyme commented May 18, 2022

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

@dsyme
Copy link
Contributor

dsyme commented May 18, 2022

Agreed this can be a known limitation

@auduchinok
Copy link
Contributor

auduchinok commented May 18, 2022

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
Copy link
Contributor Author

nojaf commented May 18, 2022

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
Copy link
Contributor

dsyme commented May 18, 2022

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
Copy link
Contributor

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

I would certainly vote for it! 🙂

@dsyme
Copy link
Contributor

dsyme commented May 26, 2022

I looked at this again and am still inclined to follow the rules here: #662 (comment)

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
Copy link
Contributor Author

nojaf commented May 30, 2022

"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
Copy link

Banashek commented Jul 7, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants