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 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 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 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.

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

(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.

(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

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.

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.

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

"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

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

(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 :-)