Editorial: Initialize [[GeneratorState]] and [[AsyncGeneratorState]]

[jhnaldo]

In the GeneratorStart AO, the first step is the following assertion:

GeneratorStart (
  _generator_: a Generator,
  _generatorBody_: a |FunctionBody| Parse Node or an Abstract Closure with no parameters,
): ~unused~
...
1. Assert: The value of _generator_.[[GeneratorState]] is *undefined*.

However, in the EvaluateGeneratorBody SDO for GeneratorBody, there is no initialization of the [[GeneratorState]] internal slot for the variable _G_ before passing it into the GeneratorStart AO:

Runtime Semantics: EvaluateGeneratorBody (
  _functionObject_: an ECMAScript function object,
  _argumentsList_: a List of ECMAScript language values,
): a throw completion or a return completion
...
2. Let _G_ be ? OrdinaryCreateFromConstructor(_functionObject_, *"%GeneratorFunction.prototype.prototype%"*, « [[GeneratorState]], [[GeneratorContext]], [[GeneratorBrand]] »).
3. Set _G_.[[GeneratorBrand]] to ~empty~.
4. Perform GeneratorStart(_G_, |FunctionBody|).
...

I think we need to initialize the [[GeneratorState]] with *undefined*, similar to the following 5th step in the CreateIteratorFromClosure AO:

CreateIteratorFromClosure (
  _closure_: an Abstract Closure with no parameters,
  _generatorBrand_: a String or ~empty~,
  _generatorPrototype_: an Object,
): a Generator
...
3. Let _generator_ be OrdinaryObjectCreate(_generatorPrototype_, _internalSlotsList_).
4. Set _generator_.[[GeneratorBrand]] to _generatorBrand_.
5. Set _generator_.[[GeneratorState]] to *undefined*.
...
13. Perform GeneratorStart(_generator_, _closure_).
...

I found a similar issue in the EvaluateAsyncGeneratorBody SDO for the AsyncGeneratorBody.

[michaelficarra]

Confirmed the issue. An assertion is immediately violated in GeneratorStart and AsyncGeneratorStart. But instead of this fix, I think I would prefer to introduce a new enum value (~not-started~?) and use that.

[bakkot]

There's no need for a state prior to ~suspended-start~. Generators immediately enter that state when created, before evaluation of any user code. I would prefer to just drop the assert, or failing that to set it to ~suspended-start~ prior to calling GeneratorStart.

[michaelficarra]

@bakkot Setting it to ~suspended-start~ also WFM.

[jhnaldo]

Thanks for the review!

@michaelficarra You mean using the enum value ~not-started~ instead of *undefined* to state Generator (or AsyncGenerator) has not yet started? If so, I agree that it could provide a more precise explanation.

@bakkot I think it would be more precise to fill out all fields before calling other AOs requiring Generator/AsyncGenerator as parameters. However, just dropping the assertion is also acceptable to me.

After the editor's call, I will update this PR.

[michaelficarra]

@jhnaldo Unfortunately we didn't get to it this week. We'll try to get to it next week.

[syg]

I would prefer just dropping the assert as well.

I think it would be more precise to fill out all fields before calling other AOs requiring Generator/AsyncGenerator as parameters. However, just dropping the assertion is also acceptable to me.

GeneratorStart is, for all intents and purposes, the initialization code for newly created generators.

Now, I think it's reasonable to say that it's perhaps too far away from the allocation site of generator objects. For that issue, I'd prefer to do something systematic instead of ad hoc for generators. For example, we can change OrdinaryObjectCreate to take a list of (internal slot, initial value) pairs instead of a list of internal slots.

[michaelficarra]

I think it's reasonable to say that it's perhaps too far away from the allocation site of generator objects

Yeah I think this is the case. Its initialisation should be "adjacent" to the object allocation.

[jmdyck]

I think the assertion is technically correct, because 6.1.7.2 "Object Internal Methods and Internal Slots" says that "Unless specified otherwise, the initial value of an internal slot is the value undefined."

However, I dislike that sentence, and would much rather see slots initialized to a reasonable value at creation time.

For example, we can change OrdinaryObjectCreate to take a list of (internal slot, initial value) pairs instead of a list of internal slots.

In my https://github.com/jmdyck/ecma262/tree/object_creation branch, I had OrdinaryObjectCreate (via MakeBasicObject) take a Record, where each field associated a slot's name with its initial value. (The branch is way out of date, but if you compare it to its parent, you'll get the idea.) (Mind you, it also introduces an ecmarkdown syntax for long record displays, one field per line, which isn't essential to substance of the changes, but kept me happy.)

[jhnaldo]

Oh, I missed that sentence. Thank you for correcting me. I totally agree with your opinion because initializing each internal slot with a proper value instead of undefined is helpful for the specification type analysis. I will see your branch later. For this PR, do you think I should close it?

[michaelficarra]

@jhnaldo No please don't close the PR. I don't think any of us are happy with the type of every slot being implicitly unioned with undefined. We'll have to figure out what editorial action we want to take.

[michaelficarra]

We decided to initialise to ~suspended-start~ and clean up the implicit *undefined* initialisation in a follow-up.