Suppress GetMethod(iterator.return) errors in IteratorClose

[LeszekSwirski]

IteratorClose is currently specified as

IteratorClose ( iteratorRecord, completion )

1. Assert: Type(iteratorRecord.[[Iterator]]) is Object.
2. Assert: completion is a Completion Record.
3. Let iterator be iteratorRecord.[[Iterator]].
4. Let return be ? GetMethod(iterator, "return").
5. If return is undefined, return Completion(completion).
6. Let innerResult be Call(return, iterator, « »).
7. If completion.[[Type]] is throw, return Completion(completion).
8. If innerResult.[[Type]] is throw, return Completion(innerResult).
9. If Type(innerResult.[[Value]]) is not Object, throw a TypeError exception.
10. Return Completion(completion).

If the outer completion is throw, then it is commonly understood that exceptions throw by iterator.return() should be suppressed. However, there is one exception that is not suppressed, which is iterator.return not being callable -- on 4., ? GetMethod(iterator, "return") will return the abrupt completion of GetMethod (caused by IsCallable failing), rather than the outer completion.

Therefore, errors in GetMethod(iterator, "return") and in Call(return, iterator, « ») are inconsistent, and the access and call have to be split across an exception handling boundary (try-catch or similar). I suggest that we amend this to:

3. ...
4. Let return be GetMethod(iterator, "return").
5. If return.[[Value]] is undefined, return Completion(completion).
6. If return.[[Type]] is throw and completion.[[Type]] is throw, return Completion(completion).
7. Let innerResult be Call(return.[[Value]], iterator, « »).
8. ...

This is now consistent, and can be desugared more simply to wrap iterator.return() in a try-catch (or the equivalent of such an operation in an engine's IR).

This should not be a web-compat issue, as it is already currently inconsistent across implementations: current versions of V8 and SpiderMonkey are spec compatible, but JSC, Chakra and XS are not. This can be tested with:

(function() {
  try {
    var it = {
      [Symbol.iterator]: function() {
        return {
          i: 0,
          next: function() {
            if (this.i > 0) return { done: true }
            return { value: this.i++, done: false };
          },
          return: 2
        }
      }
    }

    for (var x of it) {
      throw 3;
    }
  } catch (e) {
    if (e == 3) {
      print("Not spec compatible")
    } else {
      print("Spec compatible")
    }
  }
})();

Additionally, babel desugaring currently does not take this into account, desugaring the loop to something that does not check IsCallable:

var _iteratorNormalCompletion = true;
var _didIteratorError = false;
var _iteratorError = undefined;

try {
  for (var _iterator = it[Symbol.iterator](), _step; !(_iteratorNormalCompletion = (_step = _iterator.next()).done); _iteratorNormalCompletion = true) {
    var x = _step.value;

    throw 3;
  }
} catch (err) {
  _didIteratorError = true;
  _iteratorError = err;
} finally {
  try {
    if (!_iteratorNormalCompletion && _iterator.return) {
      _iterator.return();
    }
  } finally {
    if (_didIteratorError) {
      throw _iteratorError;
    }
  }
}

A similar spec incompatibility can be constructed for array destructuring.

[mathiasbynens]

This seems like another nice simplification to the iterator protocol (previously: #976, #988, #1021), unless there is a good reason for the currently specified behavior. @kmiller68 @bterlson