Fix bug in object literals with redeclarations.

Bug: v8:7791
Change-Id: I0df79f39c6f60b3cfbdc0161f7c085c635659d81
Reviewed-on: https://chromium-review.googlesource.com/1075054Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53385}

src/ast/ast.cc

@@ -360,19 +360,37 @@ void ObjectLiteral::CalculateEmitStore(Zone* zone) {
     Literal* literal = property->key()->AsLiteral();
     DCHECK(!literal->IsNullLiteral());
 
-    // If there is an existing entry do not emit a store unless the previous
-    // entry was also an accessor.
     uint32_t hash = literal->Hash();
     ZoneHashMap::Entry* entry = table.LookupOrInsert(literal, hash, allocator);
-    if (entry->value != nullptr) {
-      auto previous_kind =
+    if (entry->value == nullptr) {
+      entry->value = property;
+    } else {
+      // We already have a later definition of this property, so we don't need
+      // to emit a store for the current one.
+      //
+      // There are two subtleties here.
+      //
+      // (1) Emitting a store might actually be incorrect. For example, in {get
+      // foo() {}, foo: 42}, the getter store would override the data property
+      // (which, being a non-computed compile-time valued property, is already
+      // part of the initial literal object.
+      //
+      // (2) If the later definition is an accessor (say, a getter), and the
+      // current definition is a complementary accessor (here, a setter), then
+      // we still must emit a store for the current definition.
+
+      auto later_kind =
           static_cast<ObjectLiteral::Property*>(entry->value)->kind();
-      if (!((property->kind() == GETTER && previous_kind == SETTER) ||
-            (property->kind() == SETTER && previous_kind == GETTER))) {
+      bool complementary_accessors =
+          (property->kind() == GETTER && later_kind == SETTER) ||
+          (property->kind() == SETTER && later_kind == GETTER);
+      if (!complementary_accessors) {
         property->set_emit_store(false);
+        if (later_kind == GETTER || later_kind == SETTER) {
+          entry->value = property;
+        }
       }
     }
-    entry->value = property;
   }
 }
 

test/mjsunit/regress/regress-7791.js

+// Copyright 2018 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.
+
+"use strict";
+
+
+// Data property last.
+
+{
+  const o = {
+    get foo() { return 666 },
+    foo: 42,
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').value);
+}
+
+{
+  const o = {
+    set foo(_) { },
+    foo: 42,
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').value);
+}
+
+{
+  const o = {
+    get foo() { return 666 },
+    set foo(_) { },
+    foo: 42,
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').value);
+}
+
+{
+  const o = {
+    get foo() { return 666 },
+    set ['foo'.slice()](_) { },
+    foo: 42,
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').value);
+}
+
+{
+  const o = {
+    get ['foo'.slice()]() { return 666 },
+    set ['foo'.slice()](_) { },
+    foo: 42,
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').value);
+}
+
+
+// Data property first.
+
+{
+  const o = {
+    foo: 666,
+    get foo() { return 42 },
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').get());
+}
+
+{
+  const o = {
+    foo: 666,
+    set foo(_) { },
+  };
+  assertEquals(undefined, Object.getOwnPropertyDescriptor(o, 'foo').get);
+  assertEquals(undefined, Object.getOwnPropertyDescriptor(o, 'foo').value);
+}
+
+{
+  const o = {
+    foo: 666,
+    get foo() { return 42 },
+    set foo(_) { },
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').get());
+}
+
+{
+  const o = {
+    foo: 666,
+    get ['foo'.slice()]() { return 42 },
+    set foo(_) { },
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').get());
+}
+
+{
+  const o = {
+    foo: 666,
+    get ['foo'.slice()]() { return 42 },
+    set ['foo'](_) { },
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').get());
+}
+
+
+// Data property in the middle.
+
+{
+  const o = {
+    get foo() { return 42 },
+    foo: 666,
+    set foo(_) { },
+  };
+  assertEquals(undefined, Object.getOwnPropertyDescriptor(o, 'foo').get);
+  assertEquals(undefined, Object.getOwnPropertyDescriptor(o, 'foo').set());
+}
+
+{
+  const o = {
+    set foo(_) { },
+    foo: 666,
+    get foo() { return 42 },
+  };
+  assertEquals(42, Object.getOwnPropertyDescriptor(o, 'foo').get());
+}