bakkot Assignee
	Description • 7 years ago

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.

	bakkot Assignee
	Comment 1 • 7 years ago

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

	Shu-yu Guo [:shu]
	Comment 2 • 7 years ago

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?

	Leo Balter
	Comment 3 • 7 years ago

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

	Jeff Walden [:Waldo]
	Comment 4 • 7 years ago

Anything holding up landing this?

	bakkot Assignee
	Comment 5 • 7 years ago

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.

	bakkot Assignee
	Comment 6 • 7 years ago

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

	Pulsebot
	Comment 7 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbd780d86fc
Implement optional catch binding proposal. r=shu

	Tom S [:evilpie]
	Comment 8 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 7 years ago
bugherder

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

	Michael Ficarra
	Comment 10 • 7 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 11 • 6 years ago

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