Bug 208976

Summary: Catch parameters must not be lexically redeclared
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: JavaScriptCoreAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Ross Kirsling 2020-03-11 22:58:25 PDT
Catch parameters must not be lexically redeclared
Comment 1 Ross Kirsling 2020-03-11 23:17:22 PDT
Created attachment 393344 [details]
Patch
Comment 2 Ross Kirsling 2020-03-11 23:21:44 PDT
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;
    }
Comment 3 Keith Miller 2020-03-16 16:36:55 PDT
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?
Comment 4 Ross Kirsling 2020-03-16 16:43:59 PDT
(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.
Comment 5 Ross Kirsling 2020-03-17 17:40:01 PDT
(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
Comment 6 Ross Kirsling 2020-03-19 14:24:29 PDT
Ping?
Comment 7 Keith Miller 2020-03-23 11:18:38 PDT
(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...
Comment 8 Keith Miller 2020-03-23 11:19:21 PDT
Comment on attachment 393344 [details]
Patch

r=me.
Comment 9 EWS 2020-03-23 11:23:56 PDT
Committed r258861: <https://trac.webkit.org/changeset/258861>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393344 [details].
Comment 10 Radar WebKit Bug Importer 2020-03-23 11:24:12 PDT
<rdar://problem/60783189>