Indentation of Else block

[Smaug123]

Issue created from fantomas-online

Code

let foo () =
    if a then 3 else
    doThing()
    5

Result

let foo () =
    if a then
        3
    else
        doThing ()
        5

Problem description

I didn't realise this would be a problem for us, but sadly we have several places where we kind of rely on the ability to if/then/else without indentation. In functions with a lot of different possible outcomes, the code really marches off to the right unless we stick the else at the same indentation as the parent.

That is, because the original code had the else block unindented, it would be great if the output also had the else block unindented:

let foo () =
    if a then
        3
    else
    doThing ()
    5

I definitely understand why Fantomas does what it does, but do you think it would be reasonable to respect the user's choice not to indent else blocks?

Extra information

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

Options

Fantomas Master at 01/09/2021 08:58:25 - ca42f0d

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?}

[Smaug123]

Note that I'm pretty confident that Fantomas is already correct on if a then b else c all on one line (assuming line length forces it to break over lines):

if a then
    b
else
    c

I'm specifically only talking about cases where the user has explicitly chosen not to indent.

[Smaug123]

The same is true of match statements, by the way. The last case of a match can be unindented in exactly the same way.

[knocte]

I definitely understand why Fantomas does what it does, but do you think it would be reasonable to respect the user's choice not to indent else blocks?

No, please. I precisely want to use fantomas to catch this kind of practices and fix them to be consistent. To me, indenting to the left the way you prefer, is completely unreadable and confusing.

[Smaug123]

Well, it gets more unreadable and confusing when your code looks like:

if a then
    b
else
    doThings()
    if c then
        d
    else
        doThings2()
        if e then
            f
        else
            doThings3()
            …

This arises when using railway-oriented programming and Result types, for example, unless you use the result computation expression (which sadly does sometimes make debugging harder).

[knocte]

unless you use the result computation expression

Another readability workaround is extract more code into more small functions.

[Smaug123]

It's sad to break up a continuous flow of error-handling logic into arbitrary chunks purely to satisfy the formatter; I discussed that option with my team and (regardless of the various opinions on whether it's fine to unindent else) that was universal consensus.

My team broadly agrees that if you're using if/then/else or match to bail out early with errors, then it's fine to unindent the else or the final match. Note that this is standard practice in e.g. the Linux kernel, which uses goto for much the same effect in its if (err) {goto cleanup} pattern.

We can work around this in code using Result.bind, which maintains just the one level of indentation, and maybe that is the right answer.

[nojaf]

Hey Patrick, thanks for bringing this up.
I understand where you are coming from yet what you ask is quite controversial.

Fantomas currently does not preserve anything from code style from the original code.
It takes AST and prints that according to some settings and rules.
The beauty of this approach is the code consistency of course.

So to follow that principle, we somehow need some sort of rule and a setting to get to preferred behavior.
Something like:

if a then b
else
c

For example, if the length of if a then b matches something, and there is only one else c we could change the formatting based on a setting. This way we remain in the land of consistency. Could we come up with something that matches all the cases you wish to have the no indent style?

[Smaug123]

I still haven't come up with a rule, but here's another example which comes up a lot in our internal code:

open Library

type RunMode =
    | Dry
    | Wet

let main argv =
    let args = parse argv

    let instructions = Library.foo args
    match args.DryRun with
    | RunMode.Dry ->
        printfn "Would execute actions, but --dry-run was supplied: %+A" instructions
        0
    | RunMode.Wet ->
    // proceed with main method
    let output = Library.execute instructions
    // do more stuff
    0

It's really sad to have to wrap all the real main-method logic up into another function just to control the indentation here; it adds a layer of indirection and mental overhead for no benefit. Splitting up the function doesn't increase readability in this case.

[nojaf]

As there is more information these days about the keywords and tokens, I'd like to revisit this to request and maybe go for a simpler approach.

If the setting is true and the original code already didn't indent the elseExpr we could just restore that. Would that be acceptable? The only drawback this has is that it relies on the original code to do the right thing. This could easily lead to inconsistent behaviour, unfortunately.

let developer A wrote =
    if a then
        b
    else
    
    c // long expr

let developer B wrote =
    if a then
        b
    else
        c // long expr

What do you think @Smaug123?

[abelbraaksma]

@nojaf, I think allowing certain settings that support leniency towards individual programmer's preference is a good way to go. I personally find code that uses this approach utterly confusing and unreadable (if you have a pyramid of doom, there's probably something you should refactor).

But having a setting that allows a slightly less deep pyramid of doom in certain scenarios, at the user's discretion, is probably a good thing for some companies/policies. I prefer it over the current situation of having a "forced style for all else or last match cases", which will only deteriorate existing code further.

[nojaf]

individual programmer's preference is a good way

I do not, the downfall of this setting is that developer A made a choice that developer B may not have made. Fantomas currently outputs a consistent story regardless of which developer wrote the code. This setting does not adhere to that philosophy.

The only reason we have this setting is at the request of our enterprise sponsor G-Research.
Without their financial support, there would be no Fantomas as we know it today.
I'm happy to make an exception for them in a heartbeat, but this should not be considered a precedent.

[xperiandri]

Does architecture allow to disable a particular rule processing not to handle ifs at all?

[abelbraaksma]

I'm happy to make an exception for them in a heartbeat, but this should not be considered a precedent.

Thanks for the explanation, I see your point. Consistency guarantees trump individual preferences.