Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Path to stage 4! #2

Closed
24 of 28 tasks
hemanth opened this issue Sep 24, 2020 · 27 comments
Closed
24 of 28 tasks

Path to stage 4! #2

hemanth opened this issue Sep 24, 2020 · 27 comments

Comments

@hemanth
Copy link
Member

hemanth commented Sep 24, 2020

Stage 4

  • committee approval
  • implement in two browsers
    • node
    • Chrome Canary / v8
      • Chrome (flagged)
      • Chrome (unflagged)
    • Firefox / SpiderMonkey
      • Firefox Beta
      • Firefox (unflagged)
    • Safari / Webkit / JSC
      • Safari (unflagged)
      • Safari Technical Preview
    • Edge / Chakra
      • Edge (unflagged)
    • TypeScript
    • Hermes
  • ecma262 PR approved
  • prepare ecma262
  • merge test262 tests
  • write test262 tests

Stage 3

  • committee approval
  • spec editor signoff
  • spec reviewer signoff from @ljharb and @codehag
  • receive implementor feedback

Stage 2

  • receive developer feedback
  • committee approval
  • spec text written
  • spec reviewers selected
@bmeurer
Copy link

bmeurer commented Dec 1, 2020

From a Chrome DevTools perspective, Error.prototype.cause seems reasonable to me. I was already thinking about ways to retain the existing (internal) stack trace on Error objects when programs rethrow errors, i.e. like in this case:

try {
  ...
} catch (e) {
  // some code
  throw e;
}

Here we would keep track of the original stack trace on e and show that similar to the way we show async stack traces in DevTools front-end. The addition of Error.prototype.cause would make it possible to extend this logic to cases where the developer not simply rethrows the current exception, but wraps it in a new Error object.

cc @RReverser

@ljharb
Copy link
Member

ljharb commented Feb 26, 2021

Since this is on the agenda for stage 3, it seems like the OP of this issue needs to be updated, and a number of open issues still need to be addressed?

Additionally, who are the designated reviewers?

@legendecas
Copy link
Member

legendecas commented Mar 2, 2021

@ljharb do you mean the reviewers have been discussed at #15? If so, I believe the reviewers are you and @codehag.

@ljharb
Copy link
Member

ljharb commented Mar 2, 2021

Indeed :-) i wanted to make sure that plenary consensus on selecting reviewers was recorded in this repo.

@syg
Copy link

syg commented Mar 3, 2021

Editor review

I find the CreateNonEnumerableDataProperty unnecessary and confusing. It's confusing because:

  1. the behavior is ultimately DefinePropertyOrThrow, which is not what the name suggests. It reads like it's analogous to CreateDataProperty, which doesn't throw.
  2. The name doesn't tell me what the [[Configurable]] and [[Writable]] values are.

I'd prefer that the steps be inlined into their few call sites, it's only two steps anyway.

Delegate review

In InstallErrorCause, what's the rationale for doing both a [[Has]] and a [[Get]]? Most option bags (and the ones in 402), AFAIK, do a single [[Get]] and check if the value is undefined. I'd prefer a single [[Get]] that checks for undefined.

@bakkot
Copy link
Contributor

bakkot commented Mar 3, 2021

In InstallErrorCause, what's the rationale for doing both a [[Has]] and a [[Get]]? Most option bags (and the ones in 402), AFAIK, do a single [[Get]] and check if the value is undefined. I'd prefer a single [[Get]] that checks for undefined.

That's something I asked for. It's possible to throw undefined, so it should be possible to create an error whose cause is undefined.

@syg
Copy link

syg commented Mar 3, 2021

That's something I asked for. It's possible to throw undefined, so it should be possible to create an error whose cause is undefined.

I see, thanks for the explanation. I guess it's a legit use case, but it seems pretty edge-casey to me.

It also means that since "not having a cause" and "having a nullish cause" are supposed to be distinguished states, consumers can't consume this in the "obvious" way with nullish coalescing ??.

@bakkot
Copy link
Contributor

bakkot commented Mar 3, 2021

consumers can't consume this in the "obvious" way with nullish coalescing ??.

That depends on whether the consumers care to distinguish those cases. I suspect most won't; I just think we shouldn't make it impossible to do so.

@ljharb
Copy link
Member

ljharb commented Mar 3, 2021

I asked for the steps to be in an AO - mainly because they were repeated three times and seemed easy to mess up.

@syg
Copy link

syg commented Mar 3, 2021

I asked for the steps to be in an AO - mainly because they were repeated three times and seemed easy to mess up.

If you think the refactoring is important, I think it should have an "OrThrow" appended to it. Otherwise I'm still happier with it just inlined.

@syg
Copy link

syg commented Mar 3, 2021

That depends on whether the consumers care to distinguish those cases. I suspect most won't; I just think we shouldn't make it impossible to do so.

That's what I initially thought, but then it seemed incorrect to me for the consumer to not care if the producer uses the cause to mean "was this a caught-and-rethrown error or not". Concretely, if the producer decided to distinguish, DevTools UI may be presenting misleading information if it doesn't.

@bakkot
Copy link
Contributor

bakkot commented Mar 3, 2021

Well, the devtools UI should probably also distinguish, yes. But certainly in my own code I plan to write if (error.cause) {...} and complain to upstream libraries if that breaks because they're throwing a non-object value.

To put it another way: producers should not ever throw undefined, and it's fine for user code to be written as if that never happens. But people can do things they shouldn't, and the language should try to behave reasonably even in those cases: here, that means that if someone does catch (e) { throw new Error(msg, { cause: e }) }, they should always end up with an error with a cause, even in the case of someone else doing something they shouldn't.

I don't think we should extrapolate from "the language should accommodate people doing a weird thing" to "user code in general should accommodate that weird thing".

@syg
Copy link

syg commented Mar 3, 2021

I don't think we should extrapolate from "the language should accommodate people doing a weird thing" to "user code in general should accommodate that weird thing".

Agreed.

For this specific case, I don't feel very strongly at all. In general, there is unfortunately precedent in the language where undefined is already conflated with "absence", like optional parameters (despite presence of arguments.length). So another part of my question here is why accommodate here.

@bakkot
Copy link
Contributor

bakkot commented Mar 3, 2021

Well:

if someone does catch (e) { throw new Error(msg, { cause: e }) }, they should always end up with an error with a cause

I don't have much of an argument beyond that.

@syg
Copy link

syg commented Mar 3, 2021

Yeah... I'm fine with this behavior to be clear. I'm lamenting we just don't have a general design principle for whether to conflate undefined with absence.

@jugglinmike
Copy link

I asked for the steps to be in an AO - mainly because they were repeated three times and seemed easy to mess up.

If you think the refactoring is important, I think it should have an "OrThrow" appended to it. Otherwise I'm still happier with it just inlined.

@syg legendecas has made the name change you requested, but now, the proposal introduces an "OrThrow" method that never throws. The prior name and use of Assert seems more direct to me. Would your original motivation be addressed if this were instead defined as an infallible abstract operation? Here's a patch demonstrating what I have in mind.

@ljharb
Copy link
Member

ljharb commented Mar 12, 2021

@jugglinmike it doesn't throw with its current usage, but it could throw if called from other places, so "OrThrow" seems like the right place to me.

@MadProbe
Copy link

MadProbe commented Mar 15, 2021

ChakraCore implementation

My PR(chakra-core/ChakraCore#6621) with implementation (unflagged) of this proposal was merged to master branch of ChakraCore yesterday at time of writing of this comment

Note: Microsoft Edge no longer uses ChakraCore and now it's an open-source community project
Edit: AggregateError is not implemented in ChakraCore yet (there is a PR - chakra-core/ChakraCore#6301, - but not updated to complete the implementaion of this proposal and merged), so implementation is not full yet

@hemanth
Copy link
Member Author

hemanth commented Mar 15, 2021

That's awesome news @MadProbe 🤓

P.S: We are working on 262 tests.

@Constellation
Copy link

WebKit/WebKit@b03c4f4 @rkirsling landed WebKit/JSC implementation.

@rkirsling
Copy link
Member

Note for implementers:

According to the WASM JS API spec...

The constructor and properties of WebAssembly errors is as specified for NativeError.

So don't forget to update the WebAssembly.{CompileError, LinkError, RuntimeError} constructors too. 🙂

@avp
Copy link

avp commented Mar 24, 2021

Landed in Hermes: facebook/hermes@56e7cda

@BasixKOR
Copy link

BasixKOR commented Jul 19, 2021

Landed in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1679653

I confirmed this on Firefox 91.0b4.

@Turbo87
Copy link

Turbo87 commented Sep 1, 2021

FWIW Sentry has implemented support for a cause property back in late 2018 (see getsentry/sentry-javascript#1401) and from what I've seen so far it should be compatible with this proposal 🥳

@BasixKOR
Copy link

BasixKOR commented Nov 5, 2021

Labded in TypeScript with "target": "ES2022" microsoft/TypeScript#46291

@saschanaz
Copy link

saschanaz commented Nov 5, 2021

I guess this can be closed since it has already entered stage 4 and been merged to ecma262? tc39/ecma262#2356

@legendecas
Copy link
Member

Closing as the proposal has reached consensus on stage 4. Thanks to everyone for the help!

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

No branches or pull requests