search

fsharp / fslang-design

RFCs and docs related to the F# language design process, see https://github.com/fsharp/fslang-suggestions to submit ideas
517 stars 144 forks source link

[style-guide] Breaking of complex pattern match expressions #725

Open cmeeren opened 1 year ago

cmeeren commented 1 year ago

This is from the F# compiler:

match ty with
| SynType.App (typeName, _, typeArgs, _, _, _, _)
| SynType.LongIdentApp (typeName, _, _, typeArgs, _, _, _) -> typeName :: typeArgs |> List.tryPick (traverseSynType path)
| SynType.Fun (argType = ty1; returnType = ty2) -> [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.MeasurePower (ty, _, _)
| SynType.HashConstraint (ty, _)
| SynType.WithGlobalConstraints (ty, _, _)
| SynType.Array (_, ty, _) -> traverseSynType path ty
| SynType.StaticConstantNamed (ty1, ty2, _)
| SynType.Or (ty1, ty2, _, _) -> [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.Tuple (path = segments) -> getTypeFromTuplePath segments |> List.tryPick (traverseSynType path)
| SynType.StaticConstantExpr (expr, _) -> traverseSynExpr [] expr
| SynType.Paren (innerType = t)
| SynType.SignatureParameter (usedType = t) -> traverseSynType path t
| SynType.Anon _
| SynType.AnonRecd _
| SynType.LongIdent _
| SynType.Var _
| SynType.StaticConstant _ -> None

Apart from a few spaces, this is also how Fantomas formats it. I'm ignoring those spaces in the following.

The expression is complicated enough that I'm having a hard time separating the clauses from the bodies. This is complicated by the fact that some clauses are joined (they use the same ->), and others are not. The code is not easily readable.

I don't have a definite general rule that can be applied, but I certainly find this more readable:

match ty with
| SynType.App (typeName, _, typeArgs, _, _, _, _)
| SynType.LongIdentApp (typeName, _, _, typeArgs, _, _, _) ->
    typeName :: typeArgs |> List.tryPick (traverseSynType path)
| SynType.Fun (argType = ty1; returnType = ty2) ->
    [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.MeasurePower (ty, _, _)
| SynType.HashConstraint (ty, _)
| SynType.WithGlobalConstraints (ty, _, _)
| SynType.Array (_, ty, _) ->
    traverseSynType path ty
| SynType.StaticConstantNamed (ty1, ty2, _)
| SynType.Or (ty1, ty2, _, _) ->
    [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.Tuple (path = segments) ->
    getTypeFromTuplePath segments |> List.tryPick (traverseSynType path)
| SynType.StaticConstantExpr (expr, _) ->
    traverseSynExpr [] expr
| SynType.Paren (innerType = t)
| SynType.SignatureParameter (usedType = t) ->
    traverseSynType path t
| SynType.Anon _
| SynType.AnonRecd _
| SynType.LongIdent _
| SynType.Var _
| SynType.StaticConstant _ ->
    None

Here, I have placed all bodies on separate lines. This is of course not a rule that can be applied generally (it would look worse than single-line placement for short/simple match expressions).

The following also look better, IMHO. Here, bodies that apply to clauses with more than one line are placed on a separate line. Perhaps that can be a general rule?

match ty with
| SynType.App (typeName, _, typeArgs, _, _, _, _)
| SynType.LongIdentApp (typeName, _, _, typeArgs, _, _, _) ->
    typeName :: typeArgs |> List.tryPick (traverseSynType path)
| SynType.Fun (argType = ty1; returnType = ty2) -> [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.MeasurePower (ty, _, _)
| SynType.HashConstraint (ty, _)
| SynType.WithGlobalConstraints (ty, _, _)
| SynType.Array (_, ty, _) ->
    traverseSynType path ty
| SynType.StaticConstantNamed (ty1, ty2, _)
| SynType.Or (ty1, ty2, _, _) ->
    [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.Tuple (path = segments) -> getTypeFromTuplePath segments |> List.tryPick (traverseSynType path)
| SynType.StaticConstantExpr (expr, _) -> traverseSynExpr [] expr
| SynType.Paren (innerType = t)
| SynType.SignatureParameter (usedType = t) ->
    traverseSynType path t
| SynType.Anon _
| SynType.AnonRecd _
| SynType.LongIdent _
| SynType.Var _
| SynType.StaticConstant _ ->
    None

To be clear, in this particular case, I still find it less readable than the "always break" alternative. For example, I have to look thorouthly to see that | SynType.StaticConstantExpr (expr, _) -> traverseSynExpr [] expr is a clause and a body, and not just a long clause.

I'm not heavily invested in this, but thought I'd raise the issue when I noticed it.

nojaf commented 1 year ago

I'm not heavily invested in this, but thought I'd raise the issue when I noticed it.

Please respect the rules regarding style guide changes.

Adjustments to the style guide should generally only be made with consideration about their implementability in Fantomas, and if an adjustment is approved you should be prepared to contribute a matching pull request to Fantomas.

If there is no interest to commit, you should not propose any changes.