Add tests for the case of <iterator>.return in the iteration protocol being an object that's uncallable and compares equal to undefined

[jswalden]

This is generally very cheesy. But it happens that if you slot in $262.uncallableAndIsHTMLDDA = () => document.all in SpiderMonkey, these tests fail right now, because we did a loose-equality check against undefined rather than a strict-equality check. So, it would be nice to have upstream testing of that in case other implementations are similarly buggy.

Note that per the very peculiar requirements of $262.uncallableAndIsHTMLDDA, hosts that don't support document.all or anything stupid like it can just add

$262.uncallableAndIsHTMLDDA = function() { return {}; };

to their support files to run these tests. The [[IsHTMLDDA]] aspects could be omitted from the object returned by this function, and it wouldn't change the tests' behaviors.

https://github.com/tc39/test262/blob/master/INTERPRETING.md currently suggests the version in https://github.com/tc39/test262/blob/master/package.json will be incremented for "substantive" changes. This might or might not be. If it is, let me know what I should bump it to -- I left it alone because I'm vaguely guessing the feature-guard is enough for hosts' ease of updating here.

[bakkot]

I believe we decided in #1184 that tests involving HTML Document Dot All Exotic Objects belong in the web platform tests, rather than here.

[jswalden]

The functionality under test here is: 1) the evaluation semantics of YieldExpression: yield* AssignmentExpression in step 5c(iii), and 2) IteratorClose's step 4. It seems strange to have tests of those steps not in test262.

Looking through #1184, it seems the tests being contemplated were of how document.all works -- typeof, Boolean conversion, and equality tests. No one considered tests of algorithms or spec steps only incidentally subjected to a document.all object. It's perhaps not the most conceptually consistent way to do it, to split document.all tests across two test suites. But it seems preferable to me to putting all tests in one location, no matter the fraction of a particular test that's web-platform-related versus ECMA-262-related.

For additional context, SpiderMonkey tests have an objectEmulatingUndefined function roughly akin to the $262.uncallableAndIsHTMLDDA function described here. We have thirty-four separate files testing the behavior of objects that "emulate undefined" -- all of them targeted at specific spec steps that test whether some value "is undefined" or if ToBoolean(v) is true or false, or just how such a test is inlined or compiled by the JIT. These tests seem like a bad fit for non-language testing.

[leobalter]

There are some issues on these tests to be solved before they become good to land:

While using a host defined api is fine, some predictability from the specs is still necessary:

B.3.7 The [[IsHTMLDDA]] Internal Slot

An [[IsHTMLDDA]] internal slot may exist on implementation-defined objects. Objects with an [[IsHTMLDDA]] internal slot behave like undefined in the ToBoolean and Abstract Equality Comparison abstract operations and when used as an operand for the typeof operator.

This is what is known from the normative text in the specs. Plus the subsections:

B.3.7.1 Changes to ToBoolean
B.3.7.2 Changes to Abstract Equality Comparison
B.3.7.3 Changes to the typeof Operator

There isn't anything saying in the specs this object is callable or not.

I wonder what exactly we would be testing if we have a host defined api to return one of these objects that are non-callable.

All we know from the specs is:

1. It is an exotic object, described on the Annex B.
2. It evaluates to false on ToBoolean calls.
3. It evaluates to true on Abstract Equality Comparison (==) against null and undefined values.
4. It evaluates to "undefined" on the typeof operator.

---

Now I want to make sure we really need to differentiate a IsHTMLDDA object with and without a [[Call]] internal.

---

The first test throws a TypeError on a <iterator>.return() considering this iterator:

var iter = {
   [Symbol.iterator]() { return this; },
   next() { return {}; },
   return: $262.uncallableAndIsHTMLDDA(),
 };
var outer = (function*() { yield* iter; })();
outer.next();

Let's see the specs:

https://tc39.github.io/ecma262/#sec-generator-function-definitions-runtime-semantics-evaluation

YieldExpression : yield * AssignmentExpression

(...)
5. Let received be NormalCompletion(undefined).
6. Repeat,
  c. Else,
    i. Assert: received.[[Type]] is return.
    ii. Let return be ? GetMethod(iterator, "return").
    iii. If return is undefined, return Completion(received).

7.3.9 GetMethod ( V, P )

(...)
3. If func is either undefined or null, return undefined.
4. If IsCallable(func) is false, throw a TypeError exception.

For the second step, using a for-of loop, there is the IterationClose:

13.7.5.13 Runtime Semantics: ForIn/OfBodyEvaluation

5.k.ii.2. Return ? IteratorClose(iteratorRecord, UpdateEmpty(result, V)).

7.4.6 IteratorClose ( iteratorRecord, completion )

(...)
3. Let iterator be iteratorRecord.[[Iterator]].
4. Let return be ? GetMethod(iterator, "return").
5. If return is undefined, return Completion(completion).
(...)

---

What needs to be extracted here is whether the IsHTMLDDA object is callable or not. Because that seems the place returning the TypeError execution.

The challenge for these tests is how do we detect the specific getmethod call is failing.

This PR already shows that Test262 is missing coverage for this abrupt completion even on regular objects. on a search in the test/language/expressions/yield/ folder, there is no case to detect a TypeError when return is not null or undefined and it's not callable either, that means not only objects but also other primitive values. That's where tests needs might expand to non callable exotic objects as well, as Arrays.

It seems that tests for other IteratorClose calls might be missing as well.

So these would be cases like:

var badIter = {
  [Symbol.iterator]() { return this; },
  next() { return {}; },
  return: {}
};

var caught;
function* g() {
  try {
    yield * badIter;
  } catch (err) {
    caught = err.constructor;
  }
}
var iter = g();

iter.next();
iter.return();

assert.sameValue(caught, TypeError);

Same would go with for some calls to IteratorClose:

var badIter = {
  [Symbol.iterator]() { return this; },
  next() { return {}; },
  return: {}
};

// for-of
assert.throws(TypeError, function() {
  for (var x of badIter) break;
});

// destructuring assignment
assert.throws(TypeError, function() {
  var [x] = badIter;
});

We need to make a case for that on IsHTMLDDA, but for that we would need to assert from the specs the object is not callable.

The problem with assuming the host defined on Test262 would always return a non callable object wouldn't make much of a distinction for the coverage identified here. Which, btw, are you interested to send a PR for these too?

[leobalter]

In any case, even if I feel this is too much specific, I'm not into blocking anything, but we should make sure this goes to a proper folder where this tests will have a better identification.

Would you mind moving these specific tests to a folder named test/annexB/language/IsHTMLDDA?

[jswalden]

It's silly, but document.all is callable. Just, for the specific situations tested here, it's called with no arguments provided and a this that isn't a document -- and either of those conditions causes it to throw a TypeError when called. But really, this isn't critical. An implementation not supporting [[IsHTMLDDA]] slots (i.e. non-browsers) could use a plain {} object: that would throw the TypeError because not callable. And an implementation supporting [[IsHTMLDDA]] slots, but only on callable objects that don't throw a TypeError under iterator protocol conditions (i.e. no real-world implementation), could do so as well. This alternative, viable in all engines, tests the same spec step -- just not the precise error SpiderMonkey's implementation committed.

The rough test idea could be applied to other properties on iterators, for sure. I didn't because it's not a bug we have and because I'm already short on time for this presumed simple fix in SpiderMonkey. :-( Generally we test only errors like this when we find buggy code, or in testing during implementation.

IRC and other conversation suggests this should go in w-p-t, so I have web-platform-tests/wpt#7855 pending as well. I'd prefer a landing here because understanding these tests is wholly TC39's domain, but I'm not inclined to argue it strongly.

[anba]

But it happens that if you slot in $262.uncallableAndIsHTMLDDA = () => document.all in SpiderMonkey, these tests fail right now, because we did a loose-equality check against undefined rather than a strict-equality check.

It actually also reproduces in Chrome, so it's not just Firefox. (I didn't check other browsers.) So providing test coverage in test262 seems even more reasonable to me, because it's definitely a cross-implementation issue. :-)

[leobalter]

The tests are ES domain as they are checking for behavior on closing iterator. The object you're providing is a very specific one, but still worth of testing, as the specs predicts it.

There are minor fixes to be done on the test structure (e.g. the features flag, etc) but the only thing I want to guarantee is to store them in an exclusive folder, as I mentioned before: test/annexB/language/IsHTMLDDA. I can fix everything else in a follow up as I plan to add more tests for IsHTMLDDA using a similar approach.

This way should be easier to browser for specific IsHTMLDDA tests and prevent runs from exploding if they do not support DDA and haven't implemented any folder for this.

The rough test idea could be applied to other properties on iterators, for sure. I didn't because it's not a bug we have and because I'm already short on time for this presumed simple fix in SpiderMonkey.

That's not different for most of us, I'll have at least a new issue open so we can keep track of this missing coverage.

[rwaldron]

Note that per the very peculiar requirements of $262.uncallableAndIsHTMLDDA, hosts that don't support document.all or anything stupid like it can just add

This can be done in eshost, so no hosts actually need to do anything :)

[rwaldron]

@jswalden rwaldron/eshost@e535f86 is this sufficient?

[jswalden]

Better with the minor tweak in the comment I left on it, to be sure, but yes -- quite sufficient.

[leobalter]

@jswalden would you please confirm if you're planning to move the files to the annexB folder I mentioned above? I'm planning to merge when this is up.

[jswalden]

@leobalter Yeah, I'll move them.

[rwaldron]

@jswalden I had one question here: rwaldron/eshost@e535f86 thx!

[rwaldron]

@jswalden tc39/eshost#41