Incorrect tests: tco-cross-realm-*

[bakkot]

Before you read this issue: this is basically the lowest possible priority, since it concerns TCO and cross-realm calls interacting in a way which I suspect literally zero humans have encountered, and also I'm not at all convinced that the behavior being tested here is intentional. I'm writing this just so I don't forget about it. That said:

• tco-cross-realm-class-construct.js
• tco-cross-realm-class-derived-construct.js
• tco-cross-realm-fun-call.js
• tco-cross-realm-fun-construct.js

all became incorrect with the merge of tc39/ecma262#2216, and didn't get updated in #2878.

These are a little tricky to explain. For background, when the spec says to "Throw a TypeError exception", the realm of the constructor of that error is supposed to be derived from the execution context at the top of the execution context stack. PrepareForTailCall is the abstract operation which is used to accomplish tail calls: its behavior is to pop the execution context stack, just before doing a call, with the idea that this represents popping the current function from the call stack so that it can be replaced by the new function. However, in a couple of places there is (or was) logic which could lead to an error being thrown between the point at which the execution context stack was popped and the point at which the new function's call pushed its own execution context. In that case, the error is (per spec) supposed to be constructed from the realm of the caller of the thing which did the tail call, that is, of the grand-caller of the thing whose call ultimately was responsible for the error.

Calling a class as a function used to be such a place. (It's not anymore, which is why these tests are wrong; we'll get to that.)

Taking tco-cross-realm-fun-call.js as our example, the point of that test is that calling a class in tail position would throw an error whose realm was derived from the code which called the function which called the class, i.e., would throw an error whose realm was derived from the code which called tco rather than (as would happen without tail calls) from the realm of tco itself. Which is to say, the error was supposed to be associated with the outer realm rather than the one from createRealm. That's what the test asserts.

However, that's no longer true as of tc39/ecma262#2216, which changed the behavior of calling a class as a function so that the resulting TypeError would always be that of the class itself, rather than being determined by the context in which it was called. (As we agreed to do in plenary.) So these tests are now wrong.

There's still a way to get at the thing this is supposed to test, though: calling a revoked proxy has exactly the same "throw a TypeError derived from the current realm" behavior (see step 2 of the [[Call]] internal method for proxies) as calling a class as a function used to. So these tests could be repaired by replacing each occurrence of return (class {})(); with var { revoke, proxy } = Proxy.revocable(() => {}, {}); revoke(); return proxy();.

I think the specified behavior is kind of absurd, here. It doesn't really make sense that there is observable behavior which happens between the tail-caller being popped from the stack and the tail-callee being pushed. So rather than fixing these tests, it would perhaps be better to fix the spec so this wasn't observable.

[jugglinmike]

Thanks for the detailed report, @bakkot! I don't want to mislead implementers by testing contested semantics, but I also don't want to indefinitely maintain contradictions to the specification. There doesn't appear to be any issue or pull request in ECMA262 about this; do you know when/how the conversation will start?

[bakkot]

The semantics are not formally contested, I just think they're dumb. I've put it on my backlog to address at some point - which, realistically, isn't going to happen for a minimum of months, probably years.

In any case, any changes would go through the normal staging (or normative PR) process, including updating tests, so changing these to match the current specification now wouldn't hurt anything, if you're inclined to do so.

[jugglinmike]

Revisiting this months later, I believe I misinterpreted what's going on here. Here's what things look like from a test maintenance perspective:

@bakkot identified four invalid tests. They offered a change to correct the tests but recommended against it because they believe the specified semantics to be erroneous.

The invalid tests should be removed immediately; see gh-3182. The decision about when to add valid tests in their place (that is: before or after correcting the presumed spec bug) can be made separately. I'll file a dedicated issue to track that consideration.

Sorry for the delay!

[bakkot]

Yes, sorry I was not clear. Also, tc39/ecma262#2495 changes the semantics I objected to, and so #3181 will resolve the question of how to add new tests (by testing for the new behavior), at which point there will be no open questions about when or whether to add additional tests (from my perspective, anyway).