compilerplugins/clang/passstuffbyref.cxx

   1 /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
   2 /*
   3  * This file is part of the LibreOffice project.
   4  *
   5  * This Source Code Form is subject to the terms of the Mozilla Public
   6  * License, v. 2.0. If a copy of the MPL was not distributed with this
   7  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
   8  */
   9
  10 #include <string>
  11 #include <set>
  12 #include <iostream>
  13
  14 #include "check.hxx"
  15 #include "plugin.hxx"
  16
  17 // Find places where various things are passed by value.
  18 // It's not very efficient, because we generally end up copying it twice - once into the parameter and
  19 // again into the destination.
  20 // They should rather be passed by reference.
  21 //
  22 // Generally recommending lambda capture by-ref rather than by-copy is even more
  23 // problematic than with function parameters, as a lambda instance can easily
  24 // outlive a referenced variable. So once lambdas start to get used in more
  25 // sophisticated ways than passing them into standard algorithms, this plugin's
  26 // advice, at least for explicit captures, will need to be revisited.
  27
  28 namespace {
  29
  30 class PassStuffByRef:
  31     public RecursiveASTVisitor<PassStuffByRef>, public loplugin::Plugin
  32 {
  33 public:
  34     explicit PassStuffByRef(loplugin::InstantiationData const & data): Plugin(data), mbInsideFunctionDecl(false), mbFoundReturnValueDisqualifier(false) {}
  35
  36     virtual void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
  37
  38     // When warning about function params of primitive type that could be passed
  39     // by value instead of by reference, make sure not to warn if the parameter
  40     // is ever bound to a reference; on the one hand, this needs scaffolding in
  41     // all Traverse*Decl functions (indirectly) derived from FunctionDecl; and
  42     // on the other hand, use a hack of ignoring just the DeclRefExprs nested in
  43     // LValueToRValue ImplicitCastExprs when determining whether a param is
  44     // bound to a reference:
  45     bool TraverseFunctionDecl(FunctionDecl * decl);
  46     bool TraverseCXXMethodDecl(CXXMethodDecl * decl);
  47     bool TraverseCXXConstructorDecl(CXXConstructorDecl * decl);
  48     bool TraverseCXXDestructorDecl(CXXDestructorDecl * decl);
  49     bool TraverseCXXConversionDecl(CXXConversionDecl * decl);
  50     bool VisitFunctionDecl(const FunctionDecl * decl);
  51     bool TraverseImplicitCastExpr(ImplicitCastExpr * expr);
  52     bool VisitDeclRefExpr(const DeclRefExpr * expr);
  53
  54     bool VisitReturnStmt(const ReturnStmt * );
  55     bool VisitVarDecl(const VarDecl * );
  56
  57 private:
  58     template<typename T> bool traverseAnyFunctionDecl(
  59         T * decl, bool (RecursiveASTVisitor::* fn)(T *));
  60     void checkParams(const FunctionDecl * functionDecl);
  61     void checkReturnValue(const FunctionDecl * functionDecl, const CXXMethodDecl * methodDecl);
  62     bool isPrimitiveConstRef(QualType type);
  63     bool isReturnExprDisqualified(const Expr* expr);
  64
  65     bool mbInsideFunctionDecl;
  66     bool mbFoundReturnValueDisqualifier;
  67
  68     struct FDecl {
  69         std::set<ParmVarDecl const *> parms;
  70         bool check = false;
  71     };
  72     std::vector<FDecl> functionDecls_;
  73 };
  74
  75 bool PassStuffByRef::TraverseFunctionDecl(FunctionDecl * decl) {
  76     return traverseAnyFunctionDecl(
  77         decl, &RecursiveASTVisitor::TraverseFunctionDecl);
  78 }
  79
  80 bool PassStuffByRef::TraverseCXXMethodDecl(CXXMethodDecl * decl) {
  81     return traverseAnyFunctionDecl(
  82         decl, &RecursiveASTVisitor::TraverseCXXMethodDecl);
  83 }
  84
  85 bool PassStuffByRef::TraverseCXXConstructorDecl(CXXConstructorDecl * decl) {
  86     return traverseAnyFunctionDecl(
  87         decl, &RecursiveASTVisitor::TraverseCXXConstructorDecl);
  88 }
  89
  90 bool PassStuffByRef::TraverseCXXDestructorDecl(CXXDestructorDecl * decl) {
  91     return traverseAnyFunctionDecl(
  92         decl, &RecursiveASTVisitor::TraverseCXXDestructorDecl);
  93 }
  94
  95 bool PassStuffByRef::TraverseCXXConversionDecl(CXXConversionDecl * decl) {
  96     return traverseAnyFunctionDecl(
  97         decl, &RecursiveASTVisitor::TraverseCXXConversionDecl);
  98 }
  99
 100 template<typename T> bool PassStuffByRef::traverseAnyFunctionDecl(
 101     T * decl, bool (RecursiveASTVisitor::* fn)(T *))
 102 {
 103     if (ignoreLocation(decl)) {
 104         return true;
 105     }
 106     if (decl->doesThisDeclarationHaveABody()) {
 107         functionDecls_.emplace_back();
 108     }
 109     bool ret = (this->*fn)(decl);
 110     if (decl->doesThisDeclarationHaveABody()) {
 111         assert(!functionDecls_.empty());
 112         if (functionDecls_.back().check) {
 113             for (auto d: functionDecls_.back().parms) {
 114                 report(
 115                     DiagnosticsEngine::Warning,
 116                     ("passing primitive type param %0 by const &, rather pass"
 117                      " by value"),
 118                     d->getLocation())
 119                     << d->getType() << d->getSourceRange();
 120                 auto can = decl->getCanonicalDecl();
 121                 if (can->getLocation() != decl->getLocation()) {
 122                     report(
 123                         DiagnosticsEngine::Note, "function is declared here:",
 124                         can->getLocation())
 125                         << can->getSourceRange();
 126                 }
 127             }
 128         }
 129         functionDecls_.pop_back();
 130     }
 131     return ret;
 132 }
 133
 134 bool PassStuffByRef::VisitFunctionDecl(const FunctionDecl * functionDecl) {
 135     if (ignoreLocation(functionDecl)) {
 136         return true;
 137     }
 138     if (functionDecl->isDeleted()
 139         || functionDecl->isFunctionTemplateSpecialization())
 140     {
 141         return true;
 142     }
 143     // only consider base declarations, not overridden ones, or we warn on methods that
 144     // are overriding stuff from external libraries
 145     const CXXMethodDecl * methodDecl = dyn_cast<CXXMethodDecl>(functionDecl);
 146     if (methodDecl && methodDecl->size_overridden_methods() > 0) {
 147             return true;
 148     }
 149
 150     checkParams(functionDecl);
 151     checkReturnValue(functionDecl, methodDecl);
 152     return true;
 153 }
 154
 155 bool PassStuffByRef::TraverseImplicitCastExpr(ImplicitCastExpr * expr) {
 156     if (ignoreLocation(expr)) {
 157         return true;
 158     }
 159     return
 160         (expr->getCastKind() == CK_LValueToRValue
 161          && (dyn_cast<DeclRefExpr>(expr->getSubExpr()->IgnoreParenImpCasts())
 162              != nullptr))
 163         || RecursiveASTVisitor::TraverseImplicitCastExpr(expr);
 164 }
 165
 166 bool PassStuffByRef::VisitDeclRefExpr(const DeclRefExpr * expr) {
 167     if (ignoreLocation(expr)) {
 168         return true;
 169     }
 170     auto d = dyn_cast<ParmVarDecl>(expr->getDecl());
 171     if (d == nullptr) {
 172         return true;
 173     }
 174     for (auto & fd: functionDecls_) {
 175         if (fd.parms.erase(d) == 1) {
 176             break;
 177         }
 178     }
 179     return true;
 180 }
 181
 182 void PassStuffByRef::checkParams(const FunctionDecl * functionDecl) {
 183     // Only warn on the definition of the function:
 184     if (!functionDecl->doesThisDeclarationHaveABody()) {
 185         return;
 186     }
 187     // ignore stuff that forms part of the stable URE interface
 188     if (isInUnoIncludeFile(functionDecl)) {
 189         return;
 190     }
 191     // these functions are passed as parameters to another function
 192     if (loplugin::DeclCheck(functionDecl).MemberFunction()
 193         .Class("ShapeAttributeLayer").Namespace("internal")
 194         .Namespace("slideshow").GlobalNamespace())
 195     {
 196         return;
 197     }
 198     unsigned n = functionDecl->getNumParams();
 199     assert(!functionDecls_.empty());
 200     functionDecls_.back().check = true;
 201     for (unsigned i = 0; i != n; ++i) {
 202         const ParmVarDecl * pvDecl = functionDecl->getParamDecl(i);
 203         auto const t = pvDecl->getType();
 204         if (isPrimitiveConstRef(t)) {
 205             functionDecls_.back().parms.insert(pvDecl);
 206         }
 207     }
 208 }
 209
 210 static bool startswith(const std::string& rStr, const char* pSubStr) {
 211     return rStr.compare(0, strlen(pSubStr), pSubStr) == 0;
 212 }
 213
 214 void PassStuffByRef::checkReturnValue(const FunctionDecl * functionDecl, const CXXMethodDecl * methodDecl) {
 215     if (methodDecl && (methodDecl->isVirtual() || methodDecl->hasAttr<OverrideAttr>())) {
 216         return;
 217     }
 218     if( !functionDecl->doesThisDeclarationHaveABody()
 219         || functionDecl->isLateTemplateParsed())
 220     {
 221         return;
 222     }
 223
 224     const QualType type = functionDecl->getReturnType().getDesugaredType(compiler.getASTContext());
 225     if (type->isReferenceType() || type->isIntegralOrEnumerationType() || type->isPointerType()
 226         || type->isTemplateTypeParmType() || type->isDependentType() || type->isBuiltinType()
 227         || type->isScalarType())
 228     {
 229         return;
 230     }
 231
 232     // not sure if it's possible to modify these
 233     if (isa<CXXConversionDecl>(functionDecl))
 234         return;
 235
 236     // ignore stuff that forms part of the stable URE interface
 237     if (isInUnoIncludeFile(functionDecl)) {
 238         return;
 239     }
 240
 241     loplugin::DeclCheck dc(functionDecl);
 242     // function is passed as parameter to another function
 243     if (dc.Function("ImplColMonoFnc").Class("GDIMetaFile").GlobalNamespace()
 244         || (dc.Function("darkColor").Class("SvxBorderLine").Namespace("editeng")
 245             .GlobalNamespace())
 246         || (dc.MemberFunction().Class("Binding").Namespace("xforms")
 247             .GlobalNamespace())
 248         || (dc.MemberFunction().Class("Model").Namespace("xforms")
 249             .GlobalNamespace())
 250         || (dc.MemberFunction().Class("Submission").Namespace("xforms")
 251             .GlobalNamespace())
 252         || (dc.Function("TopLeft").Class("SwRect").GlobalNamespace())
 253         || (dc.Function("ConvDicList_CreateInstance").GlobalNamespace())
 254         || (dc.Function("Create").Class("OUnoAutoPilot").Namespace("dbp").GlobalNamespace())
 255         || (dc.Function("Size_").Class("SwRect").GlobalNamespace()))
 256     {
 257         return;
 258     }
 259     // not sure how to exclude this yet, returns copy of one of it's params
 260     if (dc.Function("sameDistColor").GlobalNamespace()
 261         || dc.Function("sameColor").GlobalNamespace()
 262         || (dc.Operator(OO_Call).Struct("StringIdentity").AnonymousNamespace()
 263             .Namespace("pcr").GlobalNamespace())
 264         || (dc.Function("accumulate").Namespace("internal")
 265             .Namespace("slideshow").GlobalNamespace())
 266         || (dc.Function("lerp").Namespace("internal").Namespace("slideshow")
 267             .GlobalNamespace()))
 268         return;
 269     // depends on a define
 270     if (dc.Function("GetSharedFileURL").Class("SfxObjectShell")
 271         .GlobalNamespace()) {
 272         return;
 273     }
 274     if (startswith(type.getAsString(), "struct o3tl::strong_int")) {
 275         return;
 276     }
 277     // extremely simple class, might as well pass by value
 278     if (loplugin::TypeCheck(functionDecl->getReturnType()).Class("Color")) {
 279         return;
 280     }
 281
 282     // functionDecl->dump();
 283
 284     mbInsideFunctionDecl = true;
 285     mbFoundReturnValueDisqualifier = false;
 286     TraverseStmt(functionDecl->getBody());
 287     mbInsideFunctionDecl = false;
 288
 289     if (mbFoundReturnValueDisqualifier)
 290         return;
 291
 292     report( DiagnosticsEngine::Warning,
 293             "rather return %0 by const& than by value, to avoid unnecessary copying",
 294             functionDecl->getSourceRange().getBegin())
 295         << type.getAsString() << functionDecl->getSourceRange();
 296
 297     // display the location of the class member declaration so I don't have to search for it by hand
 298     auto canonicalDecl = functionDecl->getCanonicalDecl();
 299     if (functionDecl != canonicalDecl)
 300     {
 301         report( DiagnosticsEngine::Note,
 302                 "decl here",
 303                 canonicalDecl->getSourceRange().getBegin())
 304             << canonicalDecl->getSourceRange();
 305     }
 306
 307     //functionDecl->dump();
 308 }
 309
 310 bool PassStuffByRef::VisitReturnStmt(const ReturnStmt * returnStmt)
 311 {
 312     if (!mbInsideFunctionDecl)
 313         return true;
 314     const Expr* expr = dyn_cast<Expr>(*returnStmt->child_begin())->IgnoreParenCasts();
 315
 316     if (isReturnExprDisqualified(expr))
 317         mbFoundReturnValueDisqualifier = true;
 318
 319     return true;
 320 }
 321
 322 /**
 323  * Does a return expression disqualify this method from doing return by const & ?
 324  */
 325 bool PassStuffByRef::isReturnExprDisqualified(const Expr* expr)
 326 {
 327     while (true)
 328     {
 329         expr = expr->IgnoreParens();
 330         if (auto implicitCast = dyn_cast<ImplicitCastExpr>(expr)) {
 331             expr = implicitCast->getSubExpr();
 332             continue;
 333         }
 334         if (auto exprWithCleanups = dyn_cast<ExprWithCleanups>(expr)) {
 335             expr = exprWithCleanups->getSubExpr();
 336             continue;
 337         }
 338         if (auto constructExpr = dyn_cast<CXXConstructExpr>(expr))
 339         {
 340             if (constructExpr->getNumArgs()==1
 341                 && constructExpr->getConstructor()->isCopyOrMoveConstructor())
 342             {
 343                 expr = constructExpr->getArg(0);
 344                 continue;
 345             }
 346             else
 347                 return true;
 348         }
 349         if (isa<CXXFunctionalCastExpr>(expr)) {
 350             return true;
 351         }
 352         if (isa<MaterializeTemporaryExpr>(expr)) {
 353             return true;
 354         }
 355         if (isa<CXXBindTemporaryExpr>(expr)) {
 356             return true;
 357         }
 358         if (isa<InitListExpr>(expr)) {
 359             return true;
 360         }
 361         expr = expr->IgnoreParenCasts();
 362         if (auto childExpr = dyn_cast<ArraySubscriptExpr>(expr)) {
 363             expr = childExpr->getLHS();
 364             continue;
 365         }
 366         if (auto memberExpr = dyn_cast<MemberExpr>(expr)) {
 367             expr = memberExpr->getBase();
 368             continue;
 369         }
 370         if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
 371             // a param might be a temporary
 372             if (isa<ParmVarDecl>(declRef->getDecl()))
 373                 return true;
 374             const VarDecl* varDecl = dyn_cast<VarDecl>(declRef->getDecl());
 375             if (varDecl) {
 376                 if (varDecl->getStorageDuration() == SD_Thread
 377                     || varDecl->getStorageDuration() == SD_Static ) {
 378                     return false;
 379                 }
 380                 return true;
 381             }
 382         }
 383         if (auto condOper = dyn_cast<ConditionalOperator>(expr)) {
 384             return isReturnExprDisqualified(condOper->getTrueExpr())
 385                 || isReturnExprDisqualified(condOper->getFalseExpr());
 386         }
 387         if (auto unaryOp = dyn_cast<UnaryOperator>(expr)) {
 388             expr = unaryOp->getSubExpr();
 389             continue;
 390         }
 391         if (auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(expr)) {
 392             // TODO could improve this, but sometimes it means we're returning a copy of a temporary.
 393             // Same logic as CXXOperatorCallExpr::isAssignmentOp(), which our supported clang
 394             // doesn't have yet.
 395             auto Opc = operatorCallExpr->getOperator();
 396             if (Opc == OO_Equal || Opc == OO_StarEqual ||
 397                 Opc == OO_SlashEqual || Opc == OO_PercentEqual ||
 398                 Opc == OO_PlusEqual || Opc == OO_MinusEqual ||
 399                 Opc == OO_LessLessEqual || Opc == OO_GreaterGreaterEqual ||
 400                 Opc == OO_AmpEqual || Opc == OO_CaretEqual ||
 401                 Opc == OO_PipeEqual)
 402                 return true;
 403             if (Opc == OO_Subscript)
 404             {
 405                 if (isReturnExprDisqualified(operatorCallExpr->getArg(0)))
 406                     return true;
 407                 // otherwise fall through to the checking below
 408             }
 409         }
 410         if (auto memberCallExpr = dyn_cast<CXXMemberCallExpr>(expr)) {
 411             if (isReturnExprDisqualified(memberCallExpr->getImplicitObjectArgument()))
 412                 return true;
 413             // otherwise fall through to the checking in CallExpr
 414         }
 415         if (auto callExpr = dyn_cast<CallExpr>(expr)) {
 416             FunctionDecl const * calleeFunctionDecl = callExpr->getDirectCallee();
 417             if (!calleeFunctionDecl)
 418                 return true;
 419             // TODO anything takes a non-integral param is suspect because it might return the param by ref.
 420             // we could tighten this to only reject functions that have a param of the same type
 421             // as the return type. Or we could check for such functions and disallow them.
 422             // Or we could force such functions to be annotated somehow.
 423             for (unsigned i = 0; i != calleeFunctionDecl->getNumParams(); ++i) {
 424                 if (!calleeFunctionDecl->getParamDecl(i)->getType()->isIntegralOrEnumerationType())
 425                     return true;
 426             }
 427             auto tc = loplugin::TypeCheck(calleeFunctionDecl->getReturnType());
 428             if (!tc.LvalueReference() && !tc.Pointer())
 429                 return true;
 430         }
 431         return false;
 432     }
 433 }
 434
 435 bool PassStuffByRef::VisitVarDecl(const VarDecl * varDecl)
 436 {
 437     if (!mbInsideFunctionDecl)
 438         return true;
 439     // things guarded by locking are probably best left alone
 440     loplugin::TypeCheck dc(varDecl->getType());
 441     if (dc.Class("Guard").Namespace("osl").GlobalNamespace())
 442         mbFoundReturnValueDisqualifier = true;
 443     if (dc.Class("ClearableGuard").Namespace("osl").GlobalNamespace())
 444         mbFoundReturnValueDisqualifier = true;
 445     if (dc.Class("ResettableGuard").Namespace("osl").GlobalNamespace())
 446         mbFoundReturnValueDisqualifier = true;
 447     else if (dc.Class("SolarMutexGuard").GlobalNamespace())
 448         mbFoundReturnValueDisqualifier = true;
 449     else if (dc.Class("SfxModelGuard").GlobalNamespace())
 450         mbFoundReturnValueDisqualifier = true;
 451     else if (dc.Class("ReadWriteGuard").Namespace("utl").GlobalNamespace())
 452         mbFoundReturnValueDisqualifier = true;
 453     return true;
 454 }
 455
 456 bool PassStuffByRef::isPrimitiveConstRef(QualType type) {
 457     if (type->isIncompleteType()) {
 458         return false;
 459     }
 460     const clang::ReferenceType* referenceType = type->getAs<ReferenceType>();
 461     if (referenceType == nullptr) {
 462         return false;
 463     }
 464     QualType pointeeQT = referenceType->getPointeeType();
 465     if (!pointeeQT.isConstQualified()) {
 466         return false;
 467     }
 468     if (!pointeeQT->isFundamentalType()) {
 469         return false;
 470     }
 471     // ignore double for now, some of our code seems to believe it is cheaper to pass by ref
 472     const BuiltinType* builtinType = pointeeQT->getAs<BuiltinType>();
 473     return builtinType->getKind() != BuiltinType::Kind::Double;
 474 }
 475
 476
 477 loplugin::Plugin::Registration< PassStuffByRef > X("passstuffbyref", false);
 478
 479 }
 480
 481 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */