Improve async dispose fallback

[JLHwung]

Q	A
Fixed Issues?	See below
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This PR aligns our implementation to spec 7.5.6 GetDisposeMethod

1. If hint is async-dispose, then
  a. Let method be ? GetMethod(V, @@asyncDispose).
  b. If method is undefined, then
    i. Set method to ? GetMethod(V, @@dispose).
    ii. If method is not undefined, then
      1. Let closure be a new Abstract Closure with no parameters that captures method and performs the following steps when called:
        a. Let O be the this value.
        b. Perform ? Call(method, O).
        c. Return undefined.
      2. NOTE: This function is not observable to user code. It is used to ensure that a Promise returned from a synchronous @@dispose method will not be awaited.
      3. Return CreateBuiltinFunction(closure, 0, "", « »).
2. Else,
  a. Let method be ? GetMethod(V, @@dispose).
3. Return method.

There are two behaviour changes:

1. Only fallback to the [Symbol.dispose] method if the [Symbol.asyncDispose] method is undefined, previously we fallback when it is null
2. Do not await the promise returned from a sync [Symbol.dispose] method.

In this PR we also improve the error message when [Symbol.asyncDispose] method is not a function, previously we throw [Symbol.dispose] is not a function, which could be very confusing.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56800

[JLHwung]

In current main, the y dispose promise body is printed before x asyncDipose body (REPL)

[liuxingbaoyu]

Maybe just throw Invalid dispose function.
Because "Property [Symbol.dispose] is not a function." it is also possible for the user to add a [Symbol.asyncDispose].

[JLHwung]

Okay, how about "Object is not disposable.", since "dispose" is a verb.

[JLHwung]

If Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose") is undefined, which implies that the asyncDispose symbol is not properly polyfilled, we can provide a better error such as "Symbol.asyncDispose is not defined" rather than throwing "object is not disposable" as we are reading the "undefined" property of the value.

[liuxingbaoyu]

I prefer to unify them into something like disposal function is invalid. :)

[JLHwung]

The disposal function may be okay. The proposed error here is for the case when Symbol.asyncDispose is not defined, rather than resource[Symbol.asyncDispose] is not defined.

[liuxingbaoyu]

You are right! I changed it to invalid.

[JLHwung]

I just realized that Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose") will always return a symbol. If Symbol.dispose is not polyfilled, user will end up defining a disposal function under the name undefined without any runtime error. So if we can't find the disposal function and value.undefined is a function, this suggests that the user may not polyfill Symbol.dispose and in this case we can provide a better error message. Anyway it will be addressed in another PR.

I think we can add a section to the plugin docs about how to use this plugin with core-js 3 or other polyfill providers. Integration tests with polyfill providers should be added as well.

[liuxingbaoyu]

Symbol.asyncDispose || Symbol.for("Symbol.asyncDispose") always returns a symbol, so shouldn't we always look for value[symbol]?

[JLHwung]

In chrome 122 or any browsers without @@dispose support,

const disposable = { [Symbol.dispose]() {} }

will be evaluated as if it were defined as

const disposable = { undefined() {} }

If users have properly polyfilled Symbol.dispose, the transformed result will be evaluated as if

const disposable = { [Symbol.for("Symbol.dispose")]() {} }

The usingCtx helper work when 1) the browser has native support of @@dispose or 2) the @@dispose has been polyfilled correctly. However, in the case when a polyfill is missing, the helper will throw object is not disposable because the desired disposal function is defined at the undefined property, which can be very confusing for end users because they thought they have defined it, without realizing that the symbol is not polyfilled. In this case we should suggest users that @@dispose may not be properly polyfilled.

[liuxingbaoyu]

Makes sense! But given that this exists in all symbol-related helpers, I'm a little hesitant that we should include it by default.

[liuxingbaoyu]

Not related to this PR:
This behavior seems interesting.

return (async function () {
  const log = [];

  async function f() {
    using y = {
      [Symbol.dispose || Symbol.for("Symbol.dispose")]() {
        log.push("y");
      },
    };
	// Commenting me will make the test fail
    await using x = {
      [Symbol.dispose || Symbol.for("Symbol.dispose")]() {},
    };
    log.push("f body");
  }

  const promise = f();

  log.push("body");

  await promise;

  expect(log).toEqual(["f body", "body", "y"]);
})();

[JLHwung]

Yes and we can definitely add more behaviour tests.

As for your example, any sync disposable declared before an async disposable will be disposed after the async disposable is disposed, which happens no earlier than the next microtick, so y is after body, even if it is in a sync dispose method. If there is no async disposable, the sync disposables will be disposed immediately, and the order becomes f body, y and body.

[JLHwung]

I renamed the enums so that hopefully two-year-later me will not get lost when reviewing the helper.

[JLHwung]

Rebased. Several update-fixtures commit were removed before I generate a final update-fixtures commit based on current main.

[nicolo-ribaudo]

Let's either wait until tc39/proposal-explicit-resource-management#219 is merged, or implement it assuming that it will be accepted.