Catch parameters must not be lexically redeclared
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].
<rdar://problem/60783189>