shiboken6: Use new API for the wrapper multimap in the generated code

This allows for removing a lot of complicated checking code.

Task-number: PYSIDE-2854
Change-Id: Ib2334437cf23862b7ca527b667525ef750ea8801
Reviewed-by: Shyamnath Premnadh <[email protected]>

5 files changed, 47 insertions, 74 deletions

diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.cpp b/sources/shiboken6/generator/shiboken/cppgenerator.cpp
index 07d782a27..2d467e7b9 100644
--- a/sources/shiboken6/generator/shiboken/cppgenerator.cpp
+++ b/sources/shiboken6/generator/shiboken/cppgenerator.cpp
@@ -88,6 +88,23 @@ TextStream &operator<<(TextStream &str, const sbkUnusedVariableCast &c)
     return str;
 }
 
+struct retrieveWrapper
+{
+    explicit retrieveWrapper(const AbstractMetaClassCPtr &klass,
+                             QAnyStringView instanceName = "this")
+        : m_klass(klass), m_instanceName(instanceName) {}
+
+    const AbstractMetaClassCPtr m_klass;
+    const QAnyStringView m_instanceName;
+};
+
+TextStream &operator<<(TextStream &str, const retrieveWrapper &rw)
+{
+    str << "Shiboken::BindingManager::instance().retrieveWrapper(" << rw.m_instanceName
+        << ", " << CppGenerator::cpythonTypeName(rw.m_klass) << ')';
+    return str;
+}
+
 struct pyTypeGetSlot
 {
     explicit pyTypeGetSlot(QAnyStringView funcType, QAnyStringView typeObject,
@@ -1016,9 +1033,8 @@ void CppGenerator::writeDestructorNative(TextStream &s,
     if (wrapperDiagnostics())
         s << R"(std::cerr << __FUNCTION__ << ' ' << this << '\n';)" << '\n';
     // kill pyobject
-    s << R"(SbkObject *wrapper = Shiboken::BindingManager::instance().retrieveWrapper(this);
-Shiboken::Object::destroy(wrapper, this);
-)" << outdent << "}\n";
+    s << "auto *wrapper = " << retrieveWrapper(classContext.metaClass())
+      << ";\nShiboken::Object::destroy(wrapper, this);\n" << outdent << "}\n";
 }
 
 // Return type for error messages when getting invalid types from virtual
@@ -1439,7 +1455,7 @@ void CppGenerator::writeVirtualMethodPythonOverride(TextStream &s,
 
     if (!snips.isEmpty()) {
         if (func->injectedCodeUsesPySelf())
-            s << "PyObject *pySelf = BindingManager::instance().retrieveWrapper(this);\n";
+            s << "PyObject *pySelf = " << retrieveWrapper(func->ownerClass()) << ";\n";
 
         const AbstractMetaArgument *lastArg = func->arguments().isEmpty()
                                             ? nullptr : &func->arguments().constLast();
@@ -1756,7 +1772,7 @@ static void writePointerToPythonConverter(TextStream &c,
                                           const QString &typeName,
                                           const QString &cpythonType)
 {
-    c << "auto *pyOut = reinterpret_cast<PyObject *>(Shiboken::BindingManager::instance().retrieveWrapper(cppIn));\n"
+    c << "auto *pyOut = reinterpret_cast<PyObject *>(" << retrieveWrapper(metaClass, "cppIn") << ");\n"
         << "if (pyOut) {\n" << indent
         << "Py_INCREF(pyOut);\nreturn pyOut;\n" << outdent
         << "}\n";
@@ -2318,12 +2334,8 @@ void CppGenerator::writeConstructorWrapper(TextStream &s, const OverloadData &ov
     // Python owns it and C++ wrapper is false.
     if (shouldGenerateCppWrapper(overloadData.referenceFunction()->ownerClass()))
         s << "Shiboken::Object::setHasCppWrapper(sbkSelf, true);\n";
-    // Need to check if a wrapper for same pointer is already registered
-    // Caused by bug PYSIDE-217, where deleted objects' wrappers are not released
-    s << "if (Shiboken::BindingManager::instance().hasWrapper(cptr)) {\n" << indent
-        << "Shiboken::BindingManager::instance().releaseWrapper("
-           "Shiboken::BindingManager::instance().retrieveWrapper(cptr));\n" << outdent
-        << "}\nShiboken::BindingManager::instance().registerWrapper(sbkSelf, cptr);\n";
+
+    s << "Shiboken::BindingManager::instance().registerWrapper(sbkSelf, cptr);\n";
 
     // Create metaObject and register signal/slot
     if (needsMetaObject) {
@@ -5026,27 +5038,7 @@ void CppGenerator::writeGetterFunction(TextStream &s,
         cppField = u"cppOut_local"_s;
     }
 
-    s << "PyObject *pyOut = {};\n";
     if (newWrapperSameObject) {
-        // Special case colocated field with same address (first field in a struct)
-        s << "if (reinterpret_cast<void *>("
-            << cppField << ") == reinterpret_cast<void *>("
-            << CPP_SELF_VAR << ")) {\n" << indent
-            << "pyOut = reinterpret_cast<PyObject *>(Shiboken::Object::findColocatedChild("
-            << "reinterpret_cast<SbkObject *>(self), "
-            << cpythonTypeNameExt(fieldType) << "));\n"
-            << "if (pyOut != nullptr) {\n" << indent
-            << "Py_IncRef(pyOut);\n"
-            << "return pyOut;\n"
-            << outdent << "}\n";
-        // Check if field wrapper has already been created.
-        s << outdent << "} else if (Shiboken::BindingManager::instance().hasWrapper("
-            << cppField << ")) {" << "\n" << indent
-            << "pyOut = reinterpret_cast<PyObject *>(Shiboken::BindingManager::instance().retrieveWrapper("
-            << cppField << "));" << "\n"
-            << "Py_IncRef(pyOut);" << "\n"
-            << "return pyOut;" << "\n"
-            << outdent << "}\n";
         // Create and register new wrapper. We force a pointer conversion also
         // for wrapped value types so that they refer to the struct member,
         // avoiding any trouble copying them. Add a parent relationship to
@@ -5055,15 +5047,23 @@ void CppGenerator::writeGetterFunction(TextStream &s,
         // unsolved issues when using temporary Python lists of structs
         // which can cause elements to be reported deleted in expressions like
         // "foo.list_of_structs[2].field".
-        s << "pyOut = "
-            << "Shiboken::Object::newObject(" << cpythonTypeNameExt(fieldType)
-            << ", " << cppField << ", false, true);\n"
-            << "Shiboken::Object::setParent(self, pyOut)";
+        s << "PyObject *pyOut = {};\n"
+            << "auto *fieldTypeObject = " << cpythonTypeNameExt(fieldType) << ";\n"
+            << "if (auto *sbkOut = Shiboken::BindingManager::instance().retrieveWrapper("
+            << cppField << ", fieldTypeObject)) {\n" << indent
+            << "pyOut = reinterpret_cast<PyObject *>(sbkOut);\n"
+            << "Py_INCREF(pyOut);\n" << outdent << "} else {\n" << indent
+            << "pyOut = Shiboken::Object::newObject(fieldTypeObject, "
+            << cppField << ", false, true);\n"
+            << "Shiboken::Object::setParent(self, pyOut);\n"
+            << outdent << "}\n"
+            << "return pyOut;\n";
     } else {
-        s << "pyOut = ";
+        s << "return ";
         writeToPythonConversion(s, fieldType, metaField.enclosingClass(), cppField);
+        s << ";\n";
     }
-    s << ";\nreturn pyOut;\n" << outdent << "}\n";
+    s << outdent << "}\n";
 }
 
 // Write a getter for QPropertySpec
diff --git a/sources/shiboken6/generator/shiboken/shibokengenerator.h b/sources/shiboken6/generator/shiboken/shibokengenerator.h
index 58e619c07..8fa057e57 100644
--- a/sources/shiboken6/generator/shiboken/shibokengenerator.h
+++ b/sources/shiboken6/generator/shiboken/shibokengenerator.h
@@ -99,6 +99,9 @@ public:
     static QString minimalConstructorExpression(const ApiExtractorResult &api,
                                                 const TypeEntryCPtr &type);
 
+    static QString cpythonTypeName(const AbstractMetaClassCPtr &metaClass);
+    static QString cpythonTypeName(const TypeEntryCPtr &type);
+
 protected:
     bool doSetup() override;
 
@@ -263,8 +266,6 @@ protected:
     static QString cpythonBaseName(const TypeEntryCPtr &type);
     static QString containerCpythonBaseName(const ContainerTypeEntryCPtr &ctype);
     static QString cpythonBaseName(const AbstractMetaType &type);
-    static QString cpythonTypeName(const AbstractMetaClassCPtr &metaClass);
-    static QString cpythonTypeName(const TypeEntryCPtr &type);
     static QString cpythonTypeNameExtSet(const TypeEntryCPtr &type);
     static QString cpythonTypeNameExtSet(const AbstractMetaType &type);
     static QString cpythonTypeNameExt(const TypeEntryCPtr &type);
diff --git a/sources/shiboken6/libshiboken/basewrapper.cpp b/sources/shiboken6/libshiboken/basewrapper.cpp
index 60f8ae77d..ae8cb6bcb 100644
--- a/sources/shiboken6/libshiboken/basewrapper.cpp
+++ b/sources/shiboken6/libshiboken/basewrapper.cpp
@@ -1582,42 +1582,16 @@ PyObject *newObjectWithHeuristics(PyTypeObject *instanceType,
 
 PyObject *newObjectForType(PyTypeObject *instanceType, void *cptr, bool hasOwnership)
 {
-    bool shouldCreate = true;
-    bool shouldRegister = true;
-    SbkObject *self = nullptr;
-
     auto &bindingManager = BindingManager::instance();
-    // Some logic to ensure that colocated child field does not overwrite the parent
-    if (SbkObject *existingWrapper = bindingManager.retrieveWrapper(cptr)) {
-        self = findColocatedChild(existingWrapper, instanceType);
-        if (self) {
-            // Wrapper already registered for cptr.
-            // This should not ideally happen, binding code should know when a wrapper
-            // already exists and retrieve it instead.
-            shouldRegister = shouldCreate = false;
-        } else if (hasOwnership &&
-                  (!(Shiboken::Object::hasCppWrapper(existingWrapper) ||
-                     Shiboken::Object::hasOwnership(existingWrapper)))) {
-            // Old wrapper is likely junk, since we have ownership and it doesn't.
-            bindingManager.releaseWrapper(existingWrapper);
-        } else {
-            // Old wrapper may be junk caused by some bug in identifying object deletion
-            // but it may not be junk when a colocated field is accessed for an
-            // object which was not created by python (returned from c++ factory function).
-            // Hence we cannot release the wrapper confidently so we do not register.
-            shouldRegister = false;
-        }
-    }
-
-    if (shouldCreate) {
+    SbkObject *self = bindingManager.retrieveWrapper(cptr, instanceType);
+    if (self != nullptr) {
+        Py_IncRef(reinterpret_cast<PyObject *>(self));
+    } else {
         self = reinterpret_cast<SbkObject *>(SbkObject_tp_new(instanceType, nullptr, nullptr));
         self->d->cptr[0] = cptr;
         self->d->hasOwnership = hasOwnership;
         self->d->validCppObject = 1;
-        if (shouldRegister)
-            bindingManager.registerWrapper(self, cptr);
-    } else {
-        Py_IncRef(reinterpret_cast<PyObject *>(self));
+        bindingManager.registerWrapper(self, cptr);
     }
     return reinterpret_cast<PyObject *>(self);
 }
diff --git a/sources/shiboken6/libshiboken/sbkconverter.cpp b/sources/shiboken6/libshiboken/sbkconverter.cpp
index 97d7a3628..94bb1e2b4 100644
--- a/sources/shiboken6/libshiboken/sbkconverter.cpp
+++ b/sources/shiboken6/libshiboken/sbkconverter.cpp
@@ -293,8 +293,8 @@ PyObject *referenceToPython(const SbkConverter *converter, const void *cppIn)
 {
     assert(cppIn);
 
-    auto *pyOut = reinterpret_cast<PyObject *>(BindingManager::instance().retrieveWrapper(cppIn));
-    if (pyOut) {
+    if (auto *sbkOut = BindingManager::instance().retrieveWrapper(cppIn, converter->pythonType)) {
+        auto *pyOut = reinterpret_cast<PyObject *>(sbkOut);
         Py_INCREF(pyOut);
         return pyOut;
     }
diff --git a/sources/shiboken6/tests/smartbinding/std_optional_test.py b/sources/shiboken6/tests/smartbinding/std_optional_test.py
index 10e72f125..cd97facfa 100644
--- a/sources/shiboken6/tests/smartbinding/std_optional_test.py
+++ b/sources/shiboken6/tests/smartbinding/std_optional_test.py
@@ -42,8 +42,6 @@ class StdOptionalTests(unittest.TestCase):
         ci = std.optional_int(43)
         self.assertEqual(ci.value(), 43)
 
-    @unittest.skipIf(True, """PYSIDE-2854, T &std::optional::value() does not work/
-                              returns self (colocated).""")
     def testInteger(self):
         b = StdOptionalTestBench()
         i = b.optionalInteger()