Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
811113002:
ES6 template literals should not use legacy octal strings (Closed)
|
Created:
6 years ago by arv (Not doing code reviews) Modified:
6 years ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionES6 template literals should not use legacy octal strings
Correctly handle SyntaxErrors in escape sequences.
BUG=v8:3736
LOG=Y
R=dslomov@chromium.org, caitpotter88@gmail.com
Committed: https://crrev.com/c05a4036b04f331196b2f84b7cbd03dee4e63058
Cr-Commit-Position: refs/heads/master@{#25891}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix backslash 0 #
Total comments: 1
Patch Set 3 : Fix some more syntax errors #
Total comments: 7
Patch Set 4 : add back test for backslash x but use z instead #Patch Set 5 : moar #
Total comments: 6
Patch Set 6 : One more test #
Messages
Total messages: 22 (1 generated)
PTAL
https://codereview.chromium.org/811113002/diff/1/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/811113002/diff/1/src/scanner.cc#newcode706 src/scanner.cc:706: if (!in_template_literal && c0_ >= 0 && unicode_cache_->IsLineTerminator(c)) { Is this check necessary? I think ScanTemplateSpan will not invoke ScanEscape for line continuations --- fine to add it but I'm not sure it's needed https://codereview.chromium.org/811113002/diff/1/src/scanner.cc#newcode851 src/scanner.cc:851: ScanEscape<capture_raw, in_template_literal>(); How are these ever not <true, true> here? https://codereview.chromium.org/811113002/diff/1/test/mjsunit/harmony/templat... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/1/test/mjsunit/harmony/templat... test/mjsunit/harmony/templates.js:477: assertEquals('0', `\0`); > The CV of EscapeSequence :: 0 is the code unit value 0. Does this not basically make this U+0000 rather than U+0030? (I might be misunderstanding it)
https://codereview.chromium.org/811113002/diff/1/test/mjsunit/harmony/templat... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/1/test/mjsunit/harmony/templat... test/mjsunit/harmony/templates.js:477: assertEquals('0', `\0`); On 2014/12/17 17:04:21, caitp wrote: > > The CV of EscapeSequence :: 0 is the code unit value 0. > > Does this not basically make this U+0000 rather than U+0030? (I might be > misunderstanding it) In SpiderMonkey, it is U+0000
PTAL https://codereview.chromium.org/811113002/diff/1/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/811113002/diff/1/src/scanner.cc#newcode706 src/scanner.cc:706: if (!in_template_literal && c0_ >= 0 && unicode_cache_->IsLineTerminator(c)) { On 2014/12/17 17:04:21, caitp wrote: > Is this check necessary? I think ScanTemplateSpan will not invoke ScanEscape for > line continuations --- fine to add it but I'm not sure it's needed It is not needed but it is dead code in case we are in a template literal. https://codereview.chromium.org/811113002/diff/1/src/scanner.cc#newcode851 src/scanner.cc:851: ScanEscape<capture_raw, in_template_literal>(); On 2014/12/17 17:04:21, caitp wrote: > How are these ever not <true, true> here? They are always true, true here. I added the const var for readability. I still want to try to only capture raw when we are in a tagged literal so I decided to introduce one more template param instead of conflating this. https://codereview.chromium.org/811113002/diff/1/test/mjsunit/harmony/templat... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/1/test/mjsunit/harmony/templat... test/mjsunit/harmony/templates.js:477: assertEquals('0', `\0`); On 2014/12/17 17:06:03, caitp wrote: > On 2014/12/17 17:04:21, caitp wrote: > > > The CV of EscapeSequence :: 0 is the code unit value 0. > > > > Does this not basically make this U+0000 rather than U+0030? (I might be > > misunderstanding it) > > In SpiderMonkey, it is U+0000 Fixed.
Fix backslash 0
https://codereview.chromium.org/811113002/diff/20001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/20001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:479: assertEquals('\u000012', `\012`); Few other things: "\0a" matches the grammar `EscapeSequence :: 0 [lookahead ∉ DecimalDigit]`, while "\012" would be an octal literal. In SpiderMonkey, they are also throwing when trying to use the legacy octal literal within a template, so it's probably a good idea to make sure we throw with message "strict_octal_literal" if we hit an octal literal within a template span.
On 2014/12/17 18:04:59, caitp wrote: > https://codereview.chromium.org/811113002/diff/20001/test/mjsunit/harmony/tem... > File test/mjsunit/harmony/templates.js (right): > > https://codereview.chromium.org/811113002/diff/20001/test/mjsunit/harmony/tem... > test/mjsunit/harmony/templates.js:479: assertEquals('\u000012', `\012`); > Few other things: > > "\0a" matches the grammar `EscapeSequence :: 0 [lookahead ∉ DecimalDigit]`, > while "\012" would be an octal literal. > > In SpiderMonkey, they are also throwing when trying to use the legacy octal > literal within a template, so it's probably a good idea to make sure we throw > with message "strict_octal_literal" if we hit an octal literal within a template > span. This patch needs more work but I'm not sure SpiderMonkey is right here. `\012` should be equal to '012' according to the spec.
On 2014/12/17 18:12:58, arv wrote: > On 2014/12/17 18:04:59, caitp wrote: > > > https://codereview.chromium.org/811113002/diff/20001/test/mjsunit/harmony/tem... > > File test/mjsunit/harmony/templates.js (right): > > > > > https://codereview.chromium.org/811113002/diff/20001/test/mjsunit/harmony/tem... > > test/mjsunit/harmony/templates.js:479: assertEquals('\u000012', `\012`); > > Few other things: > > > > "\0a" matches the grammar `EscapeSequence :: 0 [lookahead ∉ DecimalDigit]`, > > while "\012" would be an octal literal. > > > > In SpiderMonkey, they are also throwing when trying to use the legacy octal > > literal within a template, so it's probably a good idea to make sure we throw > > with message "strict_octal_literal" if we hit an octal literal within a > template > > span. > > This patch needs more work but I'm not sure SpiderMonkey is right here. > > `\012` should be equal to '012' according to the spec. I don't think `\012` would be "012" --- `\0` cannot ever match the CharacterEscapeSequence production, it can at best be `EscapeSequence :: 0 [lookahead ∉ DecimalDigit]`, and LegacyOctalEscapeSequence is not an option (per 16.1), so it shouldn't be "012", it should be a syntax error
Fix some more syntax errors
Lgtm, few things though https://codereview.chromium.org/811113002/diff/40001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/811113002/diff/40001/src/scanner.cc#newcode739 src/scanner.cc:739: return false; I feel like we should just let ScanOctalEscape do its thing and use CheckOctalLiteral() in the preparser, instead of this (better error message). I don't think that's a blocker though https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (left): https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:256: (function(s) { calls++; assertEquals("\u005Cx", s.raw[0]); })`\x`; Maybe just replace `\x` with `\z` or something? https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:475: assertThrows("`\\01`", SyntaxError); This looks like a duplicate \00, \01, \02, \03, \04, \05, \06, \07, \08, and \09 should all be errors --- but \0a, \0., \0^ etc should all be okay. https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:478: assertEquals('\\123', String.raw`\123`); My reading of the spec is that this should be a SyntaxError as well
add back test for backslash x but use z instead
moar
PTAL https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (left): https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:256: (function(s) { calls++; assertEquals("\u005Cx", s.raw[0]); })`\x`; On 2014/12/17 19:35:56, caitp wrote: > Maybe just replace `\x` with `\z` or something? Done. https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:475: assertThrows("`\\01`", SyntaxError); On 2014/12/17 19:35:56, caitp wrote: > This looks like a duplicate > > \00, \01, \02, \03, \04, \05, \06, \07, \08, and \09 should all be errors --- > but \0a, \0., \0^ etc should all be okay. Adding more tests https://codereview.chromium.org/811113002/diff/40001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:478: assertEquals('\\123', String.raw`\123`); On 2014/12/17 19:35:56, caitp wrote: > My reading of the spec is that this should be a SyntaxError as well You are right. Fixing.
Still basically looks okay https://codereview.chromium.org/811113002/diff/80001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/811113002/diff/80001/src/scanner.cc#newcode730 src/scanner.cc:730: break; Was this added changed by accident? not a big deal though https://codereview.chromium.org/811113002/diff/80001/src/scanner.cc#newcode738 src/scanner.cc:738: // \ 0 DecimalDigit is never allowed in templates. It's not really restricted to templates, this applies to strict-mode string literals as well. I did some experimenting with this, and it looks like JSC is the only engine that is doing this right. Bug filed on SM at https://bugzilla.mozilla.org/show_bug.cgi?id=1112732 (although it may or may not be really fixable depending on how much it's used in the wild) I can submit a followup for fixing this, but I don't really know how much it would break the web https://codereview.chromium.org/811113002/diff/80001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/80001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:480: assertThrows(code, SyntaxError); I think it would be good to also test `\0<nondigitcharacters>`, or at least one such test
One more test
https://codereview.chromium.org/811113002/diff/80001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/811113002/diff/80001/src/scanner.cc#newcode730 src/scanner.cc:730: break; On 2014/12/17 20:32:14, caitp wrote: > Was this added changed by accident? not a big deal though git cl format complained... https://codereview.chromium.org/811113002/diff/80001/src/scanner.cc#newcode738 src/scanner.cc:738: // \ 0 DecimalDigit is never allowed in templates. On 2014/12/17 20:32:14, caitp wrote: > It's not really restricted to templates, this applies to strict-mode string > literals as well. I did some experimenting with this, and it looks like JSC is > the only engine that is doing this right. Bug filed on SM at > https://bugzilla.mozilla.org/show_bug.cgi?id=1112732 (although it may or may not > be really fixable depending on how much it's used in the wild) > > I can submit a followup for fixing this, but I don't really know how much it > would break the web We talked about legacy octal numbers at a recent TC39 https://github.com/rwaldron/tc39-notes/blob/b26f950b438b18ac3497fd717dfb2370a... The conclusion covers '\08'. https://codereview.chromium.org/811113002/diff/80001/test/mjsunit/harmony/tem... File test/mjsunit/harmony/templates.js (right): https://codereview.chromium.org/811113002/diff/80001/test/mjsunit/harmony/tem... test/mjsunit/harmony/templates.js:480: assertThrows(code, SyntaxError); On 2014/12/17 20:32:14, caitp wrote: > I think it would be good to also test `\0<nondigitcharacters>`, or at least one > such test Done.
Dmitry, PTAL.
lgtm
The CQ bit was checked by arv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/811113002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c05a4036b04f331196b2f84b7cbd03dee4e63058 Cr-Commit-Position: refs/heads/master@{#25891} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
