Fix generator prototype chain to more correctly match ES2015 spec

[aickin]

Thanks so much for your work on such a cool project!

I found what I think is a spec-compliance bug with the generator prototype chain. According to section 25.3.1 (emphasis mine):

The Generator prototype object is the %GeneratorPrototype% intrinsic. It is also the initial value of the prototype property of the %Generator% intrinsic (the GeneratorFunction.prototype).

<snip>

The value of the [[Prototype]] internal slot of the Generator prototype object is the intrinsic object %IteratorPrototype% (25.1.2). The initial value of the [[Extensible]] internal slot of the Generator prototype object is true.

This means that %GeneratorPrototype% should not have a Symbol.iterator property, but its prototype should. In addition, %GeneratorPrototype%'s prototype should be the same as the prototype of %ArrayIteratorPrototype%, %MapIteratorPrototype%, %SetIteratorPrototype%, and %StringIteratorPrototype%.

In the current code base, though, %GeneratorPrototype% (called Gp in the code) is given a Symbol.iterator property and has its prototype set to Object.prototype. As I found out in a PR to core-js, some older browsers that support iterators evidently don't have %IteratorPrototype%, so this behavior is reasonable in those browsers, but I think it should probably be correct in browsers that do have %IteratorPrototype%.

This PR changes the unit test that checks the prototype chain for a Symbol.iterator property. The current test implementation is not spec-compliant and fails in both currently shipping Chrome and Firefox.

In this patch, if %IteratorPrototype% can't be found, it falls back to the existing behavior.

Again, thanks so much for this project, and please let me know what I can do to improve this PR!

[aickin]

I see that CI failed for node 0.12 and 0.10, where %IteratorPrototype% isn't defined, which makes sense, since the test now assumes it will always run in environments where it does exist. I'll fix!

[aickin]

I reworked the PR to be tolerant of environments where %IteratorPrototype% is not available as well as environments where you cannot reset prototypes. In the former case, it polyfills %IteratorPrototype%, and in the latter case it falls back to putting the @@iterator property on %GeneratorPrototype%, as it does currently in master. Hopefully the comments in the code make this clear; let me know if they don't!

Looks like it passes CI on all the platforms now; please let me know what you think, and thanks again!

[benjamn]

Thanks for reporting (and implementing) this!

Since this is a runtime library, brevity and minimizing assumptions are of the essence. To that end, I think we can do without the getIteratorPrototype method. Here's my attempt at simplifying the logic to find IteratorPrototype:

var IteratorPrototype = {};
var getProto = Object.getPrototypeOf;
if (getProto) {
  IteratorPrototype = getProto(getProto(values([])));
  if (getProto(IteratorPrototype) !== Object.prototype) {
    IteratorPrototype = {};
  }
}

Note that values is defined later in runtime.js.

Can you think of any problems with that implementation?

If that code works, then I think we can just do

var Gp = GeneratorFunctionPrototype.prototype =
  Generator.prototype = Object.create(IteratorPrototype);

to avoid needing to call Object.setPrototypeOf.

[aickin]

Thanks for the feedback!

Clever idea to avoid Object.setPrototypeOf; I like it a lot.

I think this will basically work (I'll test it), but I'd prefer to be a little more careful checking the results of getProto. The return value of values usually has a prototype chain of length 2, but occasionally it returns just a basic object, which has a prototype chain of just length 1 (Object.prototype). When getProto is called 3 times on a basic object, you get a TypeError. Now, it turns out that (by my reading at least) values only returns an object with a prototype chain of length 1 only when its input doesn't have a numeric .length property, so I think we won't run into that case here. But I'd still like to put in a few more checks in case the implementation of values changes.

We'd also need to make sure that IteratorPrototype has the iterator method. I think that'd make the code look something like:

// this is a polyfill for %IteratorPrototype% for environments that don't 
// natively support it.
var IteratorPrototype = {};
IteratorPrototype[iteratorSymbol] = function() {return this;};
var getProto = Object.getPrototypeOf;
if (getProto) {
  var NativeIteratorPrototype = getProto(getProto(values([])));
  if (NativeIteratorPrototype && 
    NativeIteratorPrototype !== Object.prototype &&
    NativeIteratorPrototype.hasOwnProperty(iteratorSymbol)) {
    // this environment has a native %IteratorPrototype%; use it instead of 
    // a polyfill.
    IteratorPrototype = NativeIteratorPrototype;
  }
}

I've tried this implementation, and it passes tests in node v6, v0.12, and v0.10. It's a little more wordy than your version, but I think it's a hair more defensive. I'll push it now because I have it, but please of course feel free to tell me that you want something different. Thanks!

[aickin]

Thanks so much for merging this! 🎉