String interpolation regression #1150

Closed
sstephenson opened this Issue Feb 21, 2011 · 19 comments

7 participants

@sstephenson

This used to compile:

"'#{contents.replace /'/g, '\\\''}'"

but now throws a parse error.

@michaelficarra
Collaborator

Hmm, it's parsing fine for me on 6f21f8a...

edit: Never mind, got the string wrong. I see the error now.

Error: missing ", starting on line 1

It's caused by the regex (/'/). A minimal test case: "#{/'/}"

I'd start looking at balancedString in the lexer.

@stephank

Offending commit is fba1654, which was a fix for another issue #923.

@satyr
Collaborator

Also:

$ coffee -bpe '/}/'
/}/;

$ coffee -bpe '"#{ /}/ }"'
Error: Parse error on line 2: Unexpected 'MATH'
...

$ ruby -e 'p "#{ /}/ }"'
-e:1: warning: regexp has `}' without escape
"(?-mix:})"
@michaelficarra
Collaborator

Relevent commit from satyr/coco.

@jashkenas
Owner

@satyr, I'm trying to follow the details of your fix -- mind explaining a bit about how the big picture of your change works?

@satyr
Collaborator

When you detect #{ within a string, spawn a new lexer with interpolation mode (which notes curly-brace count and exits on an extra }), merge the tokens and continue.

@ngn

Changing ['"', "'"] to ['"', "'", '/'] on this line seems to fix it.

@michaelficarra
Collaborator

@ngn: I merged your fix, thanks for that. Closing.

@satyr
Collaborator

Er, 0f523de is too adhoc:

$ bin/coffee -bpe '"#{a/b}"'
Error: missing /, starting on line 1
@michaelficarra
Collaborator

Hmm... I guess we should revert, then? It looks like you can't do any division in string interpolations anymore.

@ngn

Yes, please revert it. I'll give it another shot later.
Good catch, Satyr. I didn't expect such a simple expression to fail while the test suite passes.

@michaelficarra
Collaborator

@ngn: Already done. Like you, I also assumed there was already a test case for division inside of string interpolations. Guess I should have checked first.

@ngn ngn added a commit that referenced this issue Jun 19, 2011
@ngn ngn Another attempt to fix #1150
Here's how the algorithm in balancedString() was modified.  When we
encounter a slash in an interpolation, we:

    * try to find a heregex right after it; if found---skip it.  Three
      slashes always terminate a heregex, no matter if there is an open
      "#{" before them or not, so we don't have to bother about
      sub-interpolations inside the heregex.

    * try to find a regex right after it; if found---skip it.  Simple
      regexen can't contain interpolations.

    * otherwise, assume that the slash means division and carry on.
5ce7984
@braddunbar

Another example/test-case:

"#{/\\'/}"
@ngn

5ce7984 handles it correctly:

$ bin/coffee -bpe "\"#{/\\\\'/}\""
"" + /\\'/;
@ngn ngn added a commit that referenced this issue Jun 22, 2011
@ngn ngn added one more test for #1150 6f64fc2
@satyr
Collaborator
$ bin/coffee -bpe '"#{a/b}"; c / d'
Error: missing }, starting on line 1
@ngn
ngn commented Jul 7, 2011

That's correct, /b starts a regex.

@satyr
Collaborator

/b starts a regex

Not if it's immediately after a. Observe:

$ coffee -bpe 'g/h/i'
g / h / i;

$ coffee -bpe 'g /h/i'
g(/h/i);
@ngn
ngn commented Jul 7, 2011

Weird. True, but I don't think adapting interpolations to that behaviour is a worthwhile investment of developer time. It's good enough for me. However, if you wish to tackle it, I'd appreciate your further fix.

@michaelficarra
Collaborator

I believe #1458 fixed this and we just forgot to close it. Closing. Thanks again, @ngn.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment