1380881 - Implement optional catch binding proposal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[bakkot]

Attachment: 0001-Implement-optional-catch-binding.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

At the July meeting of TC39, a proposal to make the binding to a 'catch' optional, in which case the exception is silently dropped. See https://github.com/michaelficarra/optional-catch-binding-proposal/

This extends the syntax of try/catch statements to allow `try {} catch {}` and `try {} catch {} finally {}`, with no `(e)` (or other binding form) after the `catch`.

I've attached a patch implementing this proposal. As it hasn't even achieved stage 1 as of this writing, this probably shouldn't be merged yet. I'll update after the meeting.

Comment 1

[bakkot]

This is now stage 3. I think it is ready to ship.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8886432 [details] [diff] [review]
0001-Implement-optional-catch-binding.patch

Review of attachment 8886432 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Bakkot, when you upload a new patch with the nits fixed, please carry the r=shu and mark this bug as checkin-needed, thanks!

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6916,4 @@
>      EmitterScope emitterScope(this);
>      ScopeKind kind;
>      if (body->isKind(PNK_CATCH))
> +        kind = (!body->pn_kid1 || body->pn_kid1->isKind(PNK_NAME)) ? ScopeKind::SimpleCatch : ScopeKind::Catch;

Nit: >100 chars.

::: js/src/frontend/Parser.cpp
@@ +7045,3 @@
>               * where lhs is a name or a destructuring left-hand side.
> +             * The second is legal only #ifdef JS_HAS_CATCH_GUARD,
> +             * the third is a proposed feature from TC39.

Seems like a shoo-in to become part of the spec at this point. I think it's safe to omit the "the third is ..." sentence.

::: js/src/tests/ecma_2018/Syntax/omitted-catch-binding.js
@@ +32,5 @@
> +}
> +assertEq(reached, true);
> +
> +if (typeof reportCompare === "function")
> +    reportCompare(0, 0);

I don't mind having this test in js/src/tests, but it seems like it would be superseded by a test262 test right quick. Do you have upstream test262 PRs for this feature?

Comment 3

[Leo Balter]

WRT tests, I am sending an update with test262 tests for this soon (Monday, probably).

Comment 4

[Jeff Walden [:Waldo]]

Anything holding up landing this?

Comment 5

[bakkot]

Attachment: 0001-Implement-optional-catch-binding.patch

Sorry I lost track of this. Patch got stale in the mean time; here's a new version with nits addressed.

Not sure if I'm doing the review thing right, or how to mark this as checkin-needed.

Comment 6

[bakkot]

Attachment: 0001-Implement-optional-catch-binding.patch

Whoops, wrong version of patch file... Sigh.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbd780d86fc
Implement optional catch binding proposal. r=shu

Comment 8

[Tom S]

Thanks for your patch! I just pushed this myself after resolving a trivial merge conflict and fixing the commit message. You can request check-in by setting "checkin-needed" in the keywords field.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cbd780d86fc

Comment 10

[Michael Ficarra]

I've added a bit of documentation to the "try...catch" MDN article here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch#The_exception_identifier

Comment 11

[Florian Scholz (Open Web Docs)]

Thanks, I've also added this to Fx58 for developers 
https://developer.mozilla.org/en-US/Firefox/Releases/58#JavaScript

And to our compat data:
https://github.com/mdn/browser-compat-data/pull/1152