Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Module edge cases that are missed #326

Closed
nzakas opened this issue Oct 8, 2015 · 13 comments
Closed

Module edge cases that are missed #326

nzakas opened this issue Oct 8, 2015 · 13 comments

Comments

@nzakas
Copy link
Contributor

nzakas commented Oct 8, 2015

There are a couple of cases I've come across where the parser should throw errors but does not.

The first:

export var await;

In ES6, await is a future reserved word when in a module (reference) and should throw an error.

The second:

export *

This should throw an error because exporting * requires from and a module name, such as:

export * from "foo"
@marijnh
Copy link
Member

marijnh commented Oct 8, 2015

We're still turning off reserved word checking by default (see the allowReserved option). If you enable that, the first case does raise an error. I guess it would make sense to change the default for ECMAScript 6 and up to enable this. (It used to be widespread practice for browsers to accept reserved words.)

I can't get Acorn to accept export *. When I feed it that it yields "Uncaught SyntaxError: Unexpected token (1:8)".

@marijnh
Copy link
Member

marijnh commented Oct 8, 2015

Attached patch fixes the allowReserved default. Please let me know whether the export * thing is something that you can still reproduce.

@nzakas
Copy link
Contributor Author

nzakas commented Oct 8, 2015

Ah, looks like I wasn't using the latest version. Sorry about that.

After upgrading, I found two more issues:

export {default}

This should be a syntax error (it needs from).

@nzakas nzakas mentioned this issue Oct 8, 2015
29 tasks
@marijnh
Copy link
Member

marijnh commented Oct 9, 2015

I'm pretty sure the standard allows this (and that it means to export from the local module). See http://www.ecma-international.org/ecma-262/6.0/#sec-exports

@marijnh
Copy link
Member

marijnh commented Oct 9, 2015

(Also, you mention 'two more issues' and only list one. Did you forget to add the second?)

@nzakas
Copy link
Contributor Author

nzakas commented Oct 9, 2015

Oops, copy-paste error, I meant one more issue.

It's possible I'm misunderstanding the spec, but the Espree module implementation was written by @caridy, so I'd defer to his knowledge in this area.

@rwaldron
Copy link

rwaldron commented Oct 9, 2015

The names in export { ... } are IdentifierName:

ExportClause :
  { }
  { ExportsList }
  { ExportsList , }
ExportsList :
  ExportSpecifier
  ExportsList , ExportSpecifier
ExportSpecifier :
  IdentifierName
  IdentifierName as IdentifierName

There is a special syntax definition that prohibit the use of FutureReservedWord.

@nzakas
Copy link
Contributor Author

nzakas commented Oct 9, 2015

@rwaldron So export {default} should not be allowed because default is a keyword?

@caridy
Copy link

caridy commented Oct 9, 2015

@nzakas this is tricky because the spec doesn't provide all the details, it is just a combination of rules, but in principle, export {default} is invalid. only export {default} from "something" should be allowed. you can't have a binding for default identifier in the module body, that's why!

@TimothyGu
Copy link
Contributor

To expand upon @caridy's comment and to correct a minor mistake in @rwaldron's comment: export { default } in invalid, not because default is a FutureReservedWord, but rather that it is a Keyword, and thus a ReservedWord, which is not allowed because of the Early Error cited by @rwaldron earlier:

  • It is a Syntax Error if StringValue of n is a _ReservedWord_ or if the StringValue of n is one of: …

Note that the last Early Error link only applies to the following construct:

export ExportClause ;

not

export ExportClause FromClause ;

So

export { default } from "algo"

is supported.

@rwaldron
Copy link

rwaldron commented Oct 9, 2015

not because default is a FutureReservedWord, but rather that it is a Keyword, and thus a ReservedWord, which is not allowed because of the Early Error cited

Yep. Incidentally, I had meant to type (or thought I did) "ReservedWord and strict mode FutureReservedWord"... I must've deleted that part when I was making it a link. Thanks for spotting that and providing a correction.

@nzakas
Copy link
Contributor Author

nzakas commented Oct 9, 2015

Thanks everyone. @marijnh I'll put together a PR.

@nzakas
Copy link
Contributor Author

nzakas commented Oct 28, 2015

It looks like export var await is still being parsed with the latest release because await isn't listed as a keyword or reserved word internally. I'll try to fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants