ES6 class syntax should use block scoping

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Since we don't currently support block scoping, we should probably impose some limitations instead.

[Adam Klein]

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

[Erik Arvidsson]

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() {}
}

[Radar WebKit Bug Importer]

<rdar://problem/20125410>

[Saam Barati]

Created attachment 257696 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Geoffrey Garen]

Comment on attachment 257696 [details]
patch

Much test fail.

[Saam Barati]

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

[Saam Barati]

Created attachment 257859 [details]
patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Created attachment 257877 [details]
patch

fixing more should* nonsense.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Created attachment 257891 [details]
patch

[Geoffrey Garen]

Comment on attachment 257891 [details]
patch

r=me

[WebKit Commit Bot]

Comment on attachment 257891 [details]
patch

Clearing flags on attachment: 257891

Committed r187680: <http://trac.webkit.org/changeset/187680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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 =)

[Saam Barati]

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

[Filip Pizlo]

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

[Saam Barati]

(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

[Joseph Pecoraro]

*** Bug 151365 has been marked as a duplicate of this bug. ***