From 83dc32bd82533ae1d7eff29f3cebe51c0e78ad6e Mon Sep 17 00:00:00 2001 From: TSUYUSATO Kitsune Date: Wed, 21 Nov 2018 12:35:36 +0900 Subject: [PATCH 1/5] Format: keep trailing spaces in macros 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. --- spec/compiler/formatter/formatter_spec.cr | 43 ++++++++++++++++++----- src/compiler/crystal/tools/formatter.cr | 26 ++++++++++++++ 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/spec/compiler/formatter/formatter_spec.cr b/spec/compiler/formatter/formatter_spec.cr index b83e77e01af5..21f251f7409e 100644 --- a/spec/compiler/formatter/formatter_spec.cr +++ b/spec/compiler/formatter/formatter_spec.cr @@ -599,12 +599,12 @@ describe Crystal::Formatter do assert_format "macro foo( x = 1, y = 2, &block)\nend", "macro foo(x = 1, y = 2, &block)\nend" assert_format "macro foo\n 1 + 2\nend" assert_format "macro foo\n if 1\n 1 + 2\n end\nend" - assert_format "macro foo\n {{1 + 2}} \nend", "macro foo\n {{1 + 2}}\nend" - assert_format "macro foo\n {{ 1 + 2 }} \nend", "macro foo\n {{ 1 + 2 }}\nend" - assert_format "macro foo\n {% 1 + 2 %} \nend", "macro foo\n {% 1 + 2 %}\nend" - assert_format "macro foo\n {{ 1 + 2 }}\\ \nend", "macro foo\n {{ 1 + 2 }}\\\nend" - assert_format "macro foo\n {{ 1 + 2 }}\\ \n 1\n end", "macro foo\n {{ 1 + 2 }}\\\n 1\n end" - assert_format "macro foo\n {%1 + 2%}\\ \nend", "macro foo\n {% 1 + 2 %}\\\nend" + assert_format "macro foo\n {{1 + 2}}\nend", "macro foo\n {{1 + 2}}\nend" + assert_format "macro foo\n {{ 1 + 2 }}\nend", "macro foo\n {{ 1 + 2 }}\nend" + assert_format "macro foo\n {% 1 + 2 %}\nend", "macro foo\n {% 1 + 2 %}\nend" + assert_format "macro foo\n {{ 1 + 2 }}\\\nend", "macro foo\n {{ 1 + 2 }}\\\nend" + assert_format "macro foo\n {{ 1 + 2 }}\\\n 1\n end", "macro foo\n {{ 1 + 2 }}\\\n 1\n end" + assert_format "macro foo\n {%1 + 2%}\\\nend", "macro foo\n {% 1 + 2 %}\\\nend" assert_format "macro foo\n {% if 1 %} 2 {% end %}\nend" assert_format "macro foo\n {% unless 1 %} 2 {% end %}\nend" assert_format "macro foo\n {% if 1 %} 2 {% else %} 3 {% end %}\nend" @@ -612,7 +612,7 @@ describe Crystal::Formatter do assert_format "macro foo\n {% for x in y %} 2 {% end %}\nend" assert_format "macro foo\n {% for x in y %}\\ 2 {% end %}\\\nend" assert_format "macro foo\n %foo\nend" - assert_format "macro foo\n %foo{x.id+2} \nend", "macro foo\n %foo{x.id + 2}\nend" + assert_format "macro foo\n %foo{x.id+2}\nend", "macro foo\n %foo{x.id + 2}\nend" assert_format "macro foo\n %foo{x,y}\nend", "macro foo\n %foo{x, y}\nend" assert_format "def foo : Int32\n 1\nend" assert_format "class Foo\n macro foo\n 1\n end\nend" @@ -625,7 +625,7 @@ describe Crystal::Formatter do assert_format "if 1\n {{1 + 2}}\nend" assert_format "def foo : self | Nil\n nil\nend" assert_format "macro foo(x)\n {% if 1 %} 2 {% end %}\nend" - assert_format "macro foo()\n {% if 1 %} 2 {% end %} \nend", "macro foo\n {% if 1 %} 2 {% end %}\nend" + assert_format "macro foo()\n {% if 1 %} 2 {% end %}\nend", "macro foo\n {% if 1 %} 2 {% end %}\nend" assert_format "macro flags\n {% if 1 %}\\\n {% end %}\\\nend" assert_format "macro flags\n {% if 1 %}\\\n 1 {% else %}\\\n {% end %}\\\nend" assert_format "macro flags\n {% if 1 %}{{1}}a{{2}}{% end %}\\\nend" @@ -1253,4 +1253,31 @@ describe Crystal::Formatter do e.is_a?(Y) end)) CODE + + # Keep trailing spaces in macros. + assert_format <<-CODE + macro foo + <<-FOO + hello + FOO + end + + {% verbatim do %} + <<-FOO + hello + FOO + {% end %} + + {% if true %} + <<-FOO + hello + FOO + {% end %} + + {% for a in %w() %} + <<-FOO + hello + FOO + {% end %} + CODE end diff --git a/src/compiler/crystal/tools/formatter.cr b/src/compiler/crystal/tools/formatter.cr index 758ff2ba4b10..be4a27f836a9 100644 --- a/src/compiler/crystal/tools/formatter.cr +++ b/src/compiler/crystal/tools/formatter.cr @@ -1633,6 +1633,8 @@ module Crystal end def format_macro_body(node) + macro_line = @line + if @token.keyword?(:end) return format_macro_end end @@ -1658,9 +1660,15 @@ module Crystal end skip_space_or_newline + macro_end = @line check :MACRO_END write "end" next_token + + # Keep trailing spaces in macro body. + (macro_line...macro_end).each do |line| + @no_rstrip_lines.add line + end end def format_macro_end @@ -1694,6 +1702,7 @@ module Crystal check_keyword :do write " do" next_token_skip_space + verbatim_line = @line check :"%}" write " %}" @@ -1707,6 +1716,7 @@ module Crystal check :MACRO_CONTROL_START write "{%" next_token_skip_space_or_newline + verbatim_end = @line check_keyword :end write " end" next_token_skip_space @@ -1720,6 +1730,10 @@ module Crystal next_token end + (verbatim_line...verbatim_end).each do |line| + @no_rstrip_lines.add line + end + false end @@ -1839,6 +1853,7 @@ module Crystal def format_macro_if_epilogue(node, macro_state, check_end = true) skip_space_or_newline + macro_if_line = @line check :"%}" write " %}" @@ -1882,6 +1897,7 @@ module Crystal check_end next_token_skip_space_or_newline + macro_if_end = @line check :"%}" write "{% end %}" @@ -1892,6 +1908,10 @@ module Crystal else next_token end + + (macro_if_line...macro_if_end).each do |line| + @no_rstrip_lines.add line + end end false @@ -1931,6 +1951,7 @@ module Crystal outside_macro { indent(@column, node.exp) } skip_space_or_newline + macro_for_line = @line check :"%}" write " %}" @@ -1946,6 +1967,7 @@ module Crystal check_end next_token_skip_space_or_newline + macro_for_end = @line check :"%}" write "{% end %}" @@ -1957,6 +1979,10 @@ module Crystal next_token end + (macro_for_line...macro_for_end).each do |line| + @no_rstrip_lines.add line + end + false end From 88fa586fe6608ac29fd27f11002d8a7e71ef0420 Mon Sep 17 00:00:00 2001 From: TSUYUSATO Kitsune Date: Wed, 12 Dec 2018 16:03:29 +0900 Subject: [PATCH 2/5] Rewrite trailing spaces using interpolation https://github.com/crystal-lang/crystal/pull/7099#pullrequestreview-183726150 --- spec/compiler/formatter/formatter_spec.cr | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/compiler/formatter/formatter_spec.cr b/spec/compiler/formatter/formatter_spec.cr index 21f251f7409e..107f379ee184 100644 --- a/spec/compiler/formatter/formatter_spec.cr +++ b/spec/compiler/formatter/formatter_spec.cr @@ -1258,25 +1258,25 @@ describe Crystal::Formatter do assert_format <<-CODE macro foo <<-FOO - hello + hello#{" "} FOO end {% verbatim do %} <<-FOO - hello + hello#{" "} FOO {% end %} {% if true %} <<-FOO - hello + hello#{" "} FOO {% end %} {% for a in %w() %} <<-FOO - hello + hello#{" "} FOO {% end %} CODE From fbb37425f6ca743c145bff33de820742f6582609 Mon Sep 17 00:00:00 2001 From: TSUYUSATO Kitsune Date: Wed, 12 Dec 2018 16:28:14 +0900 Subject: [PATCH 3/5] Remove trailing spaces in macro expressions But keep trailing spaces in macro body outside macro expressions --- spec/compiler/formatter/formatter_spec.cr | 19 ++++++++++++++ src/compiler/crystal/tools/formatter.cr | 32 +++++------------------ 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/spec/compiler/formatter/formatter_spec.cr b/spec/compiler/formatter/formatter_spec.cr index 107f379ee184..4e6313ed92e3 100644 --- a/spec/compiler/formatter/formatter_spec.cr +++ b/spec/compiler/formatter/formatter_spec.cr @@ -1280,4 +1280,23 @@ describe Crystal::Formatter do FOO {% end %} CODE + + # But remove trailing space in macro expression. + assert_format <<-CODE, <<-EXPECTED + macro foo + 1#{" "} + {{#{" "} + 42#{" "} + }}#{" "} + 2#{" "} + end + CODE + macro foo + 1#{" "} + {{ + 42 + }}#{" "} + 2#{" "} + end + EXPECTED end diff --git a/src/compiler/crystal/tools/formatter.cr b/src/compiler/crystal/tools/formatter.cr index be4a27f836a9..311f0a427dcc 100644 --- a/src/compiler/crystal/tools/formatter.cr +++ b/src/compiler/crystal/tools/formatter.cr @@ -1633,8 +1633,6 @@ module Crystal end def format_macro_body(node) - macro_line = @line - if @token.keyword?(:end) return format_macro_end end @@ -1660,15 +1658,9 @@ module Crystal end skip_space_or_newline - macro_end = @line check :MACRO_END write "end" next_token - - # Keep trailing spaces in macro body. - (macro_line...macro_end).each do |line| - @no_rstrip_lines.add line - end end def format_macro_end @@ -1680,6 +1672,12 @@ module Crystal end def visit(node : MacroLiteral) + line = @line + @token.raw.scan("\n") do + line -= 1 + @no_rstrip_lines.add line + end + write @token.raw next_macro_token false @@ -1702,7 +1700,6 @@ module Crystal check_keyword :do write " do" next_token_skip_space - verbatim_line = @line check :"%}" write " %}" @@ -1716,7 +1713,6 @@ module Crystal check :MACRO_CONTROL_START write "{%" next_token_skip_space_or_newline - verbatim_end = @line check_keyword :end write " end" next_token_skip_space @@ -1730,10 +1726,6 @@ module Crystal next_token end - (verbatim_line...verbatim_end).each do |line| - @no_rstrip_lines.add line - end - false end @@ -1853,7 +1845,6 @@ module Crystal def format_macro_if_epilogue(node, macro_state, check_end = true) skip_space_or_newline - macro_if_line = @line check :"%}" write " %}" @@ -1897,7 +1888,6 @@ module Crystal check_end next_token_skip_space_or_newline - macro_if_end = @line check :"%}" write "{% end %}" @@ -1908,10 +1898,6 @@ module Crystal else next_token end - - (macro_if_line...macro_if_end).each do |line| - @no_rstrip_lines.add line - end end false @@ -1951,7 +1937,6 @@ module Crystal outside_macro { indent(@column, node.exp) } skip_space_or_newline - macro_for_line = @line check :"%}" write " %}" @@ -1967,7 +1952,6 @@ module Crystal check_end next_token_skip_space_or_newline - macro_for_end = @line check :"%}" write "{% end %}" @@ -1979,10 +1963,6 @@ module Crystal next_token end - (macro_for_line...macro_for_end).each do |line| - @no_rstrip_lines.add line - end - false end From 747d51c84fdf7eff94bdc5a5555ab764b4445353 Mon Sep 17 00:00:00 2001 From: TSUYUSATO Kitsune Date: Thu, 13 Dec 2018 11:41:11 +0900 Subject: [PATCH 4/5] Rewrite heredoc to string literal --- spec/compiler/formatter/formatter_spec.cr | 83 +++++++++++------------ 1 file changed, 41 insertions(+), 42 deletions(-) diff --git a/spec/compiler/formatter/formatter_spec.cr b/spec/compiler/formatter/formatter_spec.cr index 4e6313ed92e3..6b6a3053b09b 100644 --- a/spec/compiler/formatter/formatter_spec.cr +++ b/spec/compiler/formatter/formatter_spec.cr @@ -1255,48 +1255,47 @@ describe Crystal::Formatter do CODE # Keep trailing spaces in macros. - assert_format <<-CODE - macro foo - <<-FOO - hello#{" "} - FOO - end - - {% verbatim do %} - <<-FOO - hello#{" "} - FOO - {% end %} - - {% if true %} - <<-FOO - hello#{" "} - FOO - {% end %} - - {% for a in %w() %} - <<-FOO - hello#{" "} - FOO - {% end %} - CODE + assert_format( + "macro foo\n" + + " <<-FOO\n" + + " hello \n" + + " FOO\n" + + "end\n" + + "\n" + + "{% verbatim do %}\n" + + " <<-FOO\n" + + " hello \n" + + " FOO\n" + + "{% end %}\n" + + "\n" + + "{% if true %}\n" + + " <<-FOO\n" + + " hello \n" + + " FOO\n" + + "{% end %}\n" + + "\n" + + "{% for a in %w() %}\n" + + " <<-FOO\n" + + " hello \n" + + " FOO\n" + + "{% end %}" + ) # But remove trailing space in macro expression. - assert_format <<-CODE, <<-EXPECTED - macro foo - 1#{" "} - {{#{" "} - 42#{" "} - }}#{" "} - 2#{" "} - end - CODE - macro foo - 1#{" "} - {{ - 42 - }}#{" "} - 2#{" "} - end - EXPECTED + assert_format( + "macro foo\n" + + " 1 \n" + + " {{ \n" + + " 42 \n" + + " }} \n" + + " 2 \n" + + "end", + "macro foo\n" + + " 1 \n" + + " {{\n" + + " 42\n" + + " }} \n" + + " 2 \n" + + "end" + ) end From f52b90bec36b71cb4884c2cad8f651cb45fe472f Mon Sep 17 00:00:00 2001 From: TSUYUSATO Kitsune Date: Thu, 13 Dec 2018 20:43:36 +0900 Subject: [PATCH 5/5] Separate indivisual samples --- spec/compiler/formatter/formatter_spec.cr | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/spec/compiler/formatter/formatter_spec.cr b/spec/compiler/formatter/formatter_spec.cr index 6b6a3053b09b..5694d4f7ddc6 100644 --- a/spec/compiler/formatter/formatter_spec.cr +++ b/spec/compiler/formatter/formatter_spec.cr @@ -1260,20 +1260,23 @@ describe Crystal::Formatter do " <<-FOO\n" + " hello \n" + " FOO\n" + - "end\n" + - "\n" + + "end" + ) + assert_format( "{% verbatim do %}\n" + " <<-FOO\n" + " hello \n" + " FOO\n" + - "{% end %}\n" + - "\n" + + "{% end %}" + ) + assert_format( "{% if true %}\n" + " <<-FOO\n" + " hello \n" + " FOO\n" + - "{% end %}\n" + - "\n" + + "{% end %}" + ) + assert_format( "{% for a in %w() %}\n" + " <<-FOO\n" + " hello \n" +