Consider giving this a new type of error.

[conartist6]

Currently I understand that failures in resource cleanup will be thrown as an AggregateError, which may or may not have a cause property depending on whether or not the block which uses the resources errored.

I would like to suggest that there be a ResourceDisposalError or some new type of error, because developers will sometimes want to handle cleanup errors. It may be that I use a particular resource for which I know it is possible for disposal to fail, and I know that I don't care if it does. If so the current spec forces me to write:

try {
  await criticalActionUsingResources()
} catch(e) {
  // Did the critical action complete? Filter out disposal errors for separate handling
  if (e instanceof AggregateError) {
    // But I don't know if this is a resource cleanup error!
    // Maybe it's a failed Promise.all
  }
}

[conartist6]

This issue is to track part of an existing discussion here.

[conartist6]

I suppose the argument against this is that it would be up to the user to make all their resources' return methods in a custom DispoableError (with a cause wrapping the real issue). Then you could do e instanceof AggregateError && !e.cause && e.errors.every(e => e instanceof DispoableError).

That's a lot though, and won't work well with any library or language feature which attempts to allow Java-style catching of errors by type, something which I think will be more popular once there is a sane and supported way to rethrow certain errors.

[rbuckton]

JS doesn't have a large number of different errors in general. Most errors are just TypeError with a few others speckled in-between.

However, @waldemarhorwat did bring up during plenary that AggregateError is inconsistent in the following two cases:

// case 1
using const a = ...;
using const b = ...;

// case 2
using const a = ...;
{
  using const b = ...;
}

In case 1, if both a and b throw during disposal, you'll get an AggregateError with two entries in errors property:

AggregateError {
  cause: Error, // from the body
  errors: [
    Error, // from b.dispose
    Error, // from a.dispose
  ]
}

In case 2, you'll get an AggregateError with another AggregateError in its cause property:

AggregateError {
  cause: AggregateError {
    cause: Error, // from the body
    errors: [
      Error // from b.dispose
    ]
  },
  errors: [
    Error, // from a.dispose
  ]
}

In case 1, the single AggregateError relates to all of the resources in the same block, so they all shared the same scope, and the same is true in case 2. However, since case 2 returns a jagged error, it may be harder to reason over.

We could possibly address the "jagged error" concerns by introducing something like AggregateError.prototype.flat() to unnest a jagged AggregateError, giving users a more reasonable API to analyze and report such an error.

Another approach could be to introduce a generic SuppressedError with a cause and a suppressed property, so that all exceptions from disposal are jagged, which would make cases 1 and 2 the same:

// NOTE: two alternative API designs

// (a)
SuppressedError {
  cause: SuppressedError {
    cause: Error, // from the body
    suppressed: Error // from b.dispose
  }
  suppressed: Error // from a.dispose
}

// (b)
SuppressedError {
  cause: Error, // from the body
  suppressed: SuppressedError {
    cause: Error, // from a.dispose
    suppressed: Error // from b.dispose
  }
}
// - error from a.dispose suppresses error from b.dispose
// - error from the body suppresses already suppressed error from a.dispose

For alternative (a), such an API could also have something like a rootCause() method that walks each cause property recursively:

try {
  await criticalActionUsingResources()
} catch (e) {
  if (e instanceof SuppressedError) {
    e = e.rootCause();
  }
  e; // e is now the underlying cause of the error*
}

However, this could also become confusing. What do we throw when there's no error from the body? Do we throw a SuppressedError with no cause or do we rethrow the error from dispose without wrapping it?

Another alternative could be introduce something like an ObjectDisposedError:

ObjectDisposedError {
  cause: Error, // from the body
  suppressed: [
    Error, // from b.dispose
    Error // from a.dispose
  ]
}

In this case, ObjectDisposedError would likely need to have special semantics to unnest ObjectDisposedError instances to achieve the above representation (but not unnest ObjectDisposedError instances that have their own cause in the event of an ObjectDisposedError bubbling up from within a call to dispose).

Honestly, I'm not sure which I prefer more:

1. Just use AggregateError, maybe add a flat() and/or rootCause() method for convenience.
2. Add a new SuppressedError with a rootCause() or implicit unnesting.
3. Add a new ObjectDisposedError that mirrors AggregateError except has implicit unnesting.

[ljharb]

There is nothing wrong or confusing about an AggregateError that contains, in its errors property, an AggregateError. We should not be doing anything special to address that case.

[rbuckton]

There is nothing wrong or confusing about an AggregateError that contains, in its errors property, an AggregateError. We should not be doing anything special to address that case.

During plenary, @waldemarhorwat argued that this scenario is inconsistent:

// case 1 
using const a = ...; 
using const b = ...; 

// case 2 
using const a = ...; 
{ 
  using const b = ...;
}

I'm not sure there's a consistent way to handle this. Blindly flattening AggregateError would probably be a be a bad idea as there's a potential for data loss (i.e., extra properties added to an error instance, user-defined AggregateError instances with custom messages, stack traces, etc.).

Also, as is mentioned in the OP, there's no way to differentiate between an AggregateError from dispose or from Promise.any(), aside from the message (which isn't specified).

[ljharb]

I don't see how it's inconsistent. They're different cases.

I agree it's reasonable to provide a way to determine that an error came from disposal - but I don't think there's any need to determine which disposal it came from. Assuming a disposal tag exists, if you wrote the code nested like that, you can write it in a way to tease apart the causes.

[waldemarhorwat]

@ljharb: The problem is that this frustrates trivial code refactorings or modifications by making them change program meaning in a dangerously subtle way.

The structure of nested errors shouldn't depend on what happens to be in the same local block in a program, only on the order in which the using scopes exit. A couple ways to fix the bug are to either flatten everything or to make all exceptions jagged.

[ljharb]

Forcing flattening would frustrate nontrivial intentional scope nesting. I'm not sure what you mean by "make all exceptions jagged", though.

[bergus]

Have you considered not using AggregateError at all but simply rethrowing the last error that occurred? Like in a normal finally block? Unlike a finally block, we should however pass the completion value into the [Symbol.dispose] method, like #49 suggested, and give the disposer the responsibility to handle / rethrow / combine into an AggregateError / a SuppressedError / a CustomDisposalError as it sees fit? Sure, this will make it easy to mess up exception handling through badly written disposers, but would give them a lot of power and enable new use cases.

If that option is not on the table, I'd suggest a new variant c) of "always jagged":

// (c)
SuppressedError {
  cause: Error, // from a.dispose
  suppressed: SuppressedError {
    cause: Error, // from b.dispose
    suppressed: Error // from the body
  }
}

Imo, a disposer should generally not throw, but if it does, that error should suppress any previous error. Not the other way round. This would lead to consistent error structures regardless of refactoring (i.e. adding/removing function calls and/or blocks).
To access the error from the body, I would suggest an .earliestError() method.
The .rootCause() method in option a) doesn't sit well with me because the earliest error to occur is not necessarily the cause of the later errors.

[rbuckton]

Have you considered not using AggregateError at all but simply rethrowing the last error that occurred? Like in a normal finally block? Unlike a finally block, we should however pass the completion value into the [Symbol.dispose] method, like #49 suggested, and give the disposer the responsibility to handle / rethrow / combine into an AggregateError / a SuppressedError / a CustomDisposalError as it sees fit? Sure, this will make it easy to mess up exception handling through badly written disposers, but would give them a lot of power and enable new use cases.

That is not unlike how Python's __exit__ can receive the thrown exception. I agree it might be useful to receive the thrown exception in @@dispose, but we wouldn't want the default behavior to be to suppress the exception from the body. Suppressing by default would make it harder to reason over exceptions you expect to handle in a catch clause, and I don't imagine most @@dispose implementations will want to deal with the extra effort involved. If we did want to receive the exception, we probably would want to emulate Python's __exit__ even further and see if the return value is truthy (or the awaited return value in case of @@asyncDispose):

// example 1: Exception thrown by MyService is aggregated
class MyService {
  [Symbol.dispose](cause) {
    ...
    throw new Error("Failed to destroy resource", { cause });
  }
}
try {
  using const svc = new MyService();
  throw new Error("foo");
}
catch (e) {
  // e is AggregateError {
  //   cause: Error("foo"),
  //   errors: [
  //     Error("Failed to destroy resource") {
  //       cause: Error("foo")
  //     }
  //   ]
  // }
}

// example 2: Exception thrown by body is logged and rethrown.
class Logger {
  constructor(name) {
    this.name = name;
    console.log(`enter: ${this.name}`);
  }
  [Symbol.dispose](cause) {
    console.log(`exit: ${this.name}${ cause ? ` with error: ${cause}` : ""}`);
    return false; // return any non-truthy result (`false`, `""`, `null`, `undefined`, etc.)
  }
}
function f() {
  using const void = new Logger("f");
  throw new Error("foo");
}
try {
  f();
  // prints:
  //   enter: f
  //   exit: f with error Error: foo
}
catch (e) {
  // e is Error("foo")
}

// example 3: Exception thrown by body is possibly suppressed.
// NOTE: similar to https://docs.python.org/3/library/contextlib.html#contextlib.suppress
class Suppress {
  constructor(filter) {
    this.filter = filter;
  }
  [Symbol.dispose](cause) {
    // return `true` (or anything truthy) to suppress the original error
    return cause && this.filter(cause);
  }
}
try {
  using const void = new Suppress(e => e instanceof TypeError);
  throw new TypeError();
}
catch (e) {
  // not executed
}

There is an interesting aspect to this approach, but there are some issues that would need to be resolved:

• If a @@dispose method throws, there's no way to signal whether the exception should be aggregated or should
  suppress the original cause.
• In JS its perfectly legal to throw undefined, so its possible we need something more than just the
  value of the error to determine if we are handling an exception.
• Static languages like TypeScript would need to be able to interpret error suppression to provide accurate control-flow analysis (cc: @DanielRosenwasser):

  // CFA for type narrowing
  let x: number | undefined;
  {
    using const void = new Suppress(e => e instanceof TypeError);
    functionThatMayThrowTypeError();
    x = 1;
  }
  x; // `x: number` but should be `x: number | undefined`
  
  // tracking unused code
  {
    using const void = new Suppress(e => e instanceof TypeError);
    functionThatMayThrowTypeError();
    return; // explicit return
  }
  f(); // reports unused, but actually still reachable.

While powerful, I'm not certain I'm comfortable with the semantics of user-controlled error suppression.

Imo, a disposer should generally not throw, but if it does, that error should suppress any previous error. Not the other way round. This would lead to consistent error structures regardless of refactoring (i.e. adding/removing function calls and/or blocks). [...]

Others have argued against suppressing errors, or at least argued against making the suppressed errors unreachable. I am not certain if we can reach Stage 3 without some mechanism to address this.

[...] To access the error from the body, I would suggest an .earliestError() method. The .rootCause() method in option a) doesn't sit well with me because the earliest error to occur is not necessarily the cause of the later errors.

The intent behind .rootCause() would be to recursively walk .cause.cause.cause, etc. which should be the root cause in that case. earliestError is more ambiguous because it seems temporally related, but the only information we have is the order errors are thrown (even if it may be an error whose cause was caught at some point earlier).

// (c)
SuppressedError {
  cause: Error, // from a.dispose
  suppressed: SuppressedError {
    cause: Error, // from b.dispose
    suppressed: Error // from the body
  }
}

While less convenient, we could still use AggregateError here, so I'm not sure a new error type is strictly necessary:

AggregateError {
  cause: Error, // from a.dispose
  errors: [
    AggregateError {
      cause: Error, // from b.dispose
      errors: [
        Error // from the body
      ]
    }
  ]
}

[rbuckton]

Also, we've previously discussed error suppression in #19 and #63.

[DanielRosenwasser]

Static languages like TypeScript would need to be able to interpret error suppression to provide accurate control-flow analysis

We would probably not add any special logic here. We would either optimistically assume that every statement after a using declaration/inside a using block occurred, or pessimistically assume that it might not have. I don't know about an in-between.

In other words, Suppress wouldn't get any first-class support.

[rbuckton]

Static languages like TypeScript would need to be able to interpret error suppression to provide accurate control-flow analysis

We would probably not add any special logic here. We would either optimistically assume that every statement after a using declaration/inside a using block occurred, or pessimistically assume that it might not have. I don't know about an in-between.

In other words, Suppress wouldn't get any first-class support.

The only way I could see it working is if we had a way to unambiguously know a disposable has suppression capabilities. The easiest way to do that would be with a different global symbol (i.e, Symbol.exitContext), to differentiate between a disposable resource that doesn't have error suppression capabilities vs. one that does:

class BasicDisposable {
  [Symbol.dispose](): void {
    // cleanup, but no suppression
  }
}

class Suppress {
  #filter: (error: unknown) => boolean;
  constructor(filter: (error: unknown) => boolean) {
    this.#filter = filter;
  }

  [Symbol.exitContext](completion: "return" | "throw", cause: unknown) {
    // custom error suppression semantics, all bets are off for CFA
  }
}

{
  using const res1 = new BasicDisposable();
  // res1 has `Symbol.disposable`, CFA can be optimistic
}

{
  using const res2 = new Suppress(() => ...);
  // res2 has `Symbol.exitContext`, CFA should be pessimistic
}

For { [Symbol.dispose](): any } | null | undefined (or { [Symbol.asyncDispose](): any }) we could be optimistic, for anything else we could be pessimistic.

NOTE: Symbol.exitContext is more directly based on Python's __exit__. We could also consider adding Symbol.enterContext in parallel to Python's __enter__ to satisfy #69, if the use case is compelling enough.

[mhofman]

I'd like to not repeat the iterator close semantics which suppress closing errors, and make it impossible to report those. It can't even be reported manually (e.g. unhandled rejection or explicit console.log) because the .return has no idea if the closure is due to an error or not.

[bergus]

That is not unlike how Python's __exit__ can receive the thrown exception.

Yes. Did I not mention that? I suppose it was mentioned in #49… The only thing I don't like about Python is that it passes a type, value, traceback triple as arguments instead of a simple object :-)

I agree it might be useful to receive the thrown exception in @@dispose, but we wouldn't want the default behavior to be to suppress the exception from the body.

Oops, yes I spent no thought on that, would be a bad idea to have that as the default. Requiring explicit return true like Python or even return Symbol.suppressError; would be the way to go.

The intent behind .rootCause() would be to recursively walk .cause.cause.cause, etc. which should be the root cause in that case.

Yes, that's clear, but I disagree with the exception from the body being the "(root) cause" of the aggregated error. Notice that my variant c) has the same nesting as variant a), but with the property names cause and suppressed swapped. So to get the "body" error you'd have to recurse on .suppressed and .earliestError() was just the first name I came up with for that. The temporal connotation might not be the best, but it doesn't seem wrong either - the innermost one was thrown first, no?

Using an AggregateError instead of a new SuppressedError and recursing on .errors[0] would be fine either to me.

Imo, a disposer should generally not throw, but if it does, that error should suppress any previous error.
Not the other way round. [...]

Others have argued against suppressing errors, or at least argued against making the suppressed errors unreachable.
I am not certain if we can reach Stage 3 without some mechanism to address this.

Originally that line was only meant to argue about the inner error being .suppressed vs. being .cause of the resulting SuppressedError.
But actually, yes, I do think that an exception thrown from a disposer should take precedence over the exception thrown from the inner block:

try {
  using void = new Disposer(() => { throw new Error("dispose"); });
  throw new Error("block");
} catch(e) {
  console.log(e.message) // dispose, the "block" is lost
}

This would be consistent with how exceptions work in finally blocks, and also how it works in other languages. (And, admittedly, unlike iterator close semantics, that really should've used .throw() there imo).

there's no way to signal whether the exception should be aggregated or should suppress the original cause.

Yes. The simple approach would be to always have it suppress the previous error. A good disposer shouldn't throw exceptions, and if it does then that needs to be fixed first. A disposer that legimately throws exceptions should check whether there was an earlier error, and decide to ignore, forward, assume as .cause or .suppressed, or otherwise aggregate it. It's up to the disposer.
Do you think making the otherwise-suppressed errors available to the [Symbol.dispose]() method would make them "reachable enough" to get consensus?

Static languages like TypeScript would need to be able to interpret error suppression to provide accurate control-flow analysis

I don't think they would need to. Most code I've seen in Python that does this, does only suppress some errors (or only under specific conditions) but never all. And since TypeScript doesn't have checked exceptions (yet) anyway, it should just assume that a block using a disposer may always throw an exception or not.

// tracking unused code
{
  using const void = new Suppress(e => e instanceof TypeError);
  functionThatMayThrowTypeError();
  return; // explicit return
}
f(); // reports unused, but actually still reachable.

I don't think a disposer should be able to suppress a return completion?

[rbuckton]

I agree it might be useful to receive the thrown exception in @@dispose, but we wouldn't want the default behavior to be to suppress the exception from the body.

Oops, yes I spent no thought on that, would be a bad idea to have that as the default. Requiring explicit return true like Python or even return Symbol.suppressError; would be the way to go.

I'd still prefer that scenario just be handled by a different method (i.e., [Symbol.exitContext]), to opt in to explicit error handling. That would avoid the potential overhead of passing the completion and cause to every disposable, most of which won't need or use that information.

The intent behind .rootCause() would be to recursively walk .cause.cause.cause, etc. which should be the root cause in that case.

Yes, that's clear, but I disagree with the exception from the body being the "(root) cause" of the aggregated error. Notice that my variant c) has the same nesting as variant a), but with the property names cause and suppressed swapped. So to get the "body" error you'd have to recurse on .suppressed and .earliestError() was just the first name I came up with for that. The temporal connotation might not be the best, but it doesn't seem wrong either - the innermost one was thrown first, no?

Using an AggregateError instead of a new SuppressedError and recursing on .errors[0] would be fine either to me.

I concur with @ljharb that we should just continue to leverage AggregateError here. We could potentially improve the API for AggregateError in a seperate proposal.

Imo, a disposer should generally not throw, but if it does, that error should suppress any previous error.
Not the other way round. [...]

Others have argued against suppressing errors, or at least argued against making the suppressed errors unreachable.
I am not certain if we can reach Stage 3 without some mechanism to address this.

Originally that line was only meant to argue about the inner error being .suppressed vs. being .cause of the resulting SuppressedError. But actually, yes, I do think that an exception thrown from a disposer should take precedence over the exception thrown from the inner block:

try {
  using void = new Disposer(() => { throw new Error("dispose"); });
  throw new Error("block");
} catch(e) {
  console.log(e.message) // dispose, the "block" is lost
}

This would be consistent with how exceptions work in finally blocks, and also how it works in other languages. (And, admittedly, unlike iterator close semantics, that really should've used .throw() there imo).

As currently specified, the use of AggregateError should be sufficient, albiet wordy:

try {
  using x = { [Symbol.dispose]() { throw new Error("dispose"); } };
  throw new Error("block");
} catch (e) {
  const block = e instanceof AggregateError ? e.cause : e;
  const dispose = e instanceof AggregateError ? e.errors[0] : undefined;
  console.log(block.message); // "block"
  console.log(dispose?.message); // "dispose"
}

The wordiness here could also be potentially mitigated by pattern matching and catch guards.

there's no way to signal whether the exception should be aggregated or should suppress the original cause.

Yes. The simple approach would be to always have it suppress the previous error. A good disposer shouldn't throw exceptions, and if it does then that needs to be fixed first. A disposer that legimately throws exceptions should check whether there was an earlier error, and decide to ignore, forward, assume as .cause or .suppressed, or otherwise aggregate it. It's up to the disposer. Do you think making the otherwise-suppressed errors available to the [Symbol.dispose]() method would make them "reachable enough" to get consensus?

As mentioned, I'm not sure I'd want the overhead associated with passing the suppressed error to every call to [Symbol.dispose]().

// tracking unused code
{
  using const void = new Suppress(e => e instanceof TypeError);
  functionThatMayThrowTypeError();
  return; // explicit return
}
f(); // reports unused, but actually still reachable.

I don't think a disposer should be able to suppress a return completion?

Perhaps not, but finally can suppress a return completion, so there is precedent:

function f() {
  outer: try {
    return 1;
  }
  finally {
    break outer;
  }
  console.log("hello");
}

f(); // prints: hello

[nicolashenry]

I want to share a use case about error management. I am thinking about using it in the context of a SQL transaction in this way:

using transaction = {
  ...
  [Symbol.dispose](error) {
    if (error) {
      this.rollback();
    } else {
      this.commit();
    }
  }
 };

Or maybe in this way if the use of Symbol.exitContext is retained in the spec.

using transaction = {
  ...
  [Symbol.exitContext](completion, cause) {
    this.completion = completion;
    this.cause = cause;
  }
  [Symbol.dispose]() {
    if (this.completion === 'return') {
      this.commit();
    } else if (this.completion === 'throw') {
      this.rollback();
    }
  }
 };

(It implies Symbol.exitContext method needs to be called before Symbol.dispose method.)

I think the lack of error context could be problematic for this particular use case.

[rbuckton]

I want to share a use case about error management. I am thinking about using it in the context of a SQL transaction in this way:
[...]
(It implies Symbol.exitContext method needs to be called before Symbol.dispose method.)

I think the lack of error context could be problematic for this particular use case.

@nicolashenry I'd like to move any further discussion around error handling to #49, so I've copied your comment to that issue.

[rbuckton]

Per the September 2022 plenary discussion, we will continue to use AggregateError, however we will make the results jagged such that each AggregateError will have at most one entry in errors and a cause pointing back to any preceding error from disposal or the original completion.

[conartist6]

That looks like a reasonable solution to me.