chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx | 1
compilerplugins/clang/check.cxx | 4
compilerplugins/clang/check.hxx | 5
compilerplugins/clang/test/unoaggregation.cxx | 42 +
compilerplugins/clang/unoaggregation.cxx | 215
++++++++++
solenv/CompilerTest_compilerplugins_clang.mk | 1
6 files changed, 264 insertions(+), 4 deletions(-)
New commits:
commit 022085ece4149a31d22cc85028e71cd2340409ed
Author: Stephan Bergmann <sberg...@redhat.com>
AuthorDate: Fri Jan 20 15:21:15 2023 +0100
Commit: Stephan Bergmann <sberg...@redhat.com>
CommitDate: Fri Jan 20 20:26:36 2023 +0000
New loplugin:unoaggregation
"Find classes that derive from css::uno::XAggregation, but which implement
queryInterface in violation of the protocol laid out in the documentation at
udkapi/com/sun/star/uno/XAggregation.idl (which implies that such a class
either
doesn't actually make use of the deprecated XAggregation mechanism, which
should
thus be removed from that class hierarchy, or that its implementation of
queryInterface needs to be fixed)."
(compilerplugins/clang/unoaggregation.cxx)
The basesHaveOnlyPureQueryInterface check in
compilerplugins/clang/unoaggregation.cxx is "a crude approximation (but
which
appears to work OK)" to filter out any queryInterface base implementations
of
class hierarchies supporting XAggregation (i.e.,
cppu::OWeakAggObject::queryInterface). It only fails for the odd
ChartDocumentWrapper::queryInterface, which manually implements the
XAggregation
functionality rather than (indirectly) deriving from OWeakAggObject. But
changing that manual implementation looks tricky, so leave that alone for
now
and add a loplugin warning suppression there, and leave proper clean up for
later.
Change-Id: Ib16e26237bd703e60b0d9cb7134cb39840842d54
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/145912
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sberg...@redhat.com>
diff --git a/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx
b/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx
index 51f066c3ba3e..5ce060e9891f 100644
--- a/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx
+++ b/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx
@@ -653,6 +653,7 @@ ChartDocumentWrapper::~ChartDocumentWrapper()
}
// ____ XInterface (for new interfaces) ____
+// [-loplugin:unoaggregation]
uno::Any SAL_CALL ChartDocumentWrapper::queryInterface( const uno::Type& aType
)
{
if( m_xDelegator.is())
diff --git a/compilerplugins/clang/check.cxx b/compilerplugins/clang/check.cxx
index 7be98bf97d5c..4ff081b6923e 100644
--- a/compilerplugins/clang/check.cxx
+++ b/compilerplugins/clang/check.cxx
@@ -374,10 +374,10 @@ static bool BaseCheckNotSubclass(const
clang::CXXRecordDecl *BaseDefinition, voi
return true;
}
-bool isDerivedFrom(const clang::CXXRecordDecl *decl, DeclChecker base) {
+bool isDerivedFrom(const clang::CXXRecordDecl *decl, DeclChecker base, bool
checkSelf) {
if (!decl)
return false;
- if (base(decl))
+ if (checkSelf && base(decl))
return true;
if (!decl->hasDefinition()) {
return false;
diff --git a/compilerplugins/clang/check.hxx b/compilerplugins/clang/check.hxx
index 4ac311759832..a7b7ba820d09 100644
--- a/compilerplugins/clang/check.hxx
+++ b/compilerplugins/clang/check.hxx
@@ -160,8 +160,9 @@ private:
typedef std::function<bool(clang::Decl const *)> DeclChecker;
-// Returns true if the class has a base matching the checker, or if the class
itself matches.
-bool isDerivedFrom(const clang::CXXRecordDecl *decl, DeclChecker base);
+// Returns true if the class has a base matching the checker, or, when
checkSelf is true, if the
+// class itself matches.
+bool isDerivedFrom(const clang::CXXRecordDecl *decl, DeclChecker base, bool
checkSelf = true);
namespace detail {
diff --git a/compilerplugins/clang/test/unoaggregation.cxx
b/compilerplugins/clang/test/unoaggregation.cxx
new file mode 100644
index 000000000000..01e0dd832e9d
--- /dev/null
+++ b/compilerplugins/clang/test/unoaggregation.cxx
@@ -0,0 +1,42 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4;
fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <sal/config.h>
+
+#include <com/sun/star/lang/XInitialization.hpp>
+#include <com/sun/star/lang/XMain.hpp>
+#include <cppuhelper/implbase.hxx>
+#include <cppuhelper/implbase1.hxx>
+#include <sal/types.h>
+
+class Base : public cppu::WeakAggImplHelper1<css::lang::XInitialization>
+{
+public:
+ void SAL_CALL initialize(css::uno::Sequence<css::uno::Any> const&
aArguments) override;
+};
+
+class Good : public Base, public css::lang::XMain
+{
+public:
+ css::uno::Any SAL_CALL queryInterface(css::uno::Type const& aType) override
+ {
+ return Base::queryInterface(aType);
+ }
+};
+
+class Bad : public cppu::ImplInheritanceHelper<Base, css::lang::XMain>
+{
+public:
+ sal_Int32 SAL_CALL run(css::uno::Sequence<OUString> const& aArguments)
override;
+};
+
+// expected-error@cppuhelper/implbase.hxx:* {{'ImplInheritanceHelper<Base,
com::sun::star::lang::XMain>' derives from XAggregation, but its implementation
of queryInterface does not delegate to an appropriate base class queryInterface
[loplugin:unoaggregation]}}
+Bad bad; //make sure Bad's base cppu::ImplInheritanceHelper<Base,
css::lang::XMain> is instantiated
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s
cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unoaggregation.cxx
b/compilerplugins/clang/unoaggregation.cxx
new file mode 100644
index 000000000000..6cad29cf0915
--- /dev/null
+++ b/compilerplugins/clang/unoaggregation.cxx
@@ -0,0 +1,215 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4;
fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+// Find classes that derive from css::uno::XAggregation, but which implement
queryInterface in
+// violation of the protocol laid out in the documentation at
+// udkapi/com/sun/star/uno/XAggregation.idl (which implies that such a class
either doesn't actually
+// make use of the deprecated XAggregation mechanism, which should thus be
removed from that class
+// hierarchy, or that its implementation of queryInterface needs to be fixed).
+
+#ifndef LO_CLANG_SHARED_PLUGINS
+
+#include <cassert>
+
+#include "check.hxx"
+#include "plugin.hxx"
+
+namespace
+{
+bool isQueryInterface(CXXMethodDecl const* decl)
+{
+ auto const id = decl->getIdentifier();
+ if (id == nullptr || id->getName() != "queryInterface")
+ {
+ return false;
+ }
+ if (decl->getNumParams() != 1)
+ {
+ return false;
+ }
+ if (!loplugin::TypeCheck(decl->getParamDecl(0)->getType())
+ .LvalueReference()
+ .ConstNonVolatile()
+ .Class("Type")
+ .Namespace("uno")
+ .Namespace("star")
+ .Namespace("sun")
+ .Namespace("com")
+ .GlobalNamespace())
+ {
+ return false;
+ }
+ return true;
+}
+
+bool derivesFromXAggregation(CXXRecordDecl const* decl, bool checkSelf)
+{
+ return loplugin::isDerivedFrom(decl,
+ [](Decl const* decl) -> bool {
+ return bool(loplugin::DeclCheck(decl)
+ .Class("XAggregation")
+ .Namespace("uno")
+ .Namespace("star")
+ .Namespace("sun")
+ .Namespace("com")
+ .GlobalNamespace());
+ },
+ checkSelf);
+}
+
+// Return true if decl is an implementation of
css::uno::XInterface::queryInterface in a class
+// derived from css::uno::XAggregation:
+bool isXAggregationQueryInterface(CXXMethodDecl const* decl)
+{
+ return isQueryInterface(decl) &&
derivesFromXAggregation(decl->getParent(), false);
+}
+
+bool basesHaveOnlyPureQueryInterface(CXXRecordDecl const* decl)
+{
+ for (auto const& b : decl->bases())
+ {
+ auto const d1 = b.getType()->getAsCXXRecordDecl();
+ if (!derivesFromXAggregation(d1, true))
+ {
+ continue;
+ }
+ for (auto const d2 : d1->methods())
+ {
+ if (!isQueryInterface(d2))
+ {
+ continue;
+ }
+ if (!d2->isPure())
+ {
+ return false;
+ }
+ }
+ if (!basesHaveOnlyPureQueryInterface(d1))
+ {
+ return false;
+ }
+ }
+ return true;
+}
+
+class UnoAggregation final : public loplugin::FilteringPlugin<UnoAggregation>
+{
+public:
+ explicit UnoAggregation(loplugin::InstantiationData const& data)
+ : FilteringPlugin(data)
+ {
+ }
+
+ bool shouldVisitTemplateInstantiations() const { return true; }
+
+ bool preRun() override { return compiler.getLangOpts().CPlusPlus; }
+
+ void run() override
+ {
+ if (preRun())
+ {
+ TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+ }
+ }
+
+ bool VisitCXXMethodDecl(CXXMethodDecl const* decl)
+ {
+ if (ignoreLocation(decl))
+ {
+ return true;
+ }
+ if (!decl->isThisDeclarationADefinition())
+ {
+ return true;
+ }
+ auto const parent = decl->getParent();
+ if (parent->getDescribedClassTemplate() != nullptr)
+ {
+ // For class templates with dependent base classes,
loplugin::isDerivedFrom as used in
+ // isXAggregationQueryInterface would always return true; work
around that by not
+ // looking at any templates at all, which is OK due to
+ // shouldVisitTemplateInstantiations:
+ return true;
+ }
+ if (!isXAggregationQueryInterface(decl))
+ {
+ return true;
+ }
+ if (decl->isDeleted())
+ {
+ // Whether or not a deleted queryInterface makes sense, just leave
those alone:
+ return true;
+ }
+ auto const body = decl->getBody();
+ assert(body != nullptr);
+ // Check whether the implementation forwards to one of the base
classes that derive from
+ // XAggregation:
+ if (auto const s1 = dyn_cast<CompoundStmt>(body))
+ {
+ if (s1->size() == 1)
+ {
+ if (auto const s2 = dyn_cast<ReturnStmt>(s1->body_front()))
+ {
+ if (auto const e1 = s2->getRetValue())
+ {
+ if (auto const e2
+ =
dyn_cast<CXXMemberCallExpr>(e1->IgnoreImplicit()->IgnoreParens()))
+ {
+ return true;
+ if (e2->getImplicitObjectArgument() == nullptr)
+ {
+ if
(isXAggregationQueryInterface(e2->getMethodDecl()))
+ {
+ // e2 will thus necessarily be a call of a
base class's
+ // queryInterface (or a recursive call of
the given decl itself,
+ // but which would cause the code to have
undefined behavior
+ // anyway, so don't bother to rule that
out):
+ return true;
+ }
+ }
+ }
+ }
+ else if (isDebugMode())
+ {
+ report(DiagnosticsEngine::Warning,
+ "suspicious implementation of queryInterface
containing a return "
+ "statement with no operand",
+ decl->getLocation())
+ << decl->getSourceRange();
+ }
+ }
+ }
+ }
+ // As a crude approximation (but which appears to work OK), if all of
the base classes that
+ // derive from XAggregation only ever declare queryInterface as pure,
assume that this is
+ // the base implementation of queryInterface (which will necessarily
not match the above
+ // check for a forwarding implementation):
+ if (basesHaveOnlyPureQueryInterface(parent))
+ {
+ return true;
+ }
+ if (suppressWarningAt(decl->getBeginLoc()))
+ {
+ return true;
+ }
+ report(DiagnosticsEngine::Warning,
+ "%0 derives from XAggregation, but its implementation of
queryInterface does not "
+ "delegate to an appropriate base class queryInterface",
+ decl->getLocation())
+ << parent << decl->getSourceRange();
+ return true;
+ }
+};
+
+loplugin::Plugin::Registration<UnoAggregation>
unoaggregation("unoaggregation");
+}
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s
cinkeys+=0=break: */
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk
b/solenv/CompilerTest_compilerplugins_clang.mk
index 08fd7e974e96..afa7621df37d 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -96,6 +96,7 @@ $(eval $(call
gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
compilerplugins/clang/test/unnecessaryoverride-dtor \
compilerplugins/clang/test/unnecessaryparen \
compilerplugins/clang/test/unnecessarylocking \
+ compilerplugins/clang/test/unoaggregation \
compilerplugins/clang/test/unoany \
compilerplugins/clang/test/unoquery \
compilerplugins/clang/test/unreffun \
