927116 - Implement import declarations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Implement parser support for import statements

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Implement reflect support for import statements

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Test reflect support for import statements

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 817425 [details] [diff] [review]
Implement parser support for import statements

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

r=me with comments.

I think js::EmitTree in BytecodeEmitter.cpp needs something like this:

      case PNK_IMPORT:
        // TODO: Add emitter support for modules
        bce->reportError(nullptr, JSMSG_SYNTAX_ERROR);
        return false;

::: js/src/frontend/Parser.cpp
@@ +3637,5 @@
>  
> +template<typename ParseHandler>
> +typename ParseHandler::Node
> +Parser<ParseHandler>::importStatement()
> +{

This should probably start with
    uint32_t begin = pos().begin;
just like Parser<ParseHandler>::ifStatement().

@@ +3640,5 @@
> +Parser<ParseHandler>::importStatement()
> +{
> +    JS_ASSERT(tokenStream.currentToken().type == TOK_IMPORT);
> +
> +    Node importSpecSet = handler.newList(PNK_IMPORT_SPEC_LIST);

I think this method, in FullParseHandler, initializes the position information to 0, which means you have to set it yourself. Add a Reflect.parse test for that?

@@ +3651,5 @@
> +            /* Handle the form "import 'a' from 'b'", by adding a single module
> +             * specifier to the list, with 'default' as the import name and
> +             * 'a' as the binding name. This is equivalent to
> +             * "import { 'default' as 'a' } from 'b'".
> +             */

Please change the JS code in here to be correct:

    Handle the form "import a from 'b';" by adding a single module

Note no string quotes around `a`.

Similarly fix the second import-declaration in this comment:

    "import {default as a} from 'b';".

(The semicolons are optional. Your call.)

(Regarding SM style: If you prefer, you can use vertical bars || or backticks `` around the bits of example JS code.)

Also, style nit: SpiderMonkey style is to use // comments for this kind of thing;
or else

  /*
   * Handle the form "import ...

I vote // here (and throughout the new code), you decide.

@@ +3659,5 @@
> +                return null();
> +
> +            Node bindingName = newName(tokenStream.currentName());
> +            if (!bindingName)
> +                return null();

Unfortunately the hard part of this remains to be written.

We have to introduce a lexical scope and bindings for the names introduced by this import declaration. We also have to throw a SyntaxError if multiple import bindings are given the same name.

(The export patch won't have these complications.)

I guess we can land it without that part though. We're always throwing SyntaxError anyway. Do you agree?

@@ +3673,5 @@
> +                 * "import { ..., } from 'a'" (where ... is non empty), by
> +                 * escaping the loop early if the next token is }.
> +                 */
> +                if (tokenStream.peekToken(TokenStream::KeywordIsName) == TOK_RC)
> +                    break;

Nice comment. peekToken() requires an error check:

    TokenKind tt2 = token.peekToken(TokenStream::KeywordIsName);
    if (tt2 == TOK_ERROR)
        return null();
    if (tt2 == TOK_RC)
        break;

@@ +3733,5 @@
> +        return null();
> +    }
> +
> +    /* Handle the form "import 'a'" by leaving the list empty. This is
> +     * equivalent to "import {} from 'a'"

Put a . at the end of the sentence please.

(If you decide to put semicolons in the comments above, might as well add them here too.)

@@ +3743,5 @@
> +
> +    if (!MatchOrInsertSemicolon(tokenStream))
> +        return null();
> +
> +    return handler.newBinary(PNK_IMPORT, importSpecSet, moduleSpec);

This is where `begin` would be used. The desired position information for this new binary node is:
   TokenPos(begin, pos().end)

@@ +3752,5 @@
> +Parser<SyntaxParseHandler>::importStatement()
> +{
> +    JS_ALWAYS_FALSE(abortIfSyntaxParser());
> +    return SyntaxParseHandler::NodeFailure;
> +}

Please file the followup bug to make lazy parsing handle imports and mention it in a comment here. (Same for exports, assuming that patch does the same thing.)

@@ +5029,5 @@
>          return letStatement();
>  #endif
>  
> +      case TOK_IMPORT:
> +        return importStatement();

ImportDeclarations are only allowed at toplevel. They're not allowed in other places where statements are allowed, like:

   function f() { import $ from "jquery"; }
   Function('import $ from "jquery"');
   if (!$) import $ from "jquery";

Needs tests.

::: js/src/frontend/Parser.h
@@ +506,5 @@
>  
>  #if JS_HAS_BLOCK_SCOPE
>      Node letStatement();
>  #endif
> +    Node importStatement();

Please make this importDeclaration(), to match the spec. Our method names don't match the spec in all cases, but we might as well get it right here.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 817427 [details] [diff] [review]
Implement reflect support for import statements

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

Clearing review for now.

::: js/src/jsast.tbl
@@ +56,5 @@
>  ASTDEF(AST_TRY_STMT,              "TryStatement",                   "tryStatement")
>  ASTDEF(AST_THROW_STMT,            "ThrowStatement",                 "throwStatement")
>  ASTDEF(AST_DEBUGGER_STMT,         "DebuggerStatement",              "debuggerStatement")
>  ASTDEF(AST_LET_STMT,              "LetStatement",                   "letStatement")
> +ASTDEF(AST_IMPORT_STMT,           "ImportStatement",                "importStatement")

ImportDeclaration again please (and throughout). 

I don't think we need an ImportSpecifierList node type; just use an Array for that.

We want Reflect.parse to align with what esprima does:

https://github.com/ariya/esprima/blob/harmony/esprima.js#L1970-1985

except it's confusing that esprima uses "id" and "name" for the two parts of an ImportSpecifier; I think it should probably be "id" and "as". I'll write to them.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 817429 [details] [diff] [review]
Test reflect support for import statements

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

This is great stuff.

Make sure the conditional keywords work however they're supposed to work, please.

Clearing review for now.

::: js/src/jit-test/tests/modules/import-statements.js
@@ +5,5 @@
> +
> +function program(elts) Pattern({
> +    type: "Program",
> +    body: elts
> +})

A better way to write this now is to use an ES6 arrow function:

var program = elts => Pattern({
    type: "Program",
    body: elts
});

Otherwise we'll just have to go back and fix this when (nonstandard) expression-closures are removed...

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Implement parser support for import declarations

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Implement reflect support for import declarations

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Test reflect support for import declarations

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 818693 [details] [diff] [review]
Implement parser support for import declarations

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

::: js/src/frontend/Parser.cpp
@@ +3751,5 @@
> +
> +    if (!MatchOrInsertSemicolon(tokenStream))
> +        return null();
> +
> +    return handler.newImportStatement(begin, importSpecSet, moduleSpec);

Please do it like this:

    return handler.newImportStatement(importSpecSet, moduleSpec, TokenPos(begin, pos().end));

This is what we do with newDoWhileStatement, newContinueStatement, newReturnStatement, and so on---all the forms that have semicolons.

Comment 11

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13f60271f4f6

Comment 12

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17b2babbe34e

Comment 13

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfeee13c85fb

Comment 14

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

Backed out all three in https://hg.mozilla.org/integration/mozilla-inbound/rev/f37ed8d81612 for apparently breaking a Windows Debug XPCShell test like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=30225773&tree=Mozilla-Inbound#error0

Comment 15

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

(In reply to Wes Kocher (:KWierso) from comment #14)
> Backed out all three in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f37ed8d81612 for
> apparently breaking a Windows Debug XPCShell test like this:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30225773&tree=Mozilla-
> Inbound#error0

This is bug 845190, fwiw.

Comment 16

[Eddy Bruel [:ejpbruel]]

(In reply to Wes Kocher (:KWierso) from comment #15)
> (In reply to Wes Kocher (:KWierso) from comment #14)
> > Backed out all three in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/f37ed8d81612 for
> > apparently breaking a Windows Debug XPCShell test like this:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=30225773&tree=Mozilla-
> > Inbound#error0
> 
> This is bug 845190, fwiw.

I had a green try run for these patches before I pushed to inbound. What gives?

https://tbpl.mozilla.org/?tree=Try&rev=c4b720fef1f1

Comment 17

[Eddy Bruel [:ejpbruel]]

If I'm reading this right, you're asserting that a patch that does nothing except add a new JavaScript test file causes a shell crash in a completely unrelated test? I consider that extremely unlikely, tbh.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

The history of bug 845190 is very sordid and littered with past examples of random JS changes tipping it into bustage territory. As you can see, it was reopened yesterday for one instance, so my guess is that *something* made that test more failure-prone recently and this push then made it worse.

But yeah, that whole test situation sucks. I'd give it a couple days and if there isn't any progress, just disable the test again and get on with life.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

BTW, please push these together in one push next time :)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

"Good" news, this was failing even after your backout. Re-landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/73b5c79e3b65
https://hg.mozilla.org/integration/mozilla-inbound/rev/4604a8a2b672
https://hg.mozilla.org/integration/mozilla-inbound/rev/97a7ac9edd15

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/73b5c79e3b65
https://hg.mozilla.org/mozilla-central/rev/97a7ac9edd15
https://hg.mozilla.org/mozilla-central/rev/4604a8a2b672

Comment 22

[Eddy Bruel [:ejpbruel]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #20)
> "Good" news, this was failing even after your backout. Re-landed.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/73b5c79e3b65
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4604a8a2b672
> https://hg.mozilla.org/integration/mozilla-inbound/rev/97a7ac9edd15

Thank you! You just made my day :-)