diff --git a/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h b/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h --- a/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h +++ b/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h @@ -48,6 +48,10 @@ /// Whether to consider 'T' and 'const T'/'volatile T'/etc. arguments to be /// possible mixup. const bool CVRMixPossible; + + /// Whether to check for two parameters to be "related" when deciding the + /// mixup. + const bool CheckRelatedness; }; } // namespace experimental diff --git a/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp b/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp --- a/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp +++ b/clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp @@ -9,8 +9,11 @@ #include "CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h" #include "../utils/OptionsUtils.h" #include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "llvm/ADT/BitmaskEnum.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include @@ -86,11 +89,13 @@ static_assert(std::is_trivially_copyable::value, "keep Mixup trivially copyable!"); +using MixupVec = SmallVector; + /// Represents a (closed) range of adjacent parameters that can be mixed up at /// a call site. struct MixableAdjacentParamRange { std::size_t NumParamsChecked = 0; - llvm::SmallVector Mixups; + MixupVec Mixups; const ParmVarDecl *getFirstParm() const { // The first element added to the mixup vector has the left hand @@ -110,6 +115,211 @@ } }; +AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, ParamRefExpr) { + return expr(ignoringParenImpCasts(ignoringElidableConstructorCall( + declRefExpr(to(parmVarDecl().bind("param")))))); +} + +/// Helper class that can be used to decide if two parameters (of the same +/// function) are "related" in some sense. Related parameters should not be +/// diagnosed. +class ParamRelatedness { +public: + ParamRelatedness(const FunctionDecl *F, bool ShouldEnable) + : Enabled(ShouldEnable) { + if (!Enabled) + return; + + SameExpr.emplace(const_cast(F)); + Returns.emplace(F); + PassToSameFunction.emplace(const_cast(F)); + } + + bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const { + if (!Enabled) + // Consider every parameter unrelated. + return false; + + if (SameExpr && SameExpr->operator()(Param1, Param2)) + return true; + + if (Returns && Returns->operator()(Param1, Param2)) + return true; + + if (PassToSameFunction && PassToSameFunction->operator()(Param1, Param2)) + return true; + + return false; + } + +private: + const bool Enabled; + + /// Implements a heuristic that can be used to mark two parameters related if + /// their usage shares a "reasonably close" parent in the syntax tree - in + /// this case, that two parameters are referred in the same expression's tree. + class PassedInSameExpr : public RecursiveASTVisitor { + private: + using Base = RecursiveASTVisitor; + + public: + explicit PassedInSameExpr(FunctionDecl *FD) : Function(FD) { + TraverseFunctionDecl(FD); + } + + bool operator()(const ParmVarDecl *Param1, + const ParmVarDecl *Param2) const { + auto P1It = ParentExprsOfParamRefs.find(Param1); + auto P2It = ParentExprsOfParamRefs.find(Param2); + if (P1It == ParentExprsOfParamRefs.end() || + P2It == ParentExprsOfParamRefs.end()) + return false; + + for (const auto *P1SetE : P1It->second) + if (P2It->second.find(P1SetE) != P2It->second.end()) + return true; + + return false; + } + + bool TraverseDecl(Decl *D) { + CurrentRootOfExprOnlyTree = nullptr; + return Base::TraverseDecl(D); + } + + bool TraverseStmt(Stmt *S, DataRecursionQueue *Queue = nullptr) { + if (auto *E = dyn_cast_or_null(S)) { + bool RootSetByCurrent = false; + + if (!CurrentRootOfExprOnlyTree) { + CurrentRootOfExprOnlyTree = E; + RootSetByCurrent = true; + } + + bool Ret = Base::TraverseStmt(S); + + if (RootSetByCurrent) + CurrentRootOfExprOnlyTree = nullptr; + + return Ret; + } + + CurrentRootOfExprOnlyTree = nullptr; + + return Base::TraverseStmt(S); + } + + bool VisitDeclRefExpr(DeclRefExpr *DRE) { + if (!CurrentRootOfExprOnlyTree) + return true; + + if (auto *PVD = dyn_cast(DRE->getDecl())) + if (llvm::find(Function->parameters(), PVD)) + ParentExprsOfParamRefs[PVD].insert(CurrentRootOfExprOnlyTree); + + return true; + } + + bool shouldWalkTypesOfTypeLocs() const { return false; } + + private: + const FunctionDecl *const Function; + const Expr *CurrentRootOfExprOnlyTree = nullptr; + llvm::DenseMap> + ParentExprsOfParamRefs; + }; + + /// Implements a heuristic that considers two parameters related if both of + /// them are returned at some point in the function (i.e. the function is a + /// "switch" between N params based on a condition). + class ReturnStmts { + public: + explicit ReturnStmts(const FunctionDecl *FD) { + auto Matches = match(functionDecl(forEachDescendant( + returnStmt(hasReturnValue(ParamRefExpr())))), + *FD, FD->getASTContext()); + for (const auto &Match : Matches) { + const auto *ReturnedParam = Match.getNodeAs("param"); + assert(ReturnedParam && "Bogus matcher."); + + if (find(FD->parameters(), ReturnedParam) == FD->param_end()) + // Returning an expr to a ParmVarDecl that isn't parameter of the + // function should not be possible, but it's not our responsibility if + // Sema mishandled something. + continue; + ReturnedParams.emplace_back(ReturnedParam); + } + } + + bool operator()(const ParmVarDecl *Param1, + const ParmVarDecl *Param2) const { + return llvm::find(ReturnedParams, Param1) != ReturnedParams.end() && + llvm::find(ReturnedParams, Param2) != ReturnedParams.end(); + } + + private: + SmallVector ReturnedParams; + }; + + /// Implements a heuristic that considers two parameters related if they are + /// passed at some point to the same function in a call, at the same index, + /// i.e. f(a, b) and f(a, c) passes 'b' and 'c' at the same index (2) to f(). + class PassToSameFunction { + public: + explicit PassToSameFunction(FunctionDecl *FD) { + auto Matches = match( + functionDecl(forEachDescendant( + callExpr(forEachArgumentWithParam( + ParamRefExpr(), parmVarDecl().bind("passed-to"))) + .bind("call-expr"))), + *FD, FD->getASTContext()); + for (const auto &Match : Matches) { + const auto *CallE = Match.getNodeAs("call-expr"); + const auto *PassedTo = Match.getNodeAs("passed-to"); + const auto *PassedParam = Match.getNodeAs("param"); + assert(CallE && PassedTo && PassedParam && "Bogus matcher."); + + const FunctionDecl *CalledFun = CallE->getDirectCallee(); + if (!CalledFun) + continue; + + int TargetIdx = -1; + for (unsigned Idx = 0; Idx < CalledFun->getNumParams(); ++Idx) + if (CalledFun->getParamDecl(Idx) == PassedTo) + TargetIdx = static_cast(Idx); + assert(TargetIdx >= 0 && "Matched passing but didn't find the param?"); + + TargetParamsOfFunParams[PassedParam].insert( + std::make_pair(CalledFun, TargetIdx)); + } + } + + bool operator()(const ParmVarDecl *Param1, + const ParmVarDecl *Param2) const { + auto P1It = TargetParamsOfFunParams.find(Param1); + auto P2It = TargetParamsOfFunParams.find(Param2); + if (P1It == TargetParamsOfFunParams.end() || + P2It == TargetParamsOfFunParams.end()) + return false; + + for (const auto &P1SetE : P1It->second) + if (find(P2It->second, P1SetE) != P2It->second.end()) + return true; + + return false; + } + + private: + llvm::DenseMap, 4>> + TargetParamsOfFunParams; + }; + + llvm::Optional SameExpr; + llvm::Optional Returns; + llvm::Optional PassToSameFunction; +}; + } // namespace /// Returns whether an lvalue reference refers to the same type as T. @@ -232,7 +442,8 @@ /// index StartIdx. static MixableAdjacentParamRange BuildMixableParameterRange( const CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck &Checker, - const FunctionDecl *F, unsigned int StartIdx) { + const FunctionDecl *F, unsigned int StartIdx, + const ParamRelatedness &Relatedness) { MixableAdjacentParamRange MixRange; const unsigned int ParamCount = F->getNumParams(); assert(StartIdx < ParamCount && "invalid start index given!"); @@ -251,27 +462,38 @@ // If the next parameter in the range is ignored, break the range. break; - bool AnyMixupStored = false; + MixupVec MixupsOfIth; for (unsigned int J = StartIdx; J < I; ++J) { const ParmVarDecl *Jth = F->getParamDecl(J); Mixup M{Jth, Ith, HowPossibleToMixUpAtCallSite(Jth->getType(), Ith->getType(), Ctx, Checker.CVRMixPossible)}; + + if (Relatedness(Ith, Jth)) { + // Consider two related parameters to not be possible to mix up + // (at the user's request). + // In addition, if an argument is related to any of the previous, the + // mixing range should be broken. + MixupsOfIth.clear(); + break; + } + M.sanitise(); assert(M.Flags != MIXUP_Invalid && "Bits fell off, result is sentinel value."); - if (M.Flags != MIXUP_None) { - MixRange.Mixups.emplace_back(M); - AnyMixupStored = true; - } + if (M.Flags != MIXUP_None) + MixupsOfIth.emplace_back(M); } - if (!AnyMixupStored) + if (MixupsOfIth.empty()) // If there is no "new" mixup possibility for the Ith param, it signifies // the end of range. break; + MixRange.Mixups.insert(MixRange.Mixups.end(), MixupsOfIth.begin(), + MixupsOfIth.end()); + // If the loop did not break earlier, another parameter was found to be // mixable. ++MixRange.NumParamsChecked; @@ -456,7 +678,8 @@ Options.get("IgnoredNames", DefaultIgnoredParamNames))), IgnoredParamTypes(utils::options::parseStringList( Options.get("IgnoredTypes", DefaultIgnoredParamTypes))), - CVRMixPossible(Options.get("CVRMixPossible", false)) {} + CVRMixPossible(Options.get("CVRMixPossible", false)), + CheckRelatedness(Options.get("CheckRelatedness", true)) {} void CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { @@ -466,6 +689,7 @@ Options.store(Opts, "IgnoredTypes", utils::options::serializeStringList(IgnoredParamTypes)); Options.store(Opts, "CVRMixPossible", CVRMixPossible); + Options.store(Opts, "CheckRelatedness", CheckRelatedness); } void CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck:: @@ -502,6 +726,7 @@ const MatchFinder::MatchResult &Result) { const auto *Fun = Result.Nodes.getNodeAs("fun"); + ParamRelatedness RelatednessChecker{Fun, CheckRelatedness}; unsigned int ParamMixRangeStartIdx = 0; const unsigned int NumArgs = Fun->getNumParams(); @@ -514,7 +739,8 @@ } MixableAdjacentParamRange MixingRange = - BuildMixableParameterRange(*this, Fun, ParamMixRangeStartIdx); + BuildMixableParameterRange( + *this, Fun, ParamMixRangeStartIdx, RelatednessChecker); assert(MixingRange.NumParamsChecked > 0 && "Ensure continuity!"); ParamMixRangeStartIdx += MixingRange.NumParamsChecked; if (MixingRange.NumParamsChecked < MinimumLength) diff --git a/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp b/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp @@ -1,7 +1,8 @@ // RUN: %check_clang_tidy %s \ // RUN: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \ // RUN: -config='{CheckOptions: [ \ -// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \ +// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1}, \ +// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CheckRelatedness, value: 0} \ // RUN: ]}' -- void library(void *vp, void *vp2, void *vp3, int n, int m); diff --git a/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-relatedness.cpp b/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-relatedness.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-relatedness.cpp @@ -0,0 +1,187 @@ +// RUN: %check_clang_tidy %s \ +// RUN: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CheckRelatedness, value: 1} \ +// RUN: ]}' -- + +namespace std { +// NO-WARN: Not a definition, the user has no chance to fix this! +int max(int x, int y); +int abs(int x); + +template +bool less(const T &t1, const T &t2); +} // namespace std + +class C { +public: + void foo(int a); + void bar(int a, int b); +}; + +bool coin(); +int f(int a); +int g(int b); +int h(int a, int b); + +void myFunTakingTwoInt(int a, int b) {} +// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters for 'myFunTakingTwoInt' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in this range is 'a' +// CHECK-MESSAGES: :[[@LINE-3]]:35: note: the last parameter in this range is 'b' + +int MyMax(int a, int b) { // NO-WARN: Same expression. + return a < b ? a : b; +} + +int MyMax_2(int a, int b) { // NO-WARN: Same expression. + if (a < b) + return a; + return b; +} + +int MyMax_3(int a, int b) { // NO-WARN: Same expression. + return std::max(a, b); +} + +int BinaryToUnary(int a, int b) { + return a; +} +// CHECK-MESSAGES: :[[@LINE-3]]:19: warning: 2 adjacent parameters for 'BinaryToUnary' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-4]]:23: note: the first parameter in this range is 'a' +// CHECK-MESSAGES: :[[@LINE-5]]:30: note: the last parameter in this range is 'b' + +int RandomReturn_1(int a, int b) { // NO-WARN: Same expression. + return coin() ? a : b; +} + +int RandomReturn_2(int a, int b) { // NO-WARN: Both vars can be returned. + if (coin()) + return a; + else + return b; +} + +int RandomReturn_3(int a, int b) { // NO-WARN: Both vars can be returned. + bool Flip = coin(); + if (Flip) + return a; + Flip = coin(); + if (Flip) + return b; + Flip = coin(); + if (!Flip) + return 0; + return -1; +} + +void Passthrough_1(int a, int b) { + f(a); + g(b); +} +// CHECK-MESSAGES: :[[@LINE-4]]:20: warning: 2 adjacent parameters for 'Passthrough_1' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-5]]:24: note: the first parameter in this range is 'a' +// CHECK-MESSAGES: :[[@LINE-6]]:31: note: the last parameter in this range is 'b' + +void Passthrough_2(int a, int b) { // NO-WARN: Both passed to same func on same index. + f(a); + f(b); +} + +void Passthrough_2b(int a, int b) { // NO-WARN: Both passed to same func on same index. + g(b); + g(a); +} + +void Passthrough_Multifun(int a, int b) { // NO-WARN: Both passed to same func on same index. + h(1, a); + h(1, b); +} + +void Passthrough_Multifun_b(int a, int b) { + h(1, a); + h(b, 1); +} +// CHECK-MESSAGES: :[[@LINE-4]]:29: warning: 2 adjacent parameters for 'Passthrough_Multifun_b' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-5]]:33: note: the first parameter in this range is 'a' +// CHECK-MESSAGES: :[[@LINE-6]]:40: note: the last parameter in this range is 'b' + +void Passthrough_2_Member(int a, int b) { // NO-WARN: Both passed to same func on same index. + C{}.foo(a); + C{}.foo(b); +} + +void Passthrough_2b_Member(int a, int b) { // NO-WARN: Both passed to same func on same index. + C c; + c.foo(b); + c.foo(a); +} + +void Passthrough_Multifun_Member(int a, int b) { // NO-WARN: Both passed to same func on same index. + C c; + C c2; + + c.bar(1, a); + c2.bar(1, b); +} + +void Passthrough_Multifun_b_Member(int a, int b) { + C{}.bar(1, a); + C{}.bar(b, 1); +} +// CHECK-MESSAGES: :[[@LINE-4]]:36: warning: 2 adjacent parameters for 'Passthrough_Multifun_b_Member' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-5]]:40: note: the first parameter in this range is 'a' +// CHECK-MESSAGES: :[[@LINE-6]]:47: note: the last parameter in this range is 'b' + +int Assign(int a, int b) { + int x = a + b; + int y = a * x; + return y; +} + +int ManyArgs(int x, int y, + int Ret1, int Ret2, // NO-WARN: Ret1 and Ret2 both returned at some point. + int Comp1, int Comp2, // NO-WARN: Comp1 and Comp2 compared with each other. + int Pass1, int Pass2) { + if (Comp1 >= Comp2) + return Ret1; + + if (Comp1 < -1337 - y) { + int One = f(Pass1); + int Two = g(Pass2); + + return One * Two; + } + + if (Comp1 * 42 == x) + return Ret2; +} +// CHECK-MESSAGES: :[[@LINE-17]]:14: warning: 3 adjacent parameters for 'ManyArgs' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-18]]:18: note: the first parameter in this range is 'x' +// CHECK-MESSAGES: :[[@LINE-18]]:18: note: the last parameter in this range is 'Ret1' +// CHECK-MESSAGES: :[[@LINE-19]]:24: warning: 2 adjacent parameters for 'ManyArgs' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-20]]:28: note: the first parameter in this range is 'Ret2' +// CHECK-MESSAGES: :[[@LINE-20]]:18: note: the last parameter in this range is 'Comp1' +// CHECK-MESSAGES: :[[@LINE-21]]:25: warning: 3 adjacent parameters for 'ManyArgs' of similar type ('int') are +// CHECK-MESSAGES: :[[@LINE-22]]:29: note: the first parameter in this range is 'Comp2' +// CHECK-MESSAGES: :[[@LINE-22]]:29: note: the last parameter in this range is 'Pass2' + +struct TotallyNotAnEEtherathor { + TotallyNotAnEEtherathor &operator++(); + bool operator!=(const TotallyNotAnEEtherathor &RHS) const; + const int &operator*(); +}; + +bool Within(int Element, TotallyNotAnEEtherathor OneAfterBeforeTheZerothIndex, TotallyNotAnEEtherathor PrePastTheNextOfLast) { + // NO-WARN: Same expression (==). + while (OneAfterBeforeTheZerothIndex != PrePastTheNextOfLast) { + if (*OneAfterBeforeTheZerothIndex == Element) + return true; + ++OneAfterBeforeTheZerothIndex; + } + return false; +} + +bool isValidRange(TotallyNotAnEEtherathor East, TotallyNotAnEEtherathor West) { + // NO-WARN: Same expression (call). + return std::less(East, West); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c b/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c --- a/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c +++ b/clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c @@ -1,7 +1,8 @@ // RUN: %check_clang_tidy %s \ // RUN: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \ // RUN: -config='{CheckOptions: [ \ -// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \ +// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1}, \ +// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CheckRelatedness, value: 0} \ // RUN: ]}' -- struct T {};