Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1429983002:
[es6] early error when Identifier is an escaped reserved word (Closed)
|
Created:
5 years, 1 month ago by caitp (gmail) Modified:
5 years, 1 month ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es6] early error when Identifier is an escaped reserved word
Per http://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors (13.2.2),
make it a SyntaxError if an Identifier has the same StringValue as a ReservedWord.
BUG=v8:2222, v8:1972
LOG=N
R=adamk@chromium.org, rossberg@chromium.org, wingo@chromium.org
Committed: https://crrev.com/5bf360ef5773e5ceecc3b55e349db5a45b831d1d
Cr-Commit-Position: refs/heads/master@{#32052}
Patch Set 1 #
Total comments: 31
Patch Set 2 : Bunch of comments addressed + mozilla/test262 statuses updated #
Total comments: 5
Patch Set 3 : Fix test262.status #Patch Set 4 : More scanner-oriented version #Patch Set 5 : Remove unneeded preparser change #Patch Set 6 : Cosmetic fixup 1 #
Total comments: 14
Patch Set 7 : Style nits #Patch Set 8 : rebase #
Total comments: 15
Patch Set 9 : quick rebase #Patch Set 10 : Comments addressed + some strictness added #Patch Set 11 : Rebase #
Messages
Total messages: 41 (6 generated)
PTAL --- It's unlikely to break anything other than potential attack surfaces (as mentioned by MarkM on the bug), although I'm not sure how abusable this was in practice. https://codereview.chromium.org/1429983002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode659 src/preparser.h:659: language_mode(), this->is_generator(), allow_harmony_sloppy_let()); It would be really helpful if there was an easier way to pass the entire collection of feature flags to Scanner methods https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode663 src/preparser.h:663: if (IsNextEscapedReservedWord()) { Just used to shrink a bit of code in parser.cc / preparser.cc https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode701 src/preparser.h:701: bool* is_identifier, bool* is_escaped_keyword, I originally wrote a second method for testing this qualities separately, but it was more extra code that I didn't want to add in the patch. If people prefer that to the pile of flags, it's doable https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode2285 src/preparser.h:2285: if (IsNextEscapedReservedWord()) { IdentifierReference --- Escaped reserved words are always illegal here https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode2781 src/preparser.h:2781: &dont_care, &dont_care, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty)); MethodDefinitions and accessors use the PropertyName >> IdentifierName production, and thus don't care about keywords/escaping https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1507 src/scanner.cc:1507: Vector<const uint8_t> chars = next_.literal_chars->one_byte_literal(); I don't think this allocates a copy of the string, but if it does, let me know so I can come up with a cheaper way https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7253: "for (var i = 0; i < 100; ++i) { br\\u0065ak; }", There are probably some good test cases missing, like alternative declaration forms (`catch (th\u0079s) { ... }`), I'm not sure what else.
Description was changed from ========== [es6] early error when escaped reserved words are encountered Per http://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors (13.2.2), make it a SyntaxError if an Identifier has the same StringValue as a ReservedWord. BUG=v8:2222 LOG=N R=adamk@chromium.org, rossberg@chromium.org, wingo@chromium.org ========== to ========== [es6] early error when Identifier is an escaped reserved word Per http://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors (13.2.2), make it a SyntaxError if an Identifier has the same StringValue as a ReservedWord. BUG=v8:2222 LOG=N R=adamk@chromium.org, rossberg@chromium.org, wingo@chromium.org ==========
See also https://code.google.com/p/v8/issues/detail?id=1972. Does this fix all the test262 failures under language/identifiers/*escape*?
On 2015/11/03 21:27:24, adamk wrote: > See also https://code.google.com/p/v8/issues/detail?id=1972. Does this fix all > the test262 failures under language/identifiers/*escape*? A quick look looks like it should fix some of them. I'll update test262.status after fixups in the other CL
On 2015/11/03 21:30:53, caitp wrote: > On 2015/11/03 21:27:24, adamk wrote: > > See also https://code.google.com/p/v8/issues/detail?id=1972. Does this fix all > > the test262 failures under language/identifiers/*escape*? > > A quick look looks like it should fix some of them. I'll update test262.status > after fixups in the other CL fix *at least* some of them, if not all of them
Not sure how much to worry about performance here, I'd expect that running over identifiers twice would be generally quite fast, but the additional branches might still be bad. I guess I'm saying "I expect this might regress perf". https://codereview.chromium.org/1429983002/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1429983002/diff/1/src/parser.cc#newcode1429 src/parser.cc:1429: CheckNextEscapedKeyword(ok); You can CHECK_OK, and then please break. https://codereview.chromium.org/1429983002/diff/1/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1429983002/diff/1/src/preparser.cc#newcode206 src/preparser.cc:206: CheckNextEscapedKeyword(ok); Same here, CHECK_OK and break. https://codereview.chromium.org/1429983002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode2572 src/preparser.h:2572: bool* is_computed_name, bool* is_id, bool* is_escaped_keyword, Please keep the "is_identifier" name here as in the declaration. https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode2649 src/preparser.h:2649: bool is_ident = false; Please use "is_identifier" here too for consistency. https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1505 src/scanner.cc:1505: if (next_.literal_chars && next_.literal_chars->is_one_byte() && Please join this if() into the one on the line above https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1506 src/scanner.cc:1506: next_literal_contains_escapes(next_)) { This won't compile, I'm guessing you can drop next_literal_contains_escapes and just call (with my suggested renaming) LiteralContainsEscapes(next_). https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1507 src/scanner.cc:1507: Vector<const uint8_t> chars = next_.literal_chars->one_byte_literal(); On 2015/11/03 17:19:13, caitp wrote: > I don't think this allocates a copy of the string, but if it does, let me know > so I can come up with a cheaper way You're correct that there's no copy here. https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1516 src/scanner.cc:1516: return sloppy_let || is_strict(language_mode); My reading of the spec you pointed to is that LET is allowed in sloppy mode in this case: It is a Syntax Error if this phrase is contained in strict mode code and the StringValue of IdentifierName is: "implements", "interface", "let", "package", "private", "protected", "public", "static", or "yield". Is the sloppy_let branch here derived from some other piece of spec text? https://codereview.chromium.org/1429983002/diff/1/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/1429983002/diff/1/src/scanner.h#newcode692 src/scanner.h:692: bool literal_contains_escapes(const TokenDesc& token) const { This could be static (and thus not const). And CamelCase would also make it stand out from the members. https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7240: TEST(EscapedKeywords) { I think you also want some more non-error cases: yield outside of a generator, arguments and eval in sloppy mode, along with other things in that old "implements", "interface", etc list. https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7263: "e\\u0078port var foo;", This would already be failing since you're not parsing a module, so I don't think this makes sense in this list. You could make a separate test case for modules if you'd like (but I'm also fine with leaving that for another time). https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7269: "\\u0069mport blah from './foo.js';", same here as above for export
Description was changed from ========== [es6] early error when Identifier is an escaped reserved word Per http://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors (13.2.2), make it a SyntaxError if an Identifier has the same StringValue as a ReservedWord. BUG=v8:2222 LOG=N R=adamk@chromium.org, rossberg@chromium.org, wingo@chromium.org ========== to ========== [es6] early error when Identifier is an escaped reserved word Per http://tc39.github.io/ecma262/#sec-identifiers-static-semantics-early-errors (13.2.2), make it a SyntaxError if an Identifier has the same StringValue as a ReservedWord. BUG=v8:2222, v8:1972 LOG=N R=adamk@chromium.org, rossberg@chromium.org, wingo@chromium.org ==========
I've fixed test statuses and added addressed (I think) a bunch of comments. https://codereview.chromium.org/1429983002/diff/1/src/parser.cc File src/parser.cc (right): https://codereview.chromium.org/1429983002/diff/1/src/parser.cc#newcode1429 src/parser.cc:1429: CheckNextEscapedKeyword(ok); On 2015/11/03 23:00:57, adamk wrote: > You can CHECK_OK, and then please break. CHECK_OK means moving the macro definition here, but sgtm. Done. https://codereview.chromium.org/1429983002/diff/1/src/preparser.cc File src/preparser.cc (right): https://codereview.chromium.org/1429983002/diff/1/src/preparser.cc#newcode206 src/preparser.cc:206: CheckNextEscapedKeyword(ok); On 2015/11/03 23:00:57, adamk wrote: > Same here, CHECK_OK and break. Done. https://codereview.chromium.org/1429983002/diff/1/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode2572 src/preparser.h:2572: bool* is_computed_name, bool* is_id, bool* is_escaped_keyword, On 2015/11/03 23:00:57, adamk wrote: > Please keep the "is_identifier" name here as in the declaration. Done. https://codereview.chromium.org/1429983002/diff/1/src/preparser.h#newcode2649 src/preparser.h:2649: bool is_ident = false; On 2015/11/03 23:00:57, adamk wrote: > Please use "is_identifier" here too for consistency. Done. https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1505 src/scanner.cc:1505: if (next_.literal_chars && next_.literal_chars->is_one_byte() && On 2015/11/03 23:00:57, adamk wrote: > Please join this if() into the one on the line above Done. https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1506 src/scanner.cc:1506: next_literal_contains_escapes(next_)) { On 2015/11/03 23:00:57, adamk wrote: > This won't compile, I'm guessing you can drop next_literal_contains_escapes and > just call (with my suggested renaming) LiteralContainsEscapes(next_). It was a last minute fixup, which failed because of the parameter. I'll get rid of the next_literal_.... helper if you want, but it's more consistent with other functions in the Scanner, and a bit easier to read imo. 4 line difference, your call. https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1507 src/scanner.cc:1507: Vector<const uint8_t> chars = next_.literal_chars->one_byte_literal(); On 2015/11/03 23:00:57, adamk wrote: > On 2015/11/03 17:19:13, caitp wrote: > > I don't think this allocates a copy of the string, but if it does, let me know > > so I can come up with a cheaper way > > You're correct that there's no copy here. Acknowledged. https://codereview.chromium.org/1429983002/diff/1/src/scanner.cc#newcode1516 src/scanner.cc:1516: return sloppy_let || is_strict(language_mode); On 2015/11/03 23:00:57, adamk wrote: > My reading of the spec you pointed to is that LET is allowed in sloppy mode in > this case: > > It is a Syntax Error if this phrase is contained in strict mode code and the > StringValue of IdentifierName is: "implements", "interface", "let", "package", > "private", "protected", "public", "static", or "yield". > > Is the sloppy_let branch here derived from some other piece of spec text? It was meant for the block underneath, but I guess "let" was never a ReservedWord, so the 2nd block doesn't really apply. I'll get rid of that stuff then. Fixed this up https://codereview.chromium.org/1429983002/diff/1/src/scanner.h File src/scanner.h (right): https://codereview.chromium.org/1429983002/diff/1/src/scanner.h#newcode692 src/scanner.h:692: bool literal_contains_escapes(const TokenDesc& token) const { On 2015/11/03 23:00:57, adamk wrote: > This could be static (and thus not const). And CamelCase would also make it > stand out from the members. Done. https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7240: TEST(EscapedKeywords) { On 2015/11/03 23:00:57, adamk wrote: > I think you also want some more non-error cases: yield outside of a generator, > arguments and eval in sloppy mode, along with other things in that old > "implements", "interface", etc list. I've added sloppy vs strict tests for the future reserved words cases, as well as non-generator yield https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7263: "e\\u0078port var foo;", On 2015/11/03 23:00:57, adamk wrote: > This would already be failing since you're not parsing a module, so I don't > think this makes sense in this list. You could make a separate test case for > modules if you'd like (but I'm also fine with leaving that for another time). I've added a helper for testing module parsing, and am using that a bit, it might be useful for other tests in the future. It looked like less of a pain to do it that way than the way other module tests are written https://codereview.chromium.org/1429983002/diff/1/test/cctest/test-parsing.cc... test/cctest/test-parsing.cc:7269: "\\u0069mport blah from './foo.js';", On 2015/11/03 23:00:57, adamk wrote: > same here as above for export These would have been errors before, but only because of the `blah` following a previous `Identifier`. Now, the error is found before the second token is reached, so I think it's sort of good to keep it. But yeah, I'll add a second set of tests with the module flag enabled
some self-review / justification for decisions that might seem/be questionable https://codereview.chromium.org/1429983002/diff/20001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/20001/src/preparser.h#newcode664 src/preparser.h:664: Next(); Next() added to prevent a DCHECK() from crashing the app during PreParseProgram(), otherwise the parser can't handle it if the first token in the stream causes an error. This is probably a bug, but this was the simplest way to fix it https://codereview.chromium.org/1429983002/diff/20001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/20001/src/scanner.cc#newcode1514 src/scanner.cc:1514: return is_generator || is_strict(language_mode); yield is reserved in strict mode, so the change was needed https://codereview.chromium.org/1429983002/diff/20001/test/cctest/test-parsin... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/20001/test/cctest/test-parsin... test/cctest/test-parsing.cc:1570: if (is_module) { Might be simpler to just write a separate routine for this --- but i assume most of this gets fixed once it's possible to preparse Modules https://codereview.chromium.org/1429983002/diff/20001/test/mozilla/mozilla.st... File test/mozilla/mozilla.status (right): https://codereview.chromium.org/1429983002/diff/20001/test/mozilla/mozilla.st... test/mozilla/mozilla.status:92: 'ecma_3/Unicode/uc-003': [FAIL], unfortunately, only a small part of this test is invalid, the other assertions should be fine. But, don't have per-assertion granularity :( https://codereview.chromium.org/1429983002/diff/20001/test/test262/test262.st... File test/test262/test262.status (left): https://codereview.chromium.org/1429983002/diff/20001/test/test262/test262.st... test/test262/test262.status:462: # https://code.google.com/p/v8/issues/detail?id=1972 Oops, forgot to clear the bug # line too. fixing...
Is there a reason this cannot be fully encapsulated in the scanner? It seems like all you'd need to do is extend the KeywordOrIdentifierToken function to check whether the string contains an escape (e.g. with an extra flag its callers accumulate), and if it does, check that it does not normalise to a reserved word. If it does, simply produce an ILLEGAL token. (Or, if you insist on special error messages, some new ILLEGAL_ESCAPED_RESERVED_WORD token -- but I don't actually think a special error message is necessary.)
On 2015/11/04 12:18:54, rossberg wrote: > Is there a reason this cannot be fully encapsulated in the scanner? It seems > like all you'd need to do is extend the KeywordOrIdentifierToken function to > check whether the string contains an escape (e.g. with an extra flag its callers > accumulate), and if it does, check that it does not normalise to a reserved > word. If it does, simply produce an ILLEGAL token. (Or, if you insist on special > error messages, some new ILLEGAL_ESCAPED_RESERVED_WORD token -- but I don't > actually think a special error message is necessary.) The contextual keywords make that a bit tricky :(
On 2015/11/04 13:41:53, caitp wrote:
> On 2015/11/04 12:18:54, rossberg wrote:
> > Is there a reason this cannot be fully encapsulated in the scanner? It seems
> > like all you'd need to do is extend the KeywordOrIdentifierToken function to
> > check whether the string contains an escape (e.g. with an extra flag its
> callers
> > accumulate), and if it does, check that it does not normalise to a reserved
> > word. If it does, simply produce an ILLEGAL token. (Or, if you insist on
> special
> > error messages, some new ILLEGAL_ESCAPED_RESERVED_WORD token -- but I don't
> > actually think a special error message is necessary.)
>
> The contextual keywords make that a bit tricky :(
Hm, don't the is_{literal,next}_contextual_keyword predicates in the scanner
already ignore identifiers with escapes for free?
On 2015/11/04 14:43:57, rossberg wrote:
> On 2015/11/04 13:41:53, caitp wrote:
> > On 2015/11/04 12:18:54, rossberg wrote:
> > > Is there a reason this cannot be fully encapsulated in the scanner? It
seems
> > > like all you'd need to do is extend the KeywordOrIdentifierToken function
to
> > > check whether the string contains an escape (e.g. with an extra flag its
> > callers
> > > accumulate), and if it does, check that it does not normalise to a
reserved
> > > word. If it does, simply produce an ILLEGAL token. (Or, if you insist on
> > special
> > > error messages, some new ILLEGAL_ESCAPED_RESERVED_WORD token -- but I
don't
> > > actually think a special error message is necessary.)
> >
> > The contextual keywords make that a bit tricky :(
>
> Hm, don't the is_{literal,next}_contextual_keyword predicates in the scanner
> already ignore identifiers with escapes for free?
An example is the `yield` case --- sometimes it's a reserved word, sometimes
it's not. The scanner doesn't really know
On 2015/11/04 14:46:20, caitp wrote:
> On 2015/11/04 14:43:57, rossberg wrote:
> > On 2015/11/04 13:41:53, caitp wrote:
> > > On 2015/11/04 12:18:54, rossberg wrote:
> > > > Is there a reason this cannot be fully encapsulated in the scanner? It
> seems
> > > > like all you'd need to do is extend the KeywordOrIdentifierToken
function
> to
> > > > check whether the string contains an escape (e.g. with an extra flag its
> > > callers
> > > > accumulate), and if it does, check that it does not normalise to a
> reserved
> > > > word. If it does, simply produce an ILLEGAL token. (Or, if you insist on
> > > special
> > > > error messages, some new ILLEGAL_ESCAPED_RESERVED_WORD token -- but I
> don't
> > > > actually think a special error message is necessary.)
> > >
> > > The contextual keywords make that a bit tricky :(
> >
> > Hm, don't the is_{literal,next}_contextual_keyword predicates in the scanner
> > already ignore identifiers with escapes for free?
>
> An example is the `yield` case --- sometimes it's a reserved word, sometimes
> it's not. The scanner doesn't really know
Although to be fair, Mozilla treats an escaped "yield" as an error all the time,
so maybe that's fine.
On 2015/11/04 14:46:20, caitp wrote:
> On 2015/11/04 14:43:57, rossberg wrote:
> > On 2015/11/04 13:41:53, caitp wrote:
> > > On 2015/11/04 12:18:54, rossberg wrote:
> > > > Is there a reason this cannot be fully encapsulated in the scanner? It
> seems
> > > > like all you'd need to do is extend the KeywordOrIdentifierToken
function
> to
> > > > check whether the string contains an escape (e.g. with an extra flag its
> > > callers
> > > > accumulate), and if it does, check that it does not normalise to a
> reserved
> > > > word. If it does, simply produce an ILLEGAL token. (Or, if you insist on
> > > special
> > > > error messages, some new ILLEGAL_ESCAPED_RESERVED_WORD token -- but I
> don't
> > > > actually think a special error message is necessary.)
> > >
> > > The contextual keywords make that a bit tricky :(
> >
> > Hm, don't the is_{literal,next}_contextual_keyword predicates in the scanner
> > already ignore identifiers with escapes for free?
>
> An example is the `yield` case --- sometimes it's a reserved word, sometimes
> it's not. The scanner doesn't really know
Ah, okay. I think all other contextual keyword should work, but yield is really
special :(. Yet, couldn't that still be dealt with by merely distinguishing a
YIELD_ESCAPED token? (Which is an identifier wereever YIELD is, and not allowed
were YIELD is a keyword.)
On 2015/11/04 14:55:42, caitp wrote: > Although to be fair, Mozilla treats an escaped "yield" as an error all the time, > so maybe that's fine. I'd be fine with that, too, as long as test262 doesn't care. :)
I made a Scanner-only version that passes test262 awhile ago: https://codereview.chromium.org/1297253002/ but I was a bit unhappy with the fact that it doesn't actually match the spec.
On 2015/11/04 16:03:32, adamk wrote: > I made a Scanner-only version that passes test262 awhile ago: > > https://codereview.chromium.org/1297253002/ > > but I was a bit unhappy with the fact that it doesn't actually match the spec. I'm having a go at a more scanner-encapsulated version --- the only issue so far is FUTURE_STRICT_RESERVED_WORDS, but I think I have a way around that.
On 2015/11/04 16:05:22, caitp wrote: > On 2015/11/04 16:03:32, adamk wrote: > > I made a Scanner-only version that passes test262 awhile ago: > > > > https://codereview.chromium.org/1297253002/ > > > > but I was a bit unhappy with the fact that it doesn't actually match the spec. > > I'm having a go at a more scanner-encapsulated version --- the only issue so far > is FUTURE_STRICT_RESERVED_WORDS, but I think I have a way around that. But again, it's of course simpler to just always forbid escaped keywords, even if they're future-strict-reserved words in sloppy mode
On 2015/11/04 16:05:51, caitp wrote: > On 2015/11/04 16:05:22, caitp wrote: > > On 2015/11/04 16:03:32, adamk wrote: > > > I made a Scanner-only version that passes test262 awhile ago: > > > > > > https://codereview.chromium.org/1297253002/ > > > > > > but I was a bit unhappy with the fact that it doesn't actually match the > spec. > > > > I'm having a go at a more scanner-encapsulated version --- the only issue so > far > > is FUTURE_STRICT_RESERVED_WORDS, but I think I have a way around that. > > But again, it's of course simpler to just always forbid escaped keywords, even > if they're future-strict-reserved words in sloppy mode I've uploaded a version which produces a different token for escaped keywords and escaped strict reserved words. There are a few points in the parser which accommodate these tokens, but for the most part it should be handled transparently and automatically
Curious if rossberg likes this one better, even though it still touches the parser (feel free to wait for him before addressing my style comments). https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc#newcode... src/scanner.cc:1236: bool escaped = false; I think this mutable local actually makes this function harder to read, all the callsites know statically what its value is. https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc#newcode... src/scanner.cc:1263: return KeywordOrIdentifierToken(chars.start(), chars.length(), escaped); You could simply pass false here https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc#newcode... src/scanner.cc:1291: return ScanIdentifierSuffix(&literal, escaped); and true here https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc#newcode... src/scanner.cc:1307: return ScanIdentifierSuffix(&literal, escaped); and false here https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc#newcode... src/scanner.cc:1314: return KeywordOrIdentifierToken(chars.start(), chars.length(), escaped); and here too https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1546: if (!is_module) { Please add something of this form to make this clear: // Module syntax is only valid at toplevel and PreParser is only // used for function bodies. bool test_preparser = !is_module; https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1603: if (!is_module && !preparse_error) { and then use it here too https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1614: if (!is_module) { and here https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1629: } else if (!is_module && preparse_error) { and here https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1646: } else if (!is_module && and here
https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/100001/src/scanner.cc#newcode... src/scanner.cc:1236: bool escaped = false; On 2015/11/04 23:42:02, adamk wrote: > I think this mutable local actually makes this function harder to read, all the > callsites know statically what its value is. Re-reading the function, I agree. I thought there was another caller to ScanIdentifierUnicodeEscape() in this method, but nope.
On 2015/11/04 16:03:32, adamk wrote: > I made a Scanner-only version that passes test262 awhile ago: > > https://codereview.chromium.org/1297253002/ > > but I was a bit unhappy with the fact that it doesn't actually match the spec. Yeah, this passes test262 tests right now, but it looks to me like this is just a hole in test262 coverage. I've filed https://github.com/tc39/test262/issues/439 to get that fixed. That CL in its current form seems to do the opposite of what Mark Miller was asking for in the bug :\
On 2015/11/05 01:41:58, caitp wrote: > On 2015/11/04 16:03:32, adamk wrote: > > I made a Scanner-only version that passes test262 awhile ago: > > > > https://codereview.chromium.org/1297253002/ > > > > but I was a bit unhappy with the fact that it doesn't actually match the spec. > > Yeah, this passes test262 tests right now, but it looks to me like this is just > a hole in test262 coverage. I've filed > https://github.com/tc39/test262/issues/439 to get that fixed. > > That CL in its current form seems to do the opposite of what Mark Miller was > asking for in the bug :\ I agree that this patch is definitely better than my (lazy one). rossberg, can you comment on this version?
Yeah, this is probably as encapsulated as we can make it. :( Still have ome questions, though. https://codereview.chromium.org/1429983002/diff/100001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/100001/src/preparser.h#newcod... src/preparser.h:2059: if (next == Token::ESCAPED_STRICT_RESERVED_WORD) { Any reason not to put this into an else-if branch below? Why would we want to fall through to the IDENTIFIER case? Is that only for the duplicate finder? (If so, why is that not currently needed in the else-if? Do you think that's a bug?) https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod... src/preparser.h:2583: IdentifierT* name, bool* is_get, bool* is_set, bool* is_static, There are so many flags now that it's worth turning this into a bitset. https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod... src/preparser.h:2628: *is_identifier = true; Why does an escaped keyword qualify as an identifier? Shouldn't that be mutually exclusive? https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod... src/preparser.h:2708: if (is_escaped_keyword) { See above, I'm confused, which case would this cover? https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod... src/preparser.h:2709: classifier->RecordExpressionError( Wouldn't this be an error unconditionally? In which contexts wouldn't it be? https://codereview.chromium.org/1429983002/diff/140001/src/scanner.cc File src/scanner.cc (right): https://codereview.chromium.org/1429983002/diff/140001/src/scanner.cc#newcode... src/scanner.cc:1208: if (escaped && token == Token::FUTURE_STRICT_RESERVED_WORD) { \ Nit: Can we regroup this to if (escaped) { return token == Token::FUTURE_STRICT_RESERVED_WORD ? Token::ESCAPED_STRICT_RESERVED_WORD : Token::ESCAPED_KEYWORD; } return token; https://codereview.chromium.org/1429983002/diff/140001/src/scanner.cc#newcode... src/scanner.cc:1339: if (next_.literal_chars->is_one_byte() && escaped) { Nit: put the escaped first, since it is the cheaper condition. https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1546: if (!is_module) { Hm, this is a bit weird. This function is TestParserSync, but when by setting the module flag, it doesn't actually perform any sync test at all. Why does preparsing not work for modules?
https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1546: if (!is_module) { On 2015/11/06 13:31:05, rossberg wrote: > Hm, this is a bit weird. This function is TestParserSync, but when by setting > the module flag, it doesn't actually perform any sync test at all. Why does > preparsing not work for modules? There's currently no need to support modules in the preparser because "import" and "export" are only allowed at the toplevel, while the PreParser (in non-tests) is only ever run against function bodies. That's why I asked Caitlin to add another boolean and a comment.
https://codereview.chromium.org/1429983002/diff/100001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/100001/src/preparser.h#newcod... src/preparser.h:2059: if (next == Token::ESCAPED_STRICT_RESERVED_WORD) { On 2015/11/06 13:31:04, rossberg wrote: > Any reason not to put this into an else-if branch below? Why would we want to > fall through to the IDENTIFIER case? Is that only for the duplicate finder? (If > so, why is that not currently needed in the else-if? Do you think that's a bug?) The duplicate finder bit doesn't seem to stop `var fn = (implements, implements) => 0` from emitting an error in sloppy mode --- so maybe it's not a bug, or isn't needed anymore. I'll move the condition to the else-if https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h File src/preparser.h (right): https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod... src/preparser.h:2583: IdentifierT* name, bool* is_get, bool* is_set, bool* is_static, On 2015/11/06 13:31:05, rossberg wrote: > There are so many flags now that it's worth turning this into a bitset. I've started to do this, but it's a pretty substantial change that would bring a lot of noise into the diff --- will do this in a followup. https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod... src/preparser.h:2628: *is_identifier = true; On 2015/11/06 13:31:04, rossberg wrote: > Why does an escaped keyword qualify as an identifier? Shouldn't that be mutually > exclusive? Yeah, I guess --- the thing is, the error is only emit when `is_identifier` is set right now. I'll just change that to `is_identifier || is_escaped_keyword` or something, I guess https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsi... File test/cctest/test-parsing.cc (right): https://codereview.chromium.org/1429983002/diff/140001/test/cctest/test-parsi... test/cctest/test-parsing.cc:1546: if (!is_module) { On 2015/11/06 13:31:05, rossberg wrote: > Hm, this is a bit weird. This function is TestParserSync, but when by setting > the module flag, it doesn't actually perform any sync test at all. Why does > preparsing not work for modules? `PreParser.PreParseProgram()` doesn't handle the module case the way the full parser does (there is no `PreParser::ParseModuleItemList()`). It's not clear if it really needs to, since it's mostly used for functions
message: Addressed a bunch of comments --- i've added a small amount of code for
compat (formerly, `{ st\u0065tic }` was legal in ObjectLiterals, but a few
patchsets ago it wasn't. It's also added a little bit of code to report a
specific error in the ClassLiteral case when "static" is escaped in the keyword
position. It's an error regardless, so this isn't really needed, but It looks
good.
There also seems to be a bug which was introduced in 2014 ---
Scanner::IsGetOrSet requires that "get" or "set" are not escaped, but this isn't
mandated anywhere in the spec, and we produce different behaviour from Firefox
in this case.
Not going to change that, because it's probably a spec oversight that might
ought to be fixed, and even if it's not, it's doubtful anyone is using it
anyhow.
https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi...
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1429983002/diff/100001/test/cctest/test-parsi...
test/cctest/test-parsing.cc:1546: if (!is_module) {
On 2015/11/04 23:42:02, adamk wrote:
> Please add something of this form to make this clear:
>
> // Module syntax is only valid at toplevel and PreParser is only
> // used for function bodies.
> bool test_preparser = !is_module;
Done.
https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod...
src/preparser.h:2708: if (is_escaped_keyword) {
On 2015/11/06 13:31:05, rossberg wrote:
> See above, I'm confused, which case would this cover?
originally, I had wanted to report a slightly different message --- but then I
thought there wasn't much point, so this is dead code now. removed in the next
patch set
https://codereview.chromium.org/1429983002/diff/140001/src/preparser.h#newcod...
src/preparser.h:2709: classifier->RecordExpressionError(
On 2015/11/06 13:31:05, rossberg wrote:
> Wouldn't this be an error unconditionally? In which contexts wouldn't it be?
it always is, the original idea was just to use a different error message ---
but at some point I dropped that idea and never removed this. It's gone now
https://codereview.chromium.org/1429983002/diff/140001/src/scanner.cc
File src/scanner.cc (right):
https://codereview.chromium.org/1429983002/diff/140001/src/scanner.cc#newcode...
src/scanner.cc:1208: if (escaped && token == Token::FUTURE_STRICT_RESERVED_WORD)
{ \
On 2015/11/06 13:31:05, rossberg wrote:
> Nit: Can we regroup this to
>
> if (escaped) {
> return token == Token::FUTURE_STRICT_RESERVED_WORD
> ? Token::ESCAPED_STRICT_RESERVED_WORD : Token::ESCAPED_KEYWORD;
> }
> return token;
Done.
https://codereview.chromium.org/1429983002/diff/140001/src/scanner.cc#newcode...
src/scanner.cc:1339: if (next_.literal_chars->is_one_byte() && escaped) {
On 2015/11/06 13:31:05, rossberg wrote:
> Nit: put the escaped first, since it is the cheaper condition.
Done.
On 2015/11/06 19:08:06, caitp wrote: > There also seems to be a bug which was introduced in 2014 --- > Scanner::IsGetOrSet requires that "get" or "set" are not escaped, but this isn't > mandated anywhere in the spec, and we produce different behaviour from Firefox > in this case. > > Not going to change that, because it's probably a spec oversight that might > ought to be fixed, and even if it's not, it's doubtful anyone is using it > anyhow. This is not a bug. The spec requires (somewhere, I would need to look it up) to interpret literal strings in the grammar literally. So escapes are not allowed in contextual keywords like get/set either.
On 2015/11/10 08:11:15, rossberg wrote: > On 2015/11/06 19:08:06, caitp wrote: > > There also seems to be a bug which was introduced in 2014 --- > > Scanner::IsGetOrSet requires that "get" or "set" are not escaped, but this > isn't > > mandated anywhere in the spec, and we produce different behaviour from Firefox > > in this case. > > > > Not going to change that, because it's probably a spec oversight that might > > ought to be fixed, and even if it's not, it's doubtful anyone is using it > > anyhow. > > This is not a bug. The spec requires (somewhere, I would need to look it up) to > interpret literal strings in the grammar literally. So escapes are not allowed > in contextual keywords like get/set either. Allen pointed that out, it's mentioned in the first paragraph of the grammar notation. No problem
Ping, rossberg? Are there remaining concerns about this change?
On 2015/11/17 00:23:44, adamk wrote: > Ping, rossberg? Are there remaining concerns about this change? Yes, sorry, LGTM.
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429983002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429983002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11942) v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/7806)
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1429983002/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429983002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429983002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5bf360ef5773e5ceecc3b55e349db5a45b831d1d Cr-Commit-Position: refs/heads/master@{#32052} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
