shiboken6: Handle pointers to containers

Opaque containers were disabled for functions taking a pointer to a
container since the number of indirections generated was incorrect.
Functions taking a pointer to a container where no opaque container
exists caused a crash since shiboken generated a value conversion to
an uninitialized pointer.

Change e4c2272dc60e1ff5e2d0933238fe4508af5f7f60 fixed the number of
indirections used for arguments.

With this, enable opaque containers also for functions taking a
pointer. Use the same code path also for the case of a function taking
a container by pointer since it provides a local variable to store the
value.

As a drive by, this also allows for a virtual function
reimplemented in Python to return an opaque container.

Change writePythonToCppTypeConversion() to return the
number of indirections in case of return types.

Remove flag CppGenerator::PythonToCppTypeConversionFlag.

[ChangeLog][shiboken6] Code generation for functions taking a
pointer to a container has been fixed.

Pick-to: 6.2
Task-number: PYSIDE-1605
Task-number: PYSIDE-1790
Change-Id: Ifa0bafb1316d7edfe1efc2183459b1ee6924f5a1
Reviewed-by: Shyamnath Premnadh <Shyamnath.Premnadh@qt.io>
Reviewed-by: Christian Tismer <tismer@stackless.com>

6 files changed, 103 insertions, 29 deletions

diff --git a/sources/shiboken6/ApiExtractor/abstractmetatype.cpp b/sources/shiboken6/ApiExtractor/abstractmetatype.cpp
index b48effa56..f2b512bf0 100644
--- a/sources/shiboken6/ApiExtractor/abstractmetatype.cpp
+++ b/sources/shiboken6/ApiExtractor/abstractmetatype.cpp
@@ -964,7 +964,10 @@ AbstractMetaType AbstractMetaType::fromAbstractMetaClass(const AbstractMetaClass
 template <class Predicate> // Predicate(containerTypeEntry, signature)
 bool AbstractMetaTypeData::generateOpaqueContainer(Predicate pred) const
 {
-    if (m_pattern != AbstractMetaType::ContainerPattern)
+    // Allow for passing containers by pointer as well.
+    if (!m_typeEntry->isContainer())
+        return false;
+    if (m_indirections.size() > 1)
         return false;
     auto *containerTypeEntry = static_cast<const ContainerTypeEntry *>(m_typeEntry);
     auto kind = containerTypeEntry->containerKind();
diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.cpp b/sources/shiboken6/generator/shiboken/cppgenerator.cpp
index c73a6892d..bc471a5ec 100644
--- a/sources/shiboken6/generator/shiboken/cppgenerator.cpp
+++ b/sources/shiboken6/generator/shiboken/cppgenerator.cpp
@@ -1178,6 +1178,8 @@ void CppGenerator::writeVirtualMethodNative(TextStream &s,
                        TypeSystem::NativeCode, func, false, lastArg);
     }
 
+    qsizetype returnIndirections = 0;
+
     if (!func->injectedCodeCallsPythonOverride()) {
         s << "Shiboken::AutoDecRef " << pyRetVar << "(PyObject_Call("
             << PYTHON_OVERRIDE_VAR << ", " << PYTHON_ARGS << ", nullptr));\n";
@@ -1251,10 +1253,9 @@ void CppGenerator::writeVirtualMethodNative(TextStream &s,
                 // Has conversion rule.
                 writeConversionRule(s, func, TypeSystem::NativeCode, cppRetVar);
             } else if (!func->injectedCodeHasReturnValueAttribution(TypeSystem::NativeCode)) {
-                writePythonToCppTypeConversion(
+                returnIndirections = writePythonToCppTypeConversion(
                     s, func->type(), pyRetVar,
-                    cppRetVar, func->implementingClass(), {},
-                    PythonToCppTypeConversionFlag::DisableOpaqueContainers);
+                    cppRetVar, func->implementingClass(), {});
             }
         }
     }
@@ -1291,8 +1292,9 @@ void CppGenerator::writeVirtualMethodNative(TextStream &s,
                 s << '(' << typeCast << ')';
             }
         }
-        if (func->type().isWrapperPassedByReference())
-            s << " *";
+
+        if (returnIndirections > 0)
+            s << QByteArray(returnIndirections, '*');
         s << cppRetVar << ";\n";
     }
 
@@ -2588,17 +2590,18 @@ static void writeMinimalConstructorExpression(TextStream &s,
     s << '(' << defaultValue << ')';
 }
 
-void CppGenerator::writePythonToCppTypeConversion(TextStream &s,
+qsizetype CppGenerator::writePythonToCppTypeConversion(TextStream &s,
                                                   const AbstractMetaType &type,
                                                   const QString &pyIn,
                                                   const QString &cppOut,
                                                   const AbstractMetaClass *context,
-                                                  const QString &defaultValue,
-                                                  PythonToCppTypeConversionFlags flags) const
+                                                  const QString &defaultValue) const
 {
     const TypeEntry *typeEntry = type.typeEntry();
     if (typeEntry->isCustom() || typeEntry->isVarargs())
-        return;
+        return 0;
+
+    qsizetype indirections = -type.indirectionsV().size();
 
     QString cppOutAux = cppOut + QLatin1String("_local");
 
@@ -2606,13 +2609,11 @@ void CppGenerator::writePythonToCppTypeConversion(TextStream &s,
     const bool isEnum = typeEntry->isEnum();
     const bool isFlags = typeEntry->isFlags();
     const bool treatAsPointer = type.valueTypeWithCopyConstructorOnlyPassed();
-    const bool maybeOpaqueContainer =
-        !flags.testFlag(PythonToCppTypeConversionFlag::DisableOpaqueContainers)
-        && type.generateOpaqueContainer();
+    const bool isContainer = typeEntry->isContainer();
     bool isPointerOrObjectType = (type.isObjectType() || type.isPointer())
         && !type.isUserPrimitive() && !type.isExtendedCppPrimitive()
         && !isEnum && !isFlags;
-    const bool isNotContainerEnumOrFlags = !typeEntry->isContainer()
+    const bool isNotContainerEnumOrFlags = !isContainer
                                            && !isEnum && !isFlags;
     const bool mayHaveImplicitConversion = type.referenceType() == LValueReference
                                      && !type.isUserPrimitive()
@@ -2624,7 +2625,10 @@ void CppGenerator::writePythonToCppTypeConversion(TextStream &s,
     // may occur. An implicit conversion uses value conversion whereas the object
     // itself uses pointer conversion. For containers, the PyList/container
     // conversion is by value whereas opaque containers use pointer conversion.
-    const bool valueOrPointer = mayHaveImplicitConversion || maybeOpaqueContainer;
+    // For a container passed by pointer, a local variable is also needed.
+    const bool valueOrPointer = mayHaveImplicitConversion
+       || type.generateOpaqueContainer()
+       || (isContainer && indirections != 0);
 
     const AbstractMetaTypeList &nestedArrayTypes = type.nestedArrayTypes();
     const bool isCppPrimitiveArray = !nestedArrayTypes.isEmpty()
@@ -2647,12 +2651,16 @@ void CppGenerator::writePythonToCppTypeConversion(TextStream &s,
     if (isCppPrimitiveArray) {
         s << ' ' << cppOut;
     } else if (valueOrPointer) {
+        ++indirections;
         // Generate either value conversion for &cppOutAux or pointer
         // conversion for &cppOut
         s << ' ' << cppOutAux;
-        writeMinimalConstructorExpression(s, api(), type, isPrimitive, defaultValue);
+        // No default value for containers which can also be passed by pointer.
+        if (!isContainer)
+            writeMinimalConstructorExpression(s, api(), type, isPrimitive, defaultValue);
         s << ";\n" << typeName << " *" << cppOut << " = &" << cppOutAux;
     } else if (treatAsPointer || isPointerOrObjectType) {
+        ++indirections;
         s << " *" << cppOut;
         if (!defaultValue.isEmpty()) {
             const bool needsConstCast = !isNullPtr(defaultValue)
@@ -2693,7 +2701,7 @@ void CppGenerator::writePythonToCppTypeConversion(TextStream &s,
         s << pythonToCppCall << ";\n";
         if (!defaultValue.isEmpty())
             s << outdent;
-        return;
+        return indirections;
     }
 
     // pythonToCppFunc may be 0 when less parameters are passed and
@@ -2710,6 +2718,8 @@ void CppGenerator::writePythonToCppTypeConversion(TextStream &s,
         s << '\n';
     else
         s << "}\n" << outdent;
+
+    return indirections;
 }
 
 static void addConversionRuleCodeSnippet(CodeSnipList &snippetList, QString &rule,
diff --git a/sources/shiboken6/generator/shiboken/cppgenerator.h b/sources/shiboken6/generator/shiboken/cppgenerator.h
index 17b04b6f6..da6828aa9 100644
--- a/sources/shiboken6/generator/shiboken/cppgenerator.h
+++ b/sources/shiboken6/generator/shiboken/cppgenerator.h
@@ -44,11 +44,6 @@ class OverloadDataRootNode;
 class CppGenerator : public ShibokenGenerator
 {
 public:
-    enum class PythonToCppTypeConversionFlag {
-        DisableOpaqueContainers = 0x1
-    };
-    Q_DECLARE_FLAGS(PythonToCppTypeConversionFlags, PythonToCppTypeConversionFlag)
-
     enum class ErrorReturn {
         Default, // "{}"
         Zero,
@@ -212,13 +207,15 @@ private:
     static AbstractMetaType
         getArgumentType(const AbstractMetaFunctionCPtr &func, int index);
 
-    void writePythonToCppTypeConversion(TextStream &s,
+    /// Writes the Python to C++ Conversion for function arguments and return
+    /// values of virtual methods for wrappers.
+    /// \return The number of indirections in case of return types
+    qsizetype writePythonToCppTypeConversion(TextStream &s,
                                         const AbstractMetaType &type,
                                         const QString &pyIn,
                                         const QString &cppOut,
                                         const AbstractMetaClass *context = nullptr,
-                                        const QString &defaultValue = {},
-                                        PythonToCppTypeConversionFlags = {}) const;
+                                        const QString &defaultValue = {}) const;
 
     /// Writes the conversion rule for arguments of regular and virtual methods.
     void writeConversionRule(TextStream &s, const AbstractMetaFunctionCPtr &func,
@@ -501,6 +498,4 @@ private:
     static const char *PYTHON_TO_CPPCONVERSION_STRUCT;
 };
 
-Q_DECLARE_OPERATORS_FOR_FLAGS(CppGenerator::PythonToCppTypeConversionFlags)
-
 #endif // CPPGENERATOR_H
diff --git a/sources/shiboken6/tests/libminimal/listuser.cpp b/sources/shiboken6/tests/libminimal/listuser.cpp
index 6a0230d60..e9f1c4eab 100644
--- a/sources/shiboken6/tests/libminimal/listuser.cpp
+++ b/sources/shiboken6/tests/libminimal/listuser.cpp
@@ -133,3 +133,28 @@ const std::list<int> &ListUser::getConstIntList() const
 {
     return m_stdIntList;
 }
+
+int ListUser::modifyIntListPtr(std::list<int> *list) const
+{
+    const int oldSize = int(list->size());
+    list->push_back(42);
+    return oldSize;
+}
+
+std::list<int> *ListUser::returnIntListByPtr() const
+{
+    return nullptr;
+}
+
+int ListUser::callReturnIntListByPtr() const
+{
+    auto *list = returnIntListByPtr();
+    return list != nullptr ? int(list->size()) : 0;
+}
+
+int ListUser::modifyDoubleListPtr(std::list<double> *list) const
+{
+    const int oldSize = int(list->size());
+    list->push_back(42);
+    return oldSize;
+}
diff --git a/sources/shiboken6/tests/libminimal/listuser.h b/sources/shiboken6/tests/libminimal/listuser.h
index 0eae69ae1..8fd6f7ad1 100644
--- a/sources/shiboken6/tests/libminimal/listuser.h
+++ b/sources/shiboken6/tests/libminimal/listuser.h
@@ -74,6 +74,14 @@ struct LIBMINIMAL_API ListUser
     std::list<int> &getIntList();
     const std::list<int> &getConstIntList() const;
 
+    int modifyIntListPtr(std::list<int> *list) const;
+
+    virtual std::list<int> *returnIntListByPtr() const;
+
+    int callReturnIntListByPtr() const;
+
+    int modifyDoubleListPtr(std::list<double> *list) const;
+
     std::list<int> m_stdIntList;
 };
 
diff --git a/sources/shiboken6/tests/minimalbinding/listuser_test.py b/sources/shiboken6/tests/minimalbinding/listuser_test.py
index a1fc48c08..a31a5aab2 100644
--- a/sources/shiboken6/tests/minimalbinding/listuser_test.py
+++ b/sources/shiboken6/tests/minimalbinding/listuser_test.py
@@ -44,7 +44,10 @@ from minimal import ListUser, Val, Obj, StdIntList
 
 class ExtListUser(ListUser):
     def __init__(self):
-        ListUser.__init__(self)
+        super().__init__()
+        self._stdIntList = StdIntList()
+        self._stdIntList.append(1)
+        self._stdIntList.append(2)
 
     def createIntList(self, num):
         return list(range(0, num * 2, 2))
@@ -78,6 +81,9 @@ class ExtListUser(ListUser):
     def sumListOfIntLists(self, intListList):
         return sum([sum(line) for line in intListList]) * 2
 
+    def returnIntListByPtr(self):
+        return self._stdIntList
+
 
 class IntListConversionTest(unittest.TestCase):
 
@@ -356,7 +362,34 @@ class ListOfIntListConversionTest(unittest.TestCase):
         self.assertEqual(len(const_l), 4)
         self.assertRaises(TypeError, const_l.append, 6)
 
+    def testListByPtrOpaque(self):
+        """Test a function taking C++ list by pointer for which an opaque
+           container exists."""
+        lu = ListUser()
+        python_list = [1, 2]
+        self.assertEqual(lu.modifyIntListPtr(python_list), 2)
+
+        # Pass an opaque container and verify whether it is modified by C++
+        cpp_list = StdIntList()
+        cpp_list.append(1)
+        cpp_list.append(2)
+        self.assertEqual(lu.modifyIntListPtr(cpp_list), 2)
+        self.assertEqual(len(cpp_list), 3)
+
+    def testListByPtr(self):
+        """Test a function taking C++ list by pointer for which no opaque
+           container exists."""
+        lu = ListUser()
+        python_list = [1.1, 22.2]
+        self.assertEqual(lu.modifyDoubleListPtr(python_list), 2)
+
+    def testReturnListByPtr(self):
+        """Test that a virtual function returning a list by pointer can be
+           reimplemented by a Python function returning an opaque container."""
+        lu = ExtListUser()
+        size = lu.callReturnIntListByPtr()  # Call virtual from C++
+        self.assertEqual(size, 2)
+
 
 if __name__ == '__main__':
     unittest.main()
-