[builtins] Fix argument order inconsistency in HasProperty

The HasProperty builtin differed in its expected argument order from
the HasProperty runtime function. Like all other related spec
primitives (e.g.: GetProperty, SetProperty, DeleteProperty), it should
take {object} as the first argument and {key} as the second.

This CL changes the builtin and all related spots to use the correct
order.

There was also a tricky bug in interpreter intrinsic rewriting, which
assumes (but does not verify) that the argument order between runtime
function and builtin is identical. Besides cctests, HasProperty
intrinsic rewriting seems to be dead code.

Bug: v8:8036
Change-Id: Ia669fd6f5c73a30df4e4607064603be759ced392
Reviewed-on: https://chromium-review.googlesource.com/1167297
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55022}

src/builtins/array-copywithin.tq

@@ -63,7 +63,7 @@ module array {
       // a. Let fromKey be ! ToString(from).
       // b. Let toKey be ! ToString(to).
       // c. Let fromPresent be ? HasProperty(O, fromKey).
-      const from_present: Boolean = HasProperty(context, from, object);
+      const from_present: Boolean = HasProperty(context, object, from);
 
       // d. If fromPresent is true, then.
       if (from_present == True) {

src/builtins/base.tq

@@ -182,13 +182,10 @@ extern macro ToNumber_Inline(Context, Object): Number;
 extern macro ToString_Inline(Context, Object): String;
 extern macro GetProperty(Context, Object, Object): Object;
 
-// TODO(mvstanton): the HasProperty builtin takes the key as the first argument,
-// and the object as the second, reversing the usual order. Reverse this in the
-// builtin definition at some point.
-extern builtin HasProperty(Context, Object, JSReceiver): Boolean;
+extern builtin HasProperty(Context, JSReceiver, Object): Boolean;
 macro TorqueHasProperty(
     context: Context, receiver: JSReceiver, key: Object): Boolean {
-  return HasProperty(context, key, receiver);
+  return HasProperty(context, receiver, key);
 }
 
 extern macro ThrowRangeError(Context, constexpr MessageTemplate): never;

src/builtins/builtins-definitions.h

@@ -220,7 +220,7 @@ namespace internal {
   TFC(RunMicrotasks, RunMicrotasks, 1)                                         \
                                                                                \
   /* Object property helpers */                                                \
-  TFS(HasProperty, kKey, kObject)                                              \
+  TFS(HasProperty, kObject, kKey)                                              \
   TFS(DeleteProperty, kObject, kKey, kLanguageMode)                            \
                                                                                \
   /* Abort */                                                                  \

src/builtins/builtins-proxy-gen.cc

@@ -460,7 +460,7 @@ TF_BUILTIN(ProxyHasProperty, ProxiesCodeStubAssembler) {
   BIND(&trap_undefined);
   {
     // 7.a. Return ? target.[[HasProperty]](P).
-    TailCallBuiltin(Builtins::kHasProperty, context, name, target);
+    TailCallBuiltin(Builtins::kHasProperty, context, target, name);
   }
 
   BIND(&return_false);

src/builtins/builtins-reflect-gen.cc

@@ -18,7 +18,7 @@ TF_BUILTIN(ReflectHas, CodeStubAssembler) {
   ThrowIfNotJSReceiver(context, target, MessageTemplate::kCalledOnNonObject,
                        "Reflect.has");
 
-  Return(CallBuiltin(Builtins::kHasProperty, context, key, target));
+  Return(CallBuiltin(Builtins::kHasProperty, context, target, key));
 }
 
 }  // namespace internal

src/compiler/bytecode-graph-builder.cc

@@ -2453,19 +2453,15 @@ void BytecodeGraphBuilder::VisitTestReferenceEqual() {
   environment()->BindAccumulator(result);
 }
 
-void BytecodeGraphBuilder::BuildTestingOp(const Operator* op) {
+void BytecodeGraphBuilder::VisitTestIn() {
   PrepareEagerCheckpoint();
-  Node* left =
+  Node* object = environment()->LookupAccumulator();
+  Node* key =
       environment()->LookupRegister(bytecode_iterator().GetRegisterOperand(0));
-  Node* right = environment()->LookupAccumulator();
-  Node* node = NewNode(op, left, right);
+  Node* node = NewNode(javascript()->HasProperty(), object, key);
   environment()->BindAccumulator(node, Environment::kAttachFrameState);
 }
 
-void BytecodeGraphBuilder::VisitTestIn() {
-  BuildTestingOp(javascript()->HasProperty());
-}
-
 void BytecodeGraphBuilder::VisitTestInstanceOf() {
   int const slot_index = bytecode_iterator().GetIndexOperand(1);
   BuildCompareOp(javascript()->InstanceOf(CreateVectorSlotPair(slot_index)));

src/compiler/bytecode-graph-builder.h

@@ -184,7 +184,6 @@ class BytecodeGraphBuilder {
   void BuildBinaryOp(const Operator* op);
   void BuildBinaryOpWithImmediate(const Operator* op);
   void BuildCompareOp(const Operator* op);
-  void BuildTestingOp(const Operator* op);
   void BuildDelete(LanguageMode language_mode);
   void BuildCastOperator(const Operator* op);
   void BuildHoleCheckAndThrow(Node* condition, Runtime::FunctionId runtime_id,

src/compiler/js-call-reducer.cc

@@ -938,7 +938,7 @@ Reduction JSCallReducer::ReduceReflectHas(Node* node) {
   Node* vtrue;
   {
     vtrue = etrue = if_true =
-        graph()->NewNode(javascript()->HasProperty(), key, target, context,
+        graph()->NewNode(javascript()->HasProperty(), target, key, context,
                          frame_state, etrue, if_true);
   }
 

test/cctest/interpreter/test-interpreter-intrinsics.cc

@@ -210,14 +210,14 @@ TEST(IntrinsicAsStubCall) {
 
   InvokeIntrinsicHelper has_property_helper(isolate, handles.main_zone(),
                                             Runtime::kInlineHasProperty);
-  CHECK_EQ(*factory->true_value(),
-           *has_property_helper.Invoke(
-               has_property_helper.NewObject("'x'"),
-               has_property_helper.NewObject("({ x: 20 })")));
-  CHECK_EQ(*factory->false_value(),
-           *has_property_helper.Invoke(
-               has_property_helper.NewObject("'y'"),
-               has_property_helper.NewObject("({ x: 20 })")));
+  CHECK_EQ(
+      *factory->true_value(),
+      *has_property_helper.Invoke(has_property_helper.NewObject("({ x: 20 })"),
+                                  has_property_helper.NewObject("'x'")));
+  CHECK_EQ(
+      *factory->false_value(),
+      *has_property_helper.Invoke(has_property_helper.NewObject("({ x: 20 })"),
+                                  has_property_helper.NewObject("'y'")));
 }
 
 }  // namespace interpreter