[clang-tidy] Add readability-avoid-default-lambda-capture

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<clang::LambdaExpr>("lambda");
+  assert(Lambda);
+
+  const clang::SourceLocation DefaultCaptureLoc =
+      Lambda->getCaptureDefaultLoc();
+  if (DefaultCaptureLoc.isInvalid())
+    return;
+
+  std::vector<std::string> 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);
+}

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<TraversalKind> getCheckTraversalKind() const override {
+    return TK_IgnoreUnlessSpelledInSource;
+  }
+};
+
+} // namespace clang::tidy::readability
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_AVOIDDEFAULTLAMBDACAPTURECHECK_H

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

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<ConvertMemberFunctionsToStatic>(
         "readability-convert-member-functions-to-static");
+    CheckFactories.registerCheck<AvoidDefaultLambdaCaptureCheck>(
+        "readability-avoid-default-lambda-capture");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
     CheckFactories.registerCheck<DuplicateIncludeCheck>(

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
+  <clang-tidy/checks/readability/avoid-default-lambda-capture>` check.
+
+  Warns on default lambda captures (e.g. ``[&](){ ... }``, ``[=](){ ... }``).
+
 - New :doc:`readability-redundant-parentheses
   <clang-tidy/checks/readability/redundant-parentheses>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -377,6 +377,7 @@ Clang-Tidy Checks
    :doc:`readability-container-data-pointer <readability/container-data-pointer>`, "Yes"
    :doc:`readability-container-size-empty <readability/container-size-empty>`, "Yes"
    :doc:`readability-convert-member-functions-to-static <readability/convert-member-functions-to-static>`, "Yes"
+   :doc:`readability-avoid-default-lambda-capture <readability/avoid-default-lambda-capture>`,
    :doc:`readability-delete-null-pointer <readability/delete-null-pointer>`, "Yes"
    :doc:`readability-duplicate-include <readability/duplicate-include>`, "Yes"
    :doc:`readability-else-after-return <readability/else-after-return>`, "Yes"

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.
+
+Coding guidelines that recommend against defaulted lambda captures include:
+
+* Item 31 of Effective Modern C++ by Scott Meyers
+
+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 does not
+exist.
+
+Example
+-------
+
+.. code-block:: c++
+
+  #include <iostream>
+
+  class Widget {
+    std::vector<std::function<void(int)>> 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 <iostream>
+
+  class Widget {
+    std::vector<std::function<void(int)>> 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;
+        }
+      );
+    }
+  }

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<typename T>
+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<int>();
+  test_template_lambdas<double>();
+}
+
+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]; };
+}

clang/include/clang/ASTMatchers/ASTMatchers.h

@@ -5096,6 +5096,20 @@ AST_MATCHER_P(LambdaCapture, capturesVar, internal::Matcher<ValueDecl>,
 ///   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();