Closed Bug 1228841 Opened 8 years ago Closed 6 years ago

Remove support for conditional catch expressions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bruant.d, Assigned: arai)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(6 files)

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
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.
Flags: needinfo?(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.
Flags: needinfo?(jorendorff)
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.
actually, having conditions makes TryEmitter a bit complicated.
it should benefit from removing this feature.
(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.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
I agree we should kill this off now. With legacy extensions gone, a lot of tradeoffs are changed...
Depends on: 1416246
Depends on: 1416249
Depends on: 1416250
Depends on: 1416251, 1416252
Depends on: 1416253
Depends on: 1416254
the patch needs to be rebased onto bug 1380881
Depends on: 1380881
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.)
First, replaced consumers with standard syntax, or just remove some part.
Attachment #8928577 - Flags: review?(evilpies)
and removed tests that is specific to conditional catch, or its underlying implementation
Attachment #8928578 - Flags: review?(evilpies)
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)
Attachment #8928602 - Flags: review?(evilpies)
Tests that depends on the Reflect.parse behavior is fixed in this patch.
This will be folded into Part 3.
Attachment #8928603 - Flags: review?(evilpies)
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.
Attachment #8928577 - Flags: review?(evilpies) → review+
Attachment #8928578 - Flags: review?(evilpies) → review+
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.
Attachment #8928602 - Flags: review?(evilpies) → review?(jwalden+bmo)
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"]
Attachment #8928603 - Flags: review?(evilpies) → review+
Depends on: 1417799
Depends on: 1417800
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"
Attachment #8928602 - Flags: review?(jwalden+bmo) → review+
apparently I overlooked one consumer
Attachment #8934274 - Flags: review?(wmccloskey)
Attachment #8934274 - Flags: review?(wmccloskey) → review+
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
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
Flags: needinfo?(arai.unmht)
Removed guardedHandler (that is removed in Part 3) handing in devtools.
Flags: needinfo?(arai.unmht)
Attachment #8934844 - Flags: review?(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.
Attachment #8934844 - Flags: review?(jdescottes) → review+
Pushed by arai_a@mac.com:
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
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: