1228841 - Remove support for conditional catch expressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Bruant]

These are not standard and there doesn't seem to be a signal from standard folks that they will be anytime soon. They should probably be eventually removed from SpiderMonkey

Comment 1

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/conditional-catch-clause-support-will-be-removed/

Comment 2

[David Bruant]

I only meant to file the bug, I don't the decision has been made to the point it can be communicated yet.
ni'ing jorendorff in his quality of module owner.

Comment 3

[Jason Orendorff [:jorendorff]]

I think we should keep this extension.

Some features are either incompatible with the ECMA standard, or an implementation burden, or both. Those should go.

This is neither -- and it's been allowed in all JS versions for many years, so dropping it will cost.

Not every language extension needs to die. It's a judgment call.

Comment 4

[Mark Banner (:standard8)]

Just to note, that since we're using ESLint and ESLint doesn't support conditional catch clauses, I think pretty much all of the instances that Firefox was using have been removed.

If we did want to allow it in gecko code, we'd probably need to find a way to extend the ESLint parser in such a way that we could support it, although I suspect that could cause more work than we'd really want to do.

Comment 6

[Tooru Fujisawa [:arai]]

actually, having conditions makes TryEmitter a bit complicated.
it should benefit from removing this feature.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> I think we should keep this extension.
... snip...
> Not every language extension needs to die. It's a judgment call.

Do you still think so? As mentioned in bug 1405098, ESLint can't parse this, so there's definitely a cost. If no other browser supports this, and our frontend devs avoid it because other tools don't support it, we might as well remove it IMO.

Comment 8

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a402074e48b16b4fdce84503c7abb442200b7721

Comment 9

[Jason Orendorff [:jorendorff]]

I agree we should kill this off now. With legacy extensions gone, a lot of tradeoffs are changed...

Comment 10

[Tooru Fujisawa [:arai]]

the patch needs to be rebased onto bug 1380881

Comment 11

[Jeff Walden [:Waldo]]

It would be nice if ecma_6/ArrowFunctions/arrow-not-as-end-of-statement.js had its catch-expression test removed at the same time this is removed, for cleanliness.  (Although the test was carefully formulated such that lack of support for this extension, would not represent a failure.)

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove conditional catch consumers in js/.

First, replaced consumers with standard syntax, or just remove some part.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove testcases specific to conditional catch in js/.

and removed tests that is specific to conditional catch, or its underlying implementation

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove conditional catch.

in Parser, PNK_TRY's kid2 is now PNK_LEXICALSCOPE, instead of PNK_CATCHLIST (PNK_LEXICALSCOPE was the element of PNK_CATCHLIST before), since there's at most one catch.
PNK_CATCH is now binary instead of ternary, since there's no guard.

Reflect.parse now generates catch node without "guard", and try node without "guardedHandlers" property.
(and builder callback's arity is changed, the builder itself will be removed in bug 1415188)

in BytecodeEmitter, now TryEmitter::emitCatch can be called only once, after emitTry, and it doesn't check guards.
also, in TryEmitter::emitCatchEnd, JSOP_GOTO after the catch block is now emitted only if there's finally.
(that was always emitted before)

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 3.1: Fix Reflect.parse tests for conditional catch.

Tests that depends on the Reflect.parse behavior is fixed in this patch.
This will be folded into Part 3.

Comment 16

[Tom S]

Comment on attachment 8928577 [details] [diff] [review]
Part 1: Remove conditional catch consumers in js/.

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

::: js/src/tests/test262/language/statements/try/S12.14_A4.js
@@ +1,5 @@
>  // Copyright 2009 the Sputnik authors.  All rights reserved.
>  // This code is governed by the BSD license found in the LICENSE file.
>  
>  /*---
> +info: Sanity test for "catch(Identifier) statement"

I am not sure how local test262 changes work, maybe revert this.

Comment 17

[Tom S]

Comment on attachment 8928602 [details] [diff] [review]
Part 3: Remove conditional catch.

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

Sorry, this is a bit too complicated parsing stuff for me to feel completely comfortable with reviewing.

Comment 18

[Tom S]

Comment on attachment 8928603 [details] [diff] [review]
Part 3.1: Fix Reflect.parse tests for conditional catch.

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

::: js/src/tests/js1_8_5/reflect-parse/alternateBuilder.js
@@ +178,5 @@
>      returnStatement: function(expr) {
>          return expr ? ["ReturnStmt", {}, expr] : ["ReturnStmt", {}];
>      },
> +    tryStatement: function(body, handler, fin) {
> +        var node = ["TryStmt", body, handler || "Empty"];

Shoudln't this be ["Empty"]

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 8928602 [details] [diff] [review]
Part 3: Remove conditional catch.

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

::: js/src/builtin/ReflectParse.cpp
@@ +526,5 @@
>  
>      MOZ_MUST_USE bool switchStatement(HandleValue disc, NodeVector& elts, bool lexical, TokenPos* pos,
>                                        MutableHandleValue dst);
>  
> +    MOZ_MUST_USE bool tryStatement(HandleValue body, HandleValue handler,

Ugh, "handler" is such a bad name.  But I guess this is what was there before, so...

@@ +2196,5 @@
>  
> +    RootedValue handler(cx, NullValue());
> +    if (ParseNode* catchScope = pn->pn_kid2) {
> +        MOZ_ASSERT(catchScope->isKind(PNK_LEXICALSCOPE));
> +        if (!catchClause(catchScope->pn_expr, &handler))

Seems like maybe the scopeBody() accessor would be better.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1689,5 @@
>  
>          if (!controlInfo_)
>              return true;
>  
> +        // Gosub <finally>, if required.

Technically I think this wasn't capitalized to be like the opcode when you see it in dis() and such, but I'm not horribly particular how we do/don't capitalize here.

@@ -6774,5 @@
> -        //   debugleaveblock
> -        //   [poplexicalenv]            only if any local aliased
> -        //   throwing                   pop exception to cx->exception
> -        //   goto <next catch block>
> -        //   POST: pop

Horrifying.

::: js/src/frontend/ParseNode.cpp
@@ +406,5 @@
>              stack->push(pn->pn_kid3);
>          return PushResult::Recyclable;
>        }
>  
> +      // A catch node has left node as catch-variable pattern, and right node

"...as catch-variable pattern (or null if omitted) and right..."

(no comma because only two clauses and they're not both complete sentences)

::: js/src/frontend/ParseNode.h
@@ +267,1 @@
>   *                                   (PNK_ARRAY or PNK_OBJECT if destructuring)

Shouldn't this mention null being okay for catch-var-less catch blocks?

::: js/src/frontend/Parser.cpp
@@ +6781,5 @@
>       * kid2 is the catch node list or null
>       * kid3 is the finally statement
>       *
> +     * catch nodes are binary.
> +     * left is the lvalue or null

s/lvalue/catch-name\/pattern/ seems like is better than "lvalue"

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Remove remaining conditional catch consumers in dom/.

apparently I overlooked one consumer

Comment 21

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9651b5f82d299c7d7b043ea9dbdbd3b783af0e8a
Bug 1228841 - Part 0: Remove remaining conditional catch consumers in dom/. r=billm

https://hg.mozilla.org/integration/mozilla-inbound/rev/41b4f7b178638a76f3606f5871ae9b7036aec500
Bug 1228841 - Part 1: Remove conditional catch consumers in js/. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/37581537c8121e3edc8b3fdf1678974ce232bfc4
Bug 1228841 - Part 2: Remove testcases specific to conditional catch in js/. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/28f34951d1b3fc6c0399435f355dd9ae5a12b3fb
Bug 1228841 - Part 3: Remove conditional catch and fix Reflect.parse tests for conditional catch. r=jwalden,evilpie

Comment 22

[Andreea Pavel [:apavel]]

Backed out for devtools mochitest failure devtools/client/debugger/test/mochitest/browser_dbg_search-symbols.js  r=backout a=backout on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=28f34951d1b3fc6c0399435f355dd9ae5a12b3fb
Failure log: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=28f34951d1b3fc6c0399435f355dd9ae5a12b3fb

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/a26e71300a88

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 3.2: Remove conditional catch handling in devtools.

Removed guardedHandler (that is removed in Part 3) handing in devtools.

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8934844 [details] [diff] [review]
Part 3.2: Remove conditional catch handling in devtools.

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

r+ without the change in parser-worker.js

::: devtools/client/debugger/new/parser-worker.js
@@ -6851,5 @@
>      clause.body = this.parseBlock();
>      node.handler = this.finishNode(clause, "CatchClause");
>    }
>  
> -  node.guardedHandlers = empty;

This file is generated from https://github.com/devtools-html/debugger.html and this exact line comes from a babel plugin:
  https://github.com/babel/babel/blob/62bbee97d7245b897973d290d1700797a85fe168/packages/babylon/src/parser/statement.js

If we don't need to remove this assignment for the tests to pass, let's leave the file untouched.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e441cdecaa27
Part 0: Remove remaining conditional catch consumers in dom/. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/129e38525209
Part 1: Remove conditional catch consumers in js/. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4cbc76568b
Part 2: Remove testcases specific to conditional catch in js/. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb7aa8c2ac8
Part 3: Remove conditional catch handling in devtools. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/a90317cd54ba
Part 4: Remove conditional catch and fix Reflect.parse tests for conditional catch. r=jwalden,evilpie

Comment 26

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e441cdecaa27e4ed7638f1b12fc376e6fab4d1b6
Bug 1228841 - Part 0: Remove remaining conditional catch consumers in dom/. r=billm

https://hg.mozilla.org/integration/mozilla-inbound/rev/129e385252093fd9fa06667c863c77ca2148dee8
Bug 1228841 - Part 1: Remove conditional catch consumers in js/. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4cbc76568b6ea32330e0ef2b55c898b5095d8b
Bug 1228841 - Part 2: Remove testcases specific to conditional catch in js/. r=evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb7aa8c2ac8138ea4c2257301b09a63ec2cc908
Bug 1228841 - Part 3: Remove conditional catch handling in devtools. r=jdescottes

https://hg.mozilla.org/integration/mozilla-inbound/rev/a90317cd54ba39d47ded48c048bfd695e53c7bcd
Bug 1228841 - Part 4: Remove conditional catch and fix Reflect.parse tests for conditional catch. r=jwalden,evilpie

Comment 27

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/conditional-catch-clauses-are-no-longer-supported/

Comment 28

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e441cdecaa27
https://hg.mozilla.org/mozilla-central/rev/129e38525209
https://hg.mozilla.org/mozilla-central/rev/5e4cbc76568b
https://hg.mozilla.org/mozilla-central/rev/cbb7aa8c2ac8
https://hg.mozilla.org/mozilla-central/rev/a90317cd54ba

Comment 29

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch#Conditional_catch_clauses
https://developer.mozilla.org/en-US/Firefox/Releases/59#Removals_from_the_web_platform
https://github.com/mdn/browser-compat-data/pull/1137