Fix ASTNode#to_s to use escape sequence correctly #5842

makenowjust · 2018-03-20T03:59:47Z

ASTNode#to_s against StringInterpolation, RegexLiteral, and Call of backtick operator is buggy, it is missing to use escape sequence.

For example pp "#{1}\0" cannot be compiled by current compiler:

$ cat foo.cr
pp "#{1}\0"
$ crystal run foo.cr
Error in foo.cr:1: macro didn't expand to a valid program, it expanded to:

================================================================================
--------------------------------------------------------------------------------
   1.
   2.
   3.     __temp_20 = "#{"\"\#{1}\u0000\""} # => "
   4.     ::print __temp_20
   5.     __temp_21 = "#{1}"
   6.     PrettyPrint.format(__temp_21, STDOUT, width: 80 - __temp_20.size, indent: __temp_20.size)
   7.     ::puts
   8.     __temp_21
   9.
--------------------------------------------------------------------------------
Syntax error in expanded macro: pp:5: unterminated string literal

    __temp_21 = "#{1}"
                     ^

================================================================================

pp "#{1}\0"
^~

Because ASTNode#to_s against "#{1}\0" yields "#{1} and real null character.

Thank you.

straight-shoota · 2018-03-20T08:20:06Z

Wouldn't it be better to implement the escape logic from #5841 in a class method so it can be reused here?

makenowjust · 2018-03-20T11:33:18Z

@straight-shoota Good idea. I'll fix this PR after #5841 is merged.

makenowjust · 2018-03-21T04:29:34Z

Updated. Escape logic is only in Regex class method now.

straight-shoota · 2018-03-21T07:25:14Z

src/compiler/crystal/syntax/to_s.cr

@@ -499,9 +499,9 @@ module Crystal
      @str << '`'
      case exp
      when StringLiteral
-        @str << exp.value.inspect[1..-2]
+        @str << exp.value.inspect_unquoted.gsub('`', "\\`")


inspect_unquoted will escape \" which is not wrong but also not needed here. Not sure if this should be changed in this PR as well.

Yes. backtick operator must need escaping backtick and null character. However, it also accepts all string escape sequences, so I use inspect_unquoted.

straight-shoota · 2018-03-21T07:26:41Z

src/compiler/crystal/syntax/to_s.cr

@@ -968,9 +968,9 @@ module Crystal
      @str << "/"
      case exp = node.value
      when StringLiteral
-        @str << exp.value.gsub('/', "\\/")
+        @str << escape_regex exp.value


Regex.append_source(exp.value, @str)

straight-shoota · 2018-03-21T07:26:59Z

src/compiler/crystal/syntax/to_s.cr

      when StringInterpolation
-        visit_interpolation exp, &.gsub('/', "\\/")
+        visit_interpolation(exp) { |s| escape_regex s }
      end


I think no ditto here.

straight-shoota · 2018-03-21T08:36:11Z

src/compiler/crystal/syntax/to_s.cr

      when StringInterpolation
-        visit_interpolation exp, &.gsub('/', "\\/")
+        visit_interpolation(exp) { |s| escape_regex s }


Why not use Regex.append_source directly? visit_interpolation simply appends to @str. You can do that in the block as well and don't need the string builder. Just make sure the block returns nil or empty string.

You are trick star!! Yeah!

@straight-shoota

crystal-lang#5842 (comment) It is super tricky way. But @straight-shoota said, so it is right way also.

straight-shoota

Great work!

sdogruyol · 2018-04-14T09:22:16Z

@makenowjust this need a rebase 👍

ASTNode#to_s against StringInterpolation, RegexLiteral, and Call of backtick operator is buggy, it is missing to use escape sequence. For example `pp "#{1}\0"` cannot be compiled by current compiler.

@straight-shoota

crystal-lang#5842 (comment) It is super tricky way. But @straight-shoota said, so it is right way also.

makenowjust · 2018-04-21T00:31:43Z

@sdogruyol Thank you.

@straight-shoota

* Fix ASTNode#to_s to use escape sequence correctly ASTNode#to_s against StringInterpolation, RegexLiteral, and Call of backtick operator is buggy, it is missing to use escape sequence. For example `pp "#{1}\0"` cannot be compiled by current compiler. * Regex: make append_source as class method for RegexLiteral#to_s * Use Regex.append_source directly * Remove escape_regex and use Regex.append_source directly for all crystal-lang#5842 (comment) It is super tricky way. But @straight-shoota said, so it is right way also.

RX14 approved these changes Mar 20, 2018

View reviewed changes

makenowjust force-pushed the fix/crystal/to-s-for-string-like-literal-escape branch from e4a7fd6 to 0fb38bd Compare March 21, 2018 04:21

straight-shoota reviewed Mar 21, 2018

View reviewed changes

makenowjust added a commit to makenowjust/crystal that referenced this pull request Mar 21, 2018

Remove escape_regex and use Regex.append_source directly for all

3f1f6f1

crystal-lang#5842 (comment) It is super tricky way. But @straight-shoota said, so it is right way also.

straight-shoota approved these changes Mar 21, 2018

View reviewed changes

RX14 approved these changes Mar 21, 2018

View reviewed changes

RX14 modified the milestone: Next Mar 21, 2018

RX14 added kind:bug topic:compiler labels Mar 21, 2018

RX14 requested a review from asterite March 21, 2018 13:47

makenowjust added 4 commits April 14, 2018 22:49

Fix ASTNode#to_s to use escape sequence correctly

ea82206

ASTNode#to_s against StringInterpolation, RegexLiteral, and Call of backtick operator is buggy, it is missing to use escape sequence. For example `pp "#{1}\0"` cannot be compiled by current compiler.

Regex: make append_source as class method for RegexLiteral#to_s

1601cc6

Use Regex.append_source directly

0f38665

Remove escape_regex and use Regex.append_source directly for all

9282181

crystal-lang#5842 (comment) It is super tricky way. But @straight-shoota said, so it is right way also.

makenowjust force-pushed the fix/crystal/to-s-for-string-like-literal-escape branch from 3f1f6f1 to 9282181 Compare April 14, 2018 13:51

sdogruyol approved these changes Apr 19, 2018

View reviewed changes

sdogruyol merged commit 9f589a0 into crystal-lang:master Apr 19, 2018

sdogruyol added this to the Next milestone Apr 19, 2018

makenowjust deleted the fix/crystal/to-s-for-string-like-literal-escape branch April 21, 2018 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ASTNode#to_s to use escape sequence correctly #5842

Fix ASTNode#to_s to use escape sequence correctly #5842

makenowjust commented Mar 20, 2018

straight-shoota commented Mar 20, 2018

makenowjust commented Mar 20, 2018 •

edited

makenowjust commented Mar 21, 2018

straight-shoota Mar 21, 2018

makenowjust Mar 21, 2018 •

edited

straight-shoota Mar 21, 2018

makenowjust Mar 21, 2018

straight-shoota Mar 21, 2018

makenowjust Mar 21, 2018

straight-shoota Mar 21, 2018

makenowjust Mar 21, 2018

straight-shoota left a comment

sdogruyol commented Apr 14, 2018

makenowjust commented Apr 21, 2018

Fix ASTNode#to_s to use escape sequence correctly #5842

Fix ASTNode#to_s to use escape sequence correctly #5842

Conversation

makenowjust commented Mar 20, 2018

straight-shoota commented Mar 20, 2018

makenowjust commented Mar 20, 2018 • edited

makenowjust commented Mar 21, 2018

straight-shoota Mar 21, 2018

Choose a reason for hiding this comment

makenowjust Mar 21, 2018 • edited

Choose a reason for hiding this comment

straight-shoota Mar 21, 2018

Choose a reason for hiding this comment

makenowjust Mar 21, 2018

Choose a reason for hiding this comment

straight-shoota Mar 21, 2018

Choose a reason for hiding this comment

makenowjust Mar 21, 2018

Choose a reason for hiding this comment

straight-shoota Mar 21, 2018

Choose a reason for hiding this comment

makenowjust Mar 21, 2018

Choose a reason for hiding this comment

straight-shoota left a comment

Choose a reason for hiding this comment

sdogruyol commented Apr 14, 2018

makenowjust commented Apr 21, 2018

makenowjust commented Mar 20, 2018 •

edited

makenowjust Mar 21, 2018 •

edited