Bug 143038

Summary: ES6: Classes: Program level class statement throws exception in strict mode
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, joepeck, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 140491    
Attachments:
Description Flags
[PATCH] Proposed Fix rniwa: review+

Description Joseph Pecoraro 2015-03-24 22:04:54 PDT
* SUMMARY
Program level class expression throws exception in strict mode.

* TEST
<script>
"use strict";
class MyClass { constructor() { } };
</script>

* EXPECTED
All is fine.

* ACTUAL
[Error] ReferenceError: Can't find variable: BaseNoReturn (foo.html, line 3)
Comment 1 Joseph Pecoraro 2015-03-24 22:05:31 PDT
Err, the exception should have had the name "MyClass" instead of "BaseNoReturn". Meant to update after pasting ;)
Comment 2 Joseph Pecoraro 2015-03-24 22:22:42 PDT
In parseStatement, parseFunctionDeclaration declares a declareVariable at this top level before pushing a new scope. I wonder if parseClassDeclaration should be doing the same, putting the class name at the top level.
Comment 3 Joseph Pecoraro 2015-03-24 22:27:35 PDT
My guess is that it should. When I compare to Firefox, a class statement requires a name, and the name is put into the global scope. I'll be back later to investigate this and look up the relevant parts of the spec. I'll take this bug.
Comment 4 Joseph Pecoraro 2015-03-25 00:52:18 PDT
Actually this seems to be accomplished a different way.

My next guess is that AST ScopeNodes have a VarStack and FunctionStack. BytecodeGenerator walks these and adds Function Declaration to the UnlinkedProgramCodeBlock. Seems promising.
Comment 5 Joseph Pecoraro 2015-03-25 02:00:11 PDT
Reading the spec more, this has to deal with Lexical Environments / Environment Records. I don't fully understand the terminology of this yet.

It looks like this is meant to be the same as `let` in scoping, but we could probably get away with this as an uninitialized `var` for now. (That seems to be what Firefox does with a basic test, which returned undefined, not a ReferenceError for use of a class name above the class definition).

The special handling for functions seems to be defined in B.3.3 "Block-Level Function Declarations Web Legacy Compatibility Semantics", which doesn't seem to apply to Classes.

My current understanding is that the class statement of "class Foo { ... }" can be considered syntactic sugar for "let Foo = class Foo { ... }".
Comment 6 Joseph Pecoraro 2015-03-25 09:35:07 PDT
Created attachment 249410 [details]
[PATCH] Proposed Fix
Comment 7 Ryosuke Niwa 2015-03-25 12:26:16 PDT
Comment on attachment 249410 [details]
[PATCH] Proposed Fix

Thanks for the fix!
Comment 8 Joseph Pecoraro 2015-03-25 13:41:57 PDT
I keep re-reading the runtime semantics of class declarations:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-bindingclassdeclarationevaluation

> 14.5.15 Runtime Semantics: BindingClassDeclarationEvaluation
> 
> ClassDeclaration : class BindingIdentifier ClassTail
>     1. Let className be StringValue of BindingIdentifier.
>     2. Let value be the result of ClassDefinitionEvaluation of ClassTail with argument className.
>     3. ReturnIfAbrupt(value).
>     4. Let hasNameProperty be HasOwnProperty(value, "name").
>     5. ReturnIfAbrupt(hasNameProperty).
>     6. If hasNameProperty is false, then perform SetFunctionName(value, className).
>     7. Let env be the running execution context’s LexicalEnvironment.
>     8. Let status be InitializeBoundName(className, value, env).
>     9. ReturnIfAbrupt(status).
>     10. Return value.

So in here the InitializeBoundName -> InitializeBinding is what we are fixing.

However, I can't find anywhere that actually creates the mutable/immutable binding.

I'm trying to figure out for certain if class statements should be treated like `var` or `let`. Knowing where the binding was created would know the behavior of using the binding identifier before the evaluation of the class statement.
Comment 9 Joseph Pecoraro 2015-03-25 13:58:54 PDT
Geoff sorted this out for me!

The key is ClassDeclaration is a LexicalDeclaration and Global Declaration Initiation creates the Mutable/Immutable bindings for all LexicalDeclarations:  
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-globaldeclarationinstantiation

So, the uninitialized binding will exist, and access to it before it is initialized should throw an error. So it is correct to leave that one test failing and I'll file a bug on that.
Comment 10 Joseph Pecoraro 2015-03-25 14:36:13 PDT
> The key is ClassDeclaration is a LexicalDeclaration and Global Declaration
> Initiation creates the Mutable/Immutable bindings for all
> LexicalDeclarations:

Well, not "all", but all the ones in its scope ;)
Comment 11 Joseph Pecoraro 2015-03-25 14:36:54 PDT
http://trac.webkit.org/changeset/181973