[clang-tidy] Implement CppCoreGuideline F.54

Warn when a lambda specifies a default capture and captures ``this``. Offer FixIts to correct the code. Reviewed By: njames93, carlosgalvezp Differential Revision: https://reviews.llvm.org/D141133
llvm · Feb 2, 2023 · 8a8f77c · 8a8f77c
1 parent 66a30b9
commit 8a8f77c
Show file tree

Hide file tree

Showing 8 changed files with 284 additions and 0 deletions.
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp
@@ -0,0 +1,98 @@
+//===--- AvoidCaptureDefaultWhenCapturingThisCheck.cpp - clang-tidy--------===//
+//
+// 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 "AvoidCaptureDefaultWhenCapturingThisCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include <algorithm>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+void AvoidCaptureDefaultWhenCapturingThisCheck::registerMatchers(
+    MatchFinder *Finder) {
+  Finder->addMatcher(lambdaExpr(hasAnyCapture(capturesThis())).bind("lambda"),
+                     this);
+}
+
+static SourceLocation findDefaultCaptureEnd(const LambdaExpr *Lambda,
+                                            ASTContext &Context) {
+  for (const LambdaCapture &Capture : Lambda->explicit_captures()) {
+    if (Capture.isExplicit()) {
+      if (Capture.getCaptureKind() == LCK_ByRef) {
+        const SourceManager &SourceMgr = Context.getSourceManager();
+        SourceLocation AddressofLoc = utils::lexer::findPreviousTokenKind(
+            Capture.getLocation(), SourceMgr, Context.getLangOpts(), tok::amp);
+        return AddressofLoc;
+      } else {
+        return Capture.getLocation();
+      }
+    }
+  }
+  return Lambda->getIntroducerRange().getEnd();
+}
+
+static std::string createReplacementText(const LambdaExpr *Lambda) {
+  std::string Replacement;
+  llvm::raw_string_ostream Stream(Replacement);
+
+  auto AppendName = [&](llvm::StringRef Name) {
+    if (Replacement.size() != 0) {
+      Stream << ", ";
+    }
+    if (Lambda->getCaptureDefault() == LCD_ByRef && Name != "this") {
+      Stream << "&" << Name;
+    } else {
+      Stream << Name;
+    }
+  };
+
+  for (const LambdaCapture &Capture : Lambda->implicit_captures()) {
+    assert(Capture.isImplicit());
+    if (Capture.capturesVariable() && Capture.isImplicit()) {
+      AppendName(Capture.getCapturedVar()->getName());
+    } else if (Capture.capturesThis()) {
+      AppendName("this");
+    }
+  }
+  if (Replacement.size() &&
+      Lambda->explicit_capture_begin() != Lambda->explicit_capture_end()) {
+    // Add back separator if we are adding explicit capture variables.
+    Stream << ", ";
+  }
+  return Replacement;
+}
+
+void AvoidCaptureDefaultWhenCapturingThisCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *Lambda = Result.Nodes.getNodeAs<LambdaExpr>("lambda")) {
+    if (Lambda->getCaptureDefault() != LCD_None) {
+      bool IsThisImplicitlyCaptured = std::any_of(
+          Lambda->implicit_capture_begin(), Lambda->implicit_capture_end(),
+          [](const LambdaCapture &Capture) { return Capture.capturesThis(); });
+      auto Diag = diag(Lambda->getCaptureDefaultLoc(),
+                       "lambdas that %select{|implicitly }0capture 'this' "
+                       "should not specify a capture default")
+                  << IsThisImplicitlyCaptured;
+
+      std::string ReplacementText = createReplacementText(Lambda);
+      SourceLocation DefaultCaptureEnd =
+          findDefaultCaptureEnd(Lambda, *Result.Context);
+      Diag << FixItHint::CreateReplacement(
+          CharSourceRange::getCharRange(Lambda->getCaptureDefaultLoc(),
+                                        DefaultCaptureEnd),
+          ReplacementText);
+    }
+  }
+}
+
+} // namespace clang::tidy::cppcoreguidelines
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h
@@ -0,0 +1,41 @@
+//===--- AvoidCaptureDefaultWhenCapturingThisCheck.h - clang-tidy*- C++ -*-===//
+//
+// 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_CPPCOREGUIDELINES_AVOIDCAPTUREDEFAULTWHENCAPTURINGTHISCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTUREDEFAULTWHENCAPTURINGTHISCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::cppcoreguidelines {
+
+/// Warns when lambda specify a capture default and capture ``this``. The check
+/// also offers fix-its.
+///
+/// Capture defaults in lambas defined within member functions can be
+/// misleading about whether capturing data member is by value or reference.
+/// For example, [=] will capture local variables by value but member variables
+/// by reference. CppCoreGuideline F.54 suggests to always be explicit
+/// and never specify a capture default when also capturing this.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.html
+class AvoidCaptureDefaultWhenCapturingThisCheck : public ClangTidyCheck {
+public:
+  AvoidCaptureDefaultWhenCapturingThisCheck(StringRef Name,
+                                            ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus11;
+  }
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTUREDEFAULTWHENCAPTURINGTHISCHECK_H
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
   )
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
+  AvoidCaptureDefaultWhenCapturingThisCheck.cpp
   AvoidConstOrRefDataMembersCheck.cpp
   AvoidDoWhileCheck.cpp
   AvoidGotoCheck.cpp

diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -14,6 +14,7 @@
 #include "../modernize/AvoidCArraysCheck.h"
 #include "../modernize/UseOverrideCheck.h"
 #include "../readability/MagicNumbersCheck.h"
+#include "AvoidCaptureDefaultWhenCapturingThisCheck.h"
 #include "AvoidConstOrRefDataMembersCheck.h"
 #include "AvoidDoWhileCheck.h"
 #include "AvoidGotoCheck.h"
@@ -47,6 +48,8 @@ namespace cppcoreguidelines {
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AvoidCaptureDefaultWhenCapturingThisCheck>(
+        "cppcoreguidelines-avoid-capture-default-when-capturing-this");
     CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
         "cppcoreguidelines-avoid-c-arrays");
     CheckFactories.registerCheck<AvoidConstOrRefDataMembersCheck>(

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -97,6 +97,11 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`cppcoreguidelines-avoid-capture-default-when-capturing-this
+  <clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this>` check.
+
+  Warns when lambda specify a capture default and capture ``this``.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

diff --git a/...ang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst b/...ang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-capture-default-when-capturing-this
+
+cppcoreguidelines-avoid-capture-default-when-capturing-this
+===========================================================
+
+Warns when lambda specify a capture default and capture ``this``.
+
+Capture-defaults in member functions can be misleading about
+whether data members are captured by value or reference. For example,
+specifying the capture default ``[=]`` will still capture data members
+by reference.
+
+Examples:
+
+.. code-block:: c++
+
+      struct AClass {
+        int member;
+        void misleadingLogic() {
+          int local = 0;
+          member = 0;
+          auto f = [=]() mutable {
+            local += 1;
+            member += 1;
+          };
+          f();
+          // Here, local is 0 but member is 1
+        }
+
+        void clearLogic() {
+          int local = 0;
+          member = 0;
+          auto f = [this, local]() mutable {
+            local += 1;
+            member += 1;
+          };
+          f();
+          // Here, local is 0 but member is 1
+        }
+      };
+
+This check implements
+`CppCoreGuideline F.54 <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f54-if-you-capture-this-capture-all-variables-explicitly-no-default-capture>`_.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -178,6 +178,7 @@ Clang-Tidy Checks
    `clang-analyzer-valist.Unterminated <clang-analyzer/valist.Unterminated.html>`_,
    `concurrency-mt-unsafe <concurrency/mt-unsafe.html>`_,
    `concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous.html>`_,
+   `cppcoreguidelines-avoid-capture-default-when-capturing-this <cppcoreguidelines/avoid-capture-default-when-capturing-this.html>`_,
    `cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members.html>`_,
    `cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto.html>`_,

diff --git a/.../test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp b/.../test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp
@@ -0,0 +1,92 @@
+// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-default-when-capturing-this %t
+
+struct Obj {
+  void lambdas_that_warn_default_capture_copy() {
+    int local{};
+    int local2{};
+
+    auto explicit_this_capture = [=, this]() { };
+    // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture = [this]() { };
+
+    auto explicit_this_capture_locals1 = [=, this]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_locals1 = [local, this]() { return (local+x) > 10; };
+
+    auto explicit_this_capture_locals2 = [=, this]() { return (local+local2) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_locals2 = [local, local2, this]() { return (local+local2) > 10; };
+
+    auto explicit_this_capture_local_ref = [=, this, &local]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_local_ref = [this, &local]() { return (local+x) > 10; };
+
+    auto explicit_this_capture_local_ref2 = [=, &local, this]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_local_ref2 = [&local, this]() { return (local+x) > 10; };
+
+    auto explicit_this_capture_local_ref3 = [=, &local, this, &local2]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_local_ref3 = [&local, this, &local2]() { return (local+x) > 10; };
+
+    auto explicit_this_capture_local_ref4 = [=, &local, &local2, this]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_local_ref4 = [&local, &local2, this]() { return (local+x) > 10; };
+
+    auto explicit_this_capture_local_ref_extra_whitespace = [=, &  local, &local2, this]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:62: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_local_ref_extra_whitespace = [&  local, &local2, this]() { return (local+x) > 10; };
+
+    auto explicit_this_capture_local_ref_with_comment = [=, & /* byref */ local, &local2, this]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto explicit_this_capture_local_ref_with_comment = [& /* byref */ local, &local2, this]() { return (local+x) > 10; };
+
+    auto implicit_this_capture = [=]() { return x > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto implicit_this_capture = [this]() { return x > 10; };
+
+    auto implicit_this_capture_local = [=]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto implicit_this_capture_local = [local, this]() { return (local+x) > 10; };
+  }
+
+  void lambdas_that_warn_default_capture_ref() {
+    int local{};
+    int local2{};
+
+    auto ref_explicit_this_capture = [&, this]() { };
+    // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto ref_explicit_this_capture = [this]() { };
+
+    auto ref_explicit_this_capture_local = [&, this]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto ref_explicit_this_capture_local = [&local, this]() { return (local+x) > 10; };
+
+    auto ref_implicit_this_capture = [&]() { return x > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto ref_implicit_this_capture = [this]() { return x > 10; };
+
+    auto ref_implicit_this_capture_local = [&]() { return (local+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto ref_implicit_this_capture_local = [&local, this]() { return (local+x) > 10; };
+
+    auto ref_implicit_this_capture_locals = [&]() { return (local+local2+x) > 10; };
+    // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES: auto ref_implicit_this_capture_locals = [&local, &local2, this]() { return (local+local2+x) > 10; };
+  }
+
+  void lambdas_that_dont_warn() {
+    int local{};
+    int local2{};
+    auto explicit_this_capture = [this]() { };
+    auto explicit_this_capture_local = [this, local]() { return local > 10; };
+    auto explicit_this_capture_local_ref = [this, &local]() { return local > 10; };
+
+    auto no_captures = []() {};
+    auto capture_local_only = [local]() {};
+    auto ref_capture_local_only = [&local]() {};
+    auto capture_many = [local, &local2]() {};
+  }
+
+  int x;
+};