diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h --- a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.h @@ -23,10 +23,12 @@ /// http://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-std-bind.html class AvoidBindCheck : public ClangTidyCheck { public: - AvoidBindCheck(StringRef Name, ClangTidyContext *Context) - : ClangTidyCheck(Name, Context) {} + AvoidBindCheck(StringRef Name, ClangTidyContext *Context); void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + bool PermissiveParameterList = false; }; } // namespace modernize } // namespace tidy diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp --- a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp @@ -14,11 +14,12 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Lex/Lexer.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallSet.h" #include "llvm/ADT/SmallVector.h" -#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" +#include "llvm/Support/FormatVariadic.h" #include "llvm/Support/Regex.h" #include "llvm/Support/raw_ostream.h" #include @@ -34,45 +35,270 @@ namespace { enum BindArgumentKind { BK_Temporary, BK_Placeholder, BK_CallExpr, BK_Other }; +enum CaptureMode { CM_None, CM_ByRef, CM_ByValue, CM_InitExpression }; + +enum CallableType { + CT_Other, // unknown + CT_Function, // global or static function + CT_MemberFunction, // member function with implicit this + CT_Object, // object with operator() +}; + +enum CallableMaterializationKind { + CMK_Other, // unknown + CMK_Function, // callable is the name of a member or non-member function. + CMK_VariableRef, // callable is a simple expression involving a global or + // local variable. + CMK_CallExpression, // callable is obtained as the result of a call expression +}; struct BindArgument { - StringRef Tokens; + // A rough classification of the type of expression this argument was. BindArgumentKind Kind = BK_Other; + + // If this argument required a capture, a value indicating how it was + // captured. + CaptureMode CaptureMode = CM_None; + + // The exact spelling of this argument in the source code. + StringRef SourceTokens; + + // The identifier of the variable within the capture list. This may be + // different from UsageIdentifier for example in the expression *d, where the + // variable is captured as d, but referred to as *d. + std::string CaptureIdentifier; + + // If this is a placeholder or capture init expression, contains the tokens + // used to refer to this parameter from within the body of the lambda. + std::string UsageIdentifier; + + // If Kind == BK_Placeholder, the index of the placeholder. size_t PlaceHolderIndex = 0; + + // True if the argument is used inside the lambda, false otherwise. + bool IsUsed = false; + + // The actual Expr object representing this expression. + const Expr *E = nullptr; +}; + +struct CallableInfo { + CallableType Type = CT_Other; + CallableMaterializationKind Materialization = CMK_Other; + CaptureMode CaptureMode = CM_None; + StringRef SourceTokens; + std::string CaptureIdentifier; + std::string UsageIdentifier; + StringRef CaptureInitializer; + const FunctionDecl *Decl = nullptr; +}; + +struct LambdaProperties { + CallableInfo Callable; + SmallVector BindArguments; + StringRef BindNamespace; + bool IsFixitSupported = false; }; } // end namespace +static const Expr *ignoreTemporariesAndPointers(const Expr *E) { + if (const auto *T = dyn_cast(E)) + return ignoreTemporariesAndPointers(T->getSubExpr()); + + const Expr *F = E->IgnoreImplicit(); + if (E != F) + return ignoreTemporariesAndPointers(F); + + return E; +} + +static const Expr *ignoreTemporariesAndConstructors(const Expr *E) { + if (const auto *T = dyn_cast(E)) + return ignoreTemporariesAndConstructors(T->getArg(0)); + + const Expr *F = E->IgnoreImplicit(); + if (E != F) + return ignoreTemporariesAndPointers(F); + + return E; +} + +static StringRef getSourceTextForExpr(const MatchFinder::MatchResult &Result, + const Expr *E) { + return Lexer::getSourceText( + CharSourceRange::getTokenRange(E->getBeginLoc(), E->getEndLoc()), + *Result.SourceManager, Result.Context->getLangOpts()); +} + +static bool isCallExprNamed(const Expr *E, StringRef Name) { + const auto *CE = dyn_cast(E->IgnoreImplicit()); + if (!CE) + return false; + const auto *ND = dyn_cast(CE->getCalleeDecl()); + if (!ND) + return false; + return ND->getQualifiedNameAsString() == Name; +} + +static void +initializeBindArgumentForCallExpr(const MatchFinder::MatchResult &Result, + BindArgument &B, const CallExpr *CE, + unsigned &CaptureIndex) { + // std::ref(x) means to capture x by reference. + if (isCallExprNamed(CE, "boost::ref") || isCallExprNamed(CE, "std::ref")) { + B.Kind = BK_Other; + B.CaptureMode = CM_ByRef; + B.UsageIdentifier = getSourceTextForExpr(Result, CE->getArg(0)); + } else { + B.Kind = BK_CallExpr; + B.CaptureMode = CM_InitExpression; + B.UsageIdentifier = "capture" + llvm::utostr(CaptureIndex++); + } + B.CaptureIdentifier = B.UsageIdentifier; +} + +static bool anyDescendantIsLocal(const Stmt *Statement) { + if (const auto *DeclRef = dyn_cast(Statement)) { + const ValueDecl *Decl = DeclRef->getDecl(); + if (const auto *Var = dyn_cast_or_null(Decl)) { + if (Var->isLocalVarDeclOrParm()) + return true; + } + } else if (isa(Statement)) + return true; + + return any_of(Statement->children(), anyDescendantIsLocal); +} + +static bool tryCaptureAsLocalVariable(const MatchFinder::MatchResult &Result, + BindArgument &B, const Expr *E) { + if (const auto *BTE = dyn_cast(E)) { + if (const auto *CE = dyn_cast(BTE->getSubExpr())) + return tryCaptureAsLocalVariable(Result, B, CE->getArg(0)); + return false; + } + + const auto *DRE = dyn_cast(E->IgnoreImplicit()); + if (!DRE) + return false; + + const auto *VD = dyn_cast(DRE->getDecl()); + if (!VD || !VD->isLocalVarDeclOrParm()) + return false; + + B.CaptureMode = CM_ByValue; + B.UsageIdentifier = getSourceTextForExpr(Result, E); + B.CaptureIdentifier = B.UsageIdentifier; + return true; +} + +static bool tryCaptureAsMemberVariable(const MatchFinder::MatchResult &Result, + BindArgument &B, const Expr *E) { + if (const auto *BTE = dyn_cast(E)) { + if (const auto *CE = dyn_cast(BTE->getSubExpr())) + return tryCaptureAsMemberVariable(Result, B, CE->getArg(0)); + return false; + } + + E = E->IgnoreImplicit(); + if (isa(E)) { + B.CaptureMode = CM_ByValue; + B.UsageIdentifier = getSourceTextForExpr(Result, E); + B.CaptureIdentifier = "this"; + return true; + } + + const auto *ME = dyn_cast(E); + if (!ME) + return false; + + if (!ME->isLValue() || !isa(ME->getMemberDecl())) + return false; + + B.CaptureMode = CM_ByValue; + B.UsageIdentifier = getSourceTextForExpr(Result, E); + B.CaptureIdentifier = "this"; + return true; +} + static SmallVector -buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) { +buildBindArguments(const MatchFinder::MatchResult &Result, + const CallableInfo &Callable) { SmallVector BindArguments; llvm::Regex MatchPlaceholder("^_([0-9]+)$"); + const auto *BindCall = Result.Nodes.getNodeAs("bind"); + // Start at index 1 as first argument to bind is the function name. - for (size_t I = 1, ArgCount = C->getNumArgs(); I < ArgCount; ++I) { - const Expr *E = C->getArg(I); - BindArgument B; - if (const auto *M = dyn_cast(E)) { - const auto *TE = M->getSubExpr(); - B.Kind = isa(TE) ? BK_CallExpr : BK_Temporary; - } + unsigned CaptureIndex = 0; + for (size_t I = 1, ArgCount = BindCall->getNumArgs(); I < ArgCount; ++I) { + + const Expr *E = BindCall->getArg(I); + BindArgument &B = BindArguments.emplace_back(); + + size_t ArgIndex = I - 1; + if (Callable.Type == CT_MemberFunction) + --ArgIndex; + + bool IsObjectPtr = (I == 1 && Callable.Type == CT_MemberFunction); + B.E = E; + B.SourceTokens = getSourceTextForExpr(Result, E); - B.Tokens = Lexer::getSourceText( - CharSourceRange::getTokenRange(E->getBeginLoc(), E->getEndLoc()), - *Result.SourceManager, Result.Context->getLangOpts()); + if (!Callable.Decl || ArgIndex < Callable.Decl->getNumParams() || + IsObjectPtr) + B.IsUsed = true; SmallVector Matches; - if (B.Kind == BK_Other && MatchPlaceholder.match(B.Tokens, &Matches)) { + if (MatchPlaceholder.match(B.SourceTokens, &Matches)) { B.Kind = BK_Placeholder; B.PlaceHolderIndex = std::stoi(Matches[1]); + B.UsageIdentifier = "PH" + llvm::utostr(B.PlaceHolderIndex); + B.CaptureIdentifier = B.UsageIdentifier; + continue; + } + + if (const auto *CE = + dyn_cast(ignoreTemporariesAndConstructors(E))) { + initializeBindArgumentForCallExpr(Result, B, CE, CaptureIndex); + continue; + } + + if (tryCaptureAsLocalVariable(Result, B, B.E) || + tryCaptureAsMemberVariable(Result, B, B.E)) + continue; + + // If it's not something we recognize, capture it by init expression to be + // safe. + B.Kind = BK_Other; + if (IsObjectPtr) { + B.CaptureMode = CM_InitExpression; + B.UsageIdentifier = "ObjectPtr"; + B.CaptureIdentifier = B.UsageIdentifier; + } else if (anyDescendantIsLocal(B.E)) { + B.CaptureMode = CM_InitExpression; + B.CaptureIdentifier = "capture" + llvm::utostr(CaptureIndex++); + B.UsageIdentifier = B.CaptureIdentifier; } - BindArguments.push_back(B); } return BindArguments; } -static void addPlaceholderArgs(const ArrayRef Args, - llvm::raw_ostream &Stream) { +static int findPositionOfPlaceholderUse(ArrayRef Args, + size_t PlaceholderIndex) { + for (size_t I = 0; I < Args.size(); ++I) + if (Args[I].PlaceHolderIndex == PlaceholderIndex) + return I; + + return -1; +} + +static void addPlaceholderArgs(const LambdaProperties &LP, + llvm::raw_ostream &Stream, + bool PermissiveParameterList) { + + ArrayRef Args = LP.BindArguments; + auto MaxPlaceholderIt = std::max_element(Args.begin(), Args.end(), [](const BindArgument &B1, const BindArgument &B2) { @@ -80,27 +306,41 @@ }); // Placeholders (if present) have index 1 or greater. - if (MaxPlaceholderIt == Args.end() || MaxPlaceholderIt->PlaceHolderIndex == 0) + if (!PermissiveParameterList && (MaxPlaceholderIt == Args.end() || + MaxPlaceholderIt->PlaceHolderIndex == 0)) return; size_t PlaceholderCount = MaxPlaceholderIt->PlaceHolderIndex; Stream << "("; StringRef Delimiter = ""; for (size_t I = 1; I <= PlaceholderCount; ++I) { - Stream << Delimiter << "auto && arg" << I; + Stream << Delimiter << "auto &&"; + + int ArgIndex = findPositionOfPlaceholderUse(Args, I); + + if (ArgIndex != -1 && Args[ArgIndex].IsUsed) + Stream << " " << Args[ArgIndex].UsageIdentifier; Delimiter = ", "; } + if (PermissiveParameterList) + Stream << Delimiter << "auto && ..."; Stream << ")"; } -static void addFunctionCallArgs(const ArrayRef Args, +static void addFunctionCallArgs(ArrayRef Args, llvm::raw_ostream &Stream) { StringRef Delimiter = ""; - for (const auto &B : Args) { - if (B.PlaceHolderIndex) - Stream << Delimiter << "arg" << B.PlaceHolderIndex; - else - Stream << Delimiter << B.Tokens; + + for (int I = 0, Size = Args.size(); I < Size; ++I) { + const BindArgument &B = Args[I]; + + Stream << Delimiter; + + if (B.Kind == BK_Placeholder || B.CaptureMode != CM_None) + Stream << B.UsageIdentifier; + else if (B.CaptureMode == CM_None) + Stream << B.SourceTokens; + Delimiter = ", "; } } @@ -116,59 +356,301 @@ return false; } +static std::vector +findCandidateCallOperators(const CXXRecordDecl *RecordDecl, size_t NumArgs) { + std::vector Candidates; + + for (const clang::CXXMethodDecl *Method : RecordDecl->methods()) { + OverloadedOperatorKind OOK = Method->getOverloadedOperator(); + + if (OOK != OverloadedOperatorKind::OO_Call) + continue; + + if (Method->getNumParams() > NumArgs) + continue; + + Candidates.push_back(Method); + } + + return Candidates; +} + +static bool isFixitSupported(const CallableInfo &Callee, + ArrayRef Args) { + // Do not attempt to create fixits for nested std::bind or std::ref. + // Supporting nested std::bind will be more difficult due to placeholder + // sharing between outer and inner std::bind invocations, and std::ref + // requires us to capture some parameters by reference instead of by value. + if (any_of(Args, [](const BindArgument &B) { + return isCallExprNamed(B.E, "boost::bind") || + isCallExprNamed(B.E, "std::bind"); + })) { + return false; + } + + // Do not attempt to create fixits when placeholders are reused. + // Unused placeholders are supported by requiring C++14 generic lambdas. + // FIXME: Support this case by deducing the common type. + if (isPlaceHolderIndexRepeated(Args)) + return false; + + // If we can't determine the Decl being used, don't offer a fixit. + if (!Callee.Decl) + return false; + + if (Callee.Type == CT_Other || Callee.Materialization == CMK_Other) + return false; + + return true; +} + +const FunctionDecl *getCallOperator(const CXXRecordDecl *Callable, + size_t NumArgs) { + std::vector Candidates = + findCandidateCallOperators(Callable, NumArgs); + if (Candidates.size() != 1) + return nullptr; + + return Candidates.front(); +} + +const FunctionDecl * +getCallMethodDecl(const MatchFinder::MatchResult &Result, CallableType Type, + CallableMaterializationKind Materialization) { + + const Expr *Callee = Result.Nodes.getNodeAs("ref"); + const Expr *CallExpression = ignoreTemporariesAndPointers(Callee); + + if (Type == CT_Object) { + const auto *BindCall = Result.Nodes.getNodeAs("bind"); + size_t NumArgs = BindCall->getNumArgs() - 1; + return getCallOperator(Callee->getType()->getAsCXXRecordDecl(), NumArgs); + } + + if (Materialization == CMK_Function) { + if (const auto *DRE = dyn_cast(CallExpression)) + return dyn_cast(DRE->getDecl()); + } + + // Maybe this is an indirect call through a function pointer or something + // where we can't determine the exact decl. + return nullptr; +} + +static CallableType getCallableType(const MatchFinder::MatchResult &Result) { + const auto *CallableExpr = Result.Nodes.getNodeAs("ref"); + + QualType QT = CallableExpr->getType(); + if (QT->isMemberFunctionPointerType()) + return CT_MemberFunction; + + if (QT->isFunctionPointerType() || QT->isFunctionReferenceType() || + QT->isFunctionType()) + return CT_Function; + + if (QT->isRecordType()) { + const CXXRecordDecl *Decl = QT->getAsCXXRecordDecl(); + if (!Decl) + return CT_Other; + + return CT_Object; + } + + return CT_Other; +} + +static CallableMaterializationKind +getCallableMaterialization(const MatchFinder::MatchResult &Result) { + const auto *CallableExpr = Result.Nodes.getNodeAs("ref"); + + const auto *NoTemporaries = ignoreTemporariesAndPointers(CallableExpr); + + if (isa(NoTemporaries)) + return CMK_CallExpression; + + if (isa(NoTemporaries) || + isa(NoTemporaries)) + return CMK_Function; + + if (const auto *DRE = dyn_cast(NoTemporaries)) { + if (const auto *FD = dyn_cast(DRE->getDecl())) + return CMK_Function; + if (const auto *VD = dyn_cast(DRE->getDecl())) + return CMK_VariableRef; + } + + return CMK_Other; +} + +static LambdaProperties +getLambdaProperties(const MatchFinder::MatchResult &Result) { + const auto *CalleeExpr = Result.Nodes.getNodeAs("ref"); + + LambdaProperties LP; + + const auto *Bind = Result.Nodes.getNodeAs("bind"); + const auto *Decl = dyn_cast(Bind->getCalleeDecl()); + const auto *NS = + dyn_cast(Decl->getEnclosingNamespaceContext()); + while (NS->isInlineNamespace()) + NS = dyn_cast(NS->getDeclContext()); + LP.BindNamespace = NS->getName(); + + LP.Callable.Type = getCallableType(Result); + LP.Callable.Materialization = getCallableMaterialization(Result); + LP.Callable.Decl = + getCallMethodDecl(Result, LP.Callable.Type, LP.Callable.Materialization); + LP.Callable.SourceTokens = getSourceTextForExpr(Result, CalleeExpr); + if (LP.Callable.Materialization == CMK_VariableRef) { + LP.Callable.CaptureMode = CM_ByValue; + LP.Callable.UsageIdentifier = getSourceTextForExpr(Result, CalleeExpr); + LP.Callable.CaptureIdentifier = + getSourceTextForExpr(Result, ignoreTemporariesAndPointers(CalleeExpr)); + } else if (LP.Callable.Materialization == CMK_CallExpression) { + LP.Callable.CaptureMode = CM_InitExpression; + LP.Callable.UsageIdentifier = "Func"; + LP.Callable.CaptureIdentifier = "Func"; + LP.Callable.CaptureInitializer = getSourceTextForExpr(Result, CalleeExpr); + } + + LP.BindArguments = buildBindArguments(Result, LP.Callable); + + LP.IsFixitSupported = isFixitSupported(LP.Callable, LP.BindArguments); + + return LP; +} + +static bool emitCapture(llvm::StringSet<> &CaptureSet, StringRef Delimiter, + CaptureMode CM, StringRef Identifier, + StringRef InitExpression, raw_ostream &Stream) { + if (CM == CM_None) + return false; + + // This capture has already been emitted. + if (CaptureSet.count(Identifier) != 0) + return false; + + Stream << Delimiter; + + if (CM == CM_ByRef) + Stream << "&"; + Stream << Identifier; + if (CM == CM_InitExpression) + Stream << " = " << InitExpression; + + CaptureSet.insert(Identifier); + return true; +} + +static void emitCaptureList(const LambdaProperties &LP, + const MatchFinder::MatchResult &Result, + raw_ostream &Stream) { + llvm::StringSet<> CaptureSet; + bool AnyCapturesEmitted = false; + + AnyCapturesEmitted = emitCapture(CaptureSet, "", LP.Callable.CaptureMode, + LP.Callable.CaptureIdentifier, + LP.Callable.CaptureInitializer, Stream); + + for (const BindArgument &B : LP.BindArguments) { + if (B.CaptureMode == CM_None || !B.IsUsed) + continue; + + StringRef Delimiter = AnyCapturesEmitted ? ", " : ""; + + if (emitCapture(CaptureSet, Delimiter, B.CaptureMode, B.CaptureIdentifier, + B.SourceTokens, Stream)) + AnyCapturesEmitted = true; + } +} + +static ArrayRef +getForwardedArgumentList(const LambdaProperties &P) { + ArrayRef Args = makeArrayRef(P.BindArguments); + if (P.Callable.Type != CT_MemberFunction) + return Args; + + return Args.drop_front(); +} +AvoidBindCheck::AvoidBindCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + PermissiveParameterList(Options.get("PermissiveParameterList", 0) != 0) {} + void AvoidBindCheck::registerMatchers(MatchFinder *Finder) { if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas. return; Finder->addMatcher( callExpr( - callee(namedDecl(hasName("::std::bind"))), - hasArgument(0, declRefExpr(to(functionDecl().bind("f"))).bind("ref"))) + callee(namedDecl( + anyOf(hasName("::boost::bind"), hasName("::std::bind")))), + hasArgument( + 0, anyOf(expr(hasType(memberPointerType())).bind("ref"), + expr(hasParent(materializeTemporaryExpr().bind("ref"))), + expr().bind("ref")))) .bind("bind"), this); } void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) { const auto *MatchedDecl = Result.Nodes.getNodeAs("bind"); - auto Diag = diag(MatchedDecl->getBeginLoc(), "prefer a lambda to std::bind"); - - const auto Args = buildBindArguments(Result, MatchedDecl); - // Do not attempt to create fixits for nested call expressions. - // FIXME: Create lambda capture variables to capture output of calls. - // NOTE: Supporting nested std::bind will be more difficult due to placeholder - // sharing between outer and inner std:bind invocations. - if (llvm::any_of(Args, - [](const BindArgument &B) { return B.Kind == BK_CallExpr; })) - return; - - // Do not attempt to create fixits when placeholders are reused. - // Unused placeholders are supported by requiring C++14 generic lambdas. - // FIXME: Support this case by deducing the common type. - if (isPlaceHolderIndexRepeated(Args)) + LambdaProperties LP = getLambdaProperties(Result); + auto Diag = + diag(MatchedDecl->getBeginLoc(), + formatv("prefer a lambda to {0}::bind", LP.BindNamespace).str()); + if (!LP.IsFixitSupported) return; - const auto *F = Result.Nodes.getNodeAs("f"); - - // std::bind can support argument count mismatch between its arguments and the - // bound function's arguments. Do not attempt to generate a fixit for such - // cases. - // FIXME: Support this case by creating unused lambda capture variables. - if (F->getNumParams() != Args.size()) - return; + const auto *Ref = Result.Nodes.getNodeAs("ref"); std::string Buffer; llvm::raw_string_ostream Stream(Buffer); - bool HasCapturedArgument = llvm::any_of( - Args, [](const BindArgument &B) { return B.Kind == BK_Other; }); - const auto *Ref = Result.Nodes.getNodeAs("ref"); - Stream << "[" << (HasCapturedArgument ? "=" : "") << "]"; - addPlaceholderArgs(Args, Stream); - Stream << " { return "; - Ref->printPretty(Stream, nullptr, Result.Context->getPrintingPolicy()); + Stream << "["; + emitCaptureList(LP, Result, Stream); + Stream << "]"; + + ArrayRef FunctionCallArgs = makeArrayRef(LP.BindArguments); + + addPlaceholderArgs(LP, Stream, PermissiveParameterList); + + if (LP.Callable.Type == CT_Function) { + StringRef SourceTokens = LP.Callable.SourceTokens; + SourceTokens.consume_front("&"); + Stream << " { return " << SourceTokens; + } else if (LP.Callable.Type == CT_MemberFunction) { + const auto *MethodDecl = dyn_cast(LP.Callable.Decl); + const BindArgument &ObjPtr = FunctionCallArgs.front(); + + Stream << " { "; + if (!isa(ignoreTemporariesAndPointers(ObjPtr.E))) { + Stream << ObjPtr.UsageIdentifier; + Stream << "->"; + } + + Stream << MethodDecl->getName(); + } else { + Stream << " { return "; + switch (LP.Callable.CaptureMode) { + case CM_ByValue: + case CM_ByRef: + if (LP.Callable.UsageIdentifier != LP.Callable.CaptureIdentifier) { + Stream << "(" << LP.Callable.UsageIdentifier << ")"; + break; + } + LLVM_FALLTHROUGH; + case CM_InitExpression: + Stream << LP.Callable.UsageIdentifier; + break; + default: + Ref->printPretty(Stream, nullptr, Result.Context->getPrintingPolicy()); + } + } + Stream << "("; - addFunctionCallArgs(Args, Stream); + + addFunctionCallArgs(getForwardedArgumentList(LP), Stream); Stream << "); }"; Diag << FixItHint::CreateReplacement(MatchedDecl->getSourceRange(), diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -196,6 +196,14 @@ ` check now supports a `StringNames` option enabling its application to custom string classes. +- Improved :doc:`modernize-avoid-bind + ` check. + + The check now supports supports diagnosing and fixing arbitrary callables instead of + only simple free functions. The `PermissiveParameterList` option has also been + added to address situations where the existing fix-it logic would sometimes generate + code that no longer compiles. + Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst --- a/clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize-avoid-bind.rst @@ -3,10 +3,15 @@ modernize-avoid-bind ==================== -The check finds uses of ``std::bind`` and replaces simple uses with lambdas. -Lambdas will use value-capture where required. +The check finds uses of ``std::bind`` and ``boost::bind`` and replaces them +with lambdas. Lambdas will use value-capture unless reference capture is +explicitly requested with ``std::ref`` or ``boost::ref``. -Right now it only handles free functions, not member functions. +It supports arbitrary callables including member functions, function objects, +and free functions, and all variations thereof. Anything that you can pass +to the first argument of ``bind`` should be diagnosable. Currently, the only +known case where a fix-it is unsupported is when the same placeholder is +specified multiple times in the parameter list. Given: @@ -35,3 +40,49 @@ ``std::bind`` can be hard to read and can result in larger object files and binaries due to type information that will not be produced by equivalent lambdas. + +Options +------- + +.. option:: PermissiveParameterList + + If the option is set to non-zero, the check will append ``auto&&...`` to the end + of every placeholder parameter list. Without this, it is possible for a fix-it + to perform an incorrect transformation in the case where the result of the ``bind`` + is used in the context of a type erased functor such as ``std::function`` which + allows mismatched arguments. For example: + + +.. code-block:: c++ + + int add(int x, int y) { return x + y; } + int foo() { + std::function ignore_args = std::bind(add, 2, 2); + return ignore_args(3, 3); + } + +is valid code, and returns `4`. The actual values passed to ``ignore_args`` are +simply ignored. Without ``PermissiveParameterList``, this would be transformed into + +.. code-block:: c++ + + int add(int x, int y) { return x + y; } + int foo() { + std::function ignore_args = [] { return add(2, 2); } + return ignore_args(3, 3); + } + +which will *not* compile, since the lambda does not contain an ``operator()`` that +that accepts 2 arguments. With permissive parameter list, it instead generates + +.. code-block:: c++ + + int add(int x, int y) { return x + y; } + int foo() { + std::function ignore_args = [](auto&&...) { return add(2, 2); } + return ignore_args(3, 3); + } + +which is correct. + +This check requires using C++14 or higher to run. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind-permissive-parameter-list.cpp @@ -0,0 +1,58 @@ +// RUN: %check_clang_tidy -std=c++14-or-later %s modernize-avoid-bind %t -- \ +// RUN: -config="{CheckOptions: [ \ +// RUN: {key: modernize-avoid-bind.PermissiveParameterList, value: 1}]}" -- + +namespace std { +inline namespace impl { +template +class bind_rt {}; + +template +bind_rt bind(Fp &&, Arguments &&...); +} // namespace impl + +template +T ref(T &t); +} // namespace std + +int add(int x, int y) { return x + y; } + +// Let's fake a minimal std::function-like facility. +namespace std { +template +_Tp declval(); + +template +struct __res { + template + static decltype(declval<_Functor>()(_Args()...)) _S_test(int); + + template + static void _S_test(...); + + using type = decltype(_S_test<_ArgTypes...>(0)); +}; + +template +struct function; + +template +struct function { + template ::type> + function(_Functor) {} +}; +} // namespace std + +struct placeholder {}; +placeholder _1; + +void testLiteralParameters() { + auto AAA = std::bind(add, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto AAA = [](auto && ...) { return add(2, 2); }; + + auto BBB = std::bind(add, _1, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto BBB = [](auto && PH1, auto && ...) { return add(PH1, 2); }; +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp @@ -8,75 +8,62 @@ template bind_rt bind(Fp &&, Arguments &&...); } + +template +T ref(T &t); } -int add(int x, int y) { return x + y; } +namespace boost { +template +class bind_rt {}; -void f() { - auto clj = std::bind(add, 2, 2); - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] - // CHECK-FIXES: auto clj = [] { return add(2, 2); }; -} +template +bind_rt bind(const Fp &, Arguments...); -void g() { - int x = 2; - int y = 2; - auto clj = std::bind(add, x, y); - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind - // CHECK-FIXES: auto clj = [=] { return add(x, y); }; +template +struct reference_wrapper { + explicit reference_wrapper(T &t) {} +}; + +template +reference_wrapper const ref(T &t) { + return reference_wrapper(t); } -struct placeholder {}; -placeholder _1; -placeholder _2; +} // namespace boost -void h() { - int x = 2; - auto clj = std::bind(add, x, _1); - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind - // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); }; -} +namespace C { +int add(int x, int y) { return x + y; } +} // namespace C -struct A; -struct B; -bool ABTest(const A &, const B &); +struct Foo { + static int add(int x, int y) { return x + y; } +}; -void i() { - auto BATest = std::bind(ABTest, _2, _1); - // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind - // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); }; -} +struct D { + D() = default; + void operator()(int x, int y) const {} -void j() { - auto clj = std::bind(add, 2, 2, 2); - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind - // No fix is applied for argument mismatches. - // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2); -} + void MemberFunction(int x) {} -void k() { - auto clj = std::bind(add, _1, _1); - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind - // No fix is applied for reused placeholders. - // CHECK-FIXES: auto clj = std::bind(add, _1, _1); -} + static D *create(); +}; -void m() { - auto clj = std::bind(add, 1, add(2, 5)); - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind - // No fix is applied for nested calls. - // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5)); -} +struct F { + F(int x) {} + ~F() {} -namespace C { - int add(int x, int y){ return x + y; } -} + int get() { return 42; } +}; -void n() { - auto clj = std::bind(C::add, 1, 1); - // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind - // CHECK-FIXES: auto clj = [] { return C::add(1, 1); }; -} +void UseF(F); + +struct placeholder {}; +placeholder _1; +placeholder _2; + +int add(int x, int y) { return x + y; } +int addThree(int x, int y, int z) { return x + y + z; } // Let's fake a minimal std::function-like facility. namespace std { @@ -114,10 +101,213 @@ void Reset(std::function); }; -void test(Thing *t) { +int GlobalVariable = 42; + +struct TestCaptureByValueStruct { + int MemberVariable; + static int StaticMemberVariable; + F MemberStruct; + + void testCaptureByValue(int Param, F f) { + int x = 3; + int y = 4; + auto AAA = std::bind(add, x, y); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto AAA = [x, y] { return add(x, y); }; + + // When the captured variable is repeated, it should only appear in the capture list once. + auto BBB = std::bind(add, x, x); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto BBB = [x] { return add(x, x); }; + + int LocalVariable; + // Global variables shouldn't be captured at all, and members should be captured through this. + auto CCC = std::bind(add, MemberVariable, GlobalVariable); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto CCC = [this] { return add(MemberVariable, GlobalVariable); }; + + // Static member variables shouldn't be captured, but locals should + auto DDD = std::bind(add, TestCaptureByValueStruct::StaticMemberVariable, LocalVariable); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto DDD = [LocalVariable] { return add(TestCaptureByValueStruct::StaticMemberVariable, LocalVariable); }; + + auto EEE = std::bind(add, Param, Param); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto EEE = [Param] { return add(Param, Param); }; + + // The signature of boost::bind() is different, and causes + // CXXBindTemporaryExprs to be created in certain cases. So let's test + // those here. + auto FFF = boost::bind(UseF, f); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind] + // CHECK-FIXES: auto FFF = [f] { return UseF(f); }; + + auto GGG = boost::bind(UseF, MemberStruct); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind] + // CHECK-FIXES: auto GGG = [this] { return UseF(MemberStruct); }; + } +}; + +void testLiteralParameters() { + auto AAA = std::bind(add, 2, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto AAA = [] { return add(2, 2); }; + + auto BBB = std::bind(addThree, 2, 3, 4); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind] + // CHECK-FIXES: auto BBB = [] { return addThree(2, 3, 4); }; +} + +void testCaptureByReference() { + int x = 2; + int y = 2; + auto AAA = std::bind(add, std::ref(x), std::ref(y)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto AAA = [&x, &y] { return add(x, y); }; + + auto BBB = std::bind(add, std::ref(x), y); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto BBB = [&x, y] { return add(x, y); }; + + auto CCC = std::bind(add, y, std::ref(x)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto CCC = [y, &x] { return add(y, x); }; + + // Make sure it works with boost::ref() too which has slightly different + // semantics. + auto DDD = boost::bind(add, boost::ref(x), boost::ref(y)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to boost::bind + // CHECK-FIXES: auto DDD = [&x, &y] { return add(x, y); }; + + auto EEE = boost::bind(add, boost::ref(x), y); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to boost::bind + // CHECK-FIXES: auto EEE = [&x, y] { return add(x, y); }; + + auto FFF = boost::bind(add, y, boost::ref(x)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to boost::bind + // CHECK-FIXES: auto FFF = [y, &x] { return add(y, x); }; +} + +void testCaptureByInitExpression() { + int x = 42; + auto AAA = std::bind(add, x, F(x).get()); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto AAA = [x, capture0 = F(x).get()] { return add(x, capture0); }; +} + +void testFunctionObjects() { + D d; + D *e = nullptr; + auto AAA = std::bind(d, 1, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto AAA = [d] { return d(1, 2); } + + auto BBB = std::bind(*e, 1, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto BBB = [e] { return (*e)(1, 2); } + + auto CCC = std::bind(D{}, 1, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto CCC = [] { return D{}(1, 2); } + + auto DDD = std::bind(D(), 1, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto DDD = [] { return D()(1, 2); } + + auto EEE = std::bind(*D::create(), 1, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto EEE = [Func = *D::create()] { return Func(1, 2); }; +} + +void testPlaceholders() { + int x = 2; + auto AAA = std::bind(add, x, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto AAA = [x](auto && PH1) { return add(x, PH1); }; + + auto BBB = std::bind(add, _2, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto BBB = [](auto && PH1, auto && PH2) { return add(PH2, PH1); }; + + // No fix is applied for reused placeholders. + auto CCC = std::bind(add, _1, _1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto CCC = std::bind(add, _1, _1); + + // When a placeholder is skipped, we always add skipped ones to the lambda as + // unnamed parameters. + auto DDD = std::bind(add, _2, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto DDD = [](auto &&, auto && PH2) { return add(PH2, 1); }; +} + +void testGlobalFunctions() { + auto AAA = std::bind(C::add, 1, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto AAA = [] { return C::add(1, 1); }; + + auto BBB = std::bind(Foo::add, 1, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto BBB = [] { return Foo::add(1, 1); }; + + // The & should get removed inside of the lambda body. + auto CCC = std::bind(&C::add, 1, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto CCC = [] { return C::add(1, 1); }; + + auto DDD = std::bind(&Foo::add, 1, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto DDD = [] { return Foo::add(1, 1); }; + + auto EEE = std::bind(&add, 1, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto EEE = [] { return add(1, 1); }; +} + +void testCapturedSubexpressions() { + int x = 3; + int y = 3; + + auto AAA = std::bind(add, 1, add(2, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // Results of nested calls are captured by value. + // CHECK-FIXES: auto AAA = [capture0 = add(2, 5)] { return add(1, capture0); }; + + auto BBB = std::bind(add, x, add(y, 5)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind + // Results of nested calls are captured by value. + // CHECK-FIXES: auto BBB = [x, capture0 = add(y, 5)] { return add(x, capture0); }; +} + +struct E { + void MemberFunction(int x) {} + + void testMemberFunctions() { + D *d; + D dd; + auto AAA = std::bind(&D::MemberFunction, d, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto AAA = [d] { d->MemberFunction(1); }; + + auto BBB = std::bind(&D::MemberFunction, &dd, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto BBB = [ObjectPtr = &dd] { ObjectPtr->MemberFunction(1); }; + + auto CCC = std::bind(&E::MemberFunction, this, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto CCC = [this] { MemberFunction(1); }; + + // Test what happens when the object pointer is itself a placeholder. + auto DDD = std::bind(&D::MemberFunction, _1, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind + // CHECK-FIXES: auto DDD = [](auto && PH1) { PH1->MemberFunction(1); }; + } +}; + +void testStdFunction(Thing *t) { Callback cb; if (t) cb.Reset(std::bind(UseThing, t)); // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind - // CHECK-FIXES: cb.Reset([=] { return UseThing(t); }); + // CHECK-FIXES: cb.Reset([t] { return UseThing(t); }); }