Create a jagged AggregateError during DisposeResources()

[rbuckton]

This change creates a jagged AggregateError in DisposeResources() such that the exceptions caught in the following two cases are identical:

// case 1 - two+ using decls at same block level
try {
  using c = { [Symbol.dispose]() { throw new Error("c"); };
  using b = { [Symbol.dispose]() { throw new Error("b"); };
  throw new Error("a");
} catch (e) {
  e // AggregateError {
    //   errors: [Error { message: "c" }], 
    //   cause: AggregateError {
    //     errors: [Error { message: "b" }],
    //     cause: Error { message: "a" }
    //   }
    // }
}

// case 2 - two+ using decls at varying block levels
try {
  using c = { [Symbol.dispose]() { throw new Error("c"); };
  {
    using b = { [Symbol.dispose]() { throw new Error("b"); };
    throw new Error("a");
  }
} catch (e) {
  e // AggregateError {
    //   errors: [Error { message: "c" }], 
    //   cause: AggregateError {
    //     errors: [Error { message: "b" }],
    //     cause: Error { message: "a" }
    //   }
    // }
}

Per plenary, we will continue to use AggregateError rather than introducing a new error for this case.

Fixes #74

[github-actions]

A preview of this PR can be found at https://tc39.es/proposal-explicit-resource-management/pr/104.

[rbuckton]

With this change, I am wondering if we should always create an AggregateError for the first error from Dispose if the completion was a normal completion. In other words, which shape should we chose for the following sample:

try {
  using c = { [Symbol.dispose]() { throw new Error("c"); };
  using b = { [Symbol.dispose]() { throw new Error("b"); };
} catch (e) {
  e
}

What should the shape of e be, from the following options:

# Option 1:
AggregateError {
  errors: [Error { message: "c" }], 
  cause: AggregateError {
    errors: [Error { message: "b" }]
  }
}

# Option 2:
AggregateError {
  errors: [Error { message: "c" }], 
  cause: Error { message: "b" }
}

Option (1) above will always wrap errors raised during [Symbol.dispose]() in an AggregateError, while option (2) will only wrap errors when there is a preceding cause.

I also wonder if cause is the right term here, since Error("b") did not necessarily cause Error("c"), but rather it suppresses it. Perhaps I should install a suppressed property instead?

/cc: @ljharb

[waldemarhorwat]

A couple small bugs:

• The errors parameter to DisposeResources is never supplied and never used. Delete it?
• A couple references to a nonexistent variable error. Should those be disposeError?

[waldemarhorwat]

I don't have any strong opinions on option (1) vs option (2). Both have their merits.

[rbuckton]

@ljharb: Do you have any thoughts on #104 (comment)? You've expressed interest in how the AggregateError should be constructed in prior meetings, so I would appreciate your feedback.

[rbuckton]

A couple small bugs:

• The errors parameter to DisposeResources is never supplied and never used. Delete it?
• A couple references to a nonexistent variable error. Should those be disposeError?

Thanks, these should be addressed in db4a190.

[ljharb]

I also don't have strong opinions on option 1 vs 2. I can't really recall why we need a cause at all, rather than putting them all in errors, tbh.

[rbuckton]

I also don't have strong opinions on option 1 vs 2. I can't really recall why we need a cause at all, rather than putting them all in errors, tbh.

Ideally we want to be able to easily determine the original exception from the body. Putting both in errors can make it harder to determine which was the error that was suppressed:

try {
  using c = { [Symbol.dispose]() { if(randomBool()) throw new Error("c"); };
  {
    using b = { [Symbol.dispose]() { if(randomBool()) throw new Error("b"); };
    if(randomBool()) throw new Error("a");
  }
} catch (e) {
  e
}

If we use cause, we end up with variations like this:

1) AggregateError { // a, b, and c thrown
     errors: [Error("c")],
     cause: AggregateError {
       errors: [Error("b")],
       cause: Error("a")
     }
   }

2) AggregateError { // a and c thrown
     errors: [Error("c")],
     cause: Error("a")
   }

3) AggregateError { // a and b thrown
     errors: [Error("b")],
     cause: Error("a")
   }

4) AggregateError { // b and c thrown
     errors: [Error("c")],
     cause: AggregateError {
       errors: [Error("b")],
     }
   }

5) AggregateError { // c thrown
     errors: [Error("c")],
   }

6) AggregateError { // b thrown
     errors: [Error("b")],
   }

7) Error("a") // a thrown

You can walk down the .cause of each AggregateError to get to the innermost error from the body (barring the innermost error being another AggregateError. In cases like (4), (5), and (6), the most deeply nested AggregateError doesn't have a cause.

If we just used errors, we end up with variations like this instead:

1) AggregateError { // a, b, and c thrown
     errors: [
       Error("c"),
       AggregateError {
         errors: [
           Error("b"),
           Error("a")
         ],
       }
     ]
   }

2) AggregateError { // a and c thrown
     errors: [
       Error("c"),
       Error("a")
     ]
   }

3) AggregateError { // a and b thrown
     errors: [
       Error("b"),
       Error("a")
     ]
   }

4) AggregateError { // b and c thrown
     errors: [
       Error("c"),
       AggregateError {
         errors: [Error("b")],
       }
     ]
   }

5) AggregateError { // c thrown
     errors: [Error("c")],
   }

6) AggregateError { // b thrown
     errors: [Error("b")],
   }

7) Error("a") // a thrown

Now, instead of traversing .cause, we have to traverse .errors[1], which is less meaningful: What is an errors[1], and how does it relate to my code? The cause property at least has meaning, even if that meaning would be subverted slightly to represent a suppression.

That begs the question posed in #74: Given that other operations can produce an AggregateError, and that we aren't really making use of errors efficiently in a jagged representation, would it make more sense to introduce a separate error like SuppressedError { cause, suppressed } that more accurately models this behavior? For example:

1) SuppressedError { // a, b, and c thrown
     cause: Error("c"),
     suppressed: SuppressedError {
       cause: Error("b"),
       suppressed: Error("a")
     }
   }

2) SuppressedError { // a and c thrown
     cause: Error("c"),
     suppressed: Error("a")
   }

3) SuppressedError { // a and b thrown
     cause: Error("b"),
     suppressed: Error("a")
   }

4) SuppressedError { // b and c thrown
     cause: Error("c"),
     suppressed: Error("b")
   }

5) Error("c") // c thrown

6) Error("b") // b thrown

7) Error("a") // a thrown

There's no actual aggregation since the errors are nested, not flat. The shape of the object graph doesn't change regardless
of whether the using declarations are at the same or different nest levels. The error accurately describes what is
occuring:

• An error was thrown that suppressed another error (i.e., a SuppressedError).
• The error that was thrown is the cause of the suppression, and thus marked as the cause of the SuppressedError.
• The error that was suppressed is marked as suppressed on the SuppressedError.

Its arguably not that important to know whether an exception was specifically raised from the body or from a dispose.
Instead, its more important that a developer doesn't need to drastically refactor their error handling logic if they
happen to shift a using in or out of a nested block.

If it is important to know whether the exception came from a dispose, then I'd suggest calling it a DisposeError
and having suppressed be optional (i.e., the error from dispose may or may not have suppressed an error from the body).

[ljharb]

ahhh ok, so .cause is whatever was thrown to trigger the disposal, and the errors would be the errors thrown from disposals?

That makes sense to me, as named - cause isn't a perfect name, but it's not so wrong that we need a whole new error type imo.

[rbuckton]

Superseded by #117.