Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unconditionally passing _lastvalue_ to `next` is observable #63

Open
bakkot opened this issue Nov 7, 2019 · 11 comments
Milestone

Comments

@bakkot
Copy link
Contributor

@bakkot bakkot commented Nov 7, 2019

In, for example, Iterator.prototype.map, the spec currently unconditionally passes _lastValue_ to IteratorStep. That's currently meaningless, because IteratorStep only takes a single parameter. I assume the intent was to pass that value along to the underlying iterator by providing it as an argument to IteratorNext, which does take a second (optional) argument

But unconditionally passing that argument is kind of a strange choice. It's observable:

let iterator = {
  __proto__: Iterator.prototype,
  next: function(...args) {
    console.log(args.length);
    return { done: true };
  },
};

and that means that iterating over iterator vs iterating over iterator.map(x => x) is observably different, which seems like an invariant which ought not be violated (at least in the absence of more powerful reflection mechanisms like function.caller or stack traces).

I'm not sure if there is a way to reconcile this while still specifying these methods as generators, because generators (as far as I know) cannot observe a difference between their .next being called with undefined vs being called with no arguments. I suppose we could make that ability available to built-in generators somehow.

@devsnek devsnek added this to the Stage 3 milestone Nov 7, 2019
@devsnek

This comment has been minimized.

Copy link
Member

@devsnek devsnek commented Nov 7, 2019

This is very interesting. I definitely wouldn't want to provide native generators with some sort of metadata on the result of their yields, that feels like the wrong level of abstraction to me.

I'm tempted to say that since generators normalize next() to undefined, it makes sense for next(undefined) to be a thing, and the observable difference doesn't matter.

I'm curious what others think though.

@devsnek

This comment has been minimized.

Copy link
Member

@devsnek devsnek commented Nov 8, 2019

We might also be able to modify GeneratorNext to make value optional, and then if the generator is builtin, it can use NormalCompletion(~empty~) instead of NormalCompletion(undefined).

Possible diff
index c0e022d..b2d81a0 100644
--- a/spec.html
+++ b/spec.html
@@ -38746,7 +38746,8 @@ THH:mm:ss.sss
         <p>The `next` method performs the following steps:</p>
         <emu-alg>
           1. Let _g_ be the *this* value.
-          1. Return ? GeneratorResume(_g_, _value_).
+          1. If _value_ is present, return ? GeneratorResume(_g_, _value_).
+          1. Return ? GeneratorResume(_g_).
         </emu-alg>
       </emu-clause>
 
@@ -38853,7 +38854,7 @@ THH:mm:ss.sss
       </emu-clause>
 
       <emu-clause id="sec-generatorresume" aoid="GeneratorResume">
-        <h1>GeneratorResume ( _generator_, _value_ )</h1>
+        <h1>GeneratorResume ( _generator_ [, _value_ ] )</h1>
         <p>The abstract operation GeneratorResume with arguments _generator_ and _value_ performs the following steps:</p>
         <emu-alg>
           1. Let _state_ be ? GeneratorValidate(_generator_).
@@ -38864,7 +38865,12 @@ THH:mm:ss.sss
           1. Suspend _methodContext_.
           1. Set _generator_.[[GeneratorState]] to ~executing~.
           1. Push _genContext_ onto the execution context stack; _genContext_ is now the running execution context.
-          1. Resume the suspended evaluation of _genContext_ using NormalCompletion(_value_) as the result of the operation that suspended it. Let _result_ be the value returned by the resumed computation.
+          1. If _value_ is not present, then
+            1. If _generator_ is a built-in generator, let _resumptionValue_ be NormalCompletion(~empty~).
+            1. Otherwise, let _resumptionValue_ be NormalCompletion(*undefined*).
+          1. Else,
+            1. Let _resumptionValue_ be NormalCompletion(_value_0.
+          1. Resume the suspended evaluation of _genContext_ using _resumptionValue_ as the result of the operation that suspended it. Let _result_ be the value returned by the resumed computation.
           1. Assert: When we return here, _genContext_ has already been removed from the execution context stack and _methodContext_ is the currently running execution context.
           1. Return Completion(_result_).
         </emu-alg>
@@ -38955,8 +38961,9 @@ THH:mm:ss.sss
         <h1>AsyncGenerator.prototype.next ( _value_ )</h1>
         <emu-alg>
           1. Let _generator_ be the *this* value.
-          1. Let _completion_ be NormalCompletion(_value_).
-          1. Return ! AsyncGeneratorEnqueue(_generator_, _completion_).
+          1. If _value_ is present, return ! AsyncGeneratorEnqueue(_generator_, NormalCompletion(_value_)).
+          1. If _generator_ is a built-in generator, return ! AsyncGeneratorEnqueue(NormalCompletion(~empty~)).
+          1. Return ! AsyncGeneratorEnqueue(_generator_, NormalCompletion(*undefined*)).
         </emu-alg>
       </emu-clause>
@Jamesernator

This comment has been minimized.

Copy link

@Jamesernator Jamesernator commented Nov 12, 2019

For the record different calls already exist, for example looping over a sync iterator with a for-of loop doesn't pass value but for-await-of (via AsyncFromSyncIterator) does pass undefined.

Example:

class SyncIterator {
  i = 0;

  next(...args) {
    console.log(args);
    this.i += 1;
    return { done: this.i === 3, value: this.i };
  }

  [Symbol.iterator]() {
    return this;
  }
}

async function main() {
  for (const i of new SyncIterator()) {

  }

  for await (const i of new SyncIterator()) {

  }
}

main();

(This was the issue on it: tc39/proposal-async-iteration#113)

@devsnek

This comment has been minimized.

Copy link
Member

@devsnek devsnek commented Nov 12, 2019

yay for previous consensus! I guess this can be closed then?

@ljharb

This comment has been minimized.

Copy link
Member

@ljharb ljharb commented Nov 12, 2019

I don’t recall explicit consensus on that item, and i think this should still be handled here. “not meant to cater to argument length checking functions” seems like a stance we shouldn’t be taking.

@zloirock

This comment was marked as off-topic.

Copy link
Contributor

@zloirock zloirock commented Nov 12, 2019

Related #43 (comment)

@devsnek

This comment has been minimized.

Copy link
Member

@devsnek devsnek commented Nov 12, 2019

@ljharb We've basically got prior art with usage in the wild handed to us on a golden platter here. Have you ever seen anyone encounter this problem? I haven't.

@zloirock the point you brought up there is unrelated to this

@ljharb

This comment has been minimized.

Copy link
Member

@ljharb ljharb commented Nov 12, 2019

@devsnek no, but that doesn’t mean it wouldn’t become one. new syntax takes many years before it’s used commonly in the wild anyways.

@zloirock

This comment was marked as off-topic.

Copy link
Contributor

@zloirock zloirock commented Nov 12, 2019

@devsnek how .next arguments should be passed to the parent iterators? Directly related.

@devsnek

This comment was marked as off-topic.

Copy link
Member

@devsnek devsnek commented Nov 12, 2019

@zloirock they are only related in that they both have to do with calls to next(), not anything more. feel free to open a separate issue.

@zloirock

This comment was marked as off-topic.

Copy link
Contributor

@zloirock zloirock commented Nov 12, 2019

@devsnek they both are subsets of one big issue. The separate issue about iterators vs generators semantic already was opened, I don't know why you don't want to reopen it. Even if you wanna solve it with generators - generators way have problems which should be solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.