Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds SuppressedError #117

Merged
merged 1 commit into from
Nov 24, 2022
Merged

Adds SuppressedError #117

merged 1 commit into from
Nov 24, 2022

Conversation

rbuckton
Copy link
Collaborator

@rbuckton rbuckton commented Nov 18, 2022

Adds a new SuppressedError built-in Error subclass to represent an error that suppresses another error as a result of disposal.

A SuppressedError is defined as:

class SuppressedError extends Error {
  constructor(error, suppressed, message, options);
  name; // "SuppressedError"
  error; // The error that resulted in a suppression.
  suppressed; // The error that was suppressed.
  message;
}

Such that, given the following:

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

The exception caught by e would have the following shape:

SuppressedError {
  error: Error("a"), // from resource 'a'
  suppressed: SuppressedError {
    error: Error("b"), // from resource 'b'
    suppressed: Error("c"), // from the body
  }
}

This is due to the following execution flow:

  1. Resource a is tracked for disposal
  2. Resource b is tracked for disposal
  3. Error("c") is thrown, triggering disposal
  4. Resource b is disposed, throwing Error("b") and suppressing Error("c"), producing a SuppressedError(Error("b"), Error("c")).
  5. Resource a is disposed, throwing Error("a") and suppressing SuppressedError(Error("b"), Error("c")).

See #104 (comment) for additional context.

Supersedes #104
Fixes #74
Fixes #112

@github-actions
Copy link

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

@ljharb
Copy link
Member

ljharb commented Nov 18, 2022

Why wouldn't error be cause instead? Otherwise, you can have a SuppressedError with all of a cause, error, and suppressed, which is weird.

@rbuckton
Copy link
Collaborator Author

Why wouldn't error be cause instead? Otherwise, you can have a SuppressedError with all of a cause, error, and suppressed, which is weird.

I considered using cause, but cause is optional on other built-in Errors, and is provided via an options bag. I chose error as the singular of the errors property present on AggregateError. It's either weird for not using cause, or its weird for using cause differently than anything else. I erred on the side of "let cause be its own special thing in the platform".

@rbuckton rbuckton merged commit 4408fbf into main Nov 24, 2022
@rbuckton rbuckton deleted the suppressederror branch November 24, 2022 03:02
@ljharb
Copy link
Member

ljharb commented Dec 20, 2022

@rbuckton trying to implement this, I remain very confused why a SuppressedError would have an .error that is literally described as the cause of the suppression, instead of using .cause for that. It seems better to me to not support cause at all in this constructor rather than have two conceptual cause properties on the same error instance.

Also, as specified, neither of the arguments are required for SuppressedError - i can do new SuppressedError() just fine. So, why would cause being optional matter?

Was this discussed at the recent plenary?

@rbuckton
Copy link
Collaborator Author

@rbuckton trying to implement this, I remain very confused why a SuppressedError would have an .error that is literally described as the cause of the suppression, instead of using .cause for that.

As I mentioned above, cause has a special meaning with the built-in Error classes that I didn't want to collide with.

It seems better to me to not support cause at all in this constructor rather than have two conceptual cause properties on the same error instance.

That's a possibility. I left it in to remain consistent with other built-in Error classes.

Also, as specified, neither of the arguments are required for SuppressedError - i can do new SuppressedError() just fine. So, why would cause being optional matter?

The presence or absence of cause is observable as it won't be defined if not provided. If you invoke new SuppressedError(), the .error property will always be present (since undefined is a legal exception).

Was this discussed at the recent plenary?

Yes, this was in the slides I presented prior to Stage 3 advancement.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2023

Its special meaning is "the cause of the error", which is precisely how this proposal describes the error property - it's the same meaning.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2023

Additionally, undefined being a legal exception is why new SuppressedError() is a problem - no cause and an undefined cause are different, which is why cause is designed the way it is.

@rbuckton
Copy link
Collaborator Author

Its special meaning is "the cause of the error", which is precisely how this proposal describes the error property - it's the same meaning.

The same could be said for AggregateError's errors property, yet it also has a cause.

no cause and an undefined cause are different, which is why cause is designed the way it is.

This is precisely why I do not believe we should use cause for this, you've indicated that it does have a special meaning. The fact that new SuppressedError() treats the error as undefined is a side-effect of normal JS call/construct semantics. We could potentially throw on construction if you don't provide at least one argument, but we generally don't error when arguments that can legally be undefined are not present in the user-facing API.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2023

I wouldn't say the same for AggregateError - errors are the errors being aggregated, but it might indeed have a distinct cause.

The reality is that SuppressedError has no required arguments right now, and one of them is the cause of the error, and cause already exists. My agenda item today reflects my belief that a SuppressedError instance should only have one kind of cause - there's a few ways that could work out, including "remove error", "do not InstallErrorCause in only this one kind of error", "throw when both are provided", etc.

@rbuckton
Copy link
Collaborator Author

The reality is that SuppressedError has no required arguments right now, and one of them is the cause of the error, and cause already exists. My agenda item today reflects my belief that a SuppressedError instance should only have one kind of cause - there's a few ways that could work out, including "remove error", "do not InstallErrorCause in only this one kind of error", "throw when both are provided", etc.

Of all of these options, the only one I would consider is "do not InstallErrorCause in only this one kind of error". The intent SuppressedError is to model the relationship between an error that is suppressed and the error that suppresses it. Removing error and depending on InstallErrorCause would change the signature such that the "cause" is optional, which is directly opposed to the intent. "throw when both are provided" isn't an option because undefined is a legal JS exception so passing a { cause } should then always throw.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2023

I have use cases for a SuppressedError without a cause.

@rbuckton
Copy link
Collaborator Author

I have use cases for a SuppressedError without a cause.

In the spec, or elsewhere? If your use case is not to represent the relationship I described above, I'd suggest using a different Error type or a user-defined Error. I don't know that I would support the use of SuppressedError for a different purpose elsewhere in ECMA-262. If you are using it for a different purpose, then this is the wrong tool.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2023

Why? The name for it is "suppressed error" - that's what suppressed is for. What better type should I use to indicate I've suppressed an error?

@rbuckton
Copy link
Collaborator Author

For what purpose would you throw a SuppressedError that suppresses an error on its own? If your intent is to use it as a simple data structure to represent a suppression without throwing, then you are likely still using the wrong tool, since Error construction on most runtimes has additional cost due to .stack.

Making the error/cause somehow optional would just be a source of confusion for users since it would completely subvert the meaning of the exception and potentially overcomplicate error-checking. If you want to abuse that relationship in your own code, that is up to you, but I don't think the definition of SuppressedError should cater to that use case.

@ljharb
Copy link
Member

ljharb commented Jan 31, 2023

It's already optional.

@rbuckton
Copy link
Collaborator Author

It's already optional.

The argument, maybe, by nature of the JS construct algorithm, but not the property that is installed on the object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should DisposeResources set cause or suppressed on AggregateError Consider giving this a new type of error.
3 participants