tc39 / proposal-class-public-fields

Stage 2 proposal for public class fields in ECMAScript
https://tc39.github.io/proposal-class-public-fields/
488 stars 25 forks source link

Early error for arguments in initializer #56

Closed littledan closed 7 years ago

littledan commented 7 years ago

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.

jeffmo commented 7 years ago

u5d92lfrgpzyu

loganfsmyth commented 7 years ago

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 commented 7 years ago

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

loganfsmyth commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 https://github.com/tc39/ecma262/issues/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.)