Summary: | Catch parameters must not be lexically redeclared | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||
Component: | JavaScriptCore | Assignee: | 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
Ross Kirsling
2020-03-11 22:58:25 PDT
Created attachment 393344 [details]
Patch
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 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? (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. (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 Ping? (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 on attachment 393344 [details]
Patch
r=me.
Committed r258861: <https://trac.webkit.org/changeset/258861> All reviewed patches have been landed. Closing bug and clearing flags on attachment 393344 [details]. |