Bug 192298

Summary: Redeclaration of var over let/const/class should be a syntax error.
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: New BugsAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews, keith_miller, leo, mark.lam, msaboff, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=192830
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Ross Kirsling 2018-12-02 23:11:50 PST
Redeclaration of var over let/const/class should be a syntax error.
Comment 1 Ross Kirsling 2018-12-02 23:25:38 PST
Created attachment 356356 [details]
Patch
Comment 3 Yusuke Suzuki 2018-12-03 00:57:07 PST
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?
Comment 4 Yusuke Suzuki 2018-12-03 00:57:32 PST
(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?
Comment 5 Yusuke Suzuki 2018-12-03 00:58:04 PST
(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?
Comment 6 Ross Kirsling 2018-12-03 01:03:21 PST
(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!
Comment 7 Ross Kirsling 2018-12-03 11:10:23 PST
Created attachment 356387 [details]
Patch
Comment 8 Saam Barati 2018-12-03 14:57:22 PST
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; } } } }
Comment 9 Ross Kirsling 2018-12-04 16:38:17 PST
(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.
Comment 10 Saam Barati 2018-12-05 13:28:59 PST
(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.
Comment 11 Ross Kirsling 2018-12-08 21:35:21 PST
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.
Comment 12 Ross Kirsling 2018-12-08 22:00:02 PST
Created attachment 356907 [details]
Patch
Comment 13 Ross Kirsling 2018-12-09 21:34:12 PST
Created attachment 356945 [details]
Patch
Comment 14 Ross Kirsling 2018-12-09 21:34:50 PST
(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 15 Keith Miller 2018-12-17 12:53:54 PST
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.
Comment 16 Ross Kirsling 2018-12-17 15:48:44 PST
Created attachment 357488 [details]
Patch
Comment 17 Ross Kirsling 2018-12-17 15:51:31 PST
(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.

👍
Comment 18 Ross Kirsling 2018-12-17 16:11:19 PST
(That said, the name `declareHoistedVariable` may leave something to be desired. :P Maybe *hoistable* would be better? Anyway, feel free to counterpropose. )
Comment 19 Mark Lam 2018-12-18 12:39:20 PST
<rdar://problem/43646682>
Comment 20 Keith Miller 2018-12-18 13:38:12 PST
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 21 Saam Barati 2018-12-18 13:56:30 PST
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 22 WebKit Commit Bot 2018-12-18 14:14:28 PST
Comment on attachment 357488 [details]
Patch

Clearing flags on attachment: 357488

Committed r239354: <https://trac.webkit.org/changeset/239354>
Comment 23 WebKit Commit Bot 2018-12-18 14:14:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Ross Kirsling 2018-12-18 14:40:06 PST
(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.