[Feature request] fsharp_max_if_then_short_width

[dsyme]

I propose we add fsharp_max_if_then_short_width. Style guide update TBD

Slack conversation with @nojaf:

@dsyme: I'm wondering if there should be two settings fsharp_max_if_then_short_width and fsharp_max_if_then_else_short_width. We kind of want short-if-then-else to only apply to functional code, not imperative code. Setting large fsharp_max_if_then_else_short_width gives things like

    if something then variable <- expr

The style guide should really be explicit that such imperative code should be multi-line, and if .. then ... is always imperative. Equally people sometimes prefer one line if condition then fail()

@nojaf: Should that always be on multiple lines if there is no else branch?

@dsyme: I think having fsharp_max_if_then_short_width with default 0 sounds right to me

@nojaf: I'll take a look at that.

@nojaf: So, a couple of things come to mind here:

• You could have a nested structure without else branch if a then b elif c

• The code printing the if/then/else is a bit complex and there are some obscured parts that take F# 5 offset rules into account. It might be good to revisit maybe a bit more than just the split of the setting.

• Can we detect imperative code from an AST point of view and should we act upon that?

Pros and Cons

The advantages of making this adjustment to Fantomas are more explicit formatting of some imperative code

The disadvantages of making this adjustment to Fantomas are not all cases of imperative code are covered

Examples

Please provide multiple examples (before and after scenarios) and explain what rules did apply to achieve the outcome.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

• I have read the Contribution Guidelines.
• I have searched both open and closed suggestions on this site and believe this is not a duplicate

Please tick all that apply:

• This is not a breaking change to Fantomas
• I or my company would be willing to help implement and/or test this
• This suggestion is part of the Microsoft style guide (please add a link to section if so)
• This suggestion is part of the G-Research style guide (please add a link to section if so)

[dsyme]

This needs some discussion per @nojaf's comments above

[dsyme]

Can we detect imperative code from an AST point of view and should we act upon that?

Yes, to some extent. At least we can detect when in the first part of a sequential, which is by far the most common case. This would then mean

let someFunction() =
    if expr then expr else expr
    1+1

could always suppress single-line if-then-else and format as:

let someFunction() =
    if expr then
        expr
    else
        expr
    1+1

It would not apply in this case:

let someFunction() =
    if expr then expr else expr

but that will be relatively rare for short if-then-else. (We would also have the option of considering this to be imperative/control-flow in all cases)

However either way would be an imperfect implementation of "Use multi-line formatting for imperative code" (assuming that is what we add to the style guide).

[nojaf]

At least we can detect when in the first part of a sequential

Oh boy 🙈, this is easier said than done.Sequential, LetOrUse and LetOrUseBang are often captured together as a series of expressions via an active pattern in SourceParser.
Touching those parts might be quite tricky to always get right because it will probably depend on where you are coming from in CodePrinter code. In the past, ASTContext was used to indicate such things, and ASTContext is pure evil. Anyway, the beauty of genExpr is that it will have the same behaviour in all cases it is being invoked. Adding more logic on top of that depending on the parent expression will not be an easy task. I'd rather start with something smaller in scope.

[knocte]

this is easier said than done

How about just defaulting fsharp_max_if_then_else_short_width to 0 (and also in the style guide). A pragmatic solution that would require much less work and not cause much harm (IMO any single-line if/if-else block is unreadable).

[nojaf]

@dsyme looking at fsharp/fslang-design#646 (comment).
The suggestion is there to move the ifExpr outside of the setting.
Would it be a good start to change what we have today to:

• fsharp_max_if_then_short_width, counting the length of the thenExpr and not allowing any nested SynExpr.IfThenElse

• fsharp_max_if_then_else_short_width, counting the length from the thenExpr to the end of the elseExpr

Where would this current guidance fall into:

Do both settings have no effect on this? And we format this compactly if each line elif x then y fits on one line?