Bindings: Fix handling of forward declarations

It is not appropriate to decorate each forward declaration with the SIP
attribute /External/.  That is only needed for forward declarations of
types which are defined in a different module.

Local forward declarations can be omitted from the sip code.  In sip
code, a forward declaration followed later by a full class definition is
an error.

Omit forward declarations unless they are decorated with the external
attribute.  Introduce a rules database for consumers to decorate them
with the attribute as required.

10 files changed, 252 insertions, 48 deletions

diff --git a/find-modules/Qt5Ruleset.py b/find-modules/Qt5Ruleset.py
index 9392ae18..2d8cfaac 100644
--- a/find-modules/Qt5Ruleset.py
+++ b/find-modules/Qt5Ruleset.py
@@ -107,6 +107,9 @@ def variable_rules():
 def typedef_rules():
     return []
 
+def forward_declaration_rules():
+    return []
+
 class RuleSet(rules_engine.RuleSet):
     """
     SIP file generator rules. This is a set of (short, non-public) functions
@@ -114,6 +117,7 @@ class RuleSet(rules_engine.RuleSet):
     """
     def __init__(self):
         self._container_db = rules_engine.ContainerRuleDb(container_rules)
+        self._forward_declaration_db = rules_engine.ForwardDeclarationRuleDb(forward_declaration_rules)
         self._fn_db = rules_engine.FunctionRuleDb(function_rules)
         self._param_db = rules_engine.ParameterRuleDb(parameter_rules)
         self._typedef_db = rules_engine.TypedefRuleDb(typedef_rules)
@@ -124,6 +128,9 @@ class RuleSet(rules_engine.RuleSet):
     def container_rules(self):
         return self._container_db
 
+    def forward_declaration_rules(self):
+        return self._forward_declaration_db
+
     def function_rules(self):
         return self._fn_db
 
diff --git a/find-modules/rules_engine.py b/find-modules/rules_engine.py
index 34bd15dc..897d0433 100755
--- a/find-modules/rules_engine.py
+++ b/find-modules/rules_engine.py
@@ -235,6 +235,71 @@ class ContainerRuleDb(AbstractCompiledRuleDb):
         return None
 
 
+class ForwardDeclarationRuleDb(AbstractCompiledRuleDb):
+    """
+    THE RULES FOR FORWARD DECLARATIONS.
+
+    These are used to customise the behaviour of the SIP generator by allowing
+    the forward declaration for any container (class, struct, union) to be
+    customised, for example to add SIP compiler annotations.
+
+    Each entry in the raw rule database must be a list with members as follows:
+
+        0. A regular expression which matches the fully-qualified name of the
+        "container" enclosing the container.
+
+        1. A regular expression which matches the container name.
+
+        2. A regular expression which matches any template parameters.
+
+        3. A function.
+
+    In use, the database is walked in order from the first entry. If the regular
+    expressions are matched, the function is called, and no further entries are
+    walked. The function is called with the following contract:
+
+        def declaration_xxx(container, sip, matcher):
+            '''
+            Return a modified declaration for the given container.
+
+            :param container:   The clang.cindex.Cursor for the container.
+            :param sip:         A dict with the following keys:
+
+                                    name                The name of the container.
+                                    template_parameters Any template parameters.
+                                    annotations         Any SIP annotations.
+
+            :param matcher:         The re.Match object. This contains named
+                                    groups corresponding to the key names above
+                                    EXCEPT body and annotations.
+
+            :return: An updated set of sip.xxx values. Setting sip.name to the
+                     empty string will cause the container to be suppressed.
+            '''
+
+    :return: The compiled form of the rules.
+    """
+    def __init__(self, db):
+        super(ForwardDeclarationRuleDb, self).__init__(db, ["parents", "container", "template_parameters"])
+
+    def apply(self, container, sip):
+        """
+        Walk over the rules database for containers, applying the first matching transformation.
+
+        :param container:           The clang.cindex.Cursor for the container.
+        :param sip:                 The SIP dict (may be modified on return).
+        :return:                    Modifying rule or None (even if a rule matched, it may not modify things).
+        """
+        parents = _parents(container)
+        matcher, rule = self._match(parents, sip["name"],
+                                    ", ".join(sip["template_parameters"]))
+        if matcher:
+            before = deepcopy(sip)
+            rule.fn(container, sip, matcher)
+            return rule.trace_result(parents, container, before, sip)
+        return None
+
+
 class FunctionRuleDb(AbstractCompiledRuleDb):
     """
     THE RULES FOR FUNCTIONS.
@@ -774,6 +839,15 @@ class RuleSet(object):
         raise NotImplemented(_("Missing subclass implementation"))
 
     @abstractmethod
+    def forward_declaration_rules(self):
+        """
+        Return a compiled list of rules for containers.
+
+        :return: A ForwardDeclarationRuleDb instance
+        """
+        raise NotImplemented(_("Missing subclass implementation"))
+
+    @abstractmethod
     def function_rules(self):
         """
         Return a compiled list of rules for functions.
@@ -888,6 +962,9 @@ def typedef_discard(container, typedef, sip, matcher):
 def discard_QSharedData_base(container, sip, matcher):
     sip["base_specifiers"].remove("QSharedData")
 
+def mark_forward_declaration_external(container, sip, matcher):
+    sip["annotations"].add("External")
+
 def rules(project_rules):
     """
     Constructor.
diff --git a/find-modules/sip_generator.py b/find-modules/sip_generator.py
index 41dd6369..7df9de3d 100644
--- a/find-modules/sip_generator.py
+++ b/find-modules/sip_generator.py
@@ -313,30 +313,51 @@ class SipGenerator(object):
                     pad = " " * ((level + 1) * 4)
                     body += pad + "// {}\n".format(SipGenerator.describe(member))
                 body += decl
+
+
+        if container.kind == CursorKind.TRANSLATION_UNIT:
+            return body
+
+        if container.kind == CursorKind.NAMESPACE:
+            container_type = "namespace " + name
+        elif container.kind in [CursorKind.CLASS_DECL, CursorKind.CLASS_TEMPLATE,
+                                CursorKind.CLASS_TEMPLATE_PARTIAL_SPECIALIZATION]:
+            container_type = "class " + name
+        elif container.kind == CursorKind.STRUCT_DECL:
+            container_type = "struct " + name
+        elif container.kind == CursorKind.UNION_DECL:
+            container_type = "union " + name
+        else:
+            raise AssertionError(
+                _("Unexpected container {}: {}[{}]").format(container.kind, name, container.extent.start.line))
+
+        sip["decl"] = container_type
+        sip["template_parameters"] = template_type_parameters
+
+        pad = " " * (level * 4)
+
         #
         # Empty containers are still useful if they provide namespaces or forward declarations.
         #
-        if not body and level >= 0:
-            body = "\n"
+        if not body:
             text = self._read_source(container.extent)
             if not text.endswith("}"):
                 #
                 # Forward declaration.
                 #
-                sip["annotations"].add("External")
-        if body and level >= 0:
-            if container.kind == CursorKind.NAMESPACE:
-                container_type = "namespace " + name
-            elif container.kind in [CursorKind.CLASS_DECL, CursorKind.CLASS_TEMPLATE,
-                                    CursorKind.CLASS_TEMPLATE_PARTIAL_SPECIALIZATION]:
-                container_type = "class " + name
-            elif container.kind == CursorKind.STRUCT_DECL:
-                container_type = "struct " + name
-            elif container.kind == CursorKind.UNION_DECL:
-                container_type = "union " + name
-            else:
-                raise AssertionError(
-                    _("Unexpected container {}: {}[{}]").format(container.kind, name, container.extent.start.line))
+                modifying_rule = self.rules.forward_declaration_rules().apply(container, sip)
+                if sip["name"]:
+                    if modifying_rule:
+                        body += "// Modified {} (by {}):\n".format(SipGenerator.describe(container), modifying_rule)
+                    if "External" in sip["annotations"]:
+                        body += pad + sip["decl"]
+                        body += " /External/;\n"
+                    else:
+                        body = pad + "// Discarded {} (by {})\n".format(SipGenerator.describe(container), "default forward declaration handling")
+
+                else:
+                    body = pad + "// Discarded {} (by {})\n".format(SipGenerator.describe(container), modifying_rule)
+        else:
             #
             # Generate private copy constructor for non-copyable types.
             #
@@ -345,33 +366,24 @@ class SipGenerator(object):
             #
             # Flesh out the SIP context for the rules engine.
             #
-            sip["template_parameters"] = template_type_parameters
-            sip["decl"] = container_type
             sip["base_specifiers"] = base_specifiers
             sip["body"] = body
             modifying_rule = self.rules.container_rules().apply(container, sip)
-            pad = " " * (level * 4)
             if sip["name"]:
                 decl = ""
                 if modifying_rule:
                     decl += "// Modified {} (by {}):\n".format(SipGenerator.describe(container), modifying_rule)
                 decl += pad + sip["decl"]
-                if "External" in sip["annotations"]:
-                    #
-                    # SIP /External/ does not seem to work as one might wish. Suppress.
-                    #
-                    body = decl + " /External/;\n"
-                    body = pad + "// Discarded {} (by {})\n".format(SipGenerator.describe(container), "/External/ handling")
-                else:
-                    if sip["base_specifiers"]:
-                        decl += ": " + ", ".join(sip["base_specifiers"])
-                    if sip["annotations"]:
-                        decl += " /" + ",".join(sip["annotations"]) + "/"
-                    if sip["template_parameters"]:
-                        decl = pad + "template <" + ", ".join(sip["template_parameters"]) + ">\n" + decl
-                    decl += "\n" + pad + "{\n"
-                    decl += "%TypeHeaderCode\n#include <{}>\n%End\n".format(include_filename)
-                    body = decl + sip["body"] + pad + "};\n"
+
+                if sip["base_specifiers"]:
+                    decl += ": " + ", ".join(sip["base_specifiers"])
+                if sip["annotations"]:
+                    decl += " /" + ",".join(sip["annotations"]) + "/"
+                if sip["template_parameters"]:
+                    decl = pad + "template <" + ", ".join(sip["template_parameters"]) + ">\n" + decl
+                decl += "\n" + pad + "{\n"
+                decl += "%TypeHeaderCode\n#include <{}>\n%End\n".format(include_filename)
+                body = decl + sip["body"] + pad + "};\n"
             else:
                 body = pad + "// Discarded {} (by {})\n".format(SipGenerator.describe(container), modifying_rule)
         return body
diff --git a/tests/GenerateSipBindings/CMakeLists.txt b/tests/GenerateSipBindings/CMakeLists.txt
index 223b2fed..c223bcef 100644
--- a/tests/GenerateSipBindings/CMakeLists.txt
+++ b/tests/GenerateSipBindings/CMakeLists.txt
@@ -10,8 +10,15 @@ set(CMAKE_INCLUDE_CURRENT_DIR_IN_INTERFACE ON)
 
 set(CMAKE_CXX_STANDARD 14)
 
+add_library(ExternalLib SHARED external_lib.cpp)
+target_link_libraries(ExternalLib PUBLIC Qt5::Core)
+target_compile_features(ExternalLib PUBLIC cxx_nullptr)
+
 add_library(CppLib SHARED cpplib.cpp)
-target_link_libraries(CppLib PUBLIC Qt5::Core)
+target_link_libraries(CppLib
+    PUBLIC Qt5::Core
+    PRIVATE ExternalLib
+)
 target_compile_features(CppLib PUBLIC cxx_nullptr)
 
 list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../../find-modules)
@@ -19,6 +26,16 @@ list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/../../find-modules)
 find_package(PythonModuleGeneration REQUIRED)
 
 ecm_generate_python_binding(
+  TARGET ExternalLib
+  PYTHONNAMESPACE PyTest
+  MODULENAME ExternalLib
+  SIP_DEPENDS
+    QtCore/QtCoremod.sip
+  HEADERS
+    external_lib.h
+)
+
+ecm_generate_python_binding(
   TARGET CppLib
   PYTHONNAMESPACE PyTest
   MODULENAME CppLib
diff --git a/tests/GenerateSipBindings/cpplib.cpp b/tests/GenerateSipBindings/cpplib.cpp
index 8dc7492b..524a0936 100644
--- a/tests/GenerateSipBindings/cpplib.cpp
+++ b/tests/GenerateSipBindings/cpplib.cpp
@@ -1,6 +1,8 @@
 
 #include "cpplib.h"
 
+#include "external_lib.h"
+
 MyObject::MyObject(QObject* parent)
   : QObject(parent)
 {
@@ -65,19 +67,21 @@ int MyObject::groups(unsigned int maxCount) const
   return maxCount;
 }
 
-class FwdDecl
+int MyObject::externalFwdDecl(const ExternalFwdDecl& f)
 {
+  return f.getValue();
+}
 
-};
-
-int MyObject::fwdDecl(const FwdDecl&)
+int MyObject::externalFwdDeclRef(ExternalFwdDecl& f)
 {
-  return 42;
+  return f.getValue();
 }
 
-int MyObject::fwdDeclRef(FwdDecl&)
+int MyObject::localDeclListDecl(const QList<LocalFwdDecl>& l)
 {
-  return 42;
+  return std::accumulate(l.begin(), l.end(), 0, [](int current, LocalFwdDecl const& next){
+    return current + next.getValue();
+  });
 }
 
 int MyObject::const_parameters(const int input, QObject* const obj) const
@@ -86,6 +90,28 @@ int MyObject::const_parameters(const int input, QObject* const obj) const
   return input / 2;
 }
 
+int MyObject::localFwdDecl(const LocalFwdDecl& f)
+{
+  return f.getValue();
+}
+
+int MyObject::localListDecl(const QList<int>& l)
+{
+  return std::accumulate(l.begin(), l.end(), 0);
+}
+
+LocalFwdDecl::LocalFwdDecl(int value)
+  : m_value(value)
+{
+
+}
+
+int LocalFwdDecl::getValue() const
+{
+  return m_value;
+}
+
+
 NonCopyable::NonCopyable()
   : mNum(new int(42))
 {
diff --git a/tests/GenerateSipBindings/cpplib.h b/tests/GenerateSipBindings/cpplib.h
index 9b9adcba..3ae9448a 100644
--- a/tests/GenerateSipBindings/cpplib.h
+++ b/tests/GenerateSipBindings/cpplib.h
@@ -10,7 +10,10 @@
 
 #include <functional>
 
-class FwdDecl;
+class ExternalFwdDecl;
+class LocalFwdDecl;
+
+template<typename T> class QList;
 
 class MyObject : public QObject
 {
@@ -46,8 +49,14 @@ public:
 
   int const_parameters(const int input, QObject* const obj = 0) const;
 
-  int fwdDecl(const FwdDecl& f);
-  int fwdDeclRef(FwdDecl& f);
+  int externalFwdDecl(const ExternalFwdDecl& f);
+  int externalFwdDeclRef(ExternalFwdDecl& f);
+
+  int localFwdDecl(const LocalFwdDecl& f);
+
+  int localListDecl(const QList<int>& l);
+
+  int localDeclListDecl(const QList<LocalFwdDecl>& l);
 
   mode_t dummyFunc(QObject* parent) { return 0; }
 
@@ -77,6 +86,17 @@ private Q_SLOTS:
   void privateSlot2();
 };
 
+class LocalFwdDecl
+{
+public:
+  LocalFwdDecl(int value);
+
+  int getValue() const;
+
+private:
+  int m_value;
+};
+
 class NonCopyable
 {
 public:
diff --git a/tests/GenerateSipBindings/external_lib.cpp b/tests/GenerateSipBindings/external_lib.cpp
new file mode 100644
index 00000000..a08125ff
--- /dev/null
+++ b/tests/GenerateSipBindings/external_lib.cpp
@@ -0,0 +1,13 @@
+
+#include "external_lib.h"
+
+ExternalFwdDecl::ExternalFwdDecl(int value)
+  : m_value(value)
+{
+
+}
+
+int ExternalFwdDecl::getValue() const
+{
+  return m_value;
+}
diff --git a/tests/GenerateSipBindings/external_lib.h b/tests/GenerateSipBindings/external_lib.h
new file mode 100644
index 00000000..f5ea38b2
--- /dev/null
+++ b/tests/GenerateSipBindings/external_lib.h
@@ -0,0 +1,12 @@
+
+#pragma once
+
+class ExternalFwdDecl
+{
+public:
+  ExternalFwdDecl(int value);
+
+  int getValue() const;
+private:
+  int m_value;
+};
diff --git a/tests/GenerateSipBindings/rules_SipTest.py b/tests/GenerateSipBindings/rules_SipTest.py
index ad3fcb64..c570a039 100644
--- a/tests/GenerateSipBindings/rules_SipTest.py
+++ b/tests/GenerateSipBindings/rules_SipTest.py
@@ -10,10 +10,13 @@ def local_container_rules():
         [".*", "Shared", ".*", ".*", ".*", rules_engine.discard_QSharedData_base]
     ]
 
+def local_forward_declaration_rules():
+    return [
+        [".*", "ExternalFwdDecl", ".*", rules_engine.mark_forward_declaration_external]
+    ]
+
 def local_function_rules():
     return [
-        ["MyObject", "fwdDecl", ".*", ".*", ".*", rules_engine.function_discard],
-        ["MyObject", "fwdDeclRef", ".*", ".*", ".*", rules_engine.function_discard],
         ["TypedefUser", "setTagPattern", ".*", ".*", ".*", rules_engine.function_discard],
     ]
 
@@ -34,6 +37,7 @@ class RuleSet(Qt5Ruleset.RuleSet):
     def __init__(self):
         Qt5Ruleset.RuleSet.__init__(self)
         self._container_db = rules_engine.ContainerRuleDb(lambda: local_container_rules() + Qt5Ruleset.container_rules())
+        self._forward_declaration_db = rules_engine.ForwardDeclarationRuleDb(lambda: local_forward_declaration_rules() + Qt5Ruleset.forward_declaration_rules())
         self._fn_db = rules_engine.FunctionRuleDb(lambda: local_function_rules() + Qt5Ruleset.function_rules())
         self._typedef_db = rules_engine.TypedefRuleDb(lambda: local_typedef_rules() + Qt5Ruleset.typedef_rules())
         self._modulecode = rules_engine.ModuleCodeDb({
diff --git a/tests/GenerateSipBindings/testscript.py b/tests/GenerateSipBindings/testscript.py
index 9faea837..9e12bd17 100644
--- a/tests/GenerateSipBindings/testscript.py
+++ b/tests/GenerateSipBindings/testscript.py
@@ -26,6 +26,22 @@ assert(mo.const_parameters(30, mo) == 10)
 assert(mo.qtEnumTest(QtCore.Qt.MatchContains | QtCore.Qt.MatchStartsWith) == 3)
 assert(mo.localEnumTest(PyTest.CppLib.MyObject.Val2) == 2)
 
+lfd = PyTest.CppLib.LocalFwdDecl(18)
+
+assert(mo.localFwdDecl(lfd) == 18)
+
+import PyTest.ExternalLib
+
+efd = PyTest.ExternalLib.ExternalFwdDecl(18)
+
+assert(mo.externalFwdDecl(efd) == 18)
+
+assert(mo.localListDecl([1, 5, 7]) == 13)
+
+lfdl = [PyTest.CppLib.LocalFwdDecl(3), PyTest.CppLib.LocalFwdDecl(6)]
+
+assert(mo.localDeclListDecl(lfdl) == 9)
+
 #
 # Verify that an enum with attributes can be read.
 #