22243 - Illegally constructed catchguards should throw an exception

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Robert Ginda]

try {
  throw "foo";
}
catch (e) {
   foo();
} catch (e if true)
   bar();
}

Should throw an exception because the catchall comes before the guarded catch

Comment 1

[Mike Shaver (:shaver emeritus)]

This used to work -- I'll take a look.  How long do I have?

(mccabe was involved in the original catchguard-ectomy, maybe he recalls what
happened to this logic?)

Comment 2

[Robert Ginda]

The sooner the better, looking for a 1.5 final in less than two weeks.

Comment 3

[leger]

Removing [JS15] from Status Summary, putting js1.5 in new Keyword field :-)

Comment 4

[Mike Shaver (:shaver emeritus)]

How about:
js> load('/tmp/catch.js');
/tmp/catch.js:5: SyntaxError: conditional catch after unconditional catch:
/tmp/catch.js:5: } catch (e if true) {
/tmp/catch.js:5: ...........^
js>

I'm _tempted_ to make it a warning rather than an error, but not incredibly so.

I'll attach the patch (against JS15_20000111_BRANCH), which includes the fix for
22245 previously mailed privately to rginda and brendan.

Comment 5

[Mike Shaver (:shaver emeritus)]

Attachment: proposed fix (also fixes 22245)

Comment 6

[Mike McCabe]

I applied the patch (with some effort - looks like your diff is against stuff
that isn't in the tree!) and looks like it does the trick.

js> try { a + b } catch (a) { } catch (b if 4) { }
8: conditional catch after unconditional catch:
8: try { a + b } catch (a) { } catch (b if 4) { }
8: ...................................^

JS accepts multiple unconditional catchguards without complaint:

js> try { a + b } catch (a) { } catch (b) { }

This is a similar case - it should probably be an error as well.

r=mccabe.

Comment 7

[Mike Shaver (:shaver emeritus)]

My patch is against the branch -- were you working with the tip?  I'll make the
change you mention (any catch after a unconditional catch is an error), and
repost.

Comment 8

[Mike Shaver (:shaver emeritus)]

Attachment: new patch (error for _any_ catch after an unconditional catch)

Comment 9

[Mike Shaver (:shaver emeritus)]

js> try { a + b } catch (a) { } catch (b if 4) { }
2: catch after unconditional catch:
2: try { a + b } catch (a) { } catch (b if 4) { }
2: ............................^
js>

mccabe, did you review against 22245 as well?

Comment 10

[Brendan Eich [:brendan]]

r=[e-mail] (with fix for comment nit that I whined to shaver about
privately).

/be

Comment 11

[Brendan Eich [:brendan]]

Shaver, you wanna check this into JS15_20000111_BRANCH and I'll land it?  I want
to land stuff today.

/be

Comment 12

[Brendan Eich [:brendan]]

I landed the branch with shaver's fix to this bug.  Mail from shaver reminds
me: isn't 22245 INVALID due to a bogus testcase?

/be

Comment 13

[Robert Ginda]

Exceptions/catchguard-003-n.js added to cover
mccabe's catch (e) {} catch (e) {} case.

Comment 14

[Robert Ginda]

Marking Verified.