[GR-57802] Prioritize Python methods over foreign members

PullRequest: graalpython/3462

Author: eregon

CHANGELOG.md

@@ -6,6 +6,7 @@ language runtime. The main focus is on user-observable behavior of the engine.
 ## Version 24.2.0
 * Updated developer metadata of Maven artifacts.
 * Added gradle plugin for polyglot embedding of Python packages into Java.
+* When calling a method on a foreign object in Python code, Python methods are now prioritized over foreign members.
 
 ## Version 24.1.0
 * GraalPy is now considered stable for pure Python workloads. While many workloads involving native extension modules work, we continue to consider them experimental. You can use the command-line option `--python.WarnExperimentalFeatures` to enable warnings for such modules at runtime. In Java embeddings the warnings are enabled by default and you can suppress them by setting the context option 'python.WarnExperimentalFeatures' to 'false'.

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/foreign/ForeignObjectBuiltins.java

@@ -1101,8 +1101,9 @@ static Object doIt(VirtualFrame frame, Object self, Object name,
                         @Cached IsBuiltinObjectProfile isAttrError,
                         @Cached ForeignGetattrNode foreignGetattrNode) {
             try {
-                // We want the default Python way __getattribute__, but fallback to reading through
-                // interop library in what would be __getattr__
+                // We want the default Python attribute lookup first and try foreign members last.
+                // Because method calls in a Python source should prioritize Python methods over
+                // foreign methods.
                 return objectGetattrNode.execute(frame, self, name);
             } catch (PException e) {
                 e.expect(inliningTarget, PythonBuiltinClassType.AttributeError, isAttrError);

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/lib/PyObjectGetMethod.java

@@ -42,6 +42,7 @@
 
 import static com.oracle.graal.python.runtime.exception.PythonErrorType.AttributeError;
 
+import com.oracle.graal.python.builtins.PythonBuiltinClassType;
 import com.oracle.graal.python.builtins.objects.PNone;
 import com.oracle.graal.python.builtins.objects.PythonAbstractObject;
 import com.oracle.graal.python.builtins.objects.object.ObjectBuiltins;
@@ -61,8 +62,10 @@
 import com.oracle.graal.python.nodes.call.ForeignMethod;
 import com.oracle.graal.python.nodes.call.special.CallUnaryMethodNode;
 import com.oracle.graal.python.nodes.call.special.MaybeBindDescriptorNode;
+import com.oracle.graal.python.nodes.object.BuiltinClassProfiles;
 import com.oracle.graal.python.nodes.object.GetClassNode;
 import com.oracle.graal.python.nodes.object.IsForeignObjectNode;
+import com.oracle.graal.python.runtime.exception.PException;
 import com.oracle.truffle.api.HostCompilerDirectives.InliningCutoff;
 import com.oracle.truffle.api.dsl.Bind;
 import com.oracle.truffle.api.dsl.Cached;
@@ -212,14 +215,20 @@ static Object getDynamicAttr(Frame frame, Node inliningTarget, Object receiver,
     @InliningCutoff
     static Object getForeignMethod(VirtualFrame frame, Node inliningTarget, Object receiver, TruffleString name,
                     @SuppressWarnings("unused") @Cached IsForeignObjectNode isForeignObjectNode,
+                    @Exclusive @Cached PyObjectGetAttr getAttr,
+                    @Cached BuiltinClassProfiles.IsBuiltinObjectProfile isAttrError,
                     @Cached(inline = false) TruffleString.ToJavaStringNode toJavaString,
-                    @CachedLibrary("receiver") InteropLibrary lib,
-                    @Exclusive @Cached PyObjectGetAttr getAttr) {
-        String jName = toJavaString.execute(name);
-        if (lib.isMemberInvocable(receiver, jName)) {
-            return new BoundDescriptor(new ForeignMethod(receiver, jName));
-        } else {
-            return new BoundDescriptor(getAttr.execute(frame, inliningTarget, receiver, name));
+                    @CachedLibrary("receiver") InteropLibrary lib) {
+        try {
+            return getGenericAttr(frame, inliningTarget, receiver, name, getAttr);
+        } catch (PException e) {
+            e.expect(inliningTarget, PythonBuiltinClassType.AttributeError, isAttrError);
+            String jName = toJavaString.execute(name);
+            if (lib.isMemberInvocable(receiver, jName)) {
+                return new BoundDescriptor(new ForeignMethod(receiver, jName));
+            } else {
+                throw e;
+            }
         }
     }
 

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/ReadAttributeFromObjectNode.java

@@ -55,11 +55,7 @@
 import com.oracle.graal.python.nodes.PNodeWithContext;
 import com.oracle.graal.python.nodes.attributes.ReadAttributeFromObjectNodeGen.ReadAttributeFromObjectNotTypeNodeGen;
 import com.oracle.graal.python.nodes.attributes.ReadAttributeFromObjectNodeGen.ReadAttributeFromObjectTpDictNodeGen;
-import com.oracle.graal.python.nodes.interop.PForeignToPTypeNode;
 import com.oracle.graal.python.nodes.object.GetDictIfExistsNode;
-import com.oracle.graal.python.nodes.object.IsForeignObjectNode;
-import com.oracle.graal.python.nodes.util.CannotCastException;
-import com.oracle.graal.python.nodes.util.CastToJavaStringNode;
 import com.oracle.graal.python.runtime.PythonOptions;
 import com.oracle.truffle.api.HostCompilerDirectives.InliningCutoff;
 import com.oracle.truffle.api.dsl.Bind;
@@ -71,10 +67,6 @@
 import com.oracle.truffle.api.dsl.NeverDefault;
 import com.oracle.truffle.api.dsl.ReportPolymorphism;
 import com.oracle.truffle.api.dsl.Specialization;
-import com.oracle.truffle.api.interop.InteropLibrary;
-import com.oracle.truffle.api.interop.UnknownIdentifierException;
-import com.oracle.truffle.api.interop.UnsupportedMessageException;
-import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.profiles.InlinedConditionProfile;
 import com.oracle.truffle.api.strings.TruffleString;
@@ -162,19 +154,13 @@ protected static Object readObjectAttribute(PythonObject object, TruffleString k
     }
 
     // foreign object or primitive
-    @Specialization(guards = {"!isPythonObject(object)", "!isNativeObject(object)"})
     @InliningCutoff
-    protected static Object readForeign(Object object, TruffleString key,
-                    @Bind("this") Node inliningTarget,
-                    @Cached IsForeignObjectNode isForeignObjectNode,
-                    @Cached ReadAttributeFromForeign read) {
-        if (isForeignObjectNode.execute(inliningTarget, object)) {
-            // there's an implicit condition profile from the isForeignObjectNode active
-            // specializations
-            return read.execute(object, key);
-        } else {
-            return PNone.NO_VALUE;
-        }
+    @Specialization(guards = {"!isPythonObject(object)", "!isNativeObject(object)"})
+    protected static Object readForeignOrPrimitive(Object object, TruffleString key) {
+        // Foreign members are tried after the regular attribute lookup, see
+        // ForeignObjectBuiltins.GetAttributeNode. If we looked them up here
+        // they would get precedence over attributes in the MRO.
+        return PNone.NO_VALUE;
     }
 
     // native objects. We distinguish reading at the objects dictoffset or the tp_dict
@@ -184,7 +170,7 @@ protected static Object readForeign(Object object, TruffleString key,
     @GenerateUncached
     @GenerateInline(false) // footprint reduction 64 -> 47
     protected abstract static class ReadAttributeFromObjectNotTypeNode extends ReadAttributeFromObjectNode {
-        @Specialization(insertBefore = "readForeign")
+        @Specialization(insertBefore = "readForeignOrPrimitive")
         protected static Object readNativeObject(PythonAbstractNativeObject object, TruffleString key,
                         @Bind("this") Node inliningTarget,
                         @Exclusive @Cached GetDictIfExistsNode getDict,
@@ -196,7 +182,7 @@ protected static Object readNativeObject(PythonAbstractNativeObject object, Truf
     @GenerateUncached
     @GenerateInline(false) // footprint reduction 68 -> 51
     protected abstract static class ReadAttributeFromObjectTpDictNode extends ReadAttributeFromObjectNode {
-        @Specialization(insertBefore = "readForeign")
+        @Specialization(insertBefore = "readForeignOrPrimitive")
         protected static Object readNativeClass(PythonAbstractNativeObject object, TruffleString key,
                         @Bind("this") Node inliningTarget,
                         @Cached CStructAccess.ReadObjectNode getNativeDict,
@@ -215,25 +201,4 @@ private static Object readNative(Node inliningTarget, TruffleString key, Object
         return PNone.NO_VALUE;
     }
 
-    @ImportStatic(PythonOptions.class)
-    @GenerateUncached
-    @GenerateInline(false) // footprint reduction 32 -> 13
-    protected abstract static class ReadAttributeFromForeign extends PNodeWithContext {
-        public abstract Object execute(Object object, Object key);
-
-        @Specialization
-        static Object read(Object object, Object key,
-                        @Cached CastToJavaStringNode castNode,
-                        @Cached PForeignToPTypeNode fromForeign,
-                        @CachedLibrary(limit = "getAttributeAccessInlineCacheMaxDepth()") InteropLibrary read) {
-            try {
-                String member = castNode.execute(key);
-                if (read.isMemberReadable(object, member)) {
-                    return fromForeign.executeConvert(read.readMember(object, member));
-                }
-            } catch (CannotCastException | UnknownIdentifierException | UnsupportedMessageException ignored) {
-            }
-            return PNone.NO_VALUE;
-        }
-    }
 }