make SvXMLImport capable of mixing fast- and slow- contexts adhoc

so I can convert even *ImportContext subclasses in the middle of
a context stack, and thus break the cyclic dependency nature
of the writer import.

and adjust the xmlimport loplugin for the new rules.

As a consequence of the loplugin:xmlimport's checking, we remove
a bunch of now unnecessary overrides of startFastElement.

Change-Id: I97464522ede8ec5e345e928cdafa4b18364b1b80
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104730
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 308 insertions, 136 deletions

diff --git a/compilerplugins/clang/test/xmlimport.cxx b/compilerplugins/clang/test/xmlimport.cxx
index 36230ffdb2d8..98dba400b19e 100644
--- a/compilerplugins/clang/test/xmlimport.cxx
+++ b/compilerplugins/clang/test/xmlimport.cxx
@@ -11,78 +11,185 @@
 
 // Cannot include this, makes clang crash
 //#include "xmloff/xmlimp.hxx"
+// Cannot include this, cannot be found
+//#include <xmloff/xmlictxt.hxx>
 
 #include <com/sun/star/uno/Reference.hxx>
+#include <rtl/ref.hxx>
 
 namespace com::sun::star::xml::sax
 {
 class XAttributeList;
+class XFastContextHandler;
 }
 
+class SvXMLImportContext;
+typedef rtl::Reference<SvXMLImportContext> SvXMLImportContextRef;
 class SvXMLImportContext
 {
 public:
     virtual ~SvXMLImportContext() {}
 
-    virtual void createFastChildContext() {}
     virtual void startFastElement() {}
     virtual void endFastElement() {}
+    virtual void characters(const OUString&) {}
+    virtual css::uno::Reference<css::xml::sax::XFastContextHandler> createFastChildContext()
+    {
+        return nullptr;
+    }
+    virtual css::uno::Reference<css::xml::sax::XFastContextHandler> createUnknownChildContext()
+    {
+        return nullptr;
+    }
 
     virtual void StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>&) {}
     virtual void EndElement() {}
     virtual void Characters(const OUString&) {}
+    virtual SvXMLImportContextRef CreateChildContext() { return nullptr; }
+
+    void acquire();
+    void release();
+
+    void xxx(); // just here to avoid triggering a warning I don't want to check for
 };
 
 class Test1 : public SvXMLImportContext
 {
 public:
-    // expected-error@+1 {{must override startFastElement too [loplugin:xmlimport]}}
-    virtual void createFastChildContext() override;
+    // expected-error@+1 {{cannot override both startFastElement and StartElement [loplugin:xmlimport]}}
+    virtual void startFastElement() override { xxx(); }
+    // expected-error@+1 {{cannot override both startFastElement and StartElement [loplugin:xmlimport]}}
+    virtual void StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>&) override
+    {
+        xxx();
+    }
 };
 
 class Test2 : public SvXMLImportContext
 {
 public:
-    // no warning expected
-    virtual void createFastChildContext() override;
-    virtual void startFastElement() override {}
+    // expected-error@+1 {{cannot override both endFastElement and EndElement [loplugin:xmlimport]}}
+    virtual void endFastElement() override { xxx(); }
+    // expected-error@+1 {{cannot override both endFastElement and EndElement [loplugin:xmlimport]}}
+    virtual void EndElement() override { xxx(); }
 };
 
-class Test3 : public Test2
+class Test3 : public SvXMLImportContext
 {
 public:
-    // no warning expected
-    virtual void createFastChildContext() override;
+    // expected-error@+1 {{cannot override both characters and Characters [loplugin:xmlimport]}}
+    virtual void Characters(const OUString&) override { xxx(); }
+    // expected-error@+1 {{cannot override both characters and Characters [loplugin:xmlimport]}}
+    virtual void characters(const OUString&) override { xxx(); }
 };
 
-class Test4 : public SvXMLImportContext
+class Test7 : public SvXMLImportContext
 {
 public:
-    // expected-error@+1 {{must override startFastElement too [loplugin:xmlimport]}}
-    virtual void endFastElement() override;
+    virtual void startFastElement() override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        SvXMLImportContext::startFastElement();
+    }
+    virtual void endFastElement() override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        SvXMLImportContext::endFastElement();
+    }
+    virtual void characters(const OUString& rChars) override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        SvXMLImportContext::characters(rChars);
+    }
+    virtual css::uno::Reference<css::xml::sax::XFastContextHandler>
+    createFastChildContext() override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        return SvXMLImportContext::createFastChildContext();
+    }
+    virtual css::uno::Reference<css::xml::sax::XFastContextHandler>
+    createUnknownChildContext() override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        return SvXMLImportContext::createUnknownChildContext();
+    }
 };
 
-class Test5 : public SvXMLImportContext
+class Test8 : public SvXMLImportContext
 {
 public:
-    // expected-error@+1 {{overrides startElement, but looks like a fastparser context class, no constructor that takes slowparser args [loplugin:xmlimport]}}
     virtual void
-    StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>& xAttrList) override;
-    // expected-error@+1 {{overrides startElement, but looks like a fastparser context class, no constructor that takes slowparser args [loplugin:xmlimport]}}
-    virtual void EndElement() override;
-    // expected-error@+1 {{overrides startElement, but looks like a fastparser context class, no constructor that takes slowparser args [loplugin:xmlimport]}}
-    virtual void Characters(const OUString&) override;
+    StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>& xAttrList) override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        SvXMLImportContext::StartElement(xAttrList);
+    }
+    virtual void EndElement() override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        SvXMLImportContext::EndElement();
+    }
+    virtual void Characters(const OUString& rChars) override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        SvXMLImportContext::Characters(rChars);
+    }
+    virtual SvXMLImportContextRef CreateChildContext() override
+    {
+        // expected-error@+1 {{don't call this superclass method [loplugin:xmlimport]}}
+        return SvXMLImportContext::CreateChildContext();
+    }
 };
 
 // no warning expected
-class Test6 : public SvXMLImportContext
+class Test9a : public SvXMLImportContext
+{
+public:
+    virtual void StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>&) override
+    {
+        xxx();
+    }
+};
+class Test9b : public Test9a
 {
 public:
-    Test6(sal_uInt16, const OUString&);
     virtual void
-    StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>& xAttrList) override;
-    virtual void EndElement() override;
-    virtual void Characters(const OUString&) override;
+    StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>& xAttrList) override
+    {
+        Test9a::StartElement(xAttrList);
+    }
+};
+
+class Test10a : public SvXMLImportContext
+{
+public:
+    // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}}
+    virtual void startFastElement() override {}
+    // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}}
+    virtual void endFastElement() override {}
+    // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}}
+    virtual void characters(const OUString&) override {}
+    // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}}
+    virtual css::uno::Reference<css::xml::sax::XFastContextHandler>
+    createFastChildContext() override
+    {
+        return nullptr;
+    }
+    // expected-error@+1 {{empty, should be removed [loplugin:xmlimport]}}
+    virtual css::uno::Reference<css::xml::sax::XFastContextHandler>
+    createUnknownChildContext() override
+    {
+        return nullptr;
+    }
+};
+// no warning expected
+class Test10b : public SvXMLImportContext
+{
+public:
+    virtual void StartElement(const css::uno::Reference<css::xml::sax::XAttributeList>&) override {}
+    virtual void EndElement() override {}
+    virtual void Characters(const OUString&) override {}
+    virtual SvXMLImportContextRef CreateChildContext() override { return nullptr; }
 };
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/xmlimport.cxx b/compilerplugins/clang/xmlimport.cxx
index d6b1aa78e325..72645564a5d1 100644
--- a/compilerplugins/clang/xmlimport.cxx
+++ b/compilerplugins/clang/xmlimport.cxx
@@ -14,6 +14,7 @@
 #include "plugin.hxx"
 #include "check.hxx"
 #include <iostream>
+#include <unordered_map>
 #include "clang/AST/CXXInheritance.h"
 
 /*
@@ -36,8 +37,23 @@ public:
 
     bool preRun() override
     {
-        //        std::string fn(handler.getMainFileName());
-        //        loplugin::normalizeDotDotInFilePath(fn);
+        StringRef fn(handler.getMainFileName());
+        if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/core/xmlictxt.cxx"))
+            return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/core/xmlimp.cxx"))
+            return false;
+        // These are mostly classes delegating calls to other classes
+        if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/text/XMLTextFrameContext.cxx"))
+            return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/draw/ximpshap.cxx"))
+            return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/xmloff/source/table/XMLTableImport.cxx"))
+            return false;
+        if (loplugin::isSamePathname(fn,
+                                     SRCDIR "/sc/source/filter/xml/XMLTrackedChangesContext.cxx"))
+            return false;
+        if (loplugin::isSamePathname(fn, SRCDIR "/sc/source/filter/xml/xmlannoi.cxx"))
+            return false;
         return true;
     }
 
@@ -50,21 +66,16 @@ public:
     }
 
     bool VisitCXXMethodDecl(const CXXMethodDecl*);
-};
+    bool VisitCXXMemberCallExpr(const CXXMemberCallExpr*);
 
-static bool containsStartFastElementMethod(const CXXRecordDecl* cxxRecordDecl)
-{
-    auto dc = loplugin::DeclCheck(cxxRecordDecl);
-    if (dc.Class("XFastContextHandler"))
-        return false;
-    for (auto it = cxxRecordDecl->method_begin(); it != cxxRecordDecl->method_end(); ++it)
-    {
-        auto i = *it;
-        if (i->getIdentifier() && i->getName() == "startFastElement")
-            return true;
-    }
-    return false;
-}
+private:
+    std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> startFastElementSet;
+    std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> StartElementSet;
+    std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> endFastElementSet;
+    std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> EndElementSet;
+    std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> charactersSet;
+    std::unordered_map<const CXXRecordDecl*, const CXXMethodDecl*> CharactersSet;
+};
 
 bool XmlImport::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
 {
@@ -82,111 +93,165 @@ bool XmlImport::VisitCXXMethodDecl(const CXXMethodDecl* methodDecl)
     if (loplugin::DeclCheck(cxxRecordDecl).Class("SvXMLImportContext"))
         return true;
 
-    if (methodDecl->getName() == "createFastChildContext" || methodDecl->getName() == "characters"
-        || methodDecl->getName() == "endFastElement")
+    if (!loplugin::isDerivedFrom(cxxRecordDecl, [](Decl const* decl) -> bool {
+            auto const dc = loplugin::DeclCheck(decl);
+            return bool(dc.ClassOrStruct("SvXMLImportContext").GlobalNamespace());
+        }))
+        return true;
+
+    auto name = methodDecl->getName();
+    if (name == "startFastElement")
+        startFastElementSet.insert({ cxxRecordDecl, methodDecl });
+    else if (name == "StartElement")
+        StartElementSet.insert({ cxxRecordDecl, methodDecl });
+    else if (name == "endFastElement")
+        endFastElementSet.insert({ cxxRecordDecl, methodDecl });
+    else if (name == "EndElement")
+        EndElementSet.insert({ cxxRecordDecl, methodDecl });
+    else if (name == "characters")
     {
-        auto className = cxxRecordDecl->getName();
-        if (className == "OOXMLFactory") // writerfilter
-            return true;
-        if (className == "SvXMLLegacyToFastDocHandler" || className == "ImportDocumentHandler"
-            || className == "ExportDocumentHandler") // reportdesign
-            return true;
-        if (className == "XMLEmbeddedObjectExportFilter" || className == "XMLBasicExportFilter"
-            || className == "XMLTransformerBase" || className == "SvXMLMetaExport") // xmloff
-            return true;
-
-        if (containsStartFastElementMethod(cxxRecordDecl))
-            return true;
-
-        bool foundStartFastElement = false;
-        bool foundImportContext = false;
-
-        CXXBasePaths aPaths;
-        cxxRecordDecl->lookupInBases(
-            [&](const CXXBaseSpecifier* Specifier, CXXBasePath & /*Path*/) -> bool {
-                if (!Specifier->getType().getTypePtr())
-                    return false;
-                const CXXRecordDecl* baseCXXRecordDecl = Specifier->getType()->getAsCXXRecordDecl();
-                if (!baseCXXRecordDecl)
-                    return false;
-                if (baseCXXRecordDecl->isInvalidDecl())
-                    return false;
-                if (loplugin::DeclCheck(baseCXXRecordDecl).Class("SvXMLImportContext"))
-                    foundImportContext |= true;
-                else
-                    foundStartFastElement |= containsStartFastElementMethod(baseCXXRecordDecl);
-                return false;
-            },
-            aPaths);
-
-        if (foundImportContext && !foundStartFastElement)
-            report(DiagnosticsEngine::Warning, "must override startFastElement too",
-                   compat::getBeginLoc(methodDecl))
-                << methodDecl->getSourceRange();
+        if (methodDecl->getNumParams() == 1)
+            charactersSet.insert({ cxxRecordDecl, methodDecl });
     }
-    else if (methodDecl->getName() == "StartElement" || methodDecl->getName() == "EndElement"
-             || methodDecl->getName() == "Characters")
+    else if (name == "Characters")
     {
-        if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLAxisContext"))
-            return true;
-        if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLChartContext"))
-            return true;
-        if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLParagraphContext"))
-            return true;
-        if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLLegendContext"))
-            return true;
-        if (loplugin::DeclCheck(cxxRecordDecl).Class("SchXMLPropertyMappingContext"))
-            return true;
-
-        bool foundImportContext = false;
-        CXXBasePaths aPaths;
-        cxxRecordDecl->lookupInBases(
-            [&](const CXXBaseSpecifier* Specifier, CXXBasePath & /*Path*/) -> bool {
-                if (!Specifier->getType().getTypePtr())
-                    return false;
-                const CXXRecordDecl* baseCXXRecordDecl = Specifier->getType()->getAsCXXRecordDecl();
-                if (!baseCXXRecordDecl)
-                    return false;
-                if (baseCXXRecordDecl->isInvalidDecl())
-                    return false;
-                if (loplugin::DeclCheck(baseCXXRecordDecl).Class("SvXMLImportContext"))
-                    foundImportContext |= true;
-                return false;
-            },
-            aPaths);
-
-        if (!foundImportContext)
-            return true;
-
-        bool foundConstructor = false;
-        for (auto it = cxxRecordDecl->ctor_begin(); it != cxxRecordDecl->ctor_end(); ++it)
+        if (methodDecl->getNumParams() == 1)
+            CharactersSet.insert({ cxxRecordDecl, methodDecl });
+    }
+
+    {
+        auto it1 = endFastElementSet.find(cxxRecordDecl);
+        auto it2 = EndElementSet.find(cxxRecordDecl);
+        if (it1 != endFastElementSet.end() && it2 != EndElementSet.end())
         {
-            const CXXConstructorDecl* ctor = *it;
-            bool foundInt16 = false;
-            for (auto paramIt = ctor->param_begin(); paramIt != ctor->param_end(); ++paramIt)
-            {
-                const ParmVarDecl* pvd = *paramIt;
-                auto tc = loplugin::TypeCheck(pvd->getType());
-                if (tc.Typedef("sal_uInt16"))
-                    foundInt16 = true;
-                else if (tc.LvalueReference().Const().Class("OUString") && foundInt16)
-                    foundConstructor = true;
-                else
-                    foundInt16 = false;
-                if (tc.LvalueReference().Const().Class("OUString")
-                    && pvd->getName() == "rLocalName")
-                    foundConstructor = true;
-            }
+            auto methodDecl1 = it1->second;
+            report(DiagnosticsEngine::Warning, "cannot override both endFastElement and EndElement",
+                   compat::getBeginLoc(methodDecl1))
+                << methodDecl1->getSourceRange();
+            auto methodDecl2 = it2->second;
+            report(DiagnosticsEngine::Warning, "cannot override both endFastElement and EndElement",
+                   compat::getBeginLoc(methodDecl2))
+                << methodDecl2->getSourceRange();
         }
+    }
 
-        if (!foundConstructor)
+    {
+        auto it1 = startFastElementSet.find(cxxRecordDecl);
+        auto it2 = StartElementSet.find(cxxRecordDecl);
+        if (it1 != startFastElementSet.end() && it2 != StartElementSet.end())
+        {
+            auto methodDecl1 = it1->second;
+            report(DiagnosticsEngine::Warning,
+                   "cannot override both startFastElement and StartElement",
+                   compat::getBeginLoc(methodDecl1))
+                << methodDecl1->getSourceRange();
+            auto methodDecl2 = it2->second;
             report(DiagnosticsEngine::Warning,
-                   "overrides startElement, but looks like a fastparser context class, no "
-                   "constructor that takes slowparser args",
-                   compat::getBeginLoc(methodDecl))
-                << methodDecl->getSourceRange();
+                   "cannot override both startFastElement and StartElement",
+                   compat::getBeginLoc(methodDecl2))
+                << methodDecl2->getSourceRange();
+        }
+    }
+    {
+        auto it1 = charactersSet.find(cxxRecordDecl);
+        auto it2 = CharactersSet.find(cxxRecordDecl);
+        if (it1 != charactersSet.end() && it2 != CharactersSet.end())
+        {
+            auto methodDecl1 = it1->second;
+            report(DiagnosticsEngine::Warning, "cannot override both characters and Characters",
+                   compat::getBeginLoc(methodDecl1))
+                << methodDecl1->getSourceRange();
+            auto methodDecl2 = it2->second;
+            report(DiagnosticsEngine::Warning, "cannot override both characters and Characters",
+                   compat::getBeginLoc(methodDecl2))
+                << methodDecl2->getSourceRange();
+        }
     }
 
+    auto checkEmpty = [&]() {
+        if (!methodDecl->isThisDeclarationADefinition())
+            return;
+        auto compoundStmt = dyn_cast_or_null<CompoundStmt>(methodDecl->getBody());
+        if (compoundStmt == nullptr || compoundStmt->size() > 0)
+            return;
+        report(DiagnosticsEngine::Warning, "empty, should be removed",
+               compat::getBeginLoc(methodDecl))
+            << methodDecl->getSourceRange();
+        auto canonicalDecl = methodDecl->getCanonicalDecl();
+        if (canonicalDecl != methodDecl)
+            report(DiagnosticsEngine::Note, "definition here", compat::getBeginLoc(canonicalDecl))
+                << canonicalDecl->getSourceRange();
+    };
+    auto checkOnlyReturn = [&]() {
+        if (!methodDecl->isThisDeclarationADefinition())
+            return;
+        auto compoundStmt = dyn_cast_or_null<CompoundStmt>(methodDecl->getBody());
+        if (compoundStmt == nullptr || compoundStmt->size() > 1)
+            return;
+        auto returnStmt = dyn_cast_or_null<ReturnStmt>(*compoundStmt->body_begin());
+        if (!returnStmt)
+            return;
+        auto cxxConstructExpr
+            = dyn_cast_or_null<CXXConstructExpr>(returnStmt->getRetValue()->IgnoreImplicit());
+        if (!cxxConstructExpr)
+            return;
+        if (cxxConstructExpr->getNumArgs() != 1)
+            return;
+        if (!isa<CXXNullPtrLiteralExpr>(cxxConstructExpr->getArg(0)->IgnoreImplicit()))
+            return;
+        report(DiagnosticsEngine::Warning, "empty, should be removed",
+               compat::getBeginLoc(methodDecl))
+            << methodDecl->getSourceRange();
+        auto canonicalDecl = methodDecl->getCanonicalDecl();
+        if (canonicalDecl != methodDecl)
+            report(DiagnosticsEngine::Note, "definition here", compat::getBeginLoc(canonicalDecl))
+                << canonicalDecl->getSourceRange();
+    };
+
+    if (name == "startFastElement")
+        checkEmpty();
+    else if (name == "endFastElement")
+        checkEmpty();
+    else if (name == "characters")
+        checkEmpty();
+    else if (name == "createFastChildContext")
+        checkOnlyReturn();
+    else if (name == "createUnknownChildContext")
+        checkOnlyReturn();
+
+    return true;
+}
+
+bool XmlImport::VisitCXXMemberCallExpr(const CXXMemberCallExpr* callExpr)
+{
+    auto beginLoc = compat::getBeginLoc(callExpr);
+    if (!beginLoc.isValid() || ignoreLocation(callExpr))
+        return true;
+
+    CXXMethodDecl* methodDecl = callExpr->getMethodDecl();
+    if (!methodDecl || !methodDecl->getIdentifier())
+        return true;
+
+    auto cxxRecordDecl = methodDecl->getParent();
+    if (!cxxRecordDecl || !cxxRecordDecl->getIdentifier())
+        return true;
+
+    if (!loplugin::DeclCheck(cxxRecordDecl).Class("SvXMLImportContext"))
+        return true;
+
+    auto name = methodDecl->getName();
+    if (name == "startFastElement" || name == "characters" || name == "endFastElement"
+        || name == "createFastChildContext" || name == "createUnknownChildContext"
+        || name == "StartElement" || name == "EndElement" || name == "Characters"
+        || name == "CreateChildContext")
+    {
+        /**
+         * Calling this superclass method from a subclass method will mess with the fallback logic in the superclass.
+         */
+        report(DiagnosticsEngine::Warning, "don't call this superclass method",
+               compat::getBeginLoc(callExpr))
+            << callExpr->getSourceRange();
+    }
     return true;
 }