Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Early error for arguments in initializer #56

Merged
merged 1 commit into from
Apr 28, 2017

Conversation

littledan
Copy link
Member

At the November 2016 TC39 meeting as well as previous meetings,
the committee discussed potential confusion from the value of
arguments inside a class field initializer. This PR creates an
early error for such a reference, including inside eval. I had
previously thought that an early error for a case like this would
be unusual and a runtime error would therefore be preferable, but
@adamk pointed out that such an error is analogous to errors for
new.target in the wrong location; the specification here is
analogous as well.

At the November 2016 TC39 meeting as well as previous meetings,
the committee discussed potential confusion from the value of
`arguments` inside a class field initializer. This PR creates an
early error for such a reference, including inside eval. I had
previously thought that an early error for a case like this would
be unusual and a runtime error would therefore be preferable, but
@adamk pointed out that such an error is analogous to errors for
new.target in the wrong location; the specification here is
analogous as well.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - by doing this, there's no need to worry about what arguments should mean, since documentation and/or good error messages can help.

@jeffmo jeffmo merged commit 7ed3394 into tc39:master Apr 28, 2017
@jeffmo
Copy link
Member

jeffmo commented Apr 28, 2017

u5d92lfrgpzyu

@loganfsmyth
Copy link

loganfsmyth commented Apr 28, 2017

Was looking at this to file it on Babylon, but I think this is missing something.

The definition of Contains for arrow functions only looks inside the arrow body for new.target this and super and bails for anything else, so this spec text will still pass for things like

class Foo {
  prop = () => arguments;
}

moreover fixing this is complicated since arguments is a standard binding, so

class Foo {
  prop = (arguments) => arguments;
}

should work no matter what.

@michaelficarra
Copy link
Member

@loganfsmyth The identifier "arguments" must not be in binding position in strict mode code. Class bodies are always strict mode code.

@loganfsmyth
Copy link

@michaelficarra Oh, good call! So my second example is invalid then.

I still believe the first example will be missed by this early error due to the currently-specced behavior of Contains.

@littledan
Copy link
Member Author

Well, I'm not sure if we want to change Contains, but on the other hand writing a new recursive algorithm sounds imposing. Any thoughts on the wording? Cc @anba.

@anba
Copy link

anba commented Apr 29, 2017

It is a Syntax Error if Initializer Contains an IdentifierReference whose StringValue is "arguments".

I think there are three problems with this definition:

  1. What Logan said above about arrow functions.
  2. The Contains semantic rule actually only accepts a single symbol as its argument, whereas in this case we effectively have a symbol and an additional predicate.
  3. And there's _symbol_ is an |Identifier| ? ecma262#831, which makes it hard to reason how Contains works when it is applied with an identifier production.

Well, I'm not sure if we want to change Contains, but on the other hand writing a new recursive algorithm sounds imposing.

Do you think it makes sense to copy the approach from Contains to create a new ContainsArguments static semantic rule, for example like so https://gist.github.com/anba/aac9dd50b9f08a1bfe640a56ae3becfc ? (I'm using the minimum number of required overrides for this new ContainsArguments rule, e.g. I've omitted extra definitions for arrow functions because the default implementation already "works". But we should probably add overrides for arrow and async arrow functions for clarity reasons.)

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

Successfully merging this pull request may close these issues.

6 participants