Throws incorrectly a syntax error when declaring a top level catch variable the same as a parameter

[seba kerckhof]

I wouldn't write code like this (below) myself, but it was emitted by the minifier and so my app doesn't work in Safari, or Safari TP (build 65 / 10 september 2018).
The code looks legal to me ...

class Foo{
  foo(o){
    try{

    }catch({ o }){

    }
  }
}
results in "SyntaxError: unexpected token: '}'. Cannot declare a lexical variable twice: 'o'."

And the variation:
class Foo{
  foo(o){
    try{

    }catch({ prop: o }){

    }
  }
}

results in "SyntaxError: unexpected identifier 'o'. Cannot declare a lexical variable twice: 'o'."

[seba kerckhof]

Other variations with same problem:

Normal function:
function foo(c){try{}catch({c}){}}

Arrow function:
const foo=c=>{try{}catch({c}){}};

Object shorthand literal:
const foo = {
  bar(c){try{}catch({c}){}}
}


Note that it only seems related to destructuring catch clause arguments, e.g. this works:
function foo(c){try{}catch(c){}}

[Radar WebKit Bug Importer]

<rdar://problem/60309235>

[Simon Fraser (smfr)]

Created attachment 393248 [details]
Test case

[Keith Miller]

Ah, thanks for the report. I think this is happening because we were not incrementing the statement depth (we use the numeric depth as an optimization for determining if we should do the hash table lookup for parameter shadowing) when parsing catch parameters. Should be an easy fix.

[Keith Miller]

For what it's worth, sorry about the the delay on this. I think this got lost in our bug tracking system... :(

[Ross Kirsling]

Test262 evidently does not have a spec conformance test for this behavior.

Note that the shorthand destructuring syntax also makes the problem a bit less clear (since Error objects are only guaranteed to have a `message` field).

It's specifically this that fails:
> function foo(m) { try {} catch ({ message: m }) {} }

[Keith Miller]

(In reply to Ross Kirsling from comment #6)
> Test262 evidently does not have a spec conformance test for this behavior.

There's nothing inside a function but test/language/statements/try/scope-catch-param-lex-open.js has array destructing at the top level with a lexical variable of the same name.

> 
> Note that the shorthand destructuring syntax also makes the problem a bit
> less clear (since Error objects are only guaranteed to have a `message`
> field).

Well you don't have the throw an Error object you can throw anything you want! :P

> 
> It's specifically this that fails:
> > function foo(m) { try {} catch ({ message: m }) {} }

[Ross Kirsling]

(In reply to Keith Miller from comment #7)
> There's nothing inside a function but
> test/language/statements/try/scope-catch-param-lex-open.js has array
> destructing at the top level with a lexical variable of the same name.

That's a good find! I'm surprised that `var x; try {} catch ([x]) {}` fails, because it doesn't fail with let/const.

> Well you don't have the throw an Error object you can throw anything you
> want! :P

Oh geez, apparently I've not had enough coffee this morning. 😅

[Keith Miller]

Created attachment 393267 [details]
Patch

[Saam Barati]

Comment on attachment 393267 [details]
Patch

r=me

[Ross Kirsling]

Comment on attachment 393267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393267&action=review

r=me if EWS is green and Test262 is locally happy

> JSTests/ChangeLog:8
> +        * stress/catch-destructing-shadow-lexical-const-variable-global.js: Added.

nit: destructing -> destructuring

> JSTests/stress/catch-destructing-shadow-lexical-variable-global.js:2
> +let o = value;

Maybe combine this file with the const one and add a var case too?

[Keith Miller]

Comment on attachment 393267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393267&action=review

>> JSTests/stress/catch-destructing-shadow-lexical-variable-global.js:2
>> +let o = value;
> 
> Maybe combine this file with the const one and add a var case too?

I didn't combine them because it would be an error to declare a const and let with the same name. I guess I could choose a new variable name but that's too clever for me. :P

[Ross Kirsling]

(In reply to Keith Miller from comment #12)
> I didn't combine them because it would be an error to declare a const and
> let with the same name. I guess I could choose a new variable name but
> that's too clever for me. :P

I mean, you could use blocks. :P But either way.

[Keith Miller]

(In reply to Ross Kirsling from comment #13)
> (In reply to Keith Miller from comment #12)
> > I didn't combine them because it would be an error to declare a const and
> > let with the same name. I guess I could choose a new variable name but
> > that's too clever for me. :P
> 
> I mean, you could use blocks. :P But either way.

That wouldn't be m_statementDepth == 1 though. So it wouldn't really be testing the issue.

[Keith Miller]

Created attachment 393276 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 393276 [details]
Patch for landing

Clearing flags on attachment: 393276

Committed r258279: <https://trac.webkit.org/changeset/258279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.