Ensure that Held Class types can reference themselves.

This fixes a nasty serialization bug where mutually recursive
type groups can hold references to the same HeldClass in two places
(once as a pyobj and once as a type). This was causing serialization
of held classes that refer to themselves in their own functions
to break.

Really, we should ensure that TypeOrPyObj has a single way
of referring to any given object - there shouldn't be a distinction
in a MutuallyRecursiveTypeGroup between the Type and the
PyObject representation of it.

Author: braxtonmckee

typed_python/MutuallyRecursiveTypeGroup.cpp

@@ -181,11 +181,26 @@ void visitCompilerVisibleTypesAndPyobjects(
     };
 
     if (obj.type()) {
+        Type* objType = obj.type();
+
         hashVisit(ShaHash(1));
 
-        obj.type()->visitReferencedTypes(visit);
-        obj.type()->visitCompilerVisiblePythonObjects(visit);
-        obj.type()->visitCompilerVisibleInstances([&](Instance i) {
+        objType->visitReferencedTypes(visit);
+
+        // ensure that held and non-held versions of Class are
+        // always visible to each other.
+        if (objType->isHeldClass()) {
+            Type* t = ((HeldClass*)objType)->getClassType();
+            visit(t);
+        }
+
+        if (objType->isClass()) {
+            Type* t = ((Class*)objType)->getHeldClass();
+            visit(t);
+        }
+
+        objType->visitCompilerVisiblePythonObjects(visit);
+        objType->visitCompilerVisibleInstances([&](Instance i) {
             visit(i.type());
 
             if (i.type()->getTypeCategory() == Type::TypeCategory::catPythonObjectOfType) {

typed_python/PyInstance.cpp

@@ -202,7 +202,6 @@ void PyInstance::copyConstructFromPythonInstanceConcrete(Type* eltType, instance
         aNewName = typeName;
     }
 
-    std::string verb;
     if (level == ConversionLevel::Signature) {
         throw std::logic_error("Object of type " + std::string(pyRepresentation->ob_type->tp_name) + " is not " + typeName);
     }

typed_python/PythonSerializationContext_serialization.cpp

@@ -284,7 +284,7 @@ void PythonSerializationContext::serializePythonObjectNamedOrAsObj(PyObject* o,
 void PythonSerializationContext::serializePythonObjectRepresentation(PyObject* representation, SerializationBuffer& b, size_t fieldNumber) const {
     PyEnsureGilAcquired acquireTheGil;
 
-    if (!PyTuple_Check(representation) || PyTuple_Size(representation) < 2 
+    if (!PyTuple_Check(representation) || PyTuple_Size(representation) < 2
             || PyTuple_Size(representation) > 6) {
         throw std::runtime_error("representationFor should return None or a tuple with 2 to 6 things");
     }
@@ -492,7 +492,7 @@ void PythonSerializationContext::serializeMutuallyRecursiveTypeGroup(MutuallyRec
                     }
 
                     if (representation != Py_None) {
-                        if (!PyTuple_Check(representation) || PyTuple_Size(representation) < 2 
+                        if (!PyTuple_Check(representation) || PyTuple_Size(representation) < 2
                                 || PyTuple_Size(representation) > 6) {
                             throw std::runtime_error("representationFor should return None or a tuple with 2 to 6 things");
                         }
@@ -630,13 +630,15 @@ void PythonSerializationContext::serializeMutuallyRecursiveTypeGroup(MutuallyRec
             // then HeldClass objects, ordered with all bases before
             // their children (so that when we deserialize, any base classes
             // are no longer forwards)
-            std::map<HeldClass*, int> heldClasses;
+            std::map<HeldClass*, std::set<int> > heldClasses;
             for (auto& indexAndObj: group->getIndexToObject()) {
                 if (
                     indexAndObj.second.typeOrPyobjAsType() &&
                     indexAndObj.second.typeOrPyobjAsType()->isHeldClass()
                 ) {
-                    heldClasses[(HeldClass*)indexAndObj.second.typeOrPyobjAsType()] = indexAndObj.first;
+                    heldClasses[(HeldClass*)indexAndObj.second.typeOrPyobjAsType()].insert(
+                        indexAndObj.first
+                    );
                 }
             }
 
@@ -657,7 +659,9 @@ void PythonSerializationContext::serializeMutuallyRecursiveTypeGroup(MutuallyRec
                 }
 
                 // now we can write this object.
-                writeObjectBody(heldClasses[cls], cls);
+                for (auto index: heldClasses[cls]) {
+                    writeObjectBody(index, cls);
+                }
             };
 
             for (auto classAndIndex: heldClasses) {

typed_python/_types.cpp

@@ -2770,9 +2770,9 @@ PyObject *recursiveTypeGroupHash(PyObject* nullValue, PyObject* args) {
     });
 }
 
-PyObject *recursiveTypeGroupDeepRepr(PyObject* nullValue, PyObject* args) {
+PyObject *recursiveTypeGroupReprDeepFlag(PyObject* nullValue, PyObject* args, bool deep) {
     if (PyTuple_Size(args) != 1) {
-        PyErr_SetString(PyExc_TypeError, "recursiveTypeGroupDeepRepr takes 1 positional argument");
+        PyErr_SetString(PyExc_TypeError, "recursiveTypeGroupRepr takes 1 positional argument");
         return NULL;
     }
     PyObjectHolder a1(PyTuple_GetItem(args, 0));
@@ -2795,10 +2795,20 @@ PyObject *recursiveTypeGroupDeepRepr(PyObject* nullValue, PyObject* args) {
             return incref(Py_None);
         }
 
-        return PyUnicode_FromString(group->repr(true).c_str());
+        return PyUnicode_FromString(group->repr(deep).c_str());
     });
 }
 
+PyObject *recursiveTypeGroupRepr(PyObject* nullValue, PyObject* args) {
+    return recursiveTypeGroupReprDeepFlag(nullValue, args, false);
+}
+
+
+PyObject *recursiveTypeGroupDeepRepr(PyObject* nullValue, PyObject* args) {
+    return recursiveTypeGroupReprDeepFlag(nullValue, args, true);
+}
+
+
 PyObject *recursiveTypeGroup(PyObject* nullValue, PyObject* args) {
     if (PyTuple_Size(args) != 1) {
         PyErr_SetString(PyExc_TypeError, "recursiveTypeGroup takes 1 positional argument");
@@ -3247,6 +3257,7 @@ static PyMethodDef module_methods[] = {
     {"Forward", (PyCFunction)MakeForward, METH_VARARGS, NULL},
     {"allForwardTypesResolved", (PyCFunction)allForwardTypesResolved, METH_VARARGS, NULL},
     {"recursiveTypeGroup", (PyCFunction)recursiveTypeGroup, METH_VARARGS, NULL},
+    {"recursiveTypeGroupRepr", (PyCFunction)recursiveTypeGroupRepr, METH_VARARGS, NULL},
     {"recursiveTypeGroupDeepRepr", (PyCFunction)recursiveTypeGroupDeepRepr, METH_VARARGS, NULL},
     {"recursiveTypeGroupHash", (PyCFunction)recursiveTypeGroupHash, METH_VARARGS, NULL},
     {"typesAndObjectsVisibleToCompilerFrom", (PyCFunction)typesAndObjectsVisibleToCompilerFrom, METH_VARARGS, NULL},

typed_python/type_identity_test.py

@@ -63,6 +63,19 @@ def returnSerializedValue(filesToWrite, expression, printComments=False):
     )
 
 
+def test_class_and_held_class_in_group():
+    class C(Class):
+        pass
+
+    H = C.HeldClass
+
+    assert H in recursiveTypeGroup(H)
+    assert C in recursiveTypeGroup(H)
+
+    assert H in recursiveTypeGroup(C)
+    assert C in recursiveTypeGroup(C)
+
+
 def test_identity_of_register_types():
     assert isinstance(identityHash(UInt64), bytes)
     assert len(identityHash(UInt64)) == 20

typed_python/types_serialization_test.py

@@ -2691,6 +2691,39 @@ def makeC():
         assert C.anF() is C
         assert C.anF.overloads[0].methodOf.Class is C
 
+    def test_held_class_serialized_externally(self):
+        def makeC():
+            with tempfile.TemporaryDirectory() as tempdir:
+                path = os.path.join(tempdir, "asdf.py")
+
+                CONTENTS = (
+                    "from typed_python import Entrypoint, ListOf, Class, Held, Final\n"
+                    "@Held\n"
+                    "class C(Class, Final):\n"
+                    "    @staticmethod\n"
+                    "    def make():\n"
+                    "        return C()\n"
+                )
+
+                with open(path, "w") as f:
+                    f.write(CONTENTS)
+
+                globals = {'__file__': path}
+
+                exec(
+                    compile(CONTENTS, path, "exec"),
+                    globals
+                )
+
+                s = SerializationContext()
+                return s.serialize(globals['C'])
+
+        serializedC = callFunctionInFreshProcess(makeC, ())
+
+        C = SerializationContext().deserialize(serializedC)
+
+        assert type(C.make()) is C
+
     def test_serialization_independent_of_whether_function_is_hashed(self):
         s = SerializationContext().withoutLineInfoEncoded().withoutCompression()