Throw errors when assigning to const variables inside `with`

This code appears to have been wrong forever, as it only
threw in strict mode (presumably predating ES2015 const).

In order to get exactly the right behavior, special
handling of sloppy named function expressions is required.
Rather than polluting PropertyAttributes with another
dummy value, this CL simply adds a bool output argument
to Context::Lookup to indicate that case.

Bug: v8:6677
Change-Id: I34daa5080d291808f10cbaefc91d716f0b22963b
Reviewed-on: https://chromium-review.googlesource.com/602690Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47207}

src/contexts.cc

@@ -199,7 +199,8 @@ static PropertyAttributes GetAttributesForMode(VariableMode mode) {
 Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
                                int* index, PropertyAttributes* attributes,
                                InitializationFlag* init_flag,
-                               VariableMode* variable_mode) {
+                               VariableMode* variable_mode,
+                               bool* is_sloppy_function_name) {
   Isolate* isolate = GetIsolate();
   Handle<Context> context(this, isolate);
 
@@ -209,6 +210,9 @@ Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
   *attributes = ABSENT;
   *init_flag = kCreatedInitialized;
   *variable_mode = VAR;
+  if (is_sloppy_function_name != nullptr) {
+    *is_sloppy_function_name = false;
+  }
 
   if (FLAG_trace_contexts) {
     PrintF("Context::Lookup(");
@@ -343,6 +347,10 @@ Handle<Object> Context::Lookup(Handle<String> name, ContextLookupFlags flags,
           *attributes = READ_ONLY;
           *init_flag = kCreatedInitialized;
           *variable_mode = CONST;
+          if (is_sloppy_function_name != nullptr &&
+              is_sloppy(scope_info->language_mode())) {
+            *is_sloppy_function_name = true;
+          }
           return context;
         }
       }

src/contexts.h

@@ -674,7 +674,8 @@ class Context: public FixedArray {
   Handle<Object> Lookup(Handle<String> name, ContextLookupFlags flags,
                         int* index, PropertyAttributes* attributes,
                         InitializationFlag* init_flag,
-                        VariableMode* variable_mode);
+                        VariableMode* variable_mode,
+                        bool* is_sloppy_function_name = nullptr);
 
   // Code generation support.
   static int SlotOffset(int index) {

src/messages.h

@@ -476,7 +476,6 @@ class ErrorUtils : public AllStatic {
   T(SharedArrayBufferSpeciesThis,                                              \
     "SharedArrayBuffer subclass returned this from species constructor")       \
   T(StaticPrototype, "Classes may not have static property named prototype")   \
-  T(StrictCannotAssign, "Cannot assign to read only '%' in strict mode")       \
   T(StrictDeleteProperty, "Cannot delete property '%' of %")                   \
   T(StrictPoisonPill,                                                          \
     "'caller', 'callee', and 'arguments' properties may not be accessed on "   \

src/runtime/runtime-scopes.cc

@@ -941,19 +941,19 @@ MaybeHandle<Object> StoreLookupSlot(
   PropertyAttributes attributes;
   InitializationFlag flag;
   VariableMode mode;
-  Handle<Object> holder = context->Lookup(name, context_lookup_flags, &index,
-                                          &attributes, &flag, &mode);
+  bool is_sloppy_function_name;
+  Handle<Object> holder =
+      context->Lookup(name, context_lookup_flags, &index, &attributes, &flag,
+                      &mode, &is_sloppy_function_name);
   if (holder.is_null()) {
     // In case of JSProxy, an exception might have been thrown.
     if (isolate->has_pending_exception()) return MaybeHandle<Object>();
   } else if (holder->IsModule()) {
     if ((attributes & READ_ONLY) == 0) {
       Module::StoreVariable(Handle<Module>::cast(holder), index, value);
-    } else if (is_strict(language_mode)) {
-      // Setting read only property in strict mode.
-      THROW_NEW_ERROR(isolate,
-                      NewTypeError(MessageTemplate::kStrictCannotAssign, name),
-                      Object);
+    } else {
+      THROW_NEW_ERROR(
+          isolate, NewTypeError(MessageTemplate::kConstAssign, name), Object);
     }
     return value;
   }
@@ -967,11 +967,9 @@ MaybeHandle<Object> StoreLookupSlot(
     }
     if ((attributes & READ_ONLY) == 0) {
       Handle<Context>::cast(holder)->set(index, *value);
-    } else if (is_strict(language_mode)) {
-      // Setting read only property in strict mode.
-      THROW_NEW_ERROR(isolate,
-                      NewTypeError(MessageTemplate::kStrictCannotAssign, name),
-                      Object);
+    } else if (!is_sloppy_function_name || is_strict(language_mode)) {
+      THROW_NEW_ERROR(
+          isolate, NewTypeError(MessageTemplate::kConstAssign, name), Object);
     }
     return value;
   }

test/cctest/interpreter/bytecode_expectations/AsyncGenerators.golden

@@ -1002,7 +1002,7 @@ bytecodes: [
                 B(JumpIfToBooleanFalse), U8(4),
                 B(Jump), U8(7),
                 B(CallRuntime), U16(Runtime::kThrowIteratorResultNotAnObject), R(7), U8(1),
-                B(Wide), B(LdaSmi), I16(148),
+                B(Wide), B(LdaSmi), I16(147),
                 B(Star), R(16),
                 B(LdaConstant), U8(30),
                 B(Star), R(17),

test/mjsunit/regress/regress-6677.js

+// Copyright 2017 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Assignment to const variable inside with-statement should fail.
+const x = 0;
+assertThrows(() => { with ({}) { x = 1; } }, TypeError);
+assertEquals(0, x);
+
+assertThrows(() => { with ({}) { eval("x = 1"); } }, TypeError);
+assertEquals(0, x);
+
+// Assignment to name of named function expression inside with-statement
+// should not throw (but also not succeed).
+assertEquals('function', function f() {
+  with ({}) { f = 1 }
+  return typeof f;
+}());
+
+// But we should throw an exception if the assignment is itself in strict
+// code.
+assertEquals('function', function f() {
+  with ({}) {
+    assertThrows(function() { "use strict"; f = 1 }, TypeError);
+  }
+  return typeof f;
+}());