Bug 192298 – Redeclaration of var over let/const/class should be a syntax error.

• Component: New Bugs
• Status: RESOLVED FIXED

[Ross Kirsling]

Redeclaration of var over let/const/class should be a syntax error.

[Ross Kirsling]

Created attachment 356356 [details]
Patch

[Ross Kirsling]

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

[Yusuke Suzuki]

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?

[Yusuke Suzuki]

(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?

[Yusuke Suzuki]

(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?

[Ross Kirsling]

(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!

[Ross Kirsling]

Created attachment 356387 [details]
Patch

[Saam Barati]

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; } } } }

[Ross Kirsling]

(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.

[Saam Barati]

(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.

[Ross Kirsling]

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.

[Ross Kirsling]

Created attachment 356907 [details]
Patch

[Ross Kirsling]

Created attachment 356945 [details]
Patch

[Ross Kirsling]

(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.

[Keith Miller]

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.

[Ross Kirsling]

Created attachment 357488 [details]
Patch

[Ross Kirsling]

(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.

👍

[Ross Kirsling]

(That said, the name `declareHoistedVariable` may leave something to be desired. :P Maybe *hoistable* would be better? Anyway, feel free to counterpropose. )

[Mark Lam]

<rdar://problem/43646682>

[Keith Miller]

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...

[Saam Barati]

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

[WebKit Commit Bot]

Comment on attachment 357488 [details]
Patch

Clearing flags on attachment: 357488

Committed r239354: <https://trac.webkit.org/changeset/239354>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ross Kirsling]

(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.