Bug 31813

I would like to implement this, so if you don't mind I will use this bug for stating the requirements, coming to a strategy, and fleshing out an implementation. To summarize, there are currently two ES modes: * Strict * "Classic" (non-strict) ES.next adds a third: * "Harmony" In classic mode, `const' has no specification. In JSC it is like `var', and hoisted to function scope. In strict mode, `const' is a syntax error. In harmony mode, `const' is block-scoped, with a so-called "temporal dead zone": when the var is first made, it is logically populated with some sentinel value. If the var is accessed before it is set, that is a runtime error. If it is set twice, that is a runtime error. In some cases the compiler can elide the temporal dead zone, but not in general. There are also some wrinkles that in top-level strict contexts, `const' should create non-writable, non-configurable properties. Full details on the linked page.

So that's the spec, as it appears to be shaping up. As far as implementation goes, SpiderMonkey and V8 appear to be choosing different strategies: V8: https://mail.mozilla.org/pipermail/es-discuss/2011-November/018480.html SM: https://mail.mozilla.org/pipermail/es-discuss/2011-November/018462.html V8's approach is more conservative, preferring to add a mode rather than to add `const' to strict mode. SM is more optimistic (for better or for worse); they seem to think they can migrate their users to block-scoped `const' and everything will be fine. JSC currently forbids `const' in strict mode, and has some crufty function-scope support for `const' in classic mode. Now here's the thing, JSC currently has no harmony "mode": there's just strict and non-strict. We have to make a choice as to whether to continue like this and support some subset of harmony features within strict mode, or to add another mode, or to decide not to implement harmony, such as it is. My motivation here is that I have a bunch of SM code that uses let, const, and destructuring, and I would like to convert it to use JSC. I would appreciate feedback from JSC folk as to whether this is a good idea, or whether I should use another engine. If you are amenable to block scoping somehow, I am happy to implement it. Andy

As one data point, I think my initial strategy is going to be to not add another mode. It's probably OK to add `let' and `const' to strict mode, as SpiderMonkey does. I'll add this implementation under a compile-time flag.

(In reply to comment #1) > In harmony mode, `const' is block-scoped, with a so-called "temporal dead zone": when the var is first made, it is logically populated with some sentinel value. If the var is accessed before it is set, that is a runtime error. If it is set twice, that is a runtime error. In some cases the compiler can elide the temporal dead zone, but not in general. Please read the latest draft before you start. http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts There is no sentinel and const declaration requires an initializer. ConstDeclaration : const ConstBindingList ; ConstBindingList : ConstBinding ConstBinding List , ConstBinding ConstBinding : BindingIdentifier Initialiser BindingPattern Initialiser The draft spec does not yet cover const in for, for-in and for-of loops. The URL of this bug points to the wiki page which clearly states that there is no temporal dead zone. > There are also some wrinkles that in top-level strict contexts, `const' should create non-writable, non-configurable properties. Full details on the linked page. Also, in Harmony there is no global object like that. Global variables does not create properties on global.

I'm not sure that we want to bleed partial Harmony support into JSC - so I think we should probably keep this out of strict es 5.1 mode. That said, if we initially land this behind an ifdef, we don't necessarily need to make a decision on this from the outset, the guards will presumably still be there on the other side of the ifdef. I guess there are really three options here - always allow block const, only allow based on state external to the script, (<script> tag or command line parameter), or a "use strict" style pragma in-band. Whilst it doesn't currently sound like the ES committee are going to go for another "use strict" like directive, this might be useful for internal development purposes (e.g. "use webkit-harmony"). I doubt anyone relies on the precise behavior of our crufty function-scoped const, so it may end up making sense to replace the classic const implementation with a block-scoped one, rather than supporting multiple incompatible behaviors.

(In reply to comment #4) > Please read the latest draft before you start. > > http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts Thanks for the pointer. I still don't understand all of the tc39 process. > There is no sentinel and const declaration requires an initializer. Though the const declaration requires an initializer, there is still a sentinel AFAIU. See for example the V8 mail I linked to before, or Brendan's reply to that. > > There are also some wrinkles that in top-level strict contexts, `const' should create non-writable, non-configurable properties. Full details on the linked page. > > Also, in Harmony there is no global object like that. Global variables does not create properties on global. Well, I'll take a look again. I was basing my interpretations on es-discuss proceedings from the last month or so. Thanks for the valuable feedback, though it does make me a little less sure of what the spec is right now ;)

(In reply to comment #6) > Well, I'll take a look again. I was basing my interpretations on es-discuss proceedings from the last month or so. I stand corrected. There is still a temporal dead zone requirement for const in harmony. The reason is that you can have forward references.

Harmony (or ES.next or ES6, which are the same thing) is shaping up to be a pretty big language. There is consensus on some pieces, and some pieces are more experimental. From an implementor's perspective it is a waste of time to implement something that will change significantly in the future, so we need to identify a stable subset of Harmony to consider implementing. Given that we are looking at block-scoped const in the broader Harmony context, I thought it would be useful to look at what V8 has identified to be a stable subset, and what they are doing. V8 implements (1) harmony proxies, (2) collections (maps, sets, and weak maps), (3) typeof, and (4) block scoping (const and let). In V8, all harmony support is currently off by default. There are separate runtime flags to enable each one of these four areas. There is a master --harmony switch to enable all of them. Even when it is off, Harmony support is always compiled into V8, without an #ifdef. 1. Proxies are implemented as a separate JSReceiver implementation, JSObject being the other one. There are about 100 mentions of JSProxy in the V8 source -- so a fair amount of code, but not enormous. Proxies do have a couple of runtime helper stubs, but otherwise don't affect code generation. That's all I have to say on proxies. 2. Harmony collections (sets, maps, and weak maps) likewise don't affect the core implementation too much, as they are essentially libraries. 3. In Harmony, typeof(null) is 'null', not 'object'. This affects code generation in some places, mostly positively. I don't know how much it would break the web if it were on by default, though. 4. OK, so we are finally down to scoping. This is the most complicated piece. I have described the specification for block-scoped const in bug 31813 comment 2. V8 implements it entirely, except for `const' at toplevel. `let' is very similar to `const', except that the bindings are mutable. There is still a "temporal dead zone". I should mention first that like JSC, V8 has a legacy `const' mode. It does not reserve the `let' token in "classic" mode (see comment 1). In strict mode, it raises early errors for `const' and `let'. In harmony mode, it implements harmony scoping. Crankshaft does not handle `let' or harmony `const' yet, so using these constructs is currently a speed penalty relative to `var'. Of course the read barrier ("temporal dead zone") is also a performance lose, but probably not too significant. V8 does omit the read barrier in one case; from full-codegen-ia32.cc: // The check // can be skipped in the following situation: we have a LET or CONST // binding in harmony mode, both the Variable and the VariableProxy have // the same declaration scope (i.e. they are both in global code, in the // same function or in the same eval code) and the VariableProxy is in // the source physically located after the initializer of the variable. // // We cannot skip any initialization checks for CONST in non-harmony // mode because const variables may be declared but never initialized: // if (false) { const x; }; var y = x; // // The condition on the declaration scopes is a conservative check for // nested functions that access a binding and are called before the // binding is initialized: // function() { f(); let x = 1; function f() { x = 2; } } // Note that if you come from a functional programming backgroud, harmony `let' is more like `letrec' (or `letrec*') from other languages, hence the complications. The general strategy from "Fixing Letrec: A Faithful Yet Efficient Implementation of Scheme's Recursive Binding Construct", by Oscar Waddell, Dipanwita Sarkar, and R. Kent Dybvig could apply here. Anyway, sentinel values and read and (in the case of `const') write barriers. There's more though. In harmony mode, every call to `eval' is a strict-mode eval call. This behavior is enabled by the harmony-scoping flag. In harmony mode, you need to check for multiple declarations of the same let-bound or const-bound variable. You need to check for incompatible bindings: `var' and `let' declarations in the same scope, or `var' hoisted past a `let'. In harmony mode, a SourceElement may be not only a Statement or a FunctionDeclaration, but also a LetDeclaration or a ConstDeclaration. ConstDeclarations have non-optional initializers. Function declarations are scoped using `let' in harmony mode, and thus are block-scoped, not hoisted. For named function expressions, the closure binding between the name and the function is made using `const', in harmony mode. In harmony mode, a Block is parsed as a sequence of SourceElements, not Statements. Catch vars are bound using `let' in harmony mode. That's it, I think.

To continue a little more here, in V8 there is no support for enabling harmony mode within a script or a function. It's an outside flag that is on or off. This is easier than the strict mode case, in which the strict mode flag has to be plumbed around everywhere. It also appears to be orthogonal to strict mode, though that is probably a bug: http://code.google.com/p/v8/issues/detail?id=1829

Thanks for the notes, Gavin. (In reply to comment #5) > I guess there are really three options here - always allow block const, only allow based on state external to the script, (<script> tag or command line parameter), or a "use strict" style pragma in-band. Whilst it doesn't currently sound like the ES committee are going to go for another "use strict" like directive, this might be useful for internal development purposes (e.g. "use webkit-harmony"). I'm open to any of these. FWIW as I noted V8 went with the "external state" solution. I don't know what the right answer is. > I doubt anyone relies on the precise behavior of our crufty function-scoped const, so it may end up making sense to replace the classic const implementation with a block-scoped one, rather than supporting multiple incompatible behaviors. Again, just for what it's worth, V8 preserved the old behavior and added a new const_harmony mode. I really don't have a good idea about what enabling block const would do in a broader web context.

The plot thickens on the V8 side. Chromium hacker Steven Keuchel notes in http://code.google.com/p/v8/issues/detail?id=1829#c3 that they would like to enable some harmony features within strict mode by default, if I understand him correctly. I think I will do what they are planning to do, and just have one global flag to turn harmony on or off at runtime, and have that flag behind an #ifdef in the beginning. Then we will only recognize `let' and `const' within strict-mode code, when the harmony flag is on. We can talk about the default #ifdef and runtime flag settings later on.

Related: bug 72889 for runtime options.

FYI, V8 now has three modes: classic, strict, and "extended": http://code.google.com/p/v8/source/detail?r=10062

(In reply to comment #13) > FYI, V8 now has three modes: classic, strict, and "extended": > > http://code.google.com/p/v8/source/detail?r=10062 Given the spec defined mechanism by which strict mode is enabled (an inline directive in the source file), it's a little redundant to make this a command-line option, and would take a little extra plumbing into the parser to make this work (though not a lot to be fair, we'll probably need to tag SourceCode/SourceProvider objects with a language mode anyway if we want to support ES6).

(In reply to comment #14) > (In reply to comment #13) > > FYI, V8 now has three modes: classic, strict, and "extended": > > Given the spec defined mechanism by which strict mode is enabled (an inline directive in the source file), it's a little redundant to make this a command-line option Agreed. I think the command-line / run-time option is just to be conservative, and not enable harmony by default (yet?). A flag of some sort is still needed to control visibility of the new libraries in ES6: Map, Set, etc. In ES6 mode you would have them in the global environment, AFAIU, but not in "extended" mode.

> A flag of some sort is still needed to control visibility of the new libraries in ES6: Map, Set, etc. In ES6 mode you would have them in the global environment, AFAIU, but not in "extended" mode. Actually that might not be an issue. The plan for ES6 is that the global object won't be in scope and instead there will be a module system (it will be imported modules that are available in the outer scope in ES6 code, not the properties of the global object). As such, ES6 will likely not define that these properties exist on the global object – rather that the containing modules must exist.

Good points regarding modules. I'm going to work on this a bit more for the rest of the week and weekend (the GTK+ port has a hackfest). The tentative plan is: 1. New code is guarded by ENABLE(HARMONY) 2. There is also Options::harmony, and sub-option Options::harmonyBlockScoping, to allow runtime control, at least until things settle down 3. Adding an "extended" mode does not seem necessary yet, as the additions are not incompatible with old strict-mode code. In short, I plan to enable block-scoped `let' and `const' in strict-mode code when the global Options::harmonyBlockScoping flag is on. 4. We will have to add new binding modes, CONST_HARMONY and LET. The actual implementation mechanism is likely to be a generalization of what's used by `catch' blocks, but I don't understand everything there yet. Comments welcome.

So, a status update. I added the compile-time and run-time flags for harmony block scoping. When the flags are on, we parse `const' in strict-mode as having block scope. I check correctly for conflicting variable declarations, I think. I extended the Parser::Scope object to track block-scoped declarations. I push a scope when parsing block statements in harmony mode, and if the scope contains lexical bindings, I make a BlockScopeNode instad of a BlockStatementNode. This works but does not currently catch if (1) const x = 10; which should be an error AFAIK. Block-scoped declarations outside of functions are also collected at the top level; currently they result in lexical bindings, not read-only, non-configurable properties, but that can be fixed. Anyway, that's the parser and the AST. It used to be that ProgramNode, EvalNode, and FunctionBodyNode all descended from ScopeNode, but I changed them to descend from an interposed CodeNode, and then BlockScopeNode descends from ScopeNode but without the toplevel baggage of the others. I'm not sure if this was useful though. Implementation-wise, things are pretty terrible: I have a JSBlockScopeObject like the StaticScopeObject, but with more slots. Entering in a block scope will allocate that object and push it on the scope chain. Currently findScopedProperty punts in that case; something to fix. So the lookup is not fast. It's a shame though; in strict mode, it should be possible to do much better (with assignment conversion). There is a new VM op to create a blockscope. Perhaps that should merge with JSStaticScope. I have not yet modified either JIT. I have not yet implemented the read- or write-barriers. They will probably require additional vm ops. Unless you want the patches now, I'll clean them up before posting for review.

I have a prototype up at bug 74725. It depends on a few more patches in bugzilla; you can follow the dependencies to see. I still need to implement the temporal dead zone, figure out a better versioning strategy, and write a test suite, but much of that will probably have to wait until after the holiday season.

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

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

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

Created attachment 256546 [details] patch This currently also has: https://bugs.webkit.org/show_bug.cgi?id=142944 baked into it. This will obviously change once 142944 lands. Still need to do: - remove old tests - add new tests

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

Created attachment 256608 [details] patch This is ready to go after "let" lands. All that is left for me to do is remove old, incorrect, tests and update old tests results. New tests are written.

Created attachment 256611 [details] patch same as before but removed old code and added one new test.

Created attachment 256761 [details] WIP

Created attachment 256926 [details] patch

Comment on attachment 256926 [details] patch Attachment 256926 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5703103198265344 Number of test failures exceeded the failure limit.

Created attachment 256930 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 256926 [details] patch Attachment 256926 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5844178378424320 Number of test failures exceeded the failure limit.

Created attachment 256934 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Looks like a lot of tests use "const" as if it's a global property. I'll fix tests to switch to "var".

Created attachment 256945 [details] patch

Comment on attachment 256945 [details] patch Attachment 256945 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5409610903060480 New failing tests: inspector/console/console-table.html

Created attachment 256951 [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 on attachment 256945 [details] patch Attachment 256945 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6177052839378944 New failing tests: inspector/console/console-table.html

Created attachment 256953 [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 on attachment 256945 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=256945&action=review > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1447 > -Variable BytecodeGenerator::variable(const Identifier& property) > +std::pair<Variable, BytecodeGenerator::ScopeType> BytecodeGenerator::resolveVariable(const Identifier& property) The idea of the Variable class was to hold all of the things that this method returned. Why can't ScopeType just be another field of Variable?

Comment on attachment 256945 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=256945&action=review >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1447 >> +std::pair<Variable, BytecodeGenerator::ScopeType> BytecodeGenerator::resolveVariable(const Identifier& property) > > The idea of the Variable class was to hold all of the things that this method returned. Why can't ScopeType just be another field of Variable? There is no reason why it shouldn't. Variable's now have a notion of isLexicallyScoped(). Also, they have a notion of isConst(), where isConst() := isLexicallyScoped() && isReadOnly()

Created attachment 257039 [details] patch I'm seeing how EWS likes this as I update my machine.

Comment on attachment 257039 [details] patch Attachment 257039 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4780413226582016 Number of test failures exceeded the failure limit.

Created attachment 257040 [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 on attachment 257039 [details] patch Attachment 257039 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5796731337834496 Number of test failures exceeded the failure limit.

Created attachment 257041 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

(In reply to comment #45) > Created attachment 257041 [details] > Archive of layout-test-results from ews105 for mac-mavericks-wk2 > > The attached test failures were seen while running run-webkit-tests on the > mac-wk2-ews. > Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5 Forgetting a "return" in front of an expression will indeed make things blow up.

Created attachment 257051 [details] patch lets try this again.

landed in: http://trac.webkit.org/changeset/187012

The new tests added in this patch are incredibly slow. Can you investigate making them run faster while still testing the same things? For example, I'm not sure that running *everything* for 10000 times is particularly useful. I think that most of the interesting semantic features are in bytecode and in runtime functions that are shared by all execution engines. So, I think it would be worthwhile to reduce the running time, but write optimizing-compiler-specific tests that just cover the interesting things that happen in the optimizing JITs.

(In reply to comment #49) > The new tests added in this patch are incredibly slow. Can you investigate > making them run faster while still testing the same things? > > For example, I'm not sure that running *everything* for 10000 times is > particularly useful. I think that most of the interesting semantic features > are in bytecode and in runtime functions that are shared by all execution > engines. So, I think it would be worthwhile to reduce the running time, but > write optimizing-compiler-specific tests that just cover the interesting > things that happen in the optimizing JITs. I agree: https://bugs.webkit.org/show_bug.cgi?id=147291

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