Format: keep trailing spaces in macros

[makenowjust]

Currently the formatter removes trailing spaces in macros, however macro may contain meaningful trailing spaces (e.g. space inside string literals), so it is possible to change program meaning.
The formatter should keep such spaces because formatter doesn't change input program meaning at any time.

[straight-shoota]

Could you give an example where removing a trailing whitespace changes semantics?

[makenowjust]

@straight-shoota Just " string literal value ".

Rewriting string literal value by formatter is totally evil and unintentionally.

When the following code is given:

(NOTE: Please drag this code. Heredoc LEFT_ALIGNED lines have trailing spaces.)

left_aligned_string = <<-LEFT_ALINGED
  1        
  2        
  Fizz     
  4        
  Buzz     
  Fizz     
  7        
  8        
  Fizz     
  Buzz     
  11       
  Fizz     
  13       
  14       
  FizzBuzz 
  LEFT_ALINGED

puts left_aligned_string.lines.map { |line| line + line }.join "\n"

Var left_aligned_string lines are lieft-aligned, so the output is nice.

$ crystal run left_aligned.cr
1        1        
2        2        
Fizz     Fizz     
4        4        
Buzz     Buzz     
Fizz     Fizz     
7        7        
8        8        
Fizz     Fizz     
Buzz     Buzz     
11       11       
Fizz     Fizz     
13       13       
14       14       
FizzBuzz FizzBuzz 

Encloses this code by macro and adds this invocation, then formats the file by current crystal tool format. After, the file is that:

macro main
  left_aligned_string = <<-LEFT_ALINGED
    1
    2
    Fizz
    4
    Buzz
    Fizz
    7
    8
    Fizz
    Buzz
    11
    Fizz
    13
    14
    FizzBuzz
    LEFT_ALINGED

  puts left_aligned_string.lines.map { |line| line + line }.join "\n"
end

main

Trailing spaces inside heredocs are removed, so the output is broken.

$ crystal tool format -< fizz.cr | crystal eval
11
22
FizzFizz
44
BuzzBuzz
FizzFizz
77
88
FizzFizz
BuzzBuzz
1111
FizzFizz
1313
1414
FizzBuzzFizzBuzz

---

Yes, it is very conservative fix. I believe the formatter should not break the given source code of course. The output of crystal tool format foo.cr; crystal run foo.cr should equal the output of crystal run foo.cr.

[asterite]

Even though it's a conservative change, having invisible code is a really bad practice in my opinion.

[straight-shoota]

In your example the trailing whitespace is removed inside a string literal (heredoc). That's obviously incorrect, whether it's inside a macro method or not.

But as far as I can tell, this PR is not limited to string literals. Can you show an example where in a macro trailing whitespace is relevant outside a string literal?
Please excuse me if I didn't catch the purpose correctly.

[asterite]

@straight-shoota That's the only use case, string literals inside macros. If I remember correctly the macro processor doesn't know it's inside a string literal, everything is just "text".

[straight-shoota]

@asterite If I understand correctly, a part of the issue is that trailing whitespace in a string literal is handled differently inside a macro method. (¶ signals the presence of trailing whitespace)

# formatter preserves trailing whitespace
<<-EOF
foo  ¶
EOF

macro foo
  # formatter removes trailing whitespace
  <<-EOF
  foo  ¶
  EOF
end

The formatter should treat both cases the same, either remove or preserve trailing whitespace.

[makenowjust]

@straight-shoota Backslash line continuation is another example.

When it has trailing spaces, it cannot compile. Otherwise, it pass through the compiler if the formatter removes trailing spaces.

macro foo
  foo = 1
  p foo \  
    + 1
end

foo

---

@asterite You are right and I think so too. However I also think it is linter field.

I'm surveying other formatters and linters (with no-trailing-spaces option) status. I'll submit detailed report later, but there is no formatter removes trailing spaces inside string literal in silence for now.

[asterite]

In any case I think this should be fixed. I think the formatter does a last pass that removes trailing whitespace so this migh be a bit hard to implement, but this PR seems to work, so 👍 from me (I didn't have time to see the diff yet).

[RX14]

It's unfortunate that the formatter cannot format macros, but the formatter should not ever break code. That's it's primary directive and breaking it makes developers loose confidence in autoformat.

[bcardiff]

Editors are usually configured to trim white spaces at the end.
These specs should be written different in order to avoid loosing those spaces.

[makenowjust]

Perhaps CI will fail because I found a new formatter bug. I think it is irrelevant to this PR, so I will open a new PR when I fixed this.

[makenowjust]

done

[bcardiff]

I was expecting a single line string filled with " \n \n " to preserve trailing spaces 😄. The proposed way is far more clear.

Feel free to change it to " \n \n " if you timeout with the formatter bug you reached.

[makenowjust]

@bcardiff However the first spec of this is too long to write as single line, so I'd write to:

assert_format(
  "macro foo\n" +
  "  <<-FOO\n" +
  "    hello  \n" +
  "  FOO\n" +
  "end\n" +
  "{% verbatim do %}\n" +
  "  <<-FOO\n" +
  "    hello  \n" +
  "  FOO\n" +
  "{% end %}\n" +
  "{% if true %}\n" +
  "  <<-FOO\n" +
  "    hello  \n" +
  "  FOO\n" +
  "{% end %}\n" +
  "{% for a in %w() %}\n" +
  "  <<-FOO\n" +
  "    hello  \n" +
  "  FOO\n" +
  "{% end %}"
)

What do you think?

[straight-shoota]

@makenowjust You could put the individual samples each on a single line string. And maybe use line continuation (\) instead of concatenating strings (+)?

[sdogruyol]

Thank you @makenowjust 👍

[RX14]

needs rebase

[makenowjust]

rebased