[clang-tidy] Add check for initialization of `absl::Cleanup`.

[CJ-Johnson]

Summary

Suggests switching the initialization pattern of absl::Cleanup instances from the factory function to class template argument deduction (CTAD) in C++17 and higher.

[CJ-Johnson]

Remove unwanted changes

[CJ-Johnson]

Add trailing newline

[ymandel]

Nice!

clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp

20

47–50

nit: move this inline into the class declaration. Serves as documentation for the class then as well (in that a reader can look in the header and see the target language).

clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.h

21

clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp

8–21

Since the tests depend on the mock of the Cleanup declaration, have you run this on real code examples to double-check that it works as expected? Sometimes the divergence means real code can have unexpected implicit nodes that subtly impact the results, so it's always good to verify the mocks themselves in some way.

42

nit: this subtle, i'd add a comment pointing out the parens.

43

Here and below: you can truncate to 80 chars, any repeated message. We typically only check the whole message for the first appearance.

[CJ-Johnson]

Thanks for the review! I'll apply the same fixes to the string_view diff as well

[CJ-Johnson]

Address nits and update the absl::Cleanup stub to be accurate with the real interface (with associated changes to the AST matcher)

[CJ-Johnson]

Fix documentation nit

clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp

 //===------- AbseilTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "CleanupCtadCheck.h"
 #include "DurationAdditionCheck.h"
 #include "DurationComparisonCheck.h"
 #include "DurationConversionCastCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
 #include "DurationSubtractionCheck.h"
 #include "DurationUnnecessaryConversionCheck.h"

 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<CleanupCtadCheck>("abseil-cleanup-ctad");
     CheckFactories.registerCheck<DurationAdditionCheck>(
         "abseil-duration-addition");
     CheckFactories.registerCheck<DurationComparisonCheck>(
         "abseil-duration-comparison");
     CheckFactories.registerCheck<DurationConversionCastCheck>(
         "abseil-duration-conversion-cast");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");

clang-tools-extra/clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   support
   FrontendOpenMP
   )
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  CleanupCtadCheck.cpp
   DurationAdditionCheck.cpp
   DurationComparisonCheck.cpp
   DurationConversionCastCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
   DurationSubtractionCheck.cpp

clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.h

+//===--- CleanupCtadCheck.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_ABSEIL_CLEANUPCTADCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_CLEANUPCTADCHECK_H
+
+#include "../utils/TransformerClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Suggests switching the initialization pattern of `absl::Cleanup`
+/// instances from the factory function to class template argument
+/// deduction (CTAD) in C++17 and higher.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-cleanup-ctad.html
+class CleanupCtadCheck : public utils::TransformerClangTidyCheck {
+public:
+  CleanupCtadCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_CLEANUPCTADCHECK_H

clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp

+//===--- CleanupCtadCheck.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 "CleanupCtadCheck.h"
+#include "../utils/TransformerClangTidyCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Transformer/RangeSelector.h"
+#include "clang/Tooling/Transformer/RewriteRule.h"
+#include "clang/Tooling/Transformer/Stencil.h"
+#include "llvm/ADT/StringRef.h"
+
+using namespace clang::ast_matchers;
+using namespace ::clang::transformer;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+RewriteRule CleanupCtadCheckImpl() {
+  auto warning_message = cat("prefer absl::Cleanup's class template argument "
+                             "deduction pattern in C++17 and higher");
+
+  return makeRule(
+      declStmt(has(varDecl(
+          hasType(autoType()), hasTypeLoc(typeLoc().bind("auto_typeLoc")),
+          hasInitializer(ignoringImpCasts(
+              callExpr(callee(functionDecl(hasName("absl::MakeCleanup"))),
+                       argumentCountIs(1),
+                       hasArgument(0, expr().bind("make_cleanup_argument")))
+                  .bind("make_cleanup_call")))))),
+      {changeTo(node("auto_typeLoc"), cat("absl::Cleanup")),
+       changeTo(node("make_cleanup_call"), cat(node("make_cleanup_argument")))},
+      warning_message);
+}
+
+CleanupCtadCheck::CleanupCtadCheck(StringRef Name, ClangTidyContext *Context)
+    : utils::TransformerClangTidyCheck(CleanupCtadCheckImpl(), Name, Context) {}
+
+bool CleanupCtadCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus17;
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
 - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
   Clang-Tidy warnings over multiple lines.
 
 New checks
 ^^^^^^^^^^
 
+- New :doc:`abseil-cleanup-ctad
+  <clang-tidy/checks/abseil-cleanup-ctad>` check.
+
+  Suggests switching the initialization pattern of ``absl::Cleanup``
+  instances from the factory function to class template argument
+  deduction (CTAD) in C++17 and higher.
+
 - New :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` check.
 
   Finds potentially incorrect calls to ``memcmp()`` based on properties of the
   arguments.
 
 - New :doc:`cppcoreguidelines-virtual-class-destructor
   <clang-tidy/checks/cppcoreguidelines-virtual-class-destructor>` check.

clang-tools-extra/docs/clang-tidy/checks/abseil-cleanup-ctad.rst

+.. title:: clang-tidy - abseil-cleanup-ctad
+
+abseil-cleanup-ctad
+===================
+
+Suggests switching the initialization pattern of ``absl::Cleanup``
+instances from the factory function to class template argument
+deduction (CTAD) in C++17 and higher.
+
+.. code-block:: c++
+
+  auto c1 = absl::MakeCleanup([] {});
+
+  const auto c2 = absl::MakeCleanup(std::function<void()>([] {}));
+
+becomes
+
+.. code-block:: c++
+
+  absl::Cleanup c1 = [] {};
+
+  const absl::Cleanup c2 = std::function<void()>([] {});

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

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    :glob:
    :hidden:
 
    *
 
 .. csv-table::
    :header: "Name", "Offers fixes"
 
+   `abseil-cleanup-ctad <abseil-cleanup-ctad.html>`_, "Yes"
    `abseil-duration-addition <abseil-duration-addition.html>`_, "Yes"
    `abseil-duration-comparison <abseil-duration-comparison.html>`_, "Yes"
    `abseil-duration-conversion-cast <abseil-duration-conversion-cast.html>`_, "Yes"
    `abseil-duration-division <abseil-duration-division.html>`_, "Yes"
    `abseil-duration-factory-float <abseil-duration-factory-float.html>`_, "Yes"
    `abseil-duration-factory-scale <abseil-duration-factory-scale.html>`_, "Yes"
    `abseil-duration-subtraction <abseil-duration-subtraction.html>`_, "Yes"
    `abseil-duration-unnecessary-conversion <abseil-duration-unnecessary-conversion.html>`_, "Yes"
.. csv-table::
    `bugprone-unused-raii <bugprone-unused-raii.html>`_, "Yes"
    `bugprone-unused-return-value <bugprone-unused-return-value.html>`_,
    `bugprone-use-after-move <bugprone-use-after-move.html>`_,
    `bugprone-virtual-near-miss <bugprone-virtual-near-miss.html>`_, "Yes"
    `cert-dcl21-cpp <cert-dcl21-cpp.html>`_,
    `cert-dcl50-cpp <cert-dcl50-cpp.html>`_,
    `cert-dcl58-cpp <cert-dcl58-cpp.html>`_,
    `cert-env33-c <cert-env33-c.html>`_,
+   `cert-err33-c <cert-err33-c.html>`_,
    `cert-err34-c <cert-err34-c.html>`_,
    `cert-err52-cpp <cert-err52-cpp.html>`_,
    `cert-err58-cpp <cert-err58-cpp.html>`_,
    `cert-err60-cpp <cert-err60-cpp.html>`_,
-   `cert-exp42-c <cert-exp42-c.html>`_,
    `cert-flp30-c <cert-flp30-c.html>`_,
-   `cert-flp37-c <cert-flp37-c.html>`_,
    `cert-mem57-cpp <cert-mem57-cpp.html>`_,
    `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc51-cpp <cert-msc51-cpp.html>`_,
    `cert-oop57-cpp <cert-oop57-cpp.html>`_,
    `cert-oop58-cpp <cert-oop58-cpp.html>`_,
    `clang-analyzer-core.DynamicTypePropagation <clang-analyzer-core.DynamicTypePropagation.html>`_,
    `clang-analyzer-core.uninitialized.CapturedBlockVariable <clang-analyzer-core.uninitialized.CapturedBlockVariable.html>`_,
    `clang-analyzer-cplusplus.InnerPointer <clang-analyzer-cplusplus.InnerPointer.html>`_,
.. csv-table::
    `performance-unnecessary-value-param <performance-unnecessary-value-param.html>`_, "Yes"
    `portability-restrict-system-includes <portability-restrict-system-includes.html>`_, "Yes"
    `portability-simd-intrinsics <portability-simd-intrinsics.html>`_,
    `readability-avoid-const-params-in-decls <readability-avoid-const-params-in-decls.html>`_,
    `readability-braces-around-statements <readability-braces-around-statements.html>`_, "Yes"
    `readability-const-return-type <readability-const-return-type.html>`_, "Yes"
    `readability-container-size-empty <readability-container-size-empty.html>`_, "Yes"
    `readability-convert-member-functions-to-static <readability-convert-member-functions-to-static.html>`_,
+   `readability-data-pointer <readability-data-pointer.html>`_,
    `readability-delete-null-pointer <readability-delete-null-pointer.html>`_, "Yes"
    `readability-else-after-return <readability-else-after-return.html>`_, "Yes"
    `readability-function-cognitive-complexity <readability-function-cognitive-complexity.html>`_,
    `readability-function-size <readability-function-size.html>`_,
    `readability-identifier-length <readability-identifier-length.html>`_,
    `readability-identifier-naming <readability-identifier-naming.html>`_, "Yes"
    `readability-implicit-bool-conversion <readability-implicit-bool-conversion.html>`_, "Yes"
    `readability-inconsistent-declaration-parameter-name <readability-inconsistent-declaration-parameter-name.html>`_, "Yes"

 .. csv-table:: Aliases..
    :header: "Name", "Redirect", "Offers fixes"
 
    `cert-con36-c <cert-con36-c.html>`_, `bugprone-spuriously-wake-up-functions <bugprone-spuriously-wake-up-functions.html>`_,
    `cert-con54-cpp <cert-con54-cpp.html>`_, `bugprone-spuriously-wake-up-functions <bugprone-spuriously-wake-up-functions.html>`_,
    `cert-dcl03-c <cert-dcl03-c.html>`_, `misc-static-assert <misc-static-assert.html>`_, "Yes"
    `cert-dcl16-c <cert-dcl16-c.html>`_, `readability-uppercase-literal-suffix <readability-uppercase-literal-suffix.html>`_, "Yes"
    `cert-dcl37-c <cert-dcl37-c.html>`_, `bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
-   `cert-err33-c <cert-err33-c.html>`_, `bugprone-unused-return-value <bugprone-unused-return-value.html>`_,
    `cert-dcl51-cpp <cert-dcl51-cpp.html>`_, `bugprone-reserved-identifier <bugprone-reserved-identifier.html>`_, "Yes"
    `cert-dcl54-cpp <cert-dcl54-cpp.html>`_, `misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
    `cert-dcl59-cpp <cert-dcl59-cpp.html>`_, `google-build-namespaces <google-build-namespaces.html>`_,
    `cert-err09-cpp <cert-err09-cpp.html>`_, `misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_,
    `cert-err61-cpp <cert-err61-cpp.html>`_, `misc-throw-by-value-catch-by-reference <misc-throw-by-value-catch-by-reference.html>`_,
+   `cert-exp42-c <cert-exp42-c.html>`_, `bugprone-suspicious-memory-comparison <bugprone-suspicious-memory-comparison.html>`_,
    `cert-fio38-c <cert-fio38-c.html>`_, `misc-non-copyable-objects <misc-non-copyable-objects.html>`_,
+   `cert-flp37-c <cert-flp37-c.html>`_, `bugprone-suspicious-memory-comparison <bugprone-suspicious-memory-comparison.html>`_,
    `cert-msc30-c <cert-msc30-c.html>`_, `cert-msc50-cpp <cert-msc50-cpp.html>`_,
    `cert-msc32-c <cert-msc32-c.html>`_, `cert-msc51-cpp <cert-msc51-cpp.html>`_,
    `cert-oop11-cpp <cert-oop11-cpp.html>`_, `performance-move-constructor-init <performance-move-constructor-init.html>`_,
    `cert-oop54-cpp <cert-oop54-cpp.html>`_, `bugprone-unhandled-self-assignment <bugprone-unhandled-self-assignment.html>`_,
    `cert-pos44-c <cert-pos44-c.html>`_, `bugprone-bad-signal-to-kill-thread <bugprone-bad-signal-to-kill-thread.html>`_,
    `cert-pos47-c <cert-pos47-c.html>`_, `concurrency-thread-canceltype-asynchronous <concurrency-thread-canceltype-asynchronous.html>`_,
    `cert-sig30-c <cert-sig30-c.html>`_, `bugprone-signal-handler <bugprone-signal-handler.html>`_,
    `cert-str34-c <cert-str34-c.html>`_, `bugprone-signed-char-misuse <bugprone-signed-char-misuse.html>`_,

clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp

+// RUN: %check_clang_tidy %s abseil-cleanup-ctad -std=c++17 %t
+
+namespace absl {
+
+struct Tag {};
+
+template <typename, typename T>
+class Cleanup {
+public:
+  Cleanup(T) {}
+  Cleanup(Cleanup &&) {}
+};
+
+template <typename Callback>
+Cleanup(Callback callback) -> Cleanup<Tag, Callback>;
+
+template <typename T>
+absl::Cleanup<Tag, T> MakeCleanup(T t) {
+  return absl::Cleanup<Tag, T>(t);
+}
+
+} // namespace absl
+
+namespace std {
+
+template <typename>
+class function {
+public:
+  template <typename T>
+  function(T) {}
+  function(const function &) {}
+};
+
+} // namespace std
+
+void test() {
+  auto a = absl::MakeCleanup([] {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  absl::Cleanup a = [] {};{{$}}
+
+  auto b = absl::MakeCleanup(([] {}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  absl::Cleanup b = [] {};{{$}}
+
+  const auto c = absl::MakeCleanup([] {});
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  const absl::Cleanup c = [] {};{{$}}
+
+  const auto d = absl::MakeCleanup(([] {}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  const absl::Cleanup d = [] {};{{$}}
+
+  auto e = absl::MakeCleanup(std::function<void()>([] {}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  absl::Cleanup e = std::function<void()>([] {});{{$}}
+
+  auto f = absl::MakeCleanup((std::function<void()>([] {})));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  absl::Cleanup f = std::function<void()>([] {});{{$}}
+
+  const auto g = absl::MakeCleanup(std::function<void()>([] {}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  const absl::Cleanup g = std::function<void()>([] {});{{$}}
+
+  const auto h = absl::MakeCleanup((std::function<void()>([] {})));
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  const absl::Cleanup h = std::function<void()>([] {});{{$}}
+}