From 6ac4de87a8b01500b066a3e401e921d2d84b42db Mon Sep 17 00:00:00 2001 From: dslomov Date: Wed, 26 Nov 2014 03:21:09 -0800 Subject: [PATCH] harmony-scoping: make assignment to 'const' a late error. Per TC39 Nov 2014 decision. This patch also changes behavior for "legacy const": assignments to sloppy const in strict mode is now also a type error. This fixes v8:2243 and also brings us in compliance with other engines re assignment to function names (see updated webkit test), but might have bigger implications. That change can easily be reverted by changing Variable::IsSignallingAssignmentToConst. BUG=v8:3713,v8:2243 LOG=N Review URL: https://codereview.chromium.org/749633002 Cr-Commit-Position: refs/heads/master@{#25516} --- src/arm/full-codegen-arm.cc | 3 +- src/arm64/full-codegen-arm64.cc | 3 +- src/compiler/ast-graph-builder.cc | 32 ++++++++++++++++++---- src/compiler/ast-graph-builder.h | 1 + src/full-codegen.h | 13 +++++++++ src/hydrogen.cc | 10 +++++-- src/ia32/full-codegen-ia32.cc | 4 +-- src/runtime/runtime-scopes.cc | 8 ++++++ src/runtime/runtime.h | 1 + src/scopes.cc | 15 ---------- src/x64/full-codegen-x64.cc | 3 +- test/mjsunit/harmony/block-const-assign.js | 29 ++++++++++++-------- test/mjsunit/harmony/classes.js | 16 +++++------ test/mjsunit/harmony/module-linking.js | 4 +-- test/mjsunit/harmony/regress/regress-2243.js | 4 +-- test/mjsunit/regress/regress-2506.js | 2 +- test/webkit/fast/js/basic-strict-mode-expected.txt | 2 +- 17 files changed, 96 insertions(+), 54 deletions(-) diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index fbf3fec..3dc5420 100644 --- a/src/arm/full-codegen-arm.cc +++ b/src/arm/full-codegen-arm.cc @@ -2685,8 +2685,9 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, Token::Value op) { } EmitStoreToStackLocalOrContextSlot(var, location); } + } else if (IsSignallingAssignmentToConst(var, op, strict_mode())) { + __ CallRuntime(Runtime::kThrowConstAssignError, 0); } - // Non-initializing assignments to consts are ignored. } diff --git a/src/arm64/full-codegen-arm64.cc b/src/arm64/full-codegen-arm64.cc index 710956f..0d3d34b 100644 --- a/src/arm64/full-codegen-arm64.cc +++ b/src/arm64/full-codegen-arm64.cc @@ -2373,8 +2373,9 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, } EmitStoreToStackLocalOrContextSlot(var, location); } + } else if (IsSignallingAssignmentToConst(var, op, strict_mode())) { + __ CallRuntime(Runtime::kThrowConstAssignError, 0); } - // Non-initializing assignments to consts are ignored. } diff --git a/src/compiler/ast-graph-builder.cc b/src/compiler/ast-graph-builder.cc index e2d8aab..afe331d 100644 --- a/src/compiler/ast-graph-builder.cc +++ b/src/compiler/ast-graph-builder.cc @@ -2020,7 +2020,12 @@ Node* AstGraphBuilder::BuildVariableAssignment( value = BuildHoleCheckSilent(current, value, current); } } else if (mode == CONST_LEGACY && op != Token::INIT_CONST_LEGACY) { - // Non-initializing assignments to legacy const is ignored. + // Non-initializing assignments to legacy const is + // - exception in strict mode. + // - ignored in sloppy mode. + if (strict_mode() == STRICT) { + return BuildThrowConstAssignError(bailout_id); + } return value; } else if (mode == LET && op != Token::INIT_LET) { // Perform an initialization check for let declared variables. @@ -2034,8 +2039,8 @@ Node* AstGraphBuilder::BuildVariableAssignment( value = BuildHoleCheckThrow(current, variable, value, bailout_id); } } else if (mode == CONST && op != Token::INIT_CONST) { - // All assignments to const variables are early errors. - UNREACHABLE(); + // Non-initializing assignments to const is exception in all modes. + return BuildThrowConstAssignError(bailout_id); } environment()->Bind(variable, value); return value; @@ -2049,7 +2054,12 @@ Node* AstGraphBuilder::BuildVariableAssignment( Node* current = NewNode(op, current_context()); value = BuildHoleCheckSilent(current, value, current); } else if (mode == CONST_LEGACY && op != Token::INIT_CONST_LEGACY) { - // Non-initializing assignments to legacy const is ignored. + // Non-initializing assignments to legacy const is + // - exception in strict mode. + // - ignored in sloppy mode. + if (strict_mode() == STRICT) { + return BuildThrowConstAssignError(bailout_id); + } return value; } else if (mode == LET && op != Token::INIT_LET) { // Perform an initialization check for let declared variables. @@ -2058,8 +2068,8 @@ Node* AstGraphBuilder::BuildVariableAssignment( Node* current = NewNode(op, current_context()); value = BuildHoleCheckThrow(current, variable, value, bailout_id); } else if (mode == CONST && op != Token::INIT_CONST) { - // All assignments to const variables are early errors. - UNREACHABLE(); + // Non-initializing assignments to const is exception in all modes. + return BuildThrowConstAssignError(bailout_id); } const Operator* op = javascript()->StoreContext(depth, variable->index()); return NewNode(op, current_context(), value); @@ -2153,6 +2163,16 @@ Node* AstGraphBuilder::BuildThrowReferenceError(Variable* variable, } +Node* AstGraphBuilder::BuildThrowConstAssignError(BailoutId bailout_id) { + // TODO(mstarzinger): Should be unified with the VisitThrow implementation. + const Operator* op = + javascript()->CallRuntime(Runtime::kThrowConstAssignError, 0); + Node* call = NewNode(op); + PrepareFrameState(call, bailout_id); + return call; +} + + Node* AstGraphBuilder::BuildBinaryOp(Node* left, Node* right, Token::Value op) { const Operator* js_op; switch (op) { diff --git a/src/compiler/ast-graph-builder.h b/src/compiler/ast-graph-builder.h index a785d2f..a2cd26f 100644 --- a/src/compiler/ast-graph-builder.h +++ b/src/compiler/ast-graph-builder.h @@ -100,6 +100,7 @@ class AstGraphBuilder : public StructuredGraphBuilder, public AstVisitor { // Builders for error reporting at runtime. Node* BuildThrowReferenceError(Variable* var, BailoutId bailout_id); + Node* BuildThrowConstAssignError(BailoutId bailout_id); // Builders for dynamic hole-checks at runtime. Node* BuildHoleCheckSilent(Node* value, Node* for_hole, Node* not_hole); diff --git a/src/full-codegen.h b/src/full-codegen.h index 73a6c41..1439942 100644 --- a/src/full-codegen.h +++ b/src/full-codegen.h @@ -586,6 +586,19 @@ class FullCodeGenerator: public AstVisitor { // is expected in the accumulator. void EmitAssignment(Expression* expr); + // Shall an error be thrown if assignment with 'op' operation is perfomed + // on this variable in given language mode? + static bool IsSignallingAssignmentToConst(Variable* var, Token::Value op, + StrictMode strict_mode) { + if (var->mode() == CONST) return op != Token::INIT_CONST; + + if (var->mode() == CONST_LEGACY) { + return strict_mode == STRICT && op != Token::INIT_CONST_LEGACY; + } + + return false; + } + // Complete a variable assignment. The right-hand-side value is expected // in the accumulator. void EmitVariableAssignment(Variable* var, diff --git a/src/hydrogen.cc b/src/hydrogen.cc index 49d379a..1d33bfa 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -6647,6 +6647,9 @@ void HOptimizedGraphBuilder::HandleCompoundAssignment(Assignment* expr) { if (var->mode() == CONST_LEGACY) { return Bailout(kUnsupportedConstCompoundAssignment); } + if (var->mode() == CONST) { + return Bailout(kNonInitializerAssignmentToConst); + } BindIfLive(var, Top()); break; @@ -6673,9 +6676,7 @@ void HOptimizedGraphBuilder::HandleCompoundAssignment(Assignment* expr) { mode = HStoreContextSlot::kCheckDeoptimize; break; case CONST: - // This case is checked statically so no need to - // perform checks here - UNREACHABLE(); + return Bailout(kNonInitializerAssignmentToConst); case CONST_LEGACY: return ast_context()->ReturnValue(Pop()); default: @@ -10215,6 +10216,9 @@ void HOptimizedGraphBuilder::VisitCountOperation(CountOperation* expr) { if (var->mode() == CONST_LEGACY) { return Bailout(kUnsupportedCountOperationWithConst); } + if (var->mode() == CONST) { + return Bailout(kNonInitializerAssignmentToConst); + } // Argument of the count operation is a variable, not a property. DCHECK(prop == NULL); CHECK_ALIVE(VisitForValue(target)); diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index 672b46b..1ba4095 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -2575,7 +2575,6 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, __ CallRuntime(Runtime::kThrowReferenceError, 1); __ bind(&assign); EmitStoreToStackLocalOrContextSlot(var, location); - } else if (!var->is_const_mode() || op == Token::INIT_CONST) { if (var->IsLookupSlot()) { // Assignment to var. @@ -2597,8 +2596,9 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, } EmitStoreToStackLocalOrContextSlot(var, location); } + } else if (IsSignallingAssignmentToConst(var, op, strict_mode())) { + __ CallRuntime(Runtime::kThrowConstAssignError, 0); } - // Non-initializing assignments to consts are ignored. } diff --git a/src/runtime/runtime-scopes.cc b/src/runtime/runtime-scopes.cc index e03b850..e6d498b 100644 --- a/src/runtime/runtime-scopes.cc +++ b/src/runtime/runtime-scopes.cc @@ -22,6 +22,14 @@ static Object* ThrowRedeclarationError(Isolate* isolate, Handle name) { } +RUNTIME_FUNCTION(Runtime_ThrowConstAssignError) { + HandleScope scope(isolate); + THROW_NEW_ERROR_RETURN_FAILURE( + isolate, + NewTypeError("harmony_const_assign", HandleVector(NULL, 0))); +} + + // May throw a RedeclarationError. static Object* DeclareGlobals(Isolate* isolate, Handle global, Handle name, Handle value, diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index 481cf43..2628d25 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -500,6 +500,7 @@ namespace internal { F(ReThrow, 1, 1) \ F(ThrowReferenceError, 1, 1) \ F(ThrowNotDateError, 0, 1) \ + F(ThrowConstAssignError, 0, 1) \ F(StackGuard, 0, 1) \ F(Interrupt, 0, 1) \ F(PromoteScheduledException, 0, 1) \ diff --git a/src/scopes.cc b/src/scopes.cc index bfadc21..65a91fc 100644 --- a/src/scopes.cc +++ b/src/scopes.cc @@ -1093,21 +1093,6 @@ bool Scope::ResolveVariable(CompilationInfo* info, VariableProxy* proxy, DCHECK(var != NULL); if (proxy->is_assigned()) var->set_maybe_assigned(); - if (FLAG_harmony_scoping && strict_mode() == STRICT && - var->is_const_mode() && proxy->is_assigned()) { - // Assignment to const. Throw a syntax error. - MessageLocation location( - info->script(), proxy->position(), proxy->position()); - Isolate* isolate = info->isolate(); - Factory* factory = isolate->factory(); - Handle array = factory->NewJSArray(0); - Handle error; - MaybeHandle maybe_error = - factory->NewSyntaxError("harmony_const_assign", array); - if (maybe_error.ToHandle(&error)) isolate->Throw(*error, &location); - return false; - } - if (FLAG_harmony_modules) { bool ok; #ifdef DEBUG diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index ac31cfd..24747ee 100644 --- a/src/x64/full-codegen-x64.cc +++ b/src/x64/full-codegen-x64.cc @@ -2596,8 +2596,9 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, } EmitStoreToStackLocalOrContextSlot(var, location); } + } else if (IsSignallingAssignmentToConst(var, op, strict_mode())) { + __ CallRuntime(Runtime::kThrowConstAssignError, 0); } - // Non-initializing assignments to consts are ignored. } diff --git a/test/mjsunit/harmony/block-const-assign.js b/test/mjsunit/harmony/block-const-assign.js index b71729e..c21a0a3 100644 --- a/test/mjsunit/harmony/block-const-assign.js +++ b/test/mjsunit/harmony/block-const-assign.js @@ -35,61 +35,61 @@ // Function local const. function constDecl0(use) { - return "(function() { const constvar = 1; " + use + "; });"; + return "(function() { const constvar = 1; " + use + "; })();"; } function constDecl1(use) { - return "(function() { " + use + "; const constvar = 1; });"; + return "(function() { " + use + "; const constvar = 1; })();"; } // Function local const, assign from eval. function constDecl2(use) { - use = "eval('(function() { " + use + " })')"; + use = "eval('(function() { " + use + " })')()"; return "(function() { const constvar = 1; " + use + "; })();"; } function constDecl3(use) { - use = "eval('(function() { " + use + " })')"; + use = "eval('(function() { " + use + " })')()"; return "(function() { " + use + "; const constvar = 1; })();"; } // Block local const. function constDecl4(use) { - return "(function() { { const constvar = 1; " + use + "; } });"; + return "(function() { { const constvar = 1; " + use + "; } })();"; } function constDecl5(use) { - return "(function() { { " + use + "; const constvar = 1; } });"; + return "(function() { { " + use + "; const constvar = 1; } })();"; } // Block local const, assign from eval. function constDecl6(use) { - use = "eval('(function() {" + use + "})')"; + use = "eval('(function() {" + use + "})')()"; return "(function() { { const constvar = 1; " + use + "; } })();"; } function constDecl7(use) { - use = "eval('(function() {" + use + "})')"; + use = "eval('(function() {" + use + "})')()"; return "(function() { { " + use + "; const constvar = 1; } })();"; } // Function expression name. function constDecl8(use) { - return "(function constvar() { " + use + "; });"; + return "(function constvar() { " + use + "; })();"; } // Function expression name, assign from eval. function constDecl9(use) { - use = "eval('(function(){" + use + "})')"; + use = "eval('(function(){" + use + "})')()"; return "(function constvar() { " + use + "; })();"; } @@ -104,6 +104,7 @@ let decls = [ constDecl0, constDecl8, constDecl9 ]; +let declsForTDZ = new Set([constDecl1, constDecl3, constDecl5, constDecl7]); let uses = [ 'constvar = 1;', 'constvar += 1;', '++constvar;', @@ -116,7 +117,13 @@ function Test(d,u) { print(d(u)); eval(d(u)); } catch (e) { - assertInstanceof(e, SyntaxError); + if (declsForTDZ.has(d) && u !== uses[0]) { + // In these cases, read of a const variable occurs + // before a write to it, so TDZ kicks in before const check. + assertInstanceof(e, ReferenceError); + return; + } + assertInstanceof(e, TypeError); assertTrue(e.toString().indexOf("Assignment to constant variable") >= 0); return; } diff --git a/test/mjsunit/harmony/classes.js b/test/mjsunit/harmony/classes.js index 416c911..4291770 100644 --- a/test/mjsunit/harmony/classes.js +++ b/test/mjsunit/harmony/classes.js @@ -683,14 +683,14 @@ function assertAccessorDescriptor(object, name) { (function TestNameBindingConst() { - assertThrows('class C { constructor() { C = 42; } }', SyntaxError); - assertThrows('(class C { constructor() { C = 42; } })', SyntaxError); - assertThrows('class C { m() { C = 42; } }', SyntaxError); - assertThrows('(class C { m() { C = 42; } })', SyntaxError); - assertThrows('class C { get x() { C = 42; } }', SyntaxError); - assertThrows('(class C { get x() { C = 42; } })', SyntaxError); - assertThrows('class C { set x(_) { C = 42; } }', SyntaxError); - assertThrows('(class C { set x(_) { C = 42; } })', SyntaxError); + assertThrows('class C { constructor() { C = 42; } }; new C();', TypeError); + assertThrows('new (class C { constructor() { C = 42; } })', TypeError); + assertThrows('class C { m() { C = 42; } }; new C().m()', TypeError); + assertThrows('new (class C { m() { C = 42; } }).m()', TypeError); + assertThrows('class C { get x() { C = 42; } }; new C().x', TypeError); + assertThrows('(new (class C { get x() { C = 42; } })).x', TypeError); + assertThrows('class C { set x(_) { C = 42; } }; new C().x = 15;', TypeError); + assertThrows('(new (class C { set x(_) { C = 42; } })).x = 15;', TypeError); })(); diff --git a/test/mjsunit/harmony/module-linking.js b/test/mjsunit/harmony/module-linking.js index 3c0f18c..3a5bc89 100644 --- a/test/mjsunit/harmony/module-linking.js +++ b/test/mjsunit/harmony/module-linking.js @@ -109,7 +109,7 @@ module R { assertThrows(function() { l }, ReferenceError) assertThrows(function() { R.l }, ReferenceError) - assertThrows(function() { eval("c = -1") }, SyntaxError) + assertThrows(function() { eval("c = -1") }, TypeError) assertThrows(function() { R.c = -2 }, TypeError) // Initialize first bunch of variables. @@ -129,7 +129,7 @@ module R { assertEquals(-4, R.v = -4) assertEquals(-3, l = -3) assertEquals(-4, R.l = -4) - assertThrows(function() { eval("c = -3") }, SyntaxError) + assertThrows(function() { eval("c = -3") }, TypeError) assertThrows(function() { R.c = -4 }, TypeError) assertEquals(-4, v) diff --git a/test/mjsunit/harmony/regress/regress-2243.js b/test/mjsunit/harmony/regress/regress-2243.js index 31c2e55..e2411d2 100644 --- a/test/mjsunit/harmony/regress/regress-2243.js +++ b/test/mjsunit/harmony/regress/regress-2243.js @@ -27,5 +27,5 @@ // Flags: --harmony-scoping -assertThrows("'use strict'; (function f() { f = 123; })", SyntaxError); -assertThrows("(function f() { 'use strict'; f = 123; })", SyntaxError); +assertThrows("'use strict'; (function f() { f = 123; })()", TypeError); +assertThrows("(function f() { 'use strict'; f = 123; })()", TypeError); diff --git a/test/mjsunit/regress/regress-2506.js b/test/mjsunit/regress/regress-2506.js index ee84392..0eb2770 100644 --- a/test/mjsunit/regress/regress-2506.js +++ b/test/mjsunit/regress/regress-2506.js @@ -46,7 +46,7 @@ for (const x in [1,2,3]) { } assertEquals("012", s); -assertThrows("'use strict'; for (const x in [1,2,3]) { x++ }", SyntaxError); +assertThrows("'use strict'; for (const x in [1,2,3]) { x++ }", TypeError); // Function scope (function() { diff --git a/test/webkit/fast/js/basic-strict-mode-expected.txt b/test/webkit/fast/js/basic-strict-mode-expected.txt index 45f71bf..0e6228e 100644 --- a/test/webkit/fast/js/basic-strict-mode-expected.txt +++ b/test/webkit/fast/js/basic-strict-mode-expected.txt @@ -129,7 +129,7 @@ PASS (function (){ 'use strict'; delete someDeclaredGlobal;}) threw exception Sy PASS (function(){(function (){ 'use strict'; delete someDeclaredGlobal;})}) threw exception SyntaxError: Delete of an unqualified identifier in strict mode.. PASS 'use strict'; if (0) { someGlobal = 'Shouldn\'t be able to assign this.'; }; true; is true PASS 'use strict'; someGlobal = 'Shouldn\'t be able to assign this.'; threw exception ReferenceError: someGlobal is not defined. -FAIL 'use strict'; (function f(){ f = 'shouldn\'t be able to assign to function expression name'; })() should throw an exception. Was undefined. +PASS 'use strict'; (function f(){ f = 'shouldn\'t be able to assign to function expression name'; })() threw exception TypeError: Assignment to constant variable.. PASS 'use strict'; eval('var introducedVariable = "FAIL: variable introduced into containing scope";'); introducedVariable threw exception ReferenceError: introducedVariable is not defined. PASS 'use strict'; objectWithReadonlyProperty.prop = 'fail' threw exception TypeError: Cannot assign to read only property 'prop' of #. PASS 'use strict'; delete objectWithReadonlyProperty.prop threw exception TypeError: Cannot delete property 'prop' of #. -- 2.7.4