1154391 - Update parsing of import/export declarations to current ES6 spec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jon Coppeard (:jonco)]

Currently we don't support namespace imports or named imports following a default binding.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: update-import-declaration

Patch to update the parser and tests.

This turns PNK_IMPORT nodes into ternary nodes, with their second child being an optional namespace import name.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: update-import-declaration v2

I realised after I posted this that these import specifiers are going to be used to create ImportEntry records for some module, and so rather than having a separate way of returning a namespace import we can just add an import specifier for '*'.  This makes things much simpler.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: update-export-declaration

The spec doesn't allow |export *| except as part of |export * from 'module'| so this patch changes that to be an error and adds tests.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: update-export-declaration v2

I realised I missed a couple things so there's an updated patch that:
 - parameterises function and class parsing with grammar parameter 'Default'
 - adds support for |export default| as new parse node kind
 - adds support for exporting classes
 - disallows |export *| without following |from|

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8594001 [details] [diff] [review]
update-import-declaration v2

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

bumping review to shu because I suck

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8596525 [details] [diff] [review]
update-export-declaration v2

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

"

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8594001 [details] [diff] [review]
update-import-declaration v2

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

Looks compliant to spec by my reading.

::: js/src/frontend/Parser.cpp
@@ +4185,5 @@
> +
> +            // If the next token is a keyword, the previous call to
> +            // peekToken matched it as a TOK_NAME, and put it in the
> +            // lookahead buffer, so this call will match keywords as well.
> +            MUST_MATCH_TOKEN(TOK_NAME, JSMSG_NO_IMPORT_NAME);

So this is kinda sorta abusing the type system. MUST_MATCH_TOKEN returns null(), which for the FullParseHandler is nullptr, so gets automatically coerced to false. That said, it reads nice and I'm fine with nullptr -> false.

Could you make this method an explicit FullParseHandler specialization though? I don't see how it would compile for SyntxParseHandler.

@@ +4198,5 @@
> +                    return false;
> +                if (tt != TOK_NAME) {
> +                    report(ParseError, false, null(), JSMSG_NO_BINDING_NAME);
> +                    return false;
> +                }

Could fold to MUST_MATCH_TOKEN.

@@ +4202,5 @@
> +                }
> +            } else {
> +                // Keywords cannot be bound to themselves, so an import name
> +                // that is a keyword is a syntax error if it is not followed
> +                // by the keyword 'as'.

Could use a comment pointing to the grammar in ES6. 15.2.2 ImportSpecifier in this case.

@@ +4247,5 @@
> +
> +        if (tt != TOK_NAME) {
> +            report(ParseError, false, null(), JSMSG_NO_BINDING_NAME);
> +            return false;
> +        }

Could fold to MUST_MATCH_TOKEN.

@@ +4271,5 @@
> +template<>
> +bool
> +Parser<SyntaxParseHandler>::namedImportsOrNamespaceImport(TokenKind tt, Node importSpecSet)
> +{
> +    JS_ALWAYS_FALSE(abortIfSyntaxParser());

MOZ_ALWAYS_FALSE

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8596525 [details] [diff] [review]
update-export-declaration v2

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

This is only to update handling |export * from ...| and |export default ...|, right? Everything else was already implemented?

::: js/src/frontend/ParseNode.h
@@ +332,5 @@
>   *                          pn_right: PNK_STRING module specifier
> + * PNK_EXPORT   unary       pn_kid: declaration expression
> + * PNK_EXPORT_FROM binary   pn_left: PNK_EXPORT_SPEC_LIST export specifiers
> + *                          pn_right: PNK_STRING module specifier
> + * PNK_EXPORT_DEFAULT unary pn_kid: declaration expression

declaration expression here isn't quite right. I don't know what would be better though, without listing the spec.

::: js/src/frontend/Parser.cpp
@@ +2592,5 @@
>  }
>  
>  template <typename ParseHandler>
>  typename ParseHandler::Node
> +Parser<ParseHandler>::functionStmt(DefaultHandling defaultHandling)

Could this be fully specialized to FullParseHandler?

Comment 10

[Jon Coppeard (:jonco)]

(In reply to PTO until 05/25 from comment #9)

Thanks for the reviews!

> This is only to update handling |export * from ...| and |export default
> ...|, right? Everything else was already implemented?

Yes those are the only additions needed that I could find.  I just noticed that we currently support some syntax that is not in ES6 though so I'll write another patch for that.

> >  template <typename ParseHandler>
> >  typename ParseHandler::Node
> > +Parser<ParseHandler>::functionStmt(DefaultHandling defaultHandling)
> 
> Could this be fully specialized to FullParseHandler?

Did you mean the exportDeclaration() method here?

Comment 11

[Jon Coppeard (:jonco)]

Attachment: remove-export-assign-syntax

Patch to remove the |export name| syntax that's not present in ES6 and update the tests.

I also added tests to the syntax-error-illegal-character.js test for all the syntax added in the previous patches.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8609425 [details] [diff] [review]
remove-export-assign-syntax

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

Nice.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47804a9979fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5579f3f69a63
https://hg.mozilla.org/integration/mozilla-inbound/rev/edf9b09d1db3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47804a9979fc
https://hg.mozilla.org/mozilla-central/rev/5579f3f69a63
https://hg.mozilla.org/mozilla-central/rev/edf9b09d1db3