scalameta / scalafmt

Code formatter for Scala
http://scalameta.org/scalafmt
Apache License 2.0
1.42k stars 276 forks source link

Undesirable formatting of scala3 nested if statements #4104

Closed mtomko closed 1 month ago

mtomko commented 1 month ago

I would like a way to preserve the formatting of nested if-then statements in Scala 3 code. In my case, I have an outer if-then-else statement, each branch of which contains its own if-then-else. When I run scalafmt, the result is logically correct but makes the code (in my opinion) less clear to the reader.

See the description below for an example that correctly reproduces the bug; what follows is a synopsis that in reality behaves differently but is a little easier to see:

if b1 then
  if b2 then x else y
else
  if b2 then p else q

as:

if b1 then
  if b2 then x else y
else if b2 then p 
else q

This particular example gets formatted differently (more things get stuffed on one line, which isn't great but it's defensible), but it serves to show what I think is the problem, which is that the original code shows the programmers logic, which is that b1 controls how we interpret b2. The formatted code evaluates the same way but fails to demonstrate the same logical relationship.

See below for an example that actually reproduces the issue.

Configuration (required)

version=3.8.2
runner.dialect = scala3
...

Command-line parameters (required)

When I run scalafmt via CLI like this: scalafmt iftest.scala

Steps

Given code like this:

val aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc =
  false

val b2 = true

def f =
  if aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc then
    if b2 then 1 else 2
  else
    if b2 then 3 else 4

Problem

Scalafmt formats code like this:

val aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc =
  false

val b2 = true

def f =
  if aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc then
    if b2 then 1 else 2
  else if b2 then 3
  else 4

Expectation

I would like the formatted output to look like this:

val aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc =
  false

val b2 = true

def f =
  if aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc then
    if b2 then 1 else 2
  else
    if b2 then 3 else 4
mtomko commented 1 month ago

Musing a bit more on this issue, I'm not sure this is a tractable problem in general because the existing behavior is logically correct and understanding programmer intent is obviously not a reasonable goal for a program like scalafmt.

However, especially with Scala 3's if-then-else syntax, code can be obuscated when programmers' original linebreaks are not respected, so maybe the best middle ground would be an option to preserve linebreaks in if-then-else statements unless they grossly violate line length settings. At that point, all bets are off, because scalafmt needs to be able to break long lines; maybe at that point a heuristic would be to keep each else with the nearest if as much as possible?

Given the complexity, barring an obvious idea on the part of the contributors, maybe this is the kind of issue that just has to sit on the backburner, and I'll find some other way of structuring my code to avoid the problem?

kitbellew commented 1 month ago

... when programmers' original linebreaks are not respected, so maybe the best middle ground would be an option to preserve linebreaks in if-then-else statements ...

that's a really salient -- and frequent -- observation! thus, there's a parameter for that, it's documented and not hard to find, but let me know if it fails in this case.

mtomko commented 1 month ago

Oh no, sorry I missed it. Will check and retry.

mtomko commented 1 month ago

Do you mean newlines.source = keep?

With this config:

version=3.8.2
runner.dialect = scala3
newlines.source = keep

it's still formatting:

val aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc =
  false

val b2 = true

def f =
  if aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc then
    if b2 then 1 else 2
  else
    if b2 then 3 else 4

to

val aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc =
  false

val b2 = true

def f =
  if aVeryLongBooleanNameSuchThatWrappingIsPreservedAndSoOnAndSoForthEtc then
    if b2 then 1 else 2
  else if b2 then 3
  else 4
tgodzik commented 1 month ago

Och that looks like it changes the semantics though?