[GR-23203] Fix test_binop

timfel · timfel · commit ab7d4dd32fb5 · 2020-06-15T10:29:37.000Z
PullRequest: graalpython/1031
diff --git a/graalpython/com.oracle.graal.python.test/src/tests/unittest_tags/test_binop.txt b/graalpython/com.oracle.graal.python.test/src/tests/unittest_tags/test_binop.txt
@@ -1,5 +1,8 @@
 *graalpython.lib-python.3.test.test_binop.FallbackBlockingTests.test_fallback_ne_blocking
+*graalpython.lib-python.3.test.test_binop.FallbackBlockingTests.test_fallback_rmethod_blocking
+*graalpython.lib-python.3.test.test_binop.OperationOrderTests.test_comparison_orders
 *graalpython.lib-python.3.test.test_binop.RatTestCase.test_add
+*graalpython.lib-python.3.test.test_binop.RatTestCase.test_constructor
 *graalpython.lib-python.3.test.test_binop.RatTestCase.test_div
 *graalpython.lib-python.3.test.test_binop.RatTestCase.test_eq
 *graalpython.lib-python.3.test.test_binop.RatTestCase.test_floordiv
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/object/ObjectBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/object/ObjectBuiltins.java
@@ -234,8 +234,15 @@ public int hash(Object self) {
     public abstract static class EqNode extends PythonBinaryBuiltinNode {
         @Specialization
         Object eq(Object self, Object other,
+                        @Cached ConditionProfile isEq,
                         @Cached IsNode isNode) {
-            return isNode.execute(self, other);
+            if (isEq.profile(isNode.execute(self, other))) {
+                return true;
+            } else {
+                // Return NotImplemented instead of False, so if two objects are compared, both get
+                // a chance at the comparison
+                return PNotImplemented.NOT_IMPLEMENTED;
+            }
         }
     }
 
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/str/StringBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/str/StringBuiltins.java
@@ -84,6 +84,7 @@
 import com.oracle.graal.python.builtins.objects.str.StringNodes.StringLenNode;
 import com.oracle.graal.python.builtins.objects.str.StringUtils.StripKind;
 import com.oracle.graal.python.builtins.objects.tuple.PTuple;
+import com.oracle.graal.python.builtins.objects.tuple.TupleBuiltins;
 import com.oracle.graal.python.nodes.ErrorMessages;
 import com.oracle.graal.python.nodes.PGuards;
 import com.oracle.graal.python.nodes.SpecialMethodNames;
@@ -1512,10 +1513,12 @@ Object doStringObject(VirtualFrame frame, String self, Object right,
                         @CachedLibrary(limit = "3") PythonObjectLibrary plib,
                         @Shared("lookupAttrNode") @Cached LookupAttributeInMRONode.Dynamic lookupAttrNode,
                         @Shared("getItemNode") @Cached("create(__GETITEM__)") LookupAndCallBinaryNode getItemNode,
+                        @Shared("getTupleItemNode") @Cached TupleBuiltins.GetItemNode getTupleItemNode,
                         @Shared("context") @CachedContext(PythonLanguage.class) PythonContext context) {
             Object state = IndirectCallContext.enter(frame, context, this);
             try {
-                return new StringFormatter(context.getCore(), self).format(right, callNode, (object, key) -> lookupAttrNode.execute(plib.getLazyPythonClass(object), key), getItemNode);
+                return new StringFormatter(context.getCore(), self).format(right, callNode, (object, key) -> lookupAttrNode.execute(plib.getLazyPythonClass(object), key), getItemNode,
+                                getTupleItemNode);
             } finally {
                 IndirectCallContext.exit(frame, context, state);
             }
@@ -1528,10 +1531,11 @@ Object doGeneric(VirtualFrame frame, Object self, Object right,
                         @CachedLibrary(limit = "3") PythonObjectLibrary plib,
                         @Shared("lookupAttrNode") @Cached LookupAttributeInMRONode.Dynamic lookupAttrNode,
                         @Shared("getItemNode") @Cached("create(__GETITEM__)") LookupAndCallBinaryNode getItemNode,
+                        @Shared("getTupleItemNode") @Cached TupleBuiltins.GetItemNode getTupleItemNode,
                         @Shared("context") @CachedContext(PythonLanguage.class) PythonContext context) {
 
             String selfStr = castSelfNode.cast(self, INVALID_RECEIVER, __MOD__, self);
-            return doStringObject(frame, selfStr, right, callNode, plib, lookupAttrNode, getItemNode, context);
+            return doStringObject(frame, selfStr, right, callNode, plib, lookupAttrNode, getItemNode, getTupleItemNode, context);
         }
     }
 
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/special/LookupAndCallBinaryNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/call/special/LookupAndCallBinaryNode.java
@@ -74,6 +74,7 @@ public abstract static class NotImplementedHandler extends PNodeWithContext {
     protected final String name;
     protected final String rname;
     protected final Supplier<NotImplementedHandler> handlerFactory;
+    private final boolean alwaysCheckReverse;
 
     @Child private CallBinaryMethodNode dispatchNode;
     @Child private CallBinaryMethodNode reverseDispatchNode;
@@ -103,27 +104,32 @@ public abstract static class NotImplementedHandler extends PNodeWithContext {
 
     public abstract Object executeObject(VirtualFrame frame, Object arg, Object arg2);
 
-    LookupAndCallBinaryNode(String name, String rname, Supplier<NotImplementedHandler> handlerFactory) {
+    LookupAndCallBinaryNode(String name, String rname, Supplier<NotImplementedHandler> handlerFactory, boolean alwaysCheckReverse) {
         this.name = name;
         this.rname = rname;
         this.handlerFactory = handlerFactory;
+        this.alwaysCheckReverse = alwaysCheckReverse;
     }
 
     public static LookupAndCallBinaryNode create(String name) {
-        return LookupAndCallBinaryNodeGen.create(name, null, null);
+        return LookupAndCallBinaryNodeGen.create(name, null, null, false);
     }
 
     public static LookupAndCallBinaryNode createReversible(String name, String reverseName, Supplier<NotImplementedHandler> handlerFactory) {
         assert name.startsWith("__") && reverseName.startsWith("__r");
-        return LookupAndCallBinaryNodeGen.create(name, reverseName, handlerFactory);
+        return LookupAndCallBinaryNodeGen.create(name, reverseName, handlerFactory, false);
     }
 
     public static LookupAndCallBinaryNode create(String name, String rname) {
-        return LookupAndCallBinaryNodeGen.create(name, rname, null);
+        return LookupAndCallBinaryNodeGen.create(name, rname, null, false);
+    }
+
+    public static LookupAndCallBinaryNode create(String name, String rname, boolean alwaysCheckReverse) {
+        return LookupAndCallBinaryNodeGen.create(name, rname, null, alwaysCheckReverse);
     }
 
     public static LookupAndCallBinaryNode create(String name, String rname, Supplier<NotImplementedHandler> handlerFactory) {
-        return LookupAndCallBinaryNodeGen.create(name, rname, handlerFactory);
+        return LookupAndCallBinaryNodeGen.create(name, rname, handlerFactory, false);
     }
 
     protected Object getMethod(Object receiver, String methodName) {
@@ -316,7 +322,7 @@ Object callObject(VirtualFrame frame, Object left, Object right,
         Object leftCallable = getattr.execute(leftClass);
         Object rightClass = libRight.getLazyPythonClass(right);
         Object rightCallable = getattrR.execute(rightClass);
-        if (leftCallable == rightCallable) {
+        if (!alwaysCheckReverse && leftCallable == rightCallable) {
             rightCallable = PNone.NO_VALUE;
         }
         if (leftCallable != PNone.NO_VALUE) {
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/expression/BinaryComparisonNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/expression/BinaryComparisonNode.java
@@ -56,7 +56,8 @@ public abstract class BinaryComparisonNode extends BinaryOpNode {
         this.magicMethod = magicMethod;
         this.magicReverseMethod = magicReverseMethod;
         this.operation = operation;
-        this.callNode = LookupAndCallBinaryNode.create(magicMethod, magicReverseMethod);
+        // see cpython://Objects/object.c#do_richcompare - CPython always calls the reverse method
+        this.callNode = LookupAndCallBinaryNode.create(magicMethod, magicReverseMethod, true);
     }
 
     public static BinaryComparisonNode create(String magicMethod, String magicReverseMethod, String operation, ExpressionNode left, ExpressionNode right) {
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/formatting/StringFormatter.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/formatting/StringFormatter.java
@@ -26,6 +26,7 @@
 import com.oracle.graal.python.builtins.objects.ints.PInt;
 import com.oracle.graal.python.builtins.objects.object.PythonObjectLibrary;
 import com.oracle.graal.python.builtins.objects.str.PString;
+import com.oracle.graal.python.builtins.objects.tuple.TupleBuiltins;
 import com.oracle.graal.python.nodes.ErrorMessages;
 import com.oracle.graal.python.nodes.PGuards;
 import com.oracle.graal.python.nodes.call.CallNode;
@@ -67,7 +68,7 @@ public StringFormatter(PythonCore core, String format) {
         buffer = new StringBuilder(format.length() + 100);
     }
 
-    Object getarg(LookupAndCallBinaryNode getItemNode) {
+    Object getarg(TupleBuiltins.GetItemNode getTupleItemNode) {
         Object ret = null;
         switch (argIndex) {
             case -3: // special index indicating a mapping
@@ -79,7 +80,14 @@ Object getarg(LookupAndCallBinaryNode getItemNode) {
                 return args;
             default:
                 // NOTE: passing 'null' frame means we already took care of the global state earlier
-                ret = getItemNode.executeObject(null, args, argIndex++);
+                // args is definitely a tuple at this point. CPython access the tuple storage
+                // directly, so the only error can be IndexError, which we ignore and transform into
+                // the TypeError below.
+                try {
+                    ret = getTupleItemNode.execute(null, args, argIndex++);
+                } catch (PException e) {
+                    // fall through
+                }
                 break;
         }
         if (ret == null) {
@@ -88,10 +96,10 @@ Object getarg(LookupAndCallBinaryNode getItemNode) {
         return ret;
     }
 
-    int getNumber(LookupAndCallBinaryNode getItemNode) {
+    int getNumber(TupleBuiltins.GetItemNode getTupleItemNode) {
         char c = pop();
         if (c == '*') {
-            Object o = getarg(getItemNode);
+            Object o = getarg(getTupleItemNode);
             if (o instanceof Long) {
                 return ((Long) o).intValue();
             } else if (o instanceof Integer) {
@@ -164,7 +172,7 @@ private static Object asFloat(Object arg, CallNode callNode, BiFunction<Object,
      * construction.
      */
     @TruffleBoundary
-    public Object format(Object args1, CallNode callNode, BiFunction<Object, String, Object> lookupAttribute, LookupAndCallBinaryNode getItemNode) {
+    public Object format(Object args1, CallNode callNode, BiFunction<Object, String, Object> lookupAttribute, LookupAndCallBinaryNode getItemNode, TupleBuiltins.GetItemNode getTupleItemNode) {
         Object mapping = null;
         this.args = args1;
 
@@ -272,7 +280,7 @@ public Object format(Object args1, CallNode callNode, BiFunction<Object, String,
              * after the minimum field width and optional precision. A custom getNumber() takes care
              * of the '*' case.
              */
-            width = getNumber(getItemNode);
+            width = getNumber(getTupleItemNode);
             if (width < 0) {
                 width = -width;
                 align = '<';
@@ -286,7 +294,7 @@ public Object format(Object args1, CallNode callNode, BiFunction<Object, String,
              */
             c = pop();
             if (c == '.') {
-                precision = getNumber(getItemNode);
+                precision = getNumber(getTupleItemNode);
                 if (precision < -1) {
                     precision = 0;
                 }
@@ -341,7 +349,7 @@ public Object format(Object args1, CallNode callNode, BiFunction<Object, String,
 
             switch (spec.type) {
                 case 'b':
-                    Object arg = getarg(getItemNode);
+                    Object arg = getarg(getTupleItemNode);
                     f = ft = new TextFormatter(core, buffer, spec);
                     ft.setBytes(true);
                     Object bytesAttribute;
@@ -360,7 +368,7 @@ public Object format(Object args1, CallNode callNode, BiFunction<Object, String,
                     break;
                 case 's': // String: converts any object using __str__(), __unicode__() ...
                 case 'r': // ... or repr().
-                    arg = getarg(getItemNode);
+                    arg = getarg(getTupleItemNode);
                     // Get hold of the actual object to display (may set needUnicode)
                     Object attribute = spec.type == 's' ? lookupAttribute.apply(arg, __STR__) : lookupAttribute.apply(arg, __REPR__);
                     if (attribute != PNone.NO_VALUE) {
@@ -383,7 +391,7 @@ public Object format(Object args1, CallNode callNode, BiFunction<Object, String,
                 case 'i': // Compatibility with scanf().
                     // Format the argument using this Spec.
 
-                    arg = getarg(getItemNode);
+                    arg = getarg(getTupleItemNode);
 
                     // Note various types accepted here as long as they have an __int__ method.
                     Object argAsNumber = asNumber(arg, callNode, lookupAttribute);
@@ -422,7 +430,7 @@ public Object format(Object args1, CallNode callNode, BiFunction<Object, String,
                     f = ff = new FloatFormatter(core, buffer, spec);
 
                     // Note various types accepted here as long as they have a __float__ method.
-                    arg = getarg(getItemNode);
+                    arg = getarg(getTupleItemNode);
                     Object argAsFloat = asFloat(arg, callNode, lookupAttribute);
 
                     // We have to check what we got back..

Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,8 @@ public abstract class BinaryComparisonNode extends BinaryOpNode {`
`56`	`56`	`this.magicMethod = magicMethod;`
`57`	`57`	`this.magicReverseMethod = magicReverseMethod;`
`58`	`58`	`this.operation = operation;`
`59`		`- this.callNode = LookupAndCallBinaryNode.create(magicMethod, magicReverseMethod);`
	`59`	`+ // see cpython://Objects/object.c#do_richcompare - CPython always calls the reverse method`
	`60`	`+ this.callNode = LookupAndCallBinaryNode.create(magicMethod, magicReverseMethod, true);`
`60`	`61`	`}`
`61`	`62`
`62`	`63`	`public static BinaryComparisonNode create(String magicMethod, String magicReverseMethod, String operation, ExpressionNode left, ExpressionNode right) {`