Bug 142567

Summary: ES6 class syntax should use block scoping
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
Status: RESOLVED FIXED    
Severity: Normal CC: adamk, arv, buildbot, commit-queue, danfuzz, fpizlo, joepeck, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142944    
Bug Blocks: 140491    
Attachments:
Description Flags
patch
ggaren: review-, buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
patch
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
patch none

Description Ryosuke Niwa 2015-03-10 21:59:03 PDT
Since we don't currently support block scoping, we should probably impose some limitations instead.
Comment 1 Adam Klein 2015-03-11 12:25:48 PDT
A case to be particularly worried about is:

(function() {
  console.log(MyClass);
  class MyClass {}
})()

should throw a ReferenceError, rather than "undefined" (or even worse, not undefined; class _definitely_ shouldn't get hoisted).
Comment 2 Erik Arvidsson 2015-03-11 12:42:09 PDT
Awesome

My main concern is hoisting:

<script>
class B extends A {}
class A {}
</script>

Another thing that I think you mentioned would be to only allow class expressions to start with.

There are some subtleties regarding TDZ that you might want to keep in mind. All of these needs to have TDZ or you would be able to inspect a partially initialized constructor/prototype

class C extends func(C, Base) {}
class C {
  a() {}
  [func(C)]() {}
  b() {}
}
Comment 3 Radar WebKit Bug Importer 2015-03-11 13:17:50 PDT
<rdar://problem/20125410>
Comment 4 Saam Barati 2015-07-28 15:50:36 PDT
Created attachment 257696 [details]
patch
Comment 5 Build Bot 2015-07-28 16:24:24 PDT
Comment on attachment 257696 [details]
patch

Attachment 257696 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5460900731420672

New failing tests:
js/class-syntax-extends.html
js/class-syntax-name.html
js/class-syntax-call.html
js/class-syntax-default-constructor.html
js/class-syntax-declaration.html
js/class-syntax-super.html
js/class-constructor-return.html
Comment 6 Build Bot 2015-07-28 16:24:28 PDT
Created attachment 257699 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-07-28 16:29:49 PDT
Comment on attachment 257696 [details]
patch

Attachment 257696 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5999528285044736

New failing tests:
js/class-syntax-extends.html
js/class-syntax-name.html
js/class-syntax-call.html
js/class-syntax-default-constructor.html
js/class-syntax-declaration.html
js/class-syntax-super.html
js/class-constructor-return.html
Comment 8 Build Bot 2015-07-28 16:29:52 PDT
Created attachment 257700 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 9 Geoffrey Garen 2015-07-28 17:23:15 PDT
Comment on attachment 257696 [details]
patch

Much test fail.
Comment 10 Saam Barati 2015-07-28 17:38:44 PDT
(In reply to comment #9)
> Comment on attachment 257696 [details]
> patch
> 
> Much test fail.

Yep. Fixing locally. 

This has to do with the functions
"shouldBe", "shouldThrow", "shouldBeTrue", etc
all being evaluated in another file. That file
won't have access to the block scoped variables defined in
the actual test file. I think we need to consider other ways
of testing going forward because these functions make less sense
with block scoping. I ran into similar problems when I implemented
ES6 const semantics.
Comment 11 Saam Barati 2015-07-30 15:25:21 PDT
Created attachment 257859 [details]
patch
Comment 12 Build Bot 2015-07-30 16:25:41 PDT
Comment on attachment 257859 [details]
patch

Attachment 257859 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/611

New failing tests:
js/intl-numberformat.html
js/intl-datetimeformat.html
js/intl-collator.html
Comment 13 Build Bot 2015-07-30 16:25:45 PDT
Created attachment 257869 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 14 Build Bot 2015-07-30 16:35:22 PDT
Comment on attachment 257859 [details]
patch

Attachment 257859 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/634

New failing tests:
js/intl-numberformat.html
js/intl-datetimeformat.html
js/intl-collator.html
Comment 15 Build Bot 2015-07-30 16:35:25 PDT
Created attachment 257874 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 16 Saam Barati 2015-07-30 16:56:35 PDT
Created attachment 257877 [details]
patch

fixing more should* nonsense.
Comment 17 Build Bot 2015-07-30 17:29:41 PDT
Comment on attachment 257877 [details]
patch

Attachment 257877 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/888

New failing tests:
js/intl-numberformat.html
js/intl-datetimeformat.html
js/intl-collator.html
Comment 18 Build Bot 2015-07-30 17:29:44 PDT
Created attachment 257884 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 19 Build Bot 2015-07-30 17:38:56 PDT
Comment on attachment 257877 [details]
patch

Attachment 257877 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/904

New failing tests:
js/intl-numberformat.html
js/intl-datetimeformat.html
js/intl-collator.html
Comment 20 Build Bot 2015-07-30 17:39:01 PDT
Created attachment 257888 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 21 Saam Barati 2015-07-30 18:15:56 PDT
Created attachment 257891 [details]
patch
Comment 22 Geoffrey Garen 2015-07-31 13:14:01 PDT
Comment on attachment 257891 [details]
patch

r=me
Comment 23 WebKit Commit Bot 2015-07-31 14:05:27 PDT
Comment on attachment 257891 [details]
patch

Clearing flags on attachment: 257891

Committed r187680: <http://trac.webkit.org/changeset/187680>
Comment 24 WebKit Commit Bot 2015-07-31 14:05:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Joseph Pecoraro 2015-07-31 14:19:26 PDT
Comment on attachment 257891 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257891&action=review

> LayoutTests/js/class-constructor-return-expected.txt:57
> -PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2 is false
> -PASS (new DerivedReturnObject2) === globalVariable is true
> -PASS (new DerivedReturnString) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> -PASS (new DerivedReturnNumber) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> -PASS (new DerivedReturnNull) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> -PASS (new DerivedReturnSymbol) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> -PASS (new DerivedThrow) threw exception Thrown Exception String.
> +PASS (new DerivedNoReturn) instanceof DerivedNoReturn
> +PASS (new DerivedReturnImplicit) instanceof DerivedReturnImplicit
> +PASS (new DerivedReturnImplicit) !== undefined
> +PASS (new DerivedReturnUndefined) instanceof DerivedReturnUndefined
> +PASS (new DerivedReturnUndefined) !== undefined
> +PASS (new DerivedReturnThis) instanceof DerivedReturnThis
> +PASS (new DerivedReturnObject) instanceof DerivedReturnObject
> +PASS typeof (new DerivedReturnObject) === "object"
> +PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2

I feel like we loose a lot of information with the new output.

For instance:

    -PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2 is false
    +PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2

It is not clear from the second line that the expected result is false. It actually looks like we expect it to be true.

> LayoutTests/js/class-constructor-return-expected.txt:-67
> -PASS (new DerivedNoSuperNoReturn) threw exception ReferenceError: Cannot access uninitialized variable..
> -PASS (new DerivedNoSuperReturnImplicit) threw exception ReferenceError: Can't find variable: DerivedNoSuperReturnImplicit.

Likewise in these tests, there are different exceptions, or even did not throw exception, the new output it is unclear what the expected results should be from the output. What if the exception type changes, how would we know?

Any way we can have some of the better output in the tests? I'm not sure I understand why it needed to change at all =)
Comment 26 Saam Barati 2015-07-31 20:42:25 PDT
(In reply to comment #25)
> Comment on attachment 257891 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257891&action=review
> 
> > LayoutTests/js/class-constructor-return-expected.txt:57
> > -PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2 is false
> > -PASS (new DerivedReturnObject2) === globalVariable is true
> > -PASS (new DerivedReturnString) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> > -PASS (new DerivedReturnNumber) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> > -PASS (new DerivedReturnNull) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> > -PASS (new DerivedReturnSymbol) threw exception TypeError: Cannot return a non-object type in the constructor of a derived class..
> > -PASS (new DerivedThrow) threw exception Thrown Exception String.
> > +PASS (new DerivedNoReturn) instanceof DerivedNoReturn
> > +PASS (new DerivedReturnImplicit) instanceof DerivedReturnImplicit
> > +PASS (new DerivedReturnImplicit) !== undefined
> > +PASS (new DerivedReturnUndefined) instanceof DerivedReturnUndefined
> > +PASS (new DerivedReturnUndefined) !== undefined
> > +PASS (new DerivedReturnThis) instanceof DerivedReturnThis
> > +PASS (new DerivedReturnObject) instanceof DerivedReturnObject
> > +PASS typeof (new DerivedReturnObject) === "object"
> > +PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2
> 
> I feel like we loose a lot of information with the new output.
> 
> For instance:
> 
>     -PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2 is false
>     +PASS (new DerivedReturnObject2) instanceof DerivedReturnObject2
> 
> It is not clear from the second line that the expected result is false. It
> actually looks like we expect it to be true.
> 
> > LayoutTests/js/class-constructor-return-expected.txt:-67
> > -PASS (new DerivedNoSuperNoReturn) threw exception ReferenceError: Cannot access uninitialized variable..
> > -PASS (new DerivedNoSuperReturnImplicit) threw exception ReferenceError: Can't find variable: DerivedNoSuperReturnImplicit.
> 
> Likewise in these tests, there are different exceptions, or even did not
> throw exception, the new output it is unclear what the expected results
> should be from the output. What if the exception type changes, how would we
> know?
You're right. We should have better output here. We should really introduce some
new functions that require sensible messages so we can pass them to testPassed/testFailed. 

> Any way we can have some of the better output in the tests? I'm not sure I
> understand why it needed to change at all =)
ES6 Block scoped variables don't live on the global
object. This means they're scoped to the "Program" (read JS file/script tag)
they're defined in or the EvalProgram they're defined in.
This means a block scope variable in the top level scope of
one JS file is not visible in another JS file. In this case,
the should* functions in JS-test-pre can't see the classes 
defined in the actual test file. My suggestion is for us to start
using some should* functions that don't rely on eval at all.
Comment 27 Filip Pizlo 2015-07-31 23:21:54 PDT
(In reply to comment #23)
> Comment on attachment 257891 [details]
> patch
> 
> Clearing flags on attachment: 257891
> 
> Committed r187680: <http://trac.webkit.org/changeset/187680>

This test is noisy:

stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot declare a class twice: 'A'.
stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot declare a class twice: 'A'.
stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot declare a class twice: 'A'.
stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot declare a class twice: 'A'.
stress/class-syntax-definition-semantics.js.default: SyntaxError: Unexpected keyword 'class'. 'class' declaration is not directly within a block statement.

Can we make it quiet?
Comment 28 Saam Barati 2015-08-01 08:28:05 PDT
(In reply to comment #27)
> (In reply to comment #23)
> > Comment on attachment 257891 [details]
> > patch
> > 
> > Clearing flags on attachment: 257891
> > 
> > Committed r187680: <http://trac.webkit.org/changeset/187680>
> 
> This test is noisy:
> 
> stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot
> declare a class twice: 'A'.
> stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot
> declare a class twice: 'A'.
> stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot
> declare a class twice: 'A'.
> stress/class-syntax-definition-semantics.js.default: SyntaxError: Cannot
> declare a class twice: 'A'.
> stress/class-syntax-definition-semantics.js.default: SyntaxError: Unexpected
> keyword 'class'. 'class' declaration is not directly within a block
> statement.
> 
> Can we make it quiet?

Sorry about that, I unintentionally left in a print statement.
Fixed in:
http://trac.webkit.org/changeset/187717
Comment 29 Joseph Pecoraro 2016-05-05 22:28:23 PDT
*** Bug 151365 has been marked as a duplicate of this bug. ***