diff --git a/clang-tools-extra/clang-tidy/readability/AvoidDefaultLambdaCaptureCheck.cpp b/clang-tools-extra/clang-tidy/readability/AvoidDefaultLambdaCaptureCheck.cpp new file mode 100644 index 0000000000000..7e19ec9d98979 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/AvoidDefaultLambdaCaptureCheck.cpp @@ -0,0 +1,71 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "AvoidDefaultLambdaCaptureCheck.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/Lambda.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::tidy::readability; + +static std::string generateCaptureText(const clang::LambdaCapture &Capture) { + if (Capture.capturesThis()) { + return Capture.getCaptureKind() == clang::LCK_StarThis ? "*this" : "this"; + } + + std::string Result; + if (Capture.getCaptureKind() == clang::LCK_ByRef) { + Result += "&"; + } + Result += Capture.getCapturedVar()->getName().str(); + return Result; +} + +void AvoidDefaultLambdaCaptureCheck::registerMatchers( + clang::ast_matchers::MatchFinder *Finder) { + Finder->addMatcher( + clang::ast_matchers::lambdaExpr(clang::ast_matchers::hasDefaultCapture()) + .bind("lambda"), + this); +} + +void AvoidDefaultLambdaCaptureCheck::check( + const clang::ast_matchers::MatchFinder::MatchResult &Result) { + const auto *Lambda = Result.Nodes.getNodeAs("lambda"); + assert(Lambda); + + const clang::SourceLocation DefaultCaptureLoc = + Lambda->getCaptureDefaultLoc(); + if (DefaultCaptureLoc.isInvalid()) + return; + + std::vector ImplicitCaptures; + for (const auto &Capture : Lambda->implicit_captures()) { + // It is impossible to explicitly capture a VLA in C++, since VLAs don't + // exist in ISO C++ and so the syntax was never created to capture them. + if (Capture.getCaptureKind() == LCK_VLAType) + return; + ImplicitCaptures.push_back(generateCaptureText(Capture)); + } + + auto Diag = diag(DefaultCaptureLoc, + "lambda default captures are discouraged; " + "prefer to capture specific variables explicitly"); + + // For template-dependent lambdas, the list of captures hasn't been created + // yet, so the list of implicit captures is empty. + if (ImplicitCaptures.empty() && Lambda->isGenericLambda()) + return; + + const auto ReplacementText = [&ImplicitCaptures]() { + return llvm::join(ImplicitCaptures, ", "); + }(); + + Diag << clang::FixItHint::CreateReplacement(Lambda->getCaptureDefaultLoc(), + ReplacementText); +} diff --git a/clang-tools-extra/clang-tidy/readability/AvoidDefaultLambdaCaptureCheck.h b/clang-tools-extra/clang-tidy/readability/AvoidDefaultLambdaCaptureCheck.h new file mode 100644 index 0000000000000..82262a210e4c4 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/AvoidDefaultLambdaCaptureCheck.h @@ -0,0 +1,33 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDDEFAULTLAMBDACAPTURECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDDEFAULTLAMBDACAPTURECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::readability { + +/// Flags lambdas that use default capture modes +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-default-lambda-capture.html +class AvoidDefaultLambdaCaptureCheck : public ClangTidyCheck { +public: + AvoidDefaultLambdaCaptureCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + std::optional getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } +}; + +} // namespace clang::tidy::readability + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDDEFAULTLAMBDACAPTURECHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index 0d0641c4b22bf..34b5cc912511c 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -6,6 +6,7 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangTidyReadabilityModule STATIC AmbiguousSmartptrResetCallCheck.cpp AvoidConstParamsInDecls.cpp + AvoidDefaultLambdaCaptureCheck.cpp AvoidNestedConditionalOperatorCheck.cpp AvoidReturnWithVoidValueCheck.cpp AvoidUnconditionalPreprocessorIfCheck.cpp diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index fcfac05b000e4..e50893b8175a6 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "AmbiguousSmartptrResetCallCheck.h" #include "AvoidConstParamsInDecls.h" +#include "AvoidDefaultLambdaCaptureCheck.h" #include "AvoidNestedConditionalOperatorCheck.h" #include "AvoidReturnWithVoidValueCheck.h" #include "AvoidUnconditionalPreprocessorIfCheck.h" @@ -93,6 +94,8 @@ class ReadabilityModule : public ClangTidyModule { "readability-container-size-empty"); CheckFactories.registerCheck( "readability-convert-member-functions-to-static"); + CheckFactories.registerCheck( + "readability-avoid-default-lambda-capture"); CheckFactories.registerCheck( "readability-delete-null-pointer"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index c3a6d2f9b2890..8451efdbd0d32 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -203,6 +203,11 @@ New checks Finds virtual function overrides with different visibility than the function in the base class. +- New :doc:`readability-avoid-default-lambda-capture + ` check. + + Warns on default lambda captures (e.g. ``[&](){ ... }``, ``[=](){ ... }``). + - New :doc:`readability-redundant-parentheses ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index f94696d4ef9c7..6ea918e2b3a09 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -377,6 +377,7 @@ Clang-Tidy Checks :doc:`readability-container-data-pointer `, "Yes" :doc:`readability-container-size-empty `, "Yes" :doc:`readability-convert-member-functions-to-static `, "Yes" + :doc:`readability-avoid-default-lambda-capture `, :doc:`readability-delete-null-pointer `, "Yes" :doc:`readability-duplicate-include `, "Yes" :doc:`readability-else-after-return `, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/avoid-default-lambda-capture.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/avoid-default-lambda-capture.rst new file mode 100644 index 0000000000000..b345a566488c6 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/avoid-default-lambda-capture.rst @@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-avoid-default-lambda-capture + +readability-avoid-default-lambda-capture +======================================== + +Warns on default lambda captures (e.g. ``[&](){ ... }``, ``[=](){ ... }``). + +Captures can lead to subtle bugs including dangling references and unnecessary +copies. Writing out the name of the variables being captured reminds programmers +and reviewers to know what is being captured. And knowing is half the battle. + +This check does not lint for variable-length array (VLA) captures. VLAs are not +ISO C++, and it is impossible to explicitly capture them as the syntax for doing +so does not exist. + +Coding guidelines that recommend against defaulted lambda captures include: + +* Item 31 of Effective Modern C++ by Scott Meyers + +Example +------- + +.. code-block:: c++ + + #include + + class Widget { + std::vector> callbacks; + int widgetId; + void addCallback(int factoryId) { + callbacks.emplace_back( + [&](){ + std::cout << "Widget " << widgetId << " made in factory " << factoryId; + } + ); + } + } + +When ``callbacks`` is executed, ``factoryId`` will dangle. Writing the name of +``factoryId`` in the capture list reminds the reader that it is being captured, +which will hopefully lead to the bug being fixed during code review. + +.. code-block:: c++ + + #include + + class Widget { + std::vector> callbacks; + int widgetId; + void addCallback(int factoryId) { + callbacks.emplace_back( + [&factoryId, &widgetId](){ // Why isn't factoryId captured by value?? + std::cout << "Widget " << widgetId << " made in factory " << factoryId; + } + ); + } + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-default-lambda-capture.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-default-lambda-capture.cpp new file mode 100644 index 0000000000000..140e00960e8b2 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/avoid-default-lambda-capture.cpp @@ -0,0 +1,143 @@ +// RUN: %check_clang_tidy %s readability-avoid-default-lambda-capture %t -- -- -Wno-vla-extension + +void test_default_captures() { + int value = 42; + int another = 10; + + auto lambda1 = [=](int x) { return value + x; }; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto lambda1 = [value](int x) { return value + x; }; + + auto lambda2 = [&](int x) { return value + x; }; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto lambda2 = [&value](int x) { return value + x; }; + + auto lambda3 = [=, &another](int x) { return value + another + x; }; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto lambda3 = [value, &another](int x) { return value + another + x; }; + + auto lambda4 = [&, value](int x) { return value + another + x; }; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto lambda4 = [&another, value](int x) { return value + another + x; }; +} + +void test_acceptable_captures() { + int value = 42; + int another = 10; + + auto lambda1 = [value](int x) { return value + x; }; + auto lambda2 = [&value](int x) { return value + x; }; + auto lambda3 = [value, another](int x) { return value + another + x; }; + auto lambda4 = [&value, &another](int x) { return value + another + x; }; + + auto lambda5 = [](int x, int y) { return x + y; }; + + struct S { + int member = 5; + void foo() { + auto lambda = [this]() { return member; }; + } + }; +} + +void test_nested_lambdas() { + int outer_var = 1; + int middle_var = 2; + int inner_var = 3; + + auto outer = [=]() { + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto outer = [outer_var, middle_var, inner_var]() { + + auto inner = [&](int x) { return outer_var + middle_var + inner_var + x; }; + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto inner = [&outer_var, &middle_var, &inner_var](int x) { return outer_var + middle_var + inner_var + x; }; + + return inner(10); + }; +} + +void test_lambda_returns() { + int a = 1, b = 2, c = 3; + + auto create_adder = [=](int x) { + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto create_adder = [](int x) { + return [x](int y) { return x + y; }; // Inner lambda is fine - explicit capture + }; + + auto func1 = [&]() { return a; }; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto func1 = [&a]() { return a; }; + + auto func2 = [=]() { return b; }; + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto func2 = [b]() { return b; }; +} + +class TestClass { + int member = 42; + +public: + void test_member_function_lambdas() { + int local = 10; + + auto lambda1 = [=]() { return member + local; }; + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto lambda1 = [this, local]() { return member + local; }; + + auto lambda2 = [&]() { return member + local; }; + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: auto lambda2 = [this, &local]() { return member + local; }; + + auto lambda3 = [this, local]() { return member + local; }; + auto lambda4 = [this, &local]() { return member + local; }; + } +}; + +// Lambda captures dependent on a template parameter don't have a fix it +template +void test_template_lambdas() { + T value{}; + + auto lambda = [=](T x) { return value + x; }; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] +} + +void instantiate_templates() { + test_template_lambdas(); + test_template_lambdas(); +} + +void test_init_captures() { + int x = 3; + int nx = 5; + + int y1 = [&, z = x + 5]() -> int { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: int y1 = [&nx, z = x + 5]() -> int { + return z * z + nx; + }(); + + int y2 = [=, &ref = x]() { + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: lambda default captures are discouraged; prefer to capture specific variables explicitly [readability-avoid-default-lambda-capture] + // CHECK-FIXES: int y2 = [nx, &ref = x]() { + ref += 1; + return nx - ref; + }(); + + (void)y1; + (void)y2; +} + +void test_vla_no_crash() { + // VLAs create implicit VLA bound captures that cannot be written explicitly. + // No warning should be issued. + int n = 5; + int vla[n]; + for (int i = 0; i < n; ++i) { + vla[i] = i * 10; + } + + auto lambda = [&]() { return vla[0]; }; +} diff --git a/clang/include/clang/ASTMatchers/ASTMatchers.h b/clang/include/clang/ASTMatchers/ASTMatchers.h index 492863ddfc4a1..0a0d42ca259b8 100644 --- a/clang/include/clang/ASTMatchers/ASTMatchers.h +++ b/clang/include/clang/ASTMatchers/ASTMatchers.h @@ -5096,6 +5096,20 @@ AST_MATCHER_P(LambdaCapture, capturesVar, internal::Matcher, /// matches `[this]() { return cc; }`. AST_MATCHER(LambdaCapture, capturesThis) { return Node.capturesThis(); } +/// Matches lambda expressions that have default capture modes. +/// +/// Given +/// \code +/// auto l1 = [=]() {}; // matches +/// auto l2 = [&]() {}; // matches +/// auto l3 = []() {}; // does not match +/// \endcode +/// lambdaExpr(hasDefaultCapture()) +/// matches l1 and l2, but not l3. +AST_MATCHER(LambdaExpr, hasDefaultCapture) { + return Node.getCaptureDefault() != LCD_None; +} + /// Matches a constructor call expression which uses list initialization. AST_MATCHER(CXXConstructExpr, isListInitialization) { return Node.isListInitialization();