Editorial: Return an iterator *record* for for...in heads

[gibson042]

ForIn/OfBodyEvaluation expects an iterator Record of the sort returned by GetIterator (with [[Iterator]] and [[NextMethod]] slots) from ForIn/OfHeadEvaluation, but the for...in ("enumerate") branch was instead returning an Iterator object (with a next method) from EnumerateObjectProperties. This corrects the mistake by passing that object to GetIterator.

[bakkot]

I would regard this as editorial, since the spec is nonsensical as written and the intent is clear.

My preferred fix would be to make EnumerateObjectProperties be specified to return an iterator record, rather than calling GetIterator on its return value. ForIn/OfHeadEvaluation is the only (remaining) call site, so there shouldn't be any other impact.

[gibson042]

I would regard this as editorial, since the spec is nonsensical as written and the intent is clear.

This has come up many times before, but I've never really felt like there was a consistent answer. The spec as written is possible to implement, although following it results in obviously unintended behavior—is fixing that normative? CONTRIBUTING.md implies so, as did my most recent conversation on the topic (cf. #1608 (comment) ).

My preferred fix would be to make EnumerateObjectProperties be specified to return an iterator record, rather than calling GetIterator on its return value. ForIn/OfHeadEvaluation is the only (remaining) call site, so there shouldn't be any other impact.

I like the current form of this fix because it makes clear that every branch of ForIn/OfHeadEvaluation returns a result of the same shape (an iterator Record produced by GetIterator). But if your preference is still different, are you asking for the use of GetIterator to move into EnumerateObjectProperties, or for it to be eliminated? The latter appears quite challenging to me, because EnumerateObjectProperties is defined in terms of ECMAScript values rather than specification types and that seems like a good thing for ForIn/OfHeadEvaluation (whose other ["of"] branches use GetIterator directly on the ECMAScript expression result).

[bakkot]

The spec as written is possible to implement

Is it? As written, evaluating for-in involves calling ForIn/OfBodyEvaluation with its third argument (iteratorRecord) being the result of EnumerateObjectProperties, i.e. an Iterator object rather than a record. Then step 6.a. of ForIn/OfBodyEvaluation involves accessing iteratorRecord.[[NextMethod]], which is meaningless - iteratorRecord is not an iterator record, and does not have a [[NextMethod]] internal slot.

I like the current form of this fix because it makes clear that every branch of ForIn/OfHeadEvaluation returns a result of the same shape (an iterator Record produced by GetIterator).

My preference would be to just note that at the top of ForIn/OfHeadEvaluation. I intend to eventually annotate every abstract operation with its return type (and have taken steps towards making this easier in #1914).

But if your preference is still different, are you asking for the use of GetIterator to move into EnumerateObjectProperties, or for it to be eliminated? The latter appears quite challenging to me

Eliminated, though I guess you're right that this would be challenging and may not be worth it. I mostly dislike using ? GetIterator(_obj_) where _obj_ is the result of EnumerateObjectProperties because it implies that the GetIterator can be observable (e.g. can throw), which is not the case; it is purely a spec fiction.

Actually, now that I read closer, I think GetIterator(_obj_) is also not the right operation: EnumerateObjectProperties as currently specified returns an Iterator object, i.e. an object with a "next" method, whereas GetIterator expects an iterable, i.e. an object with a [Symbol.iterator] method.

[gibson042]

Hmm, interesting. That's not covered in general, but is covered in non-exotic cases by requiring the result to "behave as would the iterator given by CreateForInIterator" (which has inherits a Symbol.iterator property from %IteratorPrototype%). But it also seems to me that GetIterator is observable for that very reason, because it looks up and calls the Symbol.iterator method—and only engine262 exhibits that for for-in, which seems like a bug:

$ eshost -sx '
const getP = Object.getPrototypeOf;
const IteratorPrototype = getP(getP([][Symbol.iterator]()));
const iteratorMethod = IteratorPrototype[Symbol.iterator];
IteratorPrototype[Symbol.iterator] = function spy( ...args ) {
  print("[Symbol.iterator]", this, ...args);
  return iteratorMethod.apply(this, args);
};

for(let v of (function*(){yield "a"})()) print(v, "of Iterable");
for(let v in {a:0}) print(v, "in object");
'
#### ChakraCore, JavaScriptCore, Moddable XS, QuickJS, SpiderMonkey, V8
[Symbol.iterator] [object Generator]
a of Iterable
a in object

#### engine262
[Symbol.iterator] [object Generator]
a of Iterable
[Symbol.iterator] [object Object]
a in object

So I guess changes are needed regarding EnumerateObjectProperties, but I'd prefer to minimize them by just requiring the returned iterator to iterable by means of a Symbol.iterator property matching the original value of %IteratorPrototype%[ @@iterator ].

[bakkot]

I'd prefer to minimize them by just requiring the returned iterator to iterable by means of a Symbol.iterator property matching the original value of %IteratorPrototype%[ @@iterator ].

The smallest sensible change would be to change ForIn/OfHeadEvaluation so that it did

1. Let _obj_ be ! ToObject(_exprValue_).
1. Let _iterator_ be ? EnumerateObjectProperties(_obj_).
1. Let _nextMethod_ be ! GetV(_iterator_, "next").
1. Return the Record { [[Iterator]]: _iterator_, [[NextMethod]]: _nextMethod_, [[Done]]: false }.

This avoids introducing a new "iterable" layer of indirection. I would prefer to minimize the extent to which we perform computation on spec-fictional objects, as a rule.

[devsnek]

@gibson042 nice catch 😅

[Jack-Works]

I think this problem is equal to how we do for of on a string

[gibson042]

@bakkot I like that. Updated.

[bakkot]

I'm still going to mark this as editorial because the spec is unimplementable as written (and the intent is clear).

[ljharb]

tbh, i think i prefer the GetIterator version (with ! if it can’t actually throw), so there’s a single place where the iterator Record is created :-/

[bakkot]

Using GetIterator requires substantially more machinery and another layer of indirection. I don't think it's worth it.

[ljharb]

Not observably tho, right? In this case, the indirection to me is a good thing, because GetIterator is something that I consider widely understood.

[bakkot]

Not observably in the sense that we usually mean it, no.

GetIterator is a widely-understood operation for getting iterator records out of iterables. The problem here is that this is about for-in loops, which do not inherently have any iterables involved. We've conjured up a totally fictional iterator with EnumerateObjectProperties so that we could reuse some subsequent machinery, but operating on it as if there is a real iterable somewhere (i.e. by using GetIterator) would, I feel, be misleading to readers for no benefit. Instead of letting us simplify the machinery, it would make the spec larger and more opaque: now a reader would also need to understand in what way the return value of EnumerateObjectProperties was an iterable, in addition to understanding how the iterator it encapsulates works.

[syg]

lgtm

[michaelficarra]

I don't love it, but at least it's correct now.