Disallow using "bare" PyObject arg behavior for C API functions returns

Author: msimacek

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextBuiltins.java

@@ -56,6 +56,7 @@
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObjectTransfer;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyThreadState;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObject;
+import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObjectTransfer;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.SIZE_T;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.UINTPTR_T;
@@ -918,7 +919,7 @@ static TruffleString encoding() {
         }
     }
 
-    @CApiBuiltin(ret = PyTypeObject, args = {ConstCharPtrAsTruffleString}, call = Ignored)
+    @CApiBuiltin(ret = PyTypeObjectTransfer, args = {ConstCharPtrAsTruffleString}, call = Ignored)
     abstract static class PyTruffle_Type extends CApiUnaryBuiltinNode {
 
         private static final TruffleString[] LOOKUP_MODULES = new TruffleString[]{

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextSlotBuiltins.java

@@ -69,6 +69,7 @@
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PySliceObject;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTupleObject;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObject;
+import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObjectBorrowed;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyUnicodeObject;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyVarObject;
 import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
@@ -271,7 +272,7 @@ static Object get(Object object,
         }
     }
 
-    @CApiBuiltin(ret = PyTypeObject, args = {PyCMethodObject}, call = Ignored)
+    @CApiBuiltin(ret = PyTypeObjectBorrowed, args = {PyCMethodObject}, call = Ignored)
     abstract static class Py_get_PyCMethodObject_mm_class extends CApiUnaryBuiltinNode {
         @Specialization
         static Object get(PBuiltinMethod object) {
@@ -388,7 +389,7 @@ static Object get(GetSetDescriptor object) {
         }
     }
 
-    @CApiBuiltin(ret = PyTypeObject, args = {PyDescrObject}, call = Ignored)
+    @CApiBuiltin(ret = PyTypeObjectBorrowed, args = {PyDescrObject}, call = Ignored)
     abstract static class Py_get_PyDescrObject_d_type extends CApiUnaryBuiltinNode {
 
         @Specialization
@@ -682,7 +683,7 @@ static Object get(PythonAbstractObjectNativeWrapper wrapper) {
         }
     }
 
-    @CApiBuiltin(ret = PyTypeObject, args = {PyObject}, call = Ignored)
+    @CApiBuiltin(ret = PyTypeObjectBorrowed, args = {PyObject}, call = Ignored)
     abstract static class Py_get_PyObject_ob_type extends CApiUnaryBuiltinNode {
 
         @Specialization
@@ -711,23 +712,23 @@ static long get(PBaseSet object,
         }
     }
 
-    @CApiBuiltin(ret = PyObject, args = {PySliceObject}, call = Ignored)
+    @CApiBuiltin(ret = PyObjectBorrowed, args = {PySliceObject}, call = Ignored)
     abstract static class Py_get_PySliceObject_start extends CApiUnaryBuiltinNode {
         @Specialization
         static Object doStart(PSlice object) {
             return object.getStart();
         }
     }
 
-    @CApiBuiltin(ret = PyObject, args = {PySliceObject}, call = Ignored)
+    @CApiBuiltin(ret = PyObjectBorrowed, args = {PySliceObject}, call = Ignored)
     abstract static class Py_get_PySliceObject_step extends CApiUnaryBuiltinNode {
         @Specialization
         static Object doStep(PSlice object) {
             return object.getStep();
         }
     }
 
-    @CApiBuiltin(ret = PyObject, args = {PySliceObject}, call = Ignored)
+    @CApiBuiltin(ret = PyObjectBorrowed, args = {PySliceObject}, call = Ignored)
     abstract static class Py_get_PySliceObject_stop extends CApiUnaryBuiltinNode {
         @Specialization
         static Object doStop(PSlice object) {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiFunction.java

@@ -1472,6 +1472,10 @@ private static void addCApiBuiltins(List<CApiBuiltinDesc> result, Class<?> conta
                         if (!annotation.name().isEmpty()) {
                             name = annotation.name();
                         }
+                        if (!annotation.ret().isValidReturnType()) {
+                            throw new IllegalArgumentException(
+                                            String.format("Invalid @CApiBuiltin %s: %s is not an allowed return type, use PyObjectTransfer or PyObjectBorrow variants", name, annotation.ret()));
+                        }
                         Class<?> gen;
                         try {
                             gen = Class.forName(container.getName() + "Factory$" + clazz.getSimpleName() + "NodeGen");

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/ArgDescriptor.java

@@ -120,6 +120,7 @@ public enum ArgDescriptor {
     PyObjectWrapper(ArgBehavior.PyObjectWrapper, "PyObject*"),
     PyObjectAsTruffleString(ArgBehavior.PyObjectAsTruffleString, "PyObject*"),
     PyTypeObject(ArgBehavior.PyObject, "PyTypeObject*"),
+    PyTypeObjectBorrowed(ArgBehavior.PyObjectBorrowed, "PyTypeObject*"),
     PyTypeObjectTransfer(ArgBehavior.PyObject, "PyTypeObject*", true),
     PyListObject(ArgBehavior.PyObject, "PyListObject*"),
     PyTupleObject(ArgBehavior.PyObject, "PyTupleObject*"),
@@ -430,6 +431,14 @@ public boolean isPyObject() {
         return behavior == ArgBehavior.PyObject || behavior == ArgBehavior.PyObjectBorrowed;
     }
 
+    public boolean isValidReturnType() {
+        /*
+         * We don't want to allow "bare" PyObject and force ourselves to decide between
+         * PyObjectTransfer and PyObjectBorrow
+         */
+        return behavior != ArgBehavior.PyObject || transfer;
+    }
+
     public boolean isCharPtr() {
         return this == CharPtrAsTruffleString || this == CHAR_PTR || this == ConstCharPtr || this == ConstCharPtrAsTruffleString;
     }