1336783 - Remove TokenStream::KeywordIsName

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Related to bug 1319416.

TokenStream::KeywordIsName does totally different thing than other modifier,
and it seems to make it hard to fix bug 1319416.

from some experiments, checking keyword/name could be deferred to Parser::{labelOrIdentifierReference,bindingIdentifier}, by following way:
  1. tokenize every special identifier as dedicated TokenKind
     (unless it has unicode escape)
  2. when parser need possible Identifier / IdentifierName TokenKind,
     compare the given TokenKind with range that represents possible Identifier or IdentifierName,
     instead of comparing with TOK_NAME/TOK_YIELD directly
  3. In Parser::{labelOrIdentifierReference,bindingIdentifier},
     check PropertyName* of the token, to check unescaped name

with this way, we don't have to check keyword (actually Reserved Word) in TokenStream, and also it have some merits that:
  a. TOK_NAME/TOK_YIELD handling can be merged
  b. we can use dedicated TokenKind for "as", "async", "from", etc,
     as contextual keyword,
     and parser logic becomes simpler
  c. Parser doesn't have to check escapeness at several places

I'll post patch after extra try run.

Comment 1

[Tooru Fujisawa [:arai]]

try run
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4208baff1a996cb9d8d5d3a890a8c30e1c67e6f3

Comment 2

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> from some experiments, checking keyword/name could be deferred to
> Parser::{labelOrIdentifierReference,bindingIdentifier}

and also some more places that checks identifier, like
  * Parser::checkStrictBinding
  * ParserBase::isValidStrictBinding

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Rework on reserved word and remove TokenStream::KeywordIsName.

here's draft patch that removes TokenStream::KeywordIsName.
can I get your feedback, related to bug 1319416 fix?
(it touches many places and I guess it will conflict with your patch there...)

the details of the change are following:

[reserved words check]
  * Moved the common part of Parser::{labelOrIdentifierReference,bindingIdentifier}
    to Parser::checkLabelOrIdentifierReference, that also checks Reserved Word
  * Changed Parser::checkStrictBinding to report with more appropriate error message
  * Added the following functions to check TokenKind of PropertyName*:
      * IsFutureReservedWord
      * IsStrictReservedWord
      * IsReservedWordLiteral
    to use in Parser::checkLabelOrIdentifierReference
  * Removed special handling for TOK_NAME with yield/await/let/static name
  * Stop checking reserved word error in TokenStream::getTokenInternal,
    and removed the following methods:
      * TokenStream::checkForKeyword
  * Moved awaitIsKeyword handling from TokenStream to Parser
  * Removed TokenStream::KeywordIsName and TokenStream::NoneIsKeywordIsName

[contextual keyword]
  * Added the following contextual keyword TokenKind:
      * TOK_AS
      * TOK_ASYNC
      * TOK_EACH
      * TOK_FROM
      * TOK_GET
      * TOK_LET
      * TOK_OF
      * TOK_SET
      * TOK_STATIC
      * TOK_TARGET
  * Used above contextual keyword TokenKind in Parser, and
    removed the following methods:
      * Parser::checkUnescapedName
      * TokenStream::matchContextualKeyword

[reserved words]
  * Added the following future reserved words/strict reserved words TokenKind to make
    them easy to handle in error reporting:
      * TOK_ENUM
      * TOK_IMPLEMENTS
      * TOK_INTERFACE
      * TOK_PACKAGE
      * TOK_PRIVATE
      * TOK_PROTECTED
      * TOK_PUBLIC
  * Added the following functions to check TokenKind with range:
      * TokenKindIsKeyword
      * TokenKindIsContextualKeyword
      * TokenKindIsFutureReservedWord
      * TokenKindIsStrictReservedWord
      * TokenKindIsReservedWordLiteral
      * TokenKindIsReservedWord
      * TokenKindIsPossibleIdentifier
      * TokenKindIsPossibleIdentifierName
  * Added MUST_MATCH_TOKEN_COND_MOD/MUST_MATCH_TOKEN_COND macro to
    check arbitrary condition (TokenKindIs*) instead of comparing with single TokenKind
  * Use TokenKindIsPossibleIdentifier or TokenKindIsPossibleIdentifierName where
    we were checking TOK_NAME/TOK_YIELD in Parser
  * Changed TokenStream::{currentName,nextName} to return PropertyName
    for all TokenKindIsPossibleIdentifierName==true case,
    instead of only TOK_NAME/TOK_YIELD

[filename]
  * Renamed the following files and also changed "keyword" to "reserved word":
      * js/src/vm/Keywords.h => js/src/vm/ReservedWords.h
      * js/src/jsautokw.h => js/src/jsautorw.h

[other]
  * Fixed local ExportClause to check IdentifierReference

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8833732 [details] [diff] [review]
Rework on reserved word and remove TokenStream::KeywordIsName.

Review of attachment 8833732 [details] [diff] [review]:
-----------------------------------------------------------------

This is far cleaner than it has any right to be.  Never would have thought adding non-keyword reserved words to the master list would actually make things *cleaner*.

I'm trying to land my two conflicting patches as fast as possible, but tree's closed right now.  Will land and get out of the way of this ASAP.

::: js/src/frontend/Parser.cpp
@@ +4647,5 @@
>  
>              if (tt == TOK_RC)
>                  break;
>  
> +            MUST_MATCH_TOKEN_COND(TokenKindIsPossibleIdentifierName(token), JSMSG_NO_IMPORT_NAME);

I assume |token| doesn't actually compile here, right?  And many other places.  I'm guessing this is just f? purposes, mostly?

@@ +4778,5 @@
>      if (!importSpecSet)
>          return null();
>  
> +    if (TokenKindIsPossibleIdentifierName(tt) || tt == TOK_LC || tt == TOK_MUL) {
> +        if (TokenKindIsPossibleIdentifierName(tt)) {

TKIPI starts to get complex enough internally, that I'd like to avoid computing it multiple times -- and, computing it when it's not needed (i.e. when we have '{' or '*').  Please rearrange this whole |if| to

  if (tt == TOK_STRING) {
    ...
  } else {
    if (tt == TOK_LC || tt == TOK_MUL) {
      if (!namedImportsOrNamespaceImport(tt, importSpecSet))
        return null();
    } else if (TokenKindIsPossibleIdentifierName(tt)) {
      Node importName = newName(context->names().default_);
      ...
    } else {
      error(JSMSG_DECLARATION_AFTER_IMPORT);
      return null();
    }

    if (!tokenStream.getToken(&tt))
      return null();

    if (tt != TOK_FROM) {
      error(JSMSG_FROM_AFTER_IMPORT_CLAUSE);
      return null();
    }

    MUST_MATCH_TOKEN(TOK_STRING, JSMSG_MODULE_SPEC_AFTER_FROM);
  }

@@ +4956,3 @@
>                  return null();
>              if (foundAs)
> +                MUST_MATCH_TOKEN_COND(TokenKindIsPossibleIdentifierName(token),

Needs braces.

@@ +4995,5 @@
>          // In that case let matchOrInsertSemicolonAfterNonExpression sort out
>          // ASI or any necessary error.
>          TokenKind tt;
>          if (!tokenStream.getToken(&tt, TokenStream::Operand))
>              return null();

Could maybe use matchToken(TOK_FROM, TokenStream::Operand) now so you don't have to unget later.

@@ +5012,5 @@
>              if (!node || !pc->sc()->asModuleContext()->builder.processExportFrom(node))
>                  return null();
>  
>              return node;
> +        } else {

Don't need an |else| here.

@@ +5018,5 @@
> +            // should contain IdentifierReference.
> +
> +            for (ParseNode* next = kid->pn_head; next; next = next->pn_next) {
> +                if (!next->isKind(PNK_EXPORT_SPEC))
> +                    continue;

I must be reading this wrong somehow, because it looks like the only children appended to |kid| are *always* PNK_EXPORT_SPEC.  So this loop never does anything but iterate everything and continue on each node.  Why isn't this the case?  But, I assume this loop fixes the additional test you added to export-declaration.js, so I'm maybe confused here.

Assuming this loop does actually do anything, I don't see why identifier-checking can't be performed directly as the ExportClause is being parsed.  Disillusion me?

@@ +5121,3 @@
>                  return null();
>              break;
>            case TOK_CLASS:

Apropos of nothing, we really need to split up Parser::exportDeclaration into sub-functions to handle the various different possible syntaxes within it.  The shared three-ish line tail can't possibly justify it.

@@ +6767,3 @@
>      // Otherwise a let declaration must have a name.
> +    if (TokenKindIsPossibleIdentifier(next)) {
> +        // "let" edge case deserves special comment.

"""
A "let" edge case deserves special comment.  Consider this:
"""

Particularly, "'let' edge case" requires some sort of article -- a/an/the -- in front of it.  Can't just lead with the noun phrase without one.  (I can't articulate why.)

@@ +6846,5 @@
> +        // Avoid getting next token with None.
> +        if (tt == TOK_AWAIT && pc->isAsync())
> +            return expressionStatement(yieldHandling);
> +
> +        if (!TokenKindIsPossibleIdentifier(tt) && tt != TOK_LET)

TokenKindIsPossibleIdentifier(TOK_LET) is true.  So the second condition there is, if evaluated, always true.  I think the prior system here of peeking, handling |let| if encountered and actually a declaration, then if necessary falling through to the label/expression cases, was the proper one.  So some of this changing needs undoing.

@@ +7056,5 @@
> +        // Avoid getting next token with None.
> +        if (tt == TOK_AWAIT && pc->isAsync())
> +            return expressionStatement(yieldHandling);
> +
> +        if (!TokenKindIsPossibleIdentifier(tt) && tt != TOK_LET && tt != TOK_ASYNC)

Again the latter two conditions seem vacuously true.  And again, it seems to me that the previous scheme of handling specific cases, then falling through into the label/not-label cases, is the one we should keep using.

@@ +7065,5 @@
>              return null();
>  
> +        if (tt == TOK_LET && nextTokenContinuesLetDeclaration(next, yieldHandling)) {
> +            return lexicalDeclaration(yieldHandling, /* isConst = */ false);
> +        }

No braces.

@@ +7498,5 @@
>  
>      bool endsExpr;
>  
> +    // This is fast path for IdentifierReference.
> +    // Do not handle other possible identifier cases.

Maybe "This only handles identifiers that *never* have special meaning anywhere in the language.  Contextual keywords, keywords only reserved in strict mode, and other hard cases are handled outside this fast path."

@@ +8584,5 @@
> +        errorAt(offset, JSMSG_RESERVED_ID, ReservedWordToCharZ(ident));
> +        return nullptr;
> +    }
> +
> +

Extra blank line.

@@ +8873,3 @@
>          propAtom.set(tokenStream.currentName());
>          // Do not look for accessor syntax on generators
> +        if (isGenerator || isAsync || !(ltok != TOK_GET || ltok != TOK_SET)) {

Shouldn't that be

  !(ltok == TOK_GET || ltok == TOK_SET)

?  Or maybe just

   (ltok != TOK_GET && ltok != TOK_SET)

since we're basically handling things that are not get/set as accessor.  That seems clearest to me.

@@ +8956,5 @@
>          return propName;
>      }
>  
> +    if (TokenKindIsPossibleIdentifierName(ltok)
> +        && (tt == TOK_COMMA || tt == TOK_RC || tt == TOK_ASSIGN))

&& at end of previous line.

::: js/src/frontend/Parser.h
@@ +877,5 @@
>  
>  template <typename ParseHandler>
>  class Parser final : public ParserBase, private JS::AutoGCRooter
>  {
> +

Stray blank line.

@@ +1472,5 @@
> +private:
> +    Parser<ParseHandler>* parser_;
> +    bool oldAwaitIsKeyword_;
> +
> +public:

These access specifiers want to be indented two, and elsewhere in this patch.

@@ +1481,5 @@
> +    }
> +
> +    ~AutoAwaitIsKeyword() {
> +        parser_->setAwaitIsKeyword(oldAwaitIsKeyword_);
> +        parser_ = nullptr;

Doesn't seem necessary to null here, no?

::: js/src/frontend/TokenKind.h
@@ +275,5 @@
>  
> +inline MOZ_MUST_USE bool
> +TokenKindIsKeyword(TokenKind tt)
> +{
> +    return (tt >= TOK_KEYWORD_FIRST && tt <= TOK_KEYWORD_LAST) ||

I'd prefer

  BEGIN <= tt && tt <= END

as general form, to read more like the specification of a range.

::: js/src/frontend/TokenStream.cpp
@@ +182,5 @@
>  bool
>  frontend::IsKeyword(JSLinearString* str)
>  {
> +    const ReservedWordInfo* rw = FindReservedWord(str);
> +    if (rw == nullptr)

Could do

  if (const ReservedWordInfo* rw = FindReservedWord(str))
    return TokenKindIsKeyword(rw->tokentype);

  return false;

and for the others.

@@ +228,5 @@
> +
> +#define EMIT_RETURN(word, name, type) \
> +    if (rw->tokentype == type) \
> +        return js_##word##_str;
> +FOR_EACH_JAVASCRIPT_RESERVED_WORD(EMIT_RETURN)

Mm.  I think I'd rather not rely on the compiler to figure out how to tableswitch this.  Can we get some sort of switch for this, maybe, somehow?  (And also in the next function.)

@@ +1446,5 @@
>  
> +        // Represent reserved words as reserved word tokens.
> +        if (!hadUnicodeEscape) {
> +            const ReservedWordInfo* rw = FindReservedWord(chars, length);
> +            if (rw) {

if (const ...) {

::: js/src/jit-test/tests/modules/export-declaration.js
@@ +434,3 @@
>  assertThrowsInstanceOf(function() {
> +    parseAsModule("export { true }");
> +}, SyntaxError);

It'd probably be good to separate out this fix, if at all possible, from the larger patch.

::: js/src/moz.build
@@ +632,5 @@
>      ]
>  
> +GENERATED_FILES += ['jsautorw.h']
> +jsautorw = GENERATED_FILES['jsautorw.h']
> +jsautorw.script = 'jsautorw.py'

I'd prefer ReservedWordsGenerated.{h,py} for naming -- we're trying to get rid of js* names, very slowly.  Maybe in frontend/ if possible.

Comment 5

[Jeff Walden [:Waldo]]

Oh, as a practical matter, conflict away with my modifier changes.  Fixing the individual conflicts will be finicky but straightforward, I expect.

Comment 6

[Tooru Fujisawa [:arai]]

Thank you for your feedback :)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> ::: js/src/frontend/Parser.cpp
> @@ +4647,5 @@
> >  
> >              if (tt == TOK_RC)
> >                  break;
> >  
> > +            MUST_MATCH_TOKEN_COND(TokenKindIsPossibleIdentifierName(token), JSMSG_NO_IMPORT_NAME);
> 
> I assume |token| doesn't actually compile here, right?  And many other
> places.  I'm guessing this is just f? purposes, mostly?

`token` is actually a part of the macro.

> #define MUST_MATCH_TOKEN_COND_MOD(cond, modifier, errorNumber)              \
>     JS_BEGIN_MACRO                                                          \
>         TokenKind token;                                                    \
> ...
>         if (!(cond)) {                                                      \

I guess it should be rewritten to be more readable, maybe passing function name instead of partial expression.


> @@ +5018,5 @@
> > +            // should contain IdentifierReference.
> > +
> > +            for (ParseNode* next = kid->pn_head; next; next = next->pn_next) {
> > +                if (!next->isKind(PNK_EXPORT_SPEC))
> > +                    continue;
> 
> I must be reading this wrong somehow, because it looks like the only
> children appended to |kid| are *always* PNK_EXPORT_SPEC.  So this loop never
> does anything but iterate everything and continue on each node.  Why isn't
> this the case?  But, I assume this loop fixes the additional test you added
> to export-declaration.js, so I'm maybe confused here.
> 
> Assuming this loop does actually do anything, I don't see why
> identifier-checking can't be performed directly as the ExportClause is being
> parsed.  Disillusion me?

yeah, the condition was unnecessary, and "continue" won't be reached.
it checks if the ExportClause contains IdentifierReference instead of IdentifierName.

looks like the spec I was referring was rewritten today :P
https://github.com/tc39/ecma262/commit/9c0c0d2ffa39be4c0af01c8e9410a649440d636f

I'll split that part into another patch for another bug, and also rewrite to follow the updated spec.


> @@ +5121,3 @@
> >                  return null();
> >              break;
> >            case TOK_CLASS:
> 
> Apropos of nothing, we really need to split up Parser::exportDeclaration
> into sub-functions to handle the various different possible syntaxes within
> it.  The shared three-ish line tail can't possibly justify it.

will fix in another patch.
also, looks like I forgot to handle `export async function(){}` :P


> @@ +6846,5 @@
> > +        // Avoid getting next token with None.
> > +        if (tt == TOK_AWAIT && pc->isAsync())
> > +            return expressionStatement(yieldHandling);
> > +
> > +        if (!TokenKindIsPossibleIdentifier(tt) && tt != TOK_LET)
> 
> TokenKindIsPossibleIdentifier(TOK_LET) is true.  So the second condition
> there is, if evaluated, always true.  I think the prior system here of
> peeking, handling |let| if encountered and actually a declaration, then if
> necessary falling through to the label/expression cases, was the proper one.
> So some of this changing needs undoing.

removed unnecessary "tt != TOK_LET" part.
I added it just to explicitly exclude the cases that is handled below, but `!TokenKindIsPossibleIdentifier(tt)` already filtered that case out.

in patched code, `TOK_AWAIT` and `!TokenKindIsPossibleIdentifier(tt)` cases handles unpatched-default clause, that just does `return expressionStatement(yieldHandling);`.
also, remaining part handles unpatched-TOK_NAME case and some deferred error cases for reserved words.
I don't see any difference.  can you tell me more about the issue?

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Rework on reserved word and remove TokenStream::KeywordIsName.

update from previous patch:
  * Renamed jsautokw.{py,h} => frontend/ReservedWordsGenerated.{py,h}

  * Added MUST_MATCH_TOKEN_INTERNAL that is used by the following macros:
    * MUST_MATCH_TOKEN
    * MUST_MATCH_TOKEN_MOD
    * MUST_MATCH_TOKEN_FUNC
    * MUST_MATCH_TOKEN_FUNC_MOD

  * Removed the fix for export, except removing wrong testcase that fails now

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove Parser::checkStrictBinding.

now Parser::checkStrictBinding is not necessary in most case,
since bindingIdentifier checks the same thing before that point.
but some cases ([A], [B], and [C] in the following) needs extra check.

also extra warning should be shown, so changed labelOrIdentifierReference/bindingIdentifier to handle needStrictChecks and use strictModeErrorAt.


here's reverse call tree for checkStrictBinding, and possible name value:

checkStrictBinding
  notePositionalFormalParameter
    functionArguments <= bindingIdentifier(yieldHandling)
  noteDeclaredName
    functionStmt <= bindingIdentifier(yieldHandling)
                    context->names().starDefaultStar
    checkDestructuringName <= expr->name()
      checkDestructuringObject <= destructuring object pattern item
                                  identifierReference(yieldHandling) [A]
      checkDestructuringArray <= destructuring array pattern item
                                 identifierReference(yieldHandling) [A]
    declarationName <= bindingIdentifier(yieldHandling)
    namedImportsOrNamespaceImport <= importedBinding()
                                     importedBinding()
    importDeclaration <= importedBinding()
    exportDeclaration <= context->names().starDefaultStar
    tryStatement <= bindingIdentifier(yieldHandling)
    classDefinition <= bindingIdentifier(yieldHandling)
                       context->names().starDefaultStar
    comprehensionFor <= tokenStream.currentName() [B]
  functionFormalParametersAndBody <= fun->explicitName()->asPropertyName() [C]

[A]
need to check `arguments` and `eval` name.

Moved the checking part of Parser::bindingIdentifier to Parser::checkBindingIdentifier, and added checkBindingIdentifier call before noteDeclaredName.

[B]
need to check if it's bindingIdentifier

changed to bindingIdentifier(YieldIsKeyword), since the previous definition only allowed TOK_NAME.

[C]
some case needs bindingIdentifier check (see below)

Replaced checkStrictBinding with checkBindingIdentifier call.

possible source of fun->explicitName()
JSFunction::atom_, not HAS_COMPILE_TIME_NAME, not HAS_GUESSED_ATOM
  initAtom
    XDRInterpretedFunction <= already checked
    createScriptForLazilyInterpretedFunction <= already checked
    NewFunctionWithProto <= *** at least this should be checked ***
    NewFunctionClone <= already checked
    getSelfHostedFunction <= already checked
  setAtom
    intrinsic_FinishBoundFunctionInit <= already checked
    intrinsic_SetCanonicalName <= never fail
    ParseFunction <= m.parser().bindingIdentifier(YieldIsName)

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8835099 [details] [diff] [review]
Part 1: Rework on reserved word and remove TokenStream::KeywordIsName.

Review of attachment 8835099 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +66,5 @@
>      JS_BEGIN_MACRO                                                                          \
>          TokenKind token;                                                                    \
>          if (!tokenStream.getToken(&token, modifier))                                        \
>              return null();                                                                  \
> +        if (!(cond)) {                                                                      \

Instead of |cond|, make this take |func| and then do

  if (!(func)(token))

and at that point this macro can just be MUST_MATCH_TOKEN_FUNC and MUST_MATCH_TOKEN and MUST_MATCH_TOKEN_MOD can both invoke it.

@@ +76,5 @@
> +#define MUST_MATCH_TOKEN(tt, errorNumber) \
> +    MUST_MATCH_TOKEN_INTERNAL(token == tt, TokenStream::None, errorNumber)
> +
> +#define MUST_MATCH_TOKEN_MOD(tt, modifier, errorNumber) \
> +    MUST_MATCH_TOKEN_INTERNAL(token == tt, modifier, errorNumber)

The |func| passed for this macro and the one above can be

  [](TokenKind tok) { return tok == tt; }

@@ +81,5 @@
> +
> +#define MUST_MATCH_TOKEN_FUNC(func, errorNumber) \
> +    MUST_MATCH_TOKEN_INTERNAL(func(token), TokenStream::None, errorNumber)
> +
> +#define MUST_MATCH_TOKEN_FUNC_MOD(func, modifier, errorNumber) \

If the parameter is |func|, these two macros can just go away entirely.

@@ +792,5 @@
> +Parser<FullParseHandler>::setAwaitIsKeyword(bool isKeyword)
> +{
> +    awaitIsKeyword_ = isKeyword;
> +    Parser<SyntaxParseHandler>* parser = handler.syntaxParser;
> +    if (parser)

if (Parser<>* parser = ...)

@@ +951,5 @@
> +    if (name == context->names().eval)
> +        return strictModeErrorAt(pos.begin, JSMSG_BAD_BINDING, "eval");
> +
> +    if (name == context->names().let) {
> +        error(JSMSG_RESERVED_ID, "let");

Don't these need to be |errorAt| to be consistent with the cases above?  If for some bizarre reason they should be different, a comment is needed.

::: js/src/frontend/ReservedWordsGenerated.py
@@ +1,1 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public

Seems like GenerateReservedWords.py is a mildly better filename.

::: js/src/frontend/TokenStream.h
@@ +344,5 @@
>      JSVersion versionWithFlags() const { return options().version; }
>  
> +  private:
> +    PropertyName*
> +    reservedWordToPropertyName(TokenKind tt) const;

This all fits on one line, right?

@@ +351,2 @@
>      PropertyName* currentName() const {
> +        if (!isCurrentTokenType(TOK_NAME)) {

I'd prefer

  if (isCurrentTokenType(TOK_NAME))
    return currentToken().name();

  MOZ_ASSERT(TKIPIN(...));
  return rWTPN(...);

to have the common case read first.  And in nextName() too.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8835102 [details] [diff] [review]
Part 2: Remove Parser::checkStrictBinding.

Review of attachment 8835102 [details] [diff] [review]:
-----------------------------------------------------------------

Please test that the wrong testcase, removed in the previous patch, is readded here except with a check for a syntax error.

I'm running a little short on time, but I'd rather stamp now than delay longer on this.  We can do more in a followup to deal with the more involved comments here.

::: js/src/frontend/Parser.cpp
@@ +8532,5 @@
>          {
>              errorAt(offset, JSMSG_RESERVED_ID, "yield");
>              return nullptr;
>          }
> +        if (pc->sc()->needStrictChecks()) {

Hmm.  I distinctly remember changing this code to check strictness, from the strict-checks behavior.  (And adding a test that I can't find now.)  I did so because the strict-checks behavior, in concert with werror, makes violations of this in lazily-parsed code into errors *at the time of full parsing*.  That's no good, it's long after the actual parse, etc.  For example, I'm fairly sure you'd break this:

  var yield = 42;
  function f() { yield = 5; } // parses
  options("strict");
  options("werror");
  f(); // delazifies |f|, tries to parse it, ERROR

That said, it looks like this slight variant is *already* broken this way:

  function f() { var yield; } // parses
  options("strict");
  options("werror");
  f(); // delazifies |f|, tries to parse it, ERROR

I guess you can go ahead with this, but we really need to follow up quickly and figure out *some* sort of way for this stuff to play well with lazy parsing.  I'm not actually sure what that is -- maybe a way to tag the LazyScript to indicate that bad-name-warning already happened, and don't do it?  Initially set to false, then set to true after the lazy-parse completes.  Or something.

Comment 11

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
>   var yield = 42;
>   function f() { yield = 5; } // parses
>   options("strict");
>   options("werror");
>   f(); // delazifies |f|, tries to parse it, ERROR

Is this possible scenario in web, or just an example that demonstrates more complicated issue?
I don't think strictness can be changed at that point, without the testing function.

Comment 12

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/476a938ee2abc89377b9451295735919c51259f9
Bug 1336783 - Part 1: Rework on reserved word and remove TokenStream::KeywordIsName. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/761227e357b95f345ec43df0b2be4cba88393ef4
Bug 1336783 - Part 2: Remove Parser::checkStrictBinding. r=jwalden

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/476a938ee2ab
Part 1: Rework on reserved word and remove TokenStream::KeywordIsName. r=jwalden
https://hg.mozilla.org/integration/mozilla-inbound/rev/761227e357b9
Part 2: Remove Parser::checkStrictBinding. r=jwalden

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 3 - Use simpler macro to avoid internal compiler error on SM-tc(H).

lambda in MUST_MATCH_TOKEN_MOD hit internal compiler error on SM-tc(H):
https://treeherder.mozilla.org/logviewer.html#?job_id=77609299&repo=mozilla-inbound&lineNumber=4995

changed to simpler one that just passes expression.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8837642 [details] [diff] [review]
Part 3 - Use simpler macro to avoid internal compiler error on SM-tc(H).

Review of attachment 8837642 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable and I'll stamp it to avoid a backout (hopefully), but maybe you can do some Try pushes to narrow it down more? I'm wondering whether parentheses around the lambda would help.

::: js/src/frontend/Parser.cpp
@@ +79,5 @@
>  #define MUST_MATCH_TOKEN(tt, errorNumber) \
>      MUST_MATCH_TOKEN_MOD(tt, TokenStream::None, errorNumber)
>  
> +#define MUST_MATCH_TOKEN_FUNC_MOD(func, modifier, errorNumber) \
> +    MUST_MATCH_TOKEN_INTERNAL(func(token), modifier, errorNumber)

Should we put parens around func here, like before?

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9541461973eadf8ceab7422283211d643ecb24b5
Bug 1336783 - Part 3 - Use simpler macro to avoid internal compiler error on SM-tc(H). r=jandem CLOSED TREE

Comment 17

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2810dbb5675c94ba4b2f0dd5fc92504d1dfecb96
Bug 1336783 - followup: Fix rooting. r=bustage

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/476a938ee2ab
https://hg.mozilla.org/mozilla-central/rev/761227e357b9
https://hg.mozilla.org/mozilla-central/rev/9541461973ea
https://hg.mozilla.org/mozilla-central/rev/2810dbb5675c

Comment 19

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #11)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> >   var yield = 42;
> >   function f() { yield = 5; } // parses
> >   options("strict");
> >   options("werror");
> >   f(); // delazifies |f|, tries to parse it, ERROR
> 
> Is this possible scenario in web, or just an example that demonstrates more
> complicated issue?
> I don't think strictness can be changed at that point, without the testing
> function.

It's not possible if we're looking solely at the web.  However, as a matter of the APIs SpiderMonkey provides, it does exist as a concern.  (Incidentally, I was thinking of the case of functions whose names are extra-warnings violations.)

I think, if we're going to fix this concern, it probably has to be fixed system-wide: extraWarnings+werror needs to do all warning/erroring on the first parse, and never on delazifying parses.  Will have to think about how to do this, but separate from this bug.