[oxc_semantics] Improper return register when finally block throws

[TzviPM]

Consider the following code:

function not_foo() {
    try {
        try {
            return "foo";
        } finally {
            throw "foo";
        }
    } catch (e) {
        // do nothing
    }
}

What should the return value of this function be?

According to the ES Spec:

The try statement runtime semantics are defined at https://tc39.es/ecma262/#sec-try-statement-runtime-semantics-evaluation:

1. Evaluate the outer try statement's block into B:
   1. Evaluate the inner try statement's block into B:
      1. The SV of "foo" is "foo" as per https://tc39.es/ecma262/#sec-static-semantics-sv
      2. Return { [[Type]]: RETURN, [[Value]]: "foo", [[Target]]: EMPTY } as per https://tc39.es/ecma262/#sec-return-statement-runtime-semantics-evaluation
   2. Evaluate the inner try statement's finally block into F:
      1. The SV of "foo" is "foo" as per https://tc39.es/ecma262/#sec-static-semantics-sv
      2. Return { [[Type]]: THROW, [[Value]]: "foo", [[Target]]: EMPTY } as per https://tc39.es/ecma262/#sec-throw-statement-runtime-semantics-evaluation
   3. Since, F is not a normal completion, do not set F to B.
   4. Since F is a throw completion, its value is not EMPTY as per https://tc39.es/ecma262/#sec-updateempty, so return F
2. Since B is a throw completion, let C be the result of the catch block with e bound to "foo":
3. Since the block is empty, return EMPTY as per https://tc39.es/ecma262/#sec-block-runtime-semantics-evaluation
4. As per https://tc39.es/ecma262/#sec-implicit-normal-completion, return EMPTY is actually an editorial error and should be taken to mean NormalCompletion(EMPTY).
5. Return { [[Type]]: NORMAL, [[Value]]: EMPTY, [[Target]]: EMPTY }
6. Since C has a value of EMPTY, return { [[Type]]: NORMAL, [[Value]]: undefined, [[Target]]: EMPTY } as per https://tc39.es/ecma262/#sec-updateempty

When calling foo(), the result is technically implementation-defined (i.e. not part of the ES spec), but the standard behaviour, since foo results in a normal completion would be to return undefined as per https://tc39.es/ecma262/#sec-ecmascript-function-objects-call-thisargument-argumentslist.

The problem in our current CFG:

We store return values into a register called $return:

1. Since the outer try statement is a try-catch, we set the after_throw block to a reserved catch block.
2. In the inner try block, we set register to "foo".
3. after the try block completes, there is Normal edge going to the finally block.
4. The finally block throws, so there is an outgoing edge to the after_throw block (in this case, the catch).
5. The catch block completes normally

We see, thus, that the end result of the CFG flow leads to a value of "foo" in the $return register.

Solution space

One option is to somehow unset the return register when a finally block issues a throw. The problem with this is:

function not_foo() {
    try {
        return "foo"
    } finally {
        try {
            try {
                return "bar";
            } finally {
                throw "foo";
            }
        } catch {
            // do nothing
        }
    }
}

In this code, the throw from the innermost finally causes its try's "bar" return value to be unset, but since the catch squashes this, the outermost finally is a normal completion, and thus this entire statement should still return "foo".

Another approach would be to ditch the idea of a $return register and instead manage completion records more rigorously.

[bmeck]

I'd note the result is not implementation defined, per https://tc39.es/ecma262/#sec-try-statement-runtime-semantics-evaluation invoking UpdateEmpty(C, undefined).

[TzviPM]

I'd note the result is not implementation defined, per https://tc39.es/ecma262/#sec-try-statement-runtime-semantics-evaluation invoking UpdateEmpty(C, undefined).

By implementation defined, I meant that the actual [[Call]] function's internal block is not guaranteed by the spec to do anything specific. The result of the function is defined to be the result of F.[[Call]] applied to the normal completion. I did update my comment to fix the error with [[Value]] being set to Empty instead of undefined. I'm not aware of any implementation in which that result is not undefined, and certainly the reference algorithm in section 10 works that way.

[TzviPM]

This was based on my understanding of https://tc39.es/ecma262/#sec-object-internal-methods-and-internal-slots, although I imagine I'm reading it incorrectly.

[bmeck]

@TzviPM I think https://tc39.es/ecma262/#sec-ecmascript-function-objects-call-thisargument-argumentslist might be what you are meaning to get to at 10.2.1.10 ?

[TzviPM]

Ah, I see: https://tc39.es/ecma262/#sec-ecmascript-function-objects

An ECMAScript function object is an ordinary object and has the same internal slots and the same internal methods as other ordinary objects.

In https://tc39.es/ecma262/#sec-object-internal-methods-and-internal-slots:

An ordinary object is an object that satisfies all of the following criteria:
...
If the object has a [[Call]] internal method, it uses either the one defined in 10.2.1 or the one defined in 10.3.1.

The semantics for a function call are in https://tc39.es/ecma262/#sec-function-calls-runtime-semantics-evaluation. The EvaluateCall semantics defers to Call which in turn results in F.[[Call]](V, argumentsList). What I missed is that "F is the function object" and so the [[Call]] block is guaranteed to be the one you mentioned. Technically, it should be possible to have an exotic function object that behaves differently.

[bmeck]

@TzviPM correct, though I don't know of an engine that allows for it to do so even with exotic host objects.

[rzvxa]

I suspect it is fixed in our new CFG implementation since we don't use registers anymore instead we are creating a simple control flow pointing to appropriate AST nodes.

For this code:

function not_foo() {
    try {
        try {
            return "foo";
        } finally {
            throw "foo";
        }
    } catch (e) {
        // do nothing
    }
}

Now we have this flow: new CFG Visualization Old CFG Visualization

And for this:

function not_foo() {
    try {
        return "foo"
    } finally {
        try {
            try {
                return "bar";
            } finally {
                throw "foo";
            }
        } catch {
            // do nothing
        }
    }
}

We get this flow: new CFG Visaulization Old CFG Visualization

Both of which seems alright to me, I don't close this issue in case there is another issue I haven't noticed yet.

[rzvxa]

@TzviPM Would you also give this a look? There is no pressure since neither of these (this and #2312) is urgent.

[rzvxa]

@Boshen I think this can be close, Don't you think?