Closed Bug 1380881 Opened 2 years ago Closed 2 years ago

Implement optional catch binding proposal

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: bakkot, Assigned: bakkot)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(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 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.
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]
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?
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
Status: UNCONFIRMED → ASSIGNED
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 evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbd780d86fc
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
https://hg.mozilla.org/mozilla-central/rev/8cbd780d86fc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1418189
No longer depends on: 1418189
You need to log in before you can comment on or make changes to this bug.