Bug 208976 – Catch parameters must not be lexically redeclared

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ross Kirsling]

Catch parameters must not be lexically redeclared

[Ross Kirsling]

Created attachment 393344 [details]
Patch

[Ross Kirsling]

Comment on attachment 393344 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.h:1648
> -    template <class TreeBuilder> TreeStatement parseBlockStatement(TreeBuilder&);
> +    template <class TreeBuilder> TreeStatement parseBlockStatement(TreeBuilder&, bool isCatchBlock = false);

I know this isn't the greatest, but if we don't mark the catch block scope itself, our error checking would blimp out to something like this:

    if (scope.hasContainingScope()) {
        ScopeRef containingScope = scope.containingScope();
        if (containingScope->catchParameterScopeType() != CatchParameterScopeType::None && containingScope->hasLexicallyDeclaredVariable(*ident))
            return DeclarationResult::InvalidDuplicateDeclaration;
    }

[Keith Miller]

Comment on attachment 393344 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.h:1648
>> +    template <class TreeBuilder> TreeStatement parseBlockStatement(TreeBuilder&, bool isCatchBlock = false);
> 
> I know this isn't the greatest, but if we don't mark the catch block scope itself, our error checking would blimp out to something like this:
> 
>     if (scope.hasContainingScope()) {
>         ScopeRef containingScope = scope.containingScope();
>         if (containingScope->catchParameterScopeType() != CatchParameterScopeType::None && containingScope->hasLexicallyDeclaredVariable(*ident))
>             return DeclarationResult::InvalidDuplicateDeclaration;
>     }

Why can't we just use the catch parameter's scope as the block's scope?

[Ross Kirsling]

(In reply to Keith Miller from comment #3)
> Why can't we just use the catch parameter's scope as the block's scope?

I tried that briefly but it seemed like the very test you fixed in bug 189914 was expecting them to be separate. Looking more closely now though, I feel like this shouldn't be the case, so maybe I had something else mistaken.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #4)
> (In reply to Keith Miller from comment #3)
> > Why can't we just use the catch parameter's scope as the block's scope?
> 
> I tried that briefly but it seemed like the very test you fixed in bug
> 189914 was expecting them to be separate. Looking more closely now though, I
> feel like this shouldn't be the case, so maybe I had something else mistaken.

So that *particular* test failing was my own mistake, but there is a neighboring test which specifically ensures that the param scope is separate from the block scope:
https://github.com/tc39/test262/blob/master/test/language/statements/try/scope-catch-block-lex-open.js

Was a good exercise to verify though. :P

[Ross Kirsling]

Ping?

[Keith Miller]

(In reply to Ross Kirsling from comment #5)
> (In reply to Ross Kirsling from comment #4)
> > (In reply to Keith Miller from comment #3)
> > > Why can't we just use the catch parameter's scope as the block's scope?
> > 
> > I tried that briefly but it seemed like the very test you fixed in bug
> > 189914 was expecting them to be separate. Looking more closely now though, I
> > feel like this shouldn't be the case, so maybe I had something else mistaken.
> 
> So that *particular* test failing was my own mistake, but there is a
> neighboring test which specifically ensures that the param scope is separate
> from the block scope:
> https://github.com/tc39/test262/blob/master/test/language/statements/try/
> scope-catch-block-lex-open.js
> 
> Was a good exercise to verify though. :P

Ahh, lame, I guess that makes sense though...

[Keith Miller]

Comment on attachment 393344 [details]
Patch

r=me.

[EWS]

Committed r258861: <https://trac.webkit.org/changeset/258861>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393344 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60783189>