Closed Bug 1380881 Opened 7 years ago Closed 6 years ago

Implement optional catch binding proposal


(Core :: JavaScript Engine, enhancement, P3)




Tracking Status
firefox57 --- wontfix
firefox58 --- fixed


(Reporter: bakkot, Assigned: bakkot)


(Blocks 2 open bugs)


(Keywords: dev-doc-complete)


(1 file, 2 obsolete files)

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

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.
Attachment #8886432 - Attachment is patch: true
Attachment #8886432 - Attachment mime type: text/x-patch → text/plain
This is now stage 3. I think it is ready to ship.
Comment on attachment 8886432 [details] [diff] [review]

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?
Attachment #8886432 - Flags: review+
WRT tests, I am sending an update with test262 tests for this soon (Monday, probably).
Anything holding up landing this?
Assignee: nobody → bakkot
Severity: normal → enhancement
Ever confirmed: true
Priority: -- → P3
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.
Attachment #8886432 - Attachment is obsolete: true
Attachment #8927064 - Flags: review+
Whoops, wrong version of patch file... Sigh.
Attachment #8927064 - Attachment is obsolete: true
Attachment #8927101 - Flags: review+
Pushed by
Implement optional catch binding proposal. r=shu
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.
Keywords: checkin-needed
Blocks: 1228841
Blocks: test262
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.