Ecma262: Stop delegating to Array.prototype[Symbol.iterator] in default class constructor

[bmeck]

per https://twitter.com/bhathos/status/1320761730474061831

delete Array.prototype[Symbol.iterator];
new class extends class {} {}

Fails since the default class constructor attempts to spread the array of the subclass' arguments to the superclass.

Any fix could affect users reading the toString of the constructor, but this seems like a sane robustness fix.

[ljharb]

I think it's important that the default constructor be trivially representable and explainable in code. What would the default constructor, in JS, look like if it didn't use Symbol.iterator?

[bmeck]

I'm open to ideas or even a proposal to allow writing a safe constructor.

super(...(function* () {let i = 0; while (i < args.length) yield args[i++];})()) doesn't fix this since it relies on the iterator prototype.

[ljharb]

I'm super on board with finding a way to make "spreading an array" robust/safe by default - in the case of the default constructor, that'd transparently fix the OP, and promote a bunch of unsafe code on the web to be robust.

[bathos]

An “apply”-style, iterator-free realization of “a super call” exists — Reflect.construct — but then you’d need to have a guaranteed reference to the original value of %Reflect.construct% (and %getPrototypeOf%). I’m curious to understand more about why it’s important for this to be realized through ES source code, though.

[bmeck]

@bathos that would require Reflect.construct to be the intrinsic and not mutated, also you can't call it on super since you cannot get a direct reference to my knowledge.

1. to make the default code JS users write more robust.
2. We have examples of robust code that struggle with problems of internal spreading such as https://github.com/nodejs/node/blob/dae283d96fd31ad0f30840a7e55ac97294f505ac/lib/internal/per_context/primordials.js#L94 which are not desiring to use mutable globals but due to various reasons (inability to safely call super with variable arguments), do.

[bathos]

Right, I realized Reflect.construct isn’t gonna help here, just “thinking out loud” as I mentally rummage through the toolbox :) And you’re right, while you can get the correct “super constructor” through other reflection methods, this is only true if you have a reference to the function, and it might be anonymous, etc, so no generic way to get that in there.

I agree that a generalizable solution for “safe spread” would be ideal.

[jridgewell]

Why not use arguments[Symbol.iterator]?

class Sub extends Super {
  // "pseudo-code" for default constructor
-  constructor(...args) {
-     super(...args);
+  constructor() {
+     super(...arguments);
  }
}

It's already there (Arguments installs an own property to the intrinsic iterator), can't be monkey-patched, and would probably make it faster.

[bmeck]

@jridgewell it looks like that delegates to the intrinsic Array.prototype.values but that in turn delegates to the current ArrayIterator.prototype.next not the intrinsic one.

[claudepache]

If Array.prototype[Symbol.iterator] is corrupted, then many code will fail, not just implicit class constructors. It is vain to try to fix just implicit class constructors.

IMHO, the right fix (if one is needed) is to make Array.prototype[Symbol.iterator] non-deletable and non-replaceable.

[bmeck]

@claudepache I'm not entirely sure, I have plenty of code not going through Array.prototype[Symbol.iterator]. I think this case is rather unique since it prevents subclassing if that global is affected. Other things like for(let i = 0; i < arr.length; i++) continue to work even if that property is mutated.

[bathos]

I’m not sure it’s vain, @claudepache — I would love for there to be a general solution, but I think that if that weren’t possible, there’s still a case to be made that implicit constructors shouldn’t be using the iterator protocol here:

1. It’s so obscure that they _would_ be
2. It seems to provide no benefit to users (it’s like an “anti-feature,” simpler spec text paid for with an avoidable runtime hazard)
3. Implementations must account for this possibility

[gibson042]

I think @jridgewell is on the right track; the internal behavior of a built-in subclass constructor should not be observable to (let alone influenced by) ECMAScript code—but specifying that is more complex because of the unfortunately observable behavior of spreading arrays and arguments objects. The best bet seems to be an Abstract Closure constructing an isolated iterator, but it would also be possible to keep the synthetic parsing approach if it is provided with lexical access to primordial values.

Use of an implicit vs. explicit constructor has a surprising impact on maximum class nesting depth, but that's not specified anyway so I think it's safe to ignore in considering changes here.

[bathos]

@bmeck looking back, I realized that the items you enumerated earlier suggest that when I said “I’m curious to understand more about why it’s important for this to be realized through ES source code,” you may have taken that as a question about the benefits of changing the behavior. What I meant was that I wasn’t clear on why it’s valuable for the algorithm to continue being defined using “ECMAScript code” specifically (in the same sense that @gibson042 just used that term), as opposed to using CreateBuiltinFunction + spec steps (or something like that).

[bmeck]

@bathos I'm unclear on the history but my presumption was that likely this choice was based upon virtualization at some point. I have no preference personally.

[gibson042]

I thought on this a little more, and it _is_ somewhat reasonable to preserve the "parsing as source text" approach without leaking any operations beyond an extra entry on the call stack.

constructor(...args) { super(...(function*(i=0){ while(i<args.length) yield args[i++]; }())); }

or more explicitly,

constructor(...args) {
  function* enumerateArgs() {
    for (let i=0; i<args.length; i++) yield args[i];
  }
  super(...enumerateArgs());
}

[ljharb]

@gibson042 unfortunately, that's vulnerable to IteratorPrototype.next being modified.

[bakkot]

Unforgeable access to Symbol.iterator would also suffice, but unfortunately I don't think we have that either.

constructor(...args) {
  let i = 0;
  let iter = { next: () => i === args.length ? { done: true } : { done: false, value: args[i++] } };
  super(...{ [Symbol.iterator]: () => iter });
}

Anyway, I don't think there's much need to make this strictly syntactic. It seems fine - arguably better - to give an explicit algorithm.

[bathos]

@ljharb I think in that particular version it’d be sensitive to:

• GeneratorFunction.prototype.prototype.next
• IteratorPrototype[@@iterator] (or: GeneratorFunction.prototype.prototype[[Prototype]])

[gibson042]

Damn, I thought I convinced myself that was secure, but it's not. Original point stands.

[bmeck]

Anyway, I don't think there's much need to make this strictly syntactic. It seems fine - arguably better - to give an explicit algorithm.

I think in general we have left only syntax as a generally unforgeable means to obtain features. I'm not seeking to allow unforgeable access to things here, but we have seen that in getOriginals and node does something similar internally for robustness.

[gibson042]

A further further thought: it would also be nice to fix the above sub-issue. Being able to break syntax with prior code evaluation is just repulsive IMO, and it doesn't matter if the syntax is extends, ..., await, etc.

[ljharb]

@gibson042 or .valueOf, or .toString, or Symbol.toPrimitive, or Symbol.hasInstance… :-(

[allenwb]

I'm unclear on the history but my presumption was that likely this choice was based upon virtualization at some point. I have no preference personally.

The dependency upon Array.prototypes[Symbol.iterator] was unintentional and should be eliminated if it can be verified to be a non-web-breaking change (it probably is ok).

[The following is based upon by recollection. I haven't checked the dates and sequencing discussed below against the primary documents. Somebody could do that, but I don't think it is actually all that important]

The behavior of subclass definitions without explicit constructors were first discussed on es-discuss and at TC39 meetings in 2011-2012. In those discussions, participants generally used ES code snippets to describe possible alternates. For example, return super(...args);. Among TC39 members that use of snippets would have generally been understood as meaning "as if by" the code of the snippet.

At the time of those discussions, Array.prototype[Symbol.iterator] had not yet been invented. Hence at that time the definition of the spread operator was not dependent upon by any dynamically accessed methods. It was a simple iteration over the integer indexed properties. That means that "as if by" (...args) had a static meaning.

When I eventually wrote the specification for ClassDefinitionEvaluation I chose to define the default constructor by saying: "Set _constructor_ to the result of parsing the source text constructor(...args) { super(...args); }". This was an act of laziness on my part. It would have been better (but more work) to specify the body of the default constructor using pseudo code. But at that time I still thought of this as an "as if by" substitution of a code sequence with a statically fixed semantics. [It was probably early 2013 when I wrote that definition. I don't recall whether Array.protptype[Symbol.iterator] had been defined yet when I took that shortcut—Even if it was I was still thinking that the code sequence had statially fixed semantics.

In fact, I do recall believing that the definition left room for implementation to aggressively optimize such default constructors. Such as by leaving the arguments on that call stack and essentially doing a tail call to the superclass constructor.

It's pretty straightforward how to correct this. ClassDefinitionEvaluation needs to defined the default constructor body using pseudocode rather than source code. The pseudo code will be a minor variation of the algorithm given in 12.3.7.1 for evaluating the production _SuperCall_ __:__ _superArguments_. Note that no actual array objects or interative copying of arguments is required because at this level arguments are represented as immutable list values and the same argument list that was passed to the constructor can be passed in the super calls.

IMO, even with this change it will still be fine to informally explain the default constructor as constructor(...args) { super(...args); }. In doing so I might say "essentially as" or in a printed document add a footnote saying something about .... But really, any body who at the level of needing to have the default constructor explained won't want to getting into the weeds of Symbol.iterator.