diff options
| -rw-r--r-- | chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx | 1 | ||||
| -rw-r--r-- | compilerplugins/clang/check.cxx | 4 | ||||
| -rw-r--r-- | compilerplugins/clang/check.hxx | 5 | ||||
| -rw-r--r-- | compilerplugins/clang/test/unoaggregation.cxx | 42 | ||||
| -rw-r--r-- | compilerplugins/clang/unoaggregation.cxx | 215 | ||||
| -rw-r--r-- | solenv/CompilerTest_compilerplugins_clang.mk | 1 |
6 files changed, 264 insertions, 4 deletions
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 \ |