diff --git a/compilerplugins/clang/store/constmethod.cxx b/compilerplugins/clang/store/constmethod.cxx new file mode 100644 index 000000000000..d4bae3f015f3 --- /dev/null +++ b/compilerplugins/clang/store/constmethod.cxx @@ -0,0 +1,523 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * 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 +#include +#include +#include +#include + +#include "plugin.hxx" +#include "compat.hxx" +#include "check.hxx" +#include "functionaddress.hxx" + +/** + Find methods that can be declared const. + + This analysis attempts to implement "logical const" as opposed to "technical const", which means + we ignore always-const nature of std::unique_ptr::operator-> + + This is not a sophisticated analysis. It deliberately skips all of the hard cases for now. + It is an exercise in getting the most benefit for the least effort. +*/ +namespace +{ + +static bool startswith(const std::string& rStr, const char* pSubStr) { + return rStr.compare(0, strlen(pSubStr), pSubStr) == 0; +} + +class ConstMethod: + public loplugin::FunctionAddress +{ +public: + explicit ConstMethod(InstantiationData const & data): loplugin::FunctionAddress(data) {} + + virtual void run() override { + std::string fn( compiler.getSourceManager().getFileEntryForID( + compiler.getSourceManager().getMainFileID())->getName() ); + normalizeDotDotInFilePath(fn); + if (fn.find("/qa/") != std::string::npos) + return; + // the rest of the stuff in these folders is technically const, but not logically const (IMO) + if (startswith(fn, SRCDIR "/unotools/")) + return; + if (startswith(fn, SRCDIR "/svl/")) + return; + if (startswith(fn, SRCDIR "/binaryurp/")) + return; + if (startswith(fn, SRCDIR "/cpputools/")) + return; + if (startswith(fn, SRCDIR "/opencl/")) + return; + if (startswith(fn, SRCDIR "/helpcompiler/")) + return; + // very little in here can usefully be made const. Could tighten this up a little and only exclude stuff declared in .cxx files + if (startswith(fn, SRCDIR "/vcl/")) + return; + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + + for (const CXXMethodDecl *pMethodDecl : interestingMethodSet) { + if (methodCannotBeConstSet.find(pMethodDecl) != methodCannotBeConstSet.end()) + continue; + auto canonicalDecl = pMethodDecl->getCanonicalDecl(); + if (getFunctionsWithAddressTaken().find((FunctionDecl const *)canonicalDecl) + != getFunctionsWithAddressTaken().end()) + continue; + StringRef aFileName = compiler.getSourceManager().getFilename(compiler.getSourceManager().getSpellingLoc(canonicalDecl->getLocStart())); + if (loplugin::isSamePathname(aFileName, SRCDIR "/include/LibreOfficeKit/LibreOfficeKit.hxx")) + continue; + report( + DiagnosticsEngine::Warning, + "this method can be const", + pMethodDecl->getLocStart()) + << pMethodDecl->getSourceRange(); + if (canonicalDecl->getLocation() != pMethodDecl->getLocation()) { + report( + DiagnosticsEngine::Note, + "canonical method declaration here", + canonicalDecl->getLocStart()) + << canonicalDecl->getSourceRange(); + } + //pMethodDecl->dump(); + } + } + + bool TraverseCXXMethodDecl(CXXMethodDecl *); + bool TraverseCXXConversionDecl(CXXConversionDecl *); + bool VisitCXXMethodDecl(const CXXMethodDecl *); + bool VisitCXXThisExpr(const CXXThisExpr *); + +private: + bool isPointerOrReferenceToConst(const QualType& qt); + bool isPointerOrReferenceToNonConst(const QualType& qt); + bool checkIfCanBeConst(const Stmt*, const CXXMethodDecl*); + + std::unordered_set interestingMethodSet; + std::unordered_set methodCannotBeConstSet; + CXXMethodDecl const * currCXXMethodDecl; +}; + +bool ConstMethod::TraverseCXXMethodDecl(CXXMethodDecl * cxxMethodDecl) +{ + currCXXMethodDecl = cxxMethodDecl; + bool rv = RecursiveASTVisitor::TraverseCXXMethodDecl(cxxMethodDecl); + currCXXMethodDecl = nullptr; + return rv; +} + +bool ConstMethod::TraverseCXXConversionDecl(CXXConversionDecl * cxxConversionDecl) +{ + currCXXMethodDecl = cxxConversionDecl; + bool rv = RecursiveASTVisitor::TraverseCXXConversionDecl(cxxConversionDecl); + currCXXMethodDecl = nullptr; + return rv; +} + +bool ConstMethod::VisitCXXMethodDecl(const CXXMethodDecl * cxxMethodDecl) +{ + if (ignoreLocation(cxxMethodDecl) || !cxxMethodDecl->isThisDeclarationADefinition()) { + return true; + } + if (cxxMethodDecl->isConst()) + return true; + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(cxxMethodDecl)) { + return true; + } + // TODO ignore template stuff for now + if (cxxMethodDecl->getTemplatedKind() != FunctionDecl::TK_NonTemplate) { + return true; + } + if (cxxMethodDecl->isDeleted()) + return true; + if (cxxMethodDecl->isStatic()) + return true; + if (cxxMethodDecl->isOverloadedOperator()) + return true; + if (isa(cxxMethodDecl)) + return true; + if (isa(cxxMethodDecl)) + return true; + if (cxxMethodDecl->getParent()->getDescribedClassTemplate() != nullptr ) { + return true; + } + // ignore virtual methods + if (cxxMethodDecl->isVirtual() ) { + return true; + } + // ignore macro expansions so we can ignore the IMPL_LINK macros from include/tools/link.hxx + // TODO make this more precise + if (cxxMethodDecl->getLocation().isMacroID()) + return true; + + if (!cxxMethodDecl->getIdentifier()) + return true; + + StringRef name = cxxMethodDecl->getName(); + if (name == "PAGE") // otherwise we have two methods that only differ in their return type + return true; + // stuff in comphelper I'm not sure about + if (name == "CommitImageSubStorage" || name == "CloseEmbeddedObjects" || name == "flush") + return true; + + interestingMethodSet.insert(cxxMethodDecl); + + return true; +} + +bool ConstMethod::VisitCXXThisExpr( const CXXThisExpr* cxxThisExpr ) +{ + if (!currCXXMethodDecl) + return true; + if (ignoreLocation(cxxThisExpr)) + return true; + // ignore stuff that forms part of the stable URE interface + if (isInUnoIncludeFile(cxxThisExpr->getLocStart())) + return true; + if (interestingMethodSet.find(currCXXMethodDecl) == interestingMethodSet.end()) + return true; + // no need to check again if we have already eliminated this one + if (methodCannotBeConstSet.find(currCXXMethodDecl) != methodCannotBeConstSet.end()) + return true; + if (!checkIfCanBeConst(cxxThisExpr, currCXXMethodDecl)) + methodCannotBeConstSet.insert(currCXXMethodDecl); + + return true; +} + +// Walk up from a statement that contains a CXXThisExpr, checking if the usage means that the +// related CXXMethodDecl can be const. +bool ConstMethod::checkIfCanBeConst(const Stmt* stmt, const CXXMethodDecl* cxxMethodDecl) +{ + const Stmt* parent = getParentStmt( stmt ); + if (!parent) { + auto parentsRange = compiler.getASTContext().getParents(*stmt); + if ( parentsRange.begin() == parentsRange.end()) + return true; + auto varDecl = dyn_cast_or_null(parentsRange.begin()->get()); + if (!varDecl) + { + report( + DiagnosticsEngine::Warning, + "no parent?", + stmt->getLocStart()) + << stmt->getSourceRange(); + return false; + } + return varDecl->getType()->isIntegralOrEnumerationType() + || loplugin::TypeCheck(varDecl->getType()).Pointer().Const() + || loplugin::TypeCheck(varDecl->getType()).LvalueReference().Const(); + } + + if (auto unaryOperator = dyn_cast(parent)) { + UnaryOperator::Opcode op = unaryOperator->getOpcode(); + if (op == UO_AddrOf || op == UO_PreInc || op == UO_PostInc + || op == UO_PreDec || op == UO_PostDec) { + return false; + } + if (op == UO_Deref) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } + return true; + } else if (auto binaryOp = dyn_cast(parent)) { + BinaryOperator::Opcode op = binaryOp->getOpcode(); + if (binaryOp->getRHS() == stmt) { + return true; + } + if (op == BO_Assign || op == BO_PtrMemD || op == BO_PtrMemI || op == BO_MulAssign + || op == BO_DivAssign || op == BO_RemAssign || op == BO_AddAssign + || op == BO_SubAssign || op == BO_ShlAssign || op == BO_ShrAssign + || op == BO_AndAssign || op == BO_XorAssign || op == BO_OrAssign) { + return false; + } +// // for pointer arithmetic need to check parent +// if (binaryOp->getType()->isPointerType()) { +// return checkIfCanBeConst(parent, cxxMethodDecl); +// } + return true; + } else if (auto constructExpr = dyn_cast(parent)) { + const CXXConstructorDecl * constructorDecl = constructExpr->getConstructor(); + for (unsigned i = 0; i < constructExpr->getNumArgs(); ++i) { + if (constructExpr->getArg(i) == stmt) { + return isPointerOrReferenceToConst(constructorDecl->getParamDecl(i)->getType()); + } + } + return false; // TODO ?? + } else if (auto operatorCallExpr = dyn_cast(parent)) { + const CXXMethodDecl* calleeMethodDecl = dyn_cast_or_null(operatorCallExpr->getDirectCallee()); + if (calleeMethodDecl) { + // unary operator + if (calleeMethodDecl->getNumParams() == 0) { + // some classes like std::unique_ptr do not do a very good job with their operator-> which is always const + if (operatorCallExpr->getOperator() == OO_Arrow || operatorCallExpr->getOperator() == OO_Star) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } + return calleeMethodDecl->isConst(); + } + // some classes like std::unique_ptr do not do a very good job with their operator[] which is always const + if (calleeMethodDecl->getNumParams() == 1 && operatorCallExpr->getArg(0) == stmt) { + if (operatorCallExpr->getOperator() == OO_Subscript) { + return false; + } + } + // binary operator + if (operatorCallExpr->getArg(0) == stmt) { + return calleeMethodDecl->isConst(); + } + unsigned const n = std::min( + operatorCallExpr->getNumArgs(), + calleeMethodDecl->getNumParams()); + for (unsigned i = 1; i < n; ++i) + if (operatorCallExpr->getArg(i) == stmt) { + return isPointerOrReferenceToConst(calleeMethodDecl->getParamDecl(i - 1)->getType()); + } + } else { + const Expr* callee = operatorCallExpr->getCallee()->IgnoreParenImpCasts(); + const DeclRefExpr* dr = dyn_cast(callee); + const FunctionDecl* calleeFunctionDecl = nullptr; + if (dr) { + calleeFunctionDecl = dyn_cast(dr->getDecl()); + } + if (calleeFunctionDecl) { + for (unsigned i = 0; i < operatorCallExpr->getNumArgs(); ++i) { + if (operatorCallExpr->getArg(i) == stmt) { + return isPointerOrReferenceToConst(calleeFunctionDecl->getParamDecl(i)->getType()); + } + } + } + } + return false; // TODO ??? + } else if (auto callExpr = dyn_cast(parent)) { + QualType functionType = callExpr->getCallee()->getType(); + if (functionType->isFunctionPointerType()) { + functionType = functionType->getPointeeType(); + } + if (const FunctionProtoType* prototype = functionType->getAs()) { + // TODO could do better + if (prototype->isVariadic()) { + return false; + } + if (callExpr->getCallee() == stmt) { + return true; + } + for (unsigned i = 0; i < callExpr->getNumArgs(); ++i) { + if (callExpr->getArg(i) == stmt) { + return isPointerOrReferenceToConst(prototype->getParamType(i)); + } + } + } + const FunctionDecl* calleeFunctionDecl = callExpr->getDirectCallee(); + if (calleeFunctionDecl) + { + if (auto memberCallExpr = dyn_cast(parent)) { + const MemberExpr* memberExpr = dyn_cast(stmt); + if (memberExpr && memberCallExpr->getImplicitObjectArgument() == memberExpr->getBase()) + { + const CXXMethodDecl* calleeMethodDecl = dyn_cast(calleeFunctionDecl); + // some classes like std::unique_ptr do not do a very good job with their get() which is always const + if (calleeMethodDecl->getIdentifier() && calleeMethodDecl->getName() == "get") { + return checkIfCanBeConst(parent, cxxMethodDecl); + } + // VclPtr's implicit conversion to T* + if (isa(calleeMethodDecl)) { + if (loplugin::DeclCheck(calleeMethodDecl->getParent()).Class("OWeakObject").Namespace("cppu").GlobalNamespace()) + return false; + return checkIfCanBeConst(parent, cxxMethodDecl); + } + return calleeMethodDecl->isConst(); + } + } + // TODO could do better + if (calleeFunctionDecl->isVariadic()) { + return false; + } + if (callExpr->getCallee() == stmt) { + return true; + } + for (unsigned i = 0; i < callExpr->getNumArgs(); ++i) { + if (i >= calleeFunctionDecl->getNumParams()) // can happen in template code + return false; + if (callExpr->getArg(i) == stmt) { + return isPointerOrReferenceToConst(calleeFunctionDecl->getParamDecl(i)->getType()); + } + } + } + return false; // TODO ???? +// } else if (auto callExpr = dyn_cast(parent)) { +// if (callExpr->getInstanceReceiver() == stmt) { +// return true; +// } +// if (auto const method = callExpr->getMethodDecl()) { +// // TODO could do better +// if (method->isVariadic()) { +// return false; +// } +// assert(method->param_size() == callExpr->getNumArgs()); +// for (unsigned i = 0; i < callExpr->getNumArgs(); ++i) { +// if (callExpr->getArg(i) == stmt) { +// return isPointerOrReferenceToConst( +// method->param_begin()[i]->getType()); +// } +// } +// } +// return false; // TODO ???? + } else if (isa(parent)) { + return false; + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (isa(parent)) { + return false; + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); +// } else if (isa(parent)) { // all other cast expression subtypes +// if (auto e = dyn_cast(parent)) { +// if (loplugin::TypeCheck(e->getTypeAsWritten()).Void()) { +// if (auto const sub = dyn_cast( +// e->getSubExpr()->IgnoreParenImpCasts())) +// { +// if (sub->getDecl() == cxxMethodDecl) { +// return false; +// } +// } +// } +// } +// return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (auto arraySubscriptExpr = dyn_cast(parent)) { + if (arraySubscriptExpr->getIdx() == stmt) + return true; + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (auto declStmt = dyn_cast(parent)) { + for (Decl const * decl : declStmt->decls()) + if (auto varDecl = dyn_cast(decl)) { + if (varDecl->getInit() == stmt) { + auto tc = loplugin::TypeCheck(varDecl->getType()); + if (tc.LvalueReference() && !tc.LvalueReference().Const()) + return false; + if (tc.Pointer() && !tc.Pointer().Const()) + return false; + return true; + } + } + // fall through + } else if (isa(parent)) { + return !isPointerOrReferenceToNonConst(cxxMethodDecl->getReturnType()); + } else if (isa(parent)) { + return false; // TODO could be improved + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return false; +// } else if (isa(parent)) { +// return false; + } else if (isa(parent)) { + return false; + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (auto conditionalExpr = dyn_cast(parent)) { + if (conditionalExpr->getCond() == stmt) + return true; + return checkIfCanBeConst(parent, cxxMethodDecl); +// } else if (isa(parent)) { +// return false; // ??? + } else if (auto cxxNewExpr = dyn_cast(parent)) { +// for (auto pa : cxxNewExpr->placement_arguments()) +// if (pa == stmt) +// return false; + return true; // because the Stmt must be a parameter to the expression, probably an array length +// } else if (auto lambdaExpr = dyn_cast(parent)) { +//// for (auto it = lambdaExpr->capture_begin(); it != lambdaExpr->capture_end(); ++it) +//// { +//// if (it->capturesVariable() && it->getCapturedVar() == cxxMethodDecl) +//// return it->getCaptureKind() != LCK_ByRef; +//// } +// return true; +// } else if (isa(parent)) { +// return true; + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return false; +// } else if (isa(parent)) { +// return false; +// } else if (isa(parent)) { +// return false; + } else if (isa(parent)) { + return checkIfCanBeConst(parent, cxxMethodDecl); +// } else if (isa(parent)) { +// return true; +// } else if (isa(parent)) { +// return false; +// } else if (isa(parent)) { +// return false; +// } else if (isa(parent)) { +// return checkIfCanBeConst(parent, cxxMethodDecl); + } else if (isa(parent)) { + return true; + } else if (isa(parent)) { + return true; + } + if (parent) + parent->dump(); +// if (cxxMethodDecl) +// cxxMethodDecl->dump(); + report( + DiagnosticsEngine::Warning, + "oh dear, what can the matter be?", + parent->getLocStart()) + << parent->getSourceRange(); + return false; +} + +bool ConstMethod::isPointerOrReferenceToConst(const QualType& qt) { + auto const type = loplugin::TypeCheck(qt); + if (type.Pointer()) { + return bool(type.Pointer().Const()); + } else if (type.LvalueReference()) { + return bool(type.LvalueReference().Const()); + } + return false; +} + +bool ConstMethod::isPointerOrReferenceToNonConst(const QualType& qt) { + auto const type = loplugin::TypeCheck(qt); + if (type.Pointer()) { + return !bool(type.Pointer().Const()); + } else if (type.LvalueReference()) { + return !bool(type.LvalueReference().Const()); + } + return false; +} + +loplugin::Plugin::Registration< ConstMethod > X("constmethod", true); + +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/test/constmethod.cxx b/compilerplugins/clang/test/constmethod.cxx new file mode 100644 index 000000000000..e801db419aa7 --- /dev/null +++ b/compilerplugins/clang/test/constmethod.cxx @@ -0,0 +1,46 @@ +/* -*- 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 +#include + +class OutputDevice; + +struct Class1 +{ + struct Impl { + void foo_notconst(); + void foo_const() const; + int & foo_both(); + int const & foo_both() const; + }; + std::unique_ptr pImpl; + int* m_pint; + VclPtr m_pvcl; + + void foo1() { + pImpl->foo_notconst(); + } + void foo2() { // expected-error {{this method can be const [loplugin:constmethod]}} + pImpl->foo_const(); + } + // TODO this should trigger a warning, but doesn't + void foo3() { + pImpl->foo_both(); + } + Impl* foo4() { + return pImpl.get(); // no warning expected + } + int* foo5() { + return m_pint; // no warning expected + } + OutputDevice* foo6() { + return m_pvcl; // no warning expected + } +};