Reflect.parse: separate guarded/unguarded catch clauses

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dherman, Assigned: dherman)

Tracking

unspecified
mozilla17
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: reflect-parse)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
In bug 630897 we simplified the parser API to be a single array of catch clauses. But the API is getting popular independent of Firefox's implementation, so it makes sense to separate out the SpiderMonkey-specific parts of the API. I think this would be a better approach to catch clauses:

    interface TryStatement <: Statement {
        type: "TryStatement";
        block: BlockStatement;
        guardedHandlers: [ GuardedCatchClause ] | null;
        handler: CatchClause | null;
        finalizer: BlockStatement | null;
    }

    interface CatchClause <: Node {
        type: "CatchClause";
        param: Pattern;
        body: BlockStatement;
    }

    interface GuardedCatchClause <: Node {
        type: "CatchClause";
        param: Pattern;
        guard: Expression;
        body: BlockStatement;
    }

This separates out the SpiderMonkey-specific bits, because portable/standard JS code will only ever have a null .guardedHandlers property. It's also a good factoring for the SpiderMonkey grammar, because you always have to have at most one unguarded handler. So this is a more precisely-typed factoring.

I'll submit a patch as soon as I can.

Dave
(Assignee)

Updated

5 years ago
Whiteboard: reflect-parse
(Assignee)

Comment 1

5 years ago
BTW, the reason I prefer

    [ GuardedCatchClause ] | null

and not just

    [ GuardedCatchClause ]

is that it avoids allocating tons of empty arrays for all the cases where try statements have no guarded handlers.

Dave
Most scripts contain very few try blocks, if any. The extra arrays probably don't matter.
(Assignee)

Comment 3

5 years ago
There's no reason to expect this API only to be used for hand-written web code. It can be used for any JS code anywhere, including code generated from other languages.

But I guess since the rest of the API uses empty arrays, it's bad form to treat this one case differently.

Dave

Comment 4

5 years ago
Related Esprima issue: http://code.google.com/p/esprima/issues/detail?id=245.

Esprima is ready to change at a press of a button :)
(Assignee)

Comment 5

5 years ago
> Esprima is ready to change at a press of a button :)

OK, let me do a bit of publicity before making the change.

It's going to miss the next Firefox deadline (4/24), so it won't get into Nightly for another 6 weeks, and then it's 6 weeks to Aurora, 6 weeks to Beta, and 6 weeks to Firefox. So if my math is right, I believe it'll most likely land in Firefox 16. I expect that should give the few existing customers enough time to prepare for the change, but we'll see.

Dave

Comment 6

5 years ago
I'd like to release Esprima 1.0 in few weeks. Is it safe to assume that this will be the final API for TryStatement? I would prefer not to change Esprima's syntax tree format after it hits 1.0.
(Assignee)

Comment 7

5 years ago
Created attachment 653188 [details] [diff] [review]
first draft, no tests yet

WIP. I'll work on tests next.

Dave
Assignee: general → dherman
(Assignee)

Comment 8

5 years ago
Created attachment 653190 [details] [diff] [review]
second draft, now with tests

Whoops, haha, let's try not to crash SpiderMonkey. The unguarded catch clause now defaults to null instead of leaking a magic hole to the builder.

Updated tests and added some for the builder.

Dave
Attachment #653188 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #653190 - Flags: review?(jorendorff)
Comment on attachment 653190 [details] [diff] [review]
second draft, now with tests

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

::: js/src/jsreflect.cpp
@@ +931,1 @@
>          return false;

Uber-micro-nit: While you're in here, I think this reads a little better if you common up the newArray call:

    Value guardedHandlers;
    if (!newArray(guarded, &guardedHandlers))
        return false;

    Value cb = callbacks[AST_TRY_STMT];
    if (!cb.isNull)
        return callback(...);

    return newNode(...);

@@ +2049,5 @@
>  {
>      Value var, guard, body;
>  
> +    if (!pattern(pn->pn_kid1, NULL, &var) ||
> +        !optExpression(pn->pn_kid2, &guard))

Style nit: js/src style rules call for curly braces when the if-condition spans multiple lines, and '{' on its own line (just in that case).

Alternatively you could just say

    if (!pattern(...))
        return false;
    if (!optExpression(...))
        return false;
Attachment #653190 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 10

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3cff93ee869
(Assignee)

Comment 11

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/8618d9956e86
https://hg.mozilla.org/mozilla-central/rev/c3cff93ee869
https://hg.mozilla.org/mozilla-central/rev/8618d9956e86
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.