Redeclaration of var over let/const/class should be a syntax error.
Created attachment 356356 [details] Patch
Discovered via the following: https://test262.report/browse/language/statements/const/redeclaration-error-from-within-strict-mode-function-const.js https://test262.report/browse/language/statements/let/redeclaration-error-from-within-strict-mode-function.js
Comment on attachment 356356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356356&action=review > Source/JavaScriptCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Could you show and add URL to ECMA262 which describes this behavior?
(In reply to Yusuke Suzuki from comment #3) > Comment on attachment 356356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356356&action=review > > > Source/JavaScriptCore/ChangeLog:6 > > + Reviewed by NOBODY (OOPS!). > > Could you show and add URL to ECMA262 which describes this behavior? Ah, OK. URL is pasted in the bug URL. Can you add them to ChangeLog?
(In reply to Yusuke Suzuki from comment #4) > (In reply to Yusuke Suzuki from comment #3) > > Comment on attachment 356356 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=356356&action=review > > > > > Source/JavaScriptCore/ChangeLog:6 > > > + Reviewed by NOBODY (OOPS!). > > > > Could you show and add URL to ECMA262 which describes this behavior? > > Ah, OK. URL is pasted in the bug URL. > Can you add them to ChangeLog? Ah, sorry. No. They are links to test262 report. Could you add ECMA262 description?
(In reply to Yusuke Suzuki from comment #5) > (In reply to Yusuke Suzuki from comment #4) > > (In reply to Yusuke Suzuki from comment #3) > > > Comment on attachment 356356 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=356356&action=review > > > > > > > Source/JavaScriptCore/ChangeLog:6 > > > > + Reviewed by NOBODY (OOPS!). > > > > > > Could you show and add URL to ECMA262 which describes this behavior? > > > > Ah, OK. URL is pasted in the bug URL. > > Can you add them to ChangeLog? > > Ah, sorry. No. They are links to test262 report. > Could you add ECMA262 description? Will do!
Created attachment 356387 [details] Patch
Comment on attachment 356387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356387&action=review It's funny that you just uploaded a patch for this, me and Keith were just discussing this bug. We should just fix this the Right Way. This means things work irrespective of program order. I propose: When we hoist a var upwards, we drop a marker in each intervening scope, so that allows us to detect things for both of these programs: "{ let x = 20; { var x = 20; } }" and "{ { var x = 20; } let x = 20; }" > Source/JavaScriptCore/ChangeLog:13 > + As the title suggests, this patch just handles the { let x; var x; } scenario; > + it does not specifically cover { var x; let x; } or shadowed function declarations. Let's do this the right way. > Source/JavaScriptCore/parser/Parser.h:1211 > + if (currentLexicalDeclarationScope()->lexicalVariables().contains(ident->impl())) > + return DeclarationResult::InvalidDuplicateDeclaration; This looks wrong. What about nesting? { let x = 20; { { { var x = 42; } } } }
(In reply to Saam Barati from comment #8) > Comment on attachment 356387 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356387&action=review > > It's funny that you just uploaded a patch for this, me and Keith were just > discussing this bug. We should just fix this the Right Way. This means > things work irrespective of program order. > > I propose: > When we hoist a var upwards, we drop a marker in each intervening scope, so > that allows us to detect things for both of these programs: > "{ let x = 20; { var x = 20; } }" > and > "{ { var x = 20; } let x = 20; }" Sounds good! Although my original thought (having started with the test cases instead of the spec text) was to make a bite-size first-stage patch, I hadn't actually considered nested cases and now realize that I drew a pretty arbitrary line. :) Thankfully though, this spurred on an enrichment of the test cases too (https://github.com/tc39/test262/pull/1982)! I may reach out to you and/or Keith on IRC to unpack the specifics of "when we hoist", should I happen to get stuck.
(In reply to Ross Kirsling from comment #9) > (In reply to Saam Barati from comment #8) > > Comment on attachment 356387 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=356387&action=review > > > > It's funny that you just uploaded a patch for this, me and Keith were just > > discussing this bug. We should just fix this the Right Way. This means > > things work irrespective of program order. > > > > I propose: > > When we hoist a var upwards, we drop a marker in each intervening scope, so > > that allows us to detect things for both of these programs: > > "{ let x = 20; { var x = 20; } }" > > and > > "{ { var x = 20; } let x = 20; }" > > Sounds good! Although my original thought (having started with the test > cases instead of the spec text) was to make a bite-size first-stage patch, I > hadn't actually considered nested cases and now realize that I drew a pretty > arbitrary line. :) Thankfully though, this spurred on an enrichment of the > test cases too (https://github.com/tc39/test262/pull/1982)! > > I may reach out to you and/or Keith on IRC to unpack the specifics of "when > we hoist", should I happen to get stuck. Sounds good. Happy to help.
Finally getting around to looking at this properly. :) Here are the two approaches that I can clearly see: 1. "Verify before each declaration", in which case var-after and var-before are two unrelated codepaths. a. { let x; { var x; } }: At the var declaration site, we need only look up; i.e., we check the existing lexical names at every enclosing scope up to (and including) the current var scope. b. { { var x; } let x; }: At the let declaration site, we want to be able to easily check any var names being hoisted over the current lexical scope. To make this possible, we ensure that any var names with a declaration site at or below the current lexical scope are preserved within it. (This sounds a bit like what you were saying, but it's not a uniform approach...?) 2. "Verify all declarations at end of scope", which unifies var-after and var-before but implies a significant difference in user-facing behavior. (This is the only uniform approach that occurs to me, but I don't think it's what you meant...?) In investigating existing implementations, it appears that (roughly speaking) SM does the former and V8 does the latter. This has visible implications when there's a subsequent syntax error in the same block: > λ eshost -x "{ let x; { var x; } % }" > #### v8 > SyntaxError: Unexpected token % > > #### sm > SyntaxError: redeclaration of let x: ...and is also interesting when compared against global or top-level function scope behavior: > λ eshost -x "let x; { var x; } %" > #### v8 > SyntaxError: Identifier 'x' has already been declared > > #### sm > SyntaxError: redeclaration of let x: As approach 1 is cleanly separable into two patches, I'm going to attach a proper patch for 1a here, at least for discussion's sake.
Created attachment 356907 [details] Patch
Created attachment 356945 [details] Patch
(In reply to Ross Kirsling from comment #11) > As approach 1 is cleanly separable into two patches, I'm going to attach a > proper patch for 1a here, at least for discussion's sake. Scratch that -- replaced it with a patch for the entire thing.
Comment on attachment 356945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356945&action=review > Source/JavaScriptCore/parser/Parser.h:1248 > + if (!canDeclareVariable(*ident)) > + return DeclarationResult::InvalidDuplicateDeclaration; > + > + auto varScope = currentVariableScope(); > + auto lexicalScope = currentLexicalDeclarationScope(); > + if (lexicalScope != varScope) > + lexicalScope->addVariableBeingHoisted(ident); > + > + return varScope->declareVariable(ident); I think we should do this by only looping through the scope stack once. Nit: I would also probably have it leave a trail of markers to each scope with the variables being hoisted as it works it way to the var scope. Rather than preserving that as scopes are popped.
Created attachment 357488 [details] Patch
(In reply to Keith Miller from comment #15) > Comment on attachment 356945 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356945&action=review > > > Source/JavaScriptCore/parser/Parser.h:1248 > > + if (!canDeclareVariable(*ident)) > > + return DeclarationResult::InvalidDuplicateDeclaration; > > + > > + auto varScope = currentVariableScope(); > > + auto lexicalScope = currentLexicalDeclarationScope(); > > + if (lexicalScope != varScope) > > + lexicalScope->addVariableBeingHoisted(ident); > > + > > + return varScope->declareVariable(ident); > > I think we should do this by only looping through the scope stack once. Good call! I moved all of this logic (including `canDeclareVariable` itself) into a `declareHoistedVariable` instead. > Nit: I would also probably have it leave a trail of markers to each scope > with the variables being hoisted as it works it way to the var scope. Rather > than preserving that as scopes are popped. 👍
(That said, the name `declareHoistedVariable` may leave something to be desired. :P Maybe *hoistable* would be better? Anyway, feel free to counterpropose. )
<rdar://problem/43646682>
Comment on attachment 357488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357488&action=review r=me. > Source/JavaScriptCore/parser/Parser.h:1233 > + // FIXME: This exemption should not apply if the var declaration is a for-of initializer. Lol...
Comment on attachment 357488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357488&action=review LGTM too >> Source/JavaScriptCore/parser/Parser.h:1233 >> + // FIXME: This exemption should not apply if the var declaration is a for-of initializer. > > Lol... you should file a bug
Comment on attachment 357488 [details] Patch Clearing flags on attachment: 357488 Committed r239354: <https://trac.webkit.org/changeset/239354>
All reviewed patches have been landed. Closing bug.
(In reply to Saam Barati from comment #21) > Comment on attachment 357488 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357488&action=review > > LGTM too Thanks! > >> Source/JavaScriptCore/parser/Parser.h:1233 > >> + // FIXME: This exemption should not apply if the var declaration is a for-of initializer. > > > > Lol... > > you should file a bug Submitted bug 192830.