fsprojects / fantomas

FSharp source code formatter
https://fsprojects.github.io/fantomas
Other
764 stars 190 forks source link

Opening parens make unstable/incorrect indentation for an Enum wrapping a record #2943

Closed pbiggar closed 11 months ago

pbiggar commented 12 months ago

Issue created from fantomas-online

Code

((Combo { e1 = "Making this long so it goes on a new line new line new line new line making it long so it goes on a new line new line" }))

Result

((Combo

  { e1 =
      "Making this long so it goes on a new line new line new line new line making it long so it goes on a new line new line" }))

Expected Result

((Combo
    { e1 =
        "Making this long so it goes on a new line new line new line new line making it long so it goes on a new line new line" }))

Problem description

AFAICT, this only happens when we have 2 parens around this, which happened in my original example because I was piping the result.

Extra information

Options

Fantomas main branch at 1/1/1990

    { config with
                IndentSize = 2 }
nojaf commented 11 months ago

Hi Paul, This is an interesting case. Thanks for reporting it!

My first impression is that the parentheses turn the application into a sequential expression. Something where ((a b)) becomes

((a
  b))

Where b is no longer applied to a but the two are indidual expressions in a sequence. I'm able to replicate this with indent = 4 and 4 parentheses. The additional newline is introduced because the b is multiline and that follows the newline heuristic.

I'm not sure what the best way would be to solve this one. Changing this behaviour will for sure break something else.

pbiggar commented 11 months ago

I'm not sure I follow. Are you saying that ((a b)) is semantically the same as

a
b

That seems weird to me but it could be the case.

nojaf commented 11 months ago

Oh no, it isn't and that is part of the problem. b at least needs one space (I think) before it is considered applied to a instead of being two expressions (that are not ignored).

pbiggar commented 11 months ago

Oh right, that's what I thought. I got confused that changing the behaviour will break something else - what are you thinking of there?

nojaf commented 11 months ago

Well, this is tricky as there a multiple places where we deal with parentheses. But right now we do:

(a
    b)

Where b is indented from the point of view of the (, following the normal flow. b will be placed at one indent_size unit.

I initially was thinking of maybe touching this and having b indent from a offset instead. This would of course mean one indent_size + whatever offset a currently has. The trade-off is that b is put on at column which won't be a multitude of the selected indent_size. This upsets a lot of people of course.

Now, we might want to have a sanity check on the formatting of the application. If we could assert that the arguments are at least one space further we could add an additional indent if that is not the case. The downside here is that applications are also covered in a lot of places. Meaning, that we might be able to fix the problem at hand but it might surface again in a slightly different code sample.

I took a stab at this idea at https://github.com/fsprojects/fantomas/pull/2946 I'll probably consult @dawedawe and others next week on this one as well.