Fix proxy defineProperty descriptors to omit absent fields

lib/VM/Operations.cpp

@@ -2283,22 +2283,24 @@ CallResult<HermesValue> objectFromPropertyDescriptor(
 
   if (!dpFlags.isAccessor()) {
     // Data Descriptor
-    auto result = JSObject::defineOwnProperty(
-        obj,
-        runtime,
-        Predefined::getSymbolID(Predefined::value),
-        dpf,
-        valueOrAccessor,
-        PropOpFlags().plusThrowOnError());
-    assert(
-        result != ExecutionStatus::EXCEPTION &&
-        "defineOwnProperty() failed on a new object");
-    if (result == ExecutionStatus::EXCEPTION) {
-      return ExecutionStatus::EXCEPTION;
+    if (dpFlags.setValue) {
+      auto result = JSObject::defineOwnProperty(
+          obj,
+          runtime,
+          Predefined::getSymbolID(Predefined::value),
+          dpf,
+          valueOrAccessor,
+          PropOpFlags().plusThrowOnError());
+      assert(
+          result != ExecutionStatus::EXCEPTION &&
+          "defineOwnProperty() failed on a new object");
+      if (result == ExecutionStatus::EXCEPTION) {
+        return ExecutionStatus::EXCEPTION;
+      }
     }
 
     if (dpFlags.setWritable) {
-      result = JSObject::defineOwnProperty(
+      auto result = JSObject::defineOwnProperty(
           obj,
           runtime,
           Predefined::getSymbolID(Predefined::writable),
@@ -2316,42 +2318,46 @@ CallResult<HermesValue> objectFromPropertyDescriptor(
     // Accessor
     auto *accessor = vmcast<PropertyAccessor>(valueOrAccessor.get());
 
-    auto getter = runtime.makeHandle(
-        accessor->getter ? HermesValue::encodeObjectValue(
-                               accessor->getter.getNonNull(runtime))
-                         : HermesValue::encodeUndefinedValue());
-
-    auto setter = runtime.makeHandle(
-        accessor->setter ? HermesValue::encodeObjectValue(
-                               accessor->setter.getNonNull(runtime))
-                         : HermesValue::encodeUndefinedValue());
+    if (dpFlags.setGetter) {
+      auto getter = runtime.makeHandle(
+          accessor->getter ? HermesValue::encodeObjectValue(
+                                 accessor->getter.getNonNull(runtime))
+                           : HermesValue::encodeUndefinedValue());
 
-    auto result = JSObject::defineOwnProperty(
-        obj,
-        runtime,
-        Predefined::getSymbolID(Predefined::get),
-        dpf,
-        getter,
-        PropOpFlags().plusThrowOnError());
-    assert(
-        result != ExecutionStatus::EXCEPTION &&
-        "defineOwnProperty() failed on a new object");
-    if (result == ExecutionStatus::EXCEPTION) {
-      return ExecutionStatus::EXCEPTION;
+      auto result = JSObject::defineOwnProperty(
+          obj,
+          runtime,
+          Predefined::getSymbolID(Predefined::get),
+          dpf,
+          getter,
+          PropOpFlags().plusThrowOnError());
+      assert(
+          result != ExecutionStatus::EXCEPTION &&
+          "defineOwnProperty() failed on a new object");
+      if (result == ExecutionStatus::EXCEPTION) {
+        return ExecutionStatus::EXCEPTION;
+      }
     }
 
-    result = JSObject::defineOwnProperty(
-        obj,
-        runtime,
-        Predefined::getSymbolID(Predefined::set),
-        dpf,
-        setter,
-        PropOpFlags().plusThrowOnError());
-    assert(
-        result != ExecutionStatus::EXCEPTION &&
-        "defineOwnProperty() failed on a new object");
-    if (result == ExecutionStatus::EXCEPTION) {
-      return ExecutionStatus::EXCEPTION;
+    if (dpFlags.setSetter) {
+      auto setter = runtime.makeHandle(
+          accessor->setter ? HermesValue::encodeObjectValue(
+                                 accessor->setter.getNonNull(runtime))
+                           : HermesValue::encodeUndefinedValue());
+
+      auto result = JSObject::defineOwnProperty(
+          obj,
+          runtime,
+          Predefined::getSymbolID(Predefined::set),
+          dpf,
+          setter,
+          PropOpFlags().plusThrowOnError());
+      assert(
+          result != ExecutionStatus::EXCEPTION &&
+          "defineOwnProperty() failed on a new object");
+      if (result == ExecutionStatus::EXCEPTION) {
+        return ExecutionStatus::EXCEPTION;
+      }
     }
   }
 

test/hermes/regress-proxy-accessor-descriptor.js

@@ -0,0 +1,48 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -Xes6-proxy -non-strict -O -target=HBC %s | %FileCheck --match-full-lines %s
+
+var setterCalled = false;
+var target = {};
+
+Object.defineProperty(target, 'x', {
+  get: function() {
+    return 42;
+  },
+  set: function(v) {
+    setterCalled = true;
+    print('setter called:', v);
+  },
+  configurable: true,
+});
+
+var proxy = new Proxy(target, {
+  defineProperty: function(obj, prop, desc) {
+    print('has get:', Object.prototype.hasOwnProperty.call(desc, 'get'));
+    print('has set:', Object.prototype.hasOwnProperty.call(desc, 'set'));
+    print('configurable:', desc.configurable);
+    return Reflect.defineProperty(obj, prop, desc);
+  },
+});
+
+Object.defineProperty(proxy, 'x', {
+  get: function() {
+    return 99;
+  },
+  configurable: true,
+});
+
+target.x = 5;
+
+print('setter intact:', setterCalled);
+
+// CHECK: has get: true
+// CHECK-NEXT: has set: false
+// CHECK-NEXT: configurable: true
+// CHECK-NEXT: setter called: 5
+// CHECK-NEXT: setter intact: true

test/hermes/regress-proxy-defineproperty-value.js

@@ -0,0 +1,32 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -Xes6-proxy -non-strict -O -target=HBC %s | %FileCheck --match-full-lines %s
+
+var target = {x: 42};
+
+var proxy = new Proxy(target, {
+  defineProperty: function(obj, prop, desc) {
+    print('has value:', Object.prototype.hasOwnProperty.call(desc, 'value'));
+    print(
+        'has writable:',
+        Object.prototype.hasOwnProperty.call(desc, 'writable'));
+    return Reflect.defineProperty(obj, prop, desc);
+  },
+});
+
+Object.defineProperty(proxy, 'x', {writable: false});
+
+print('target.x:', target.x);
+print(
+    'descriptor.writable:',
+    Object.getOwnPropertyDescriptor(target, 'x').writable);
+
+// CHECK: has value: false
+// CHECK-NEXT: has writable: true
+// CHECK-NEXT: target.x: 42
+// CHECK-NEXT: descriptor.writable: false