[clang-tidy] Add abseil-wrap-unique check

[Dosi-Dough]

Summary

Based on abseil tip of the week 126: https://abseil.io/tips/126

Created a check which looks for instances of a factory function that uses a non-public constructor and return a std::unique_ptr<T>.

[lebedev.ri]

.DS_Store files clearly should not be there.

[Eugene.Zelenko]

clang-tidy/abseil/AbseilTidyModule.cpp

70

Unnecessary empty line.

clang-tidy/abseil/WrapUniqueCheck.cpp

11

Please run Clang-format.

76

Unnecessary empty line.

docs/ReleaseNotes.rst

70

Please use alphabetical order in new checks list.

74

Please use `` to highlight language constructs. Same in documentation.

docs/clang-tidy/checks/abseil-wrap-unique.rst

7

Please synchronize first statement with Release Notes.

25

Please run Clang-format over code snippets.

test/clang-tidy/abseil-wrap-unique.cpp

4

Unnecessary empty line.

32

Unnecessary empty line.

89

Unnecessary empty line.

91

Unnecessary empty lines.

[Dosi-Dough]

fixed white space/ formatting issues.

removed .DS_Store.

[Eugene.Zelenko]

clang-tidy/abseil/WrapUniqueCheck.cpp

80

Please elide braces.

[Dosi-Dough]

based on feedback:

fixed whitespacing / formatting.

Removed .DS_Store

[MyDeveloperDay]

clang-tidy/abseil/WrapUniqueCheck.cpp

28

couldn't this be

return (!ArgRef.empty()) ? ArgRef.str() + ")" : "()";

You know I think there is a clang-tidy check for using empty() which was recently extended to handle length() too... I wonder if that would have fired

63

can we declare this when we create it see below?

73

std::string newText = ......

85

clang-tidy file in llvm/tools/clang says

key: readability-identifier-naming.VariableCase
value: CamelCase

this means that local variables like this should be DiagText and NewText (I think!)

86

remove empty declarations until they are created

94

std::string Left = (cons......

95

std::string newText = ....

docs/ReleaseNotes.rst

89

remove double bank line

[MyDeveloperDay]

clang-tidy/abseil/WrapUniqueCheck.cpp

55

I'm not sure how expensive getNodeAs is... or if the convention is to do them all at the top of the function

but couldn't some of these getNodeAs go inside the scope where they are used

if (facExpr){

const auto *callExpr = ....

60

same with the 2 above they are only used in the if (cons) scope

[MyDeveloperDay]

clang-tidy/abseil/WrapUniqueCheck.h

28

Nit: could this method could be static in the cpp file and not in the header?

[MyDeveloperDay]

This is the clang-tidy revision that mgiht have caught the str().length() > 0 case D56644: [clang-tidy] readability-container-size-empty handle std::string length() adding a a cross reference for that work.

[Dosi-Dough]

added style and performance refactoring based on code reviews

[MyDeveloperDay]

clang-tidy/abseil/WrapUniqueCheck.cpp

8

License as above

78

we don't put braces on small lines, just

if (Cons->isListInitialization())
  return;

clang-tidy/abseil/WrapUniqueCheck.h

7

this is the wrong license wording now (its changed recently..) check out the other files in trunk

but its something like this....

//
// 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
//

docs/clang-tidy/checks/abseil-wrap-unique.rst

11

there is normally a newline after a code-block

[Dosi-Dough]

fixed bracketed return and added updated license

[Eugene.Zelenko]

clang-tidy/abseil/WrapUniqueCheck.h

26

Please remove obsolete commend and private keyword.

docs/clang-tidy/checks/abseil-wrap-unique.rst

5

Please separate with empty line.

16

Please use = default;

[Dosi-Dough]

added minor style changes and removed unnecessary inline comments.

[Dosi-Dough]

fixed docs and white spacing

[Eugene.Zelenko]

clang-tidy/abseil/WrapUniqueCheck.cpp

86

I don't think that you need round brackets around ConsDecl. Same below.

docs/clang-tidy/checks/abseil-wrap-unique.rst

6

Please separate with empty line.

[Dosi-Dough]

removed parens from turnary, added whitespace.

[Eugene.Zelenko]

docs/clang-tidy/checks/abseil-wrap-unique.rst

6

Sorry, if I was not clear. I meant empty line between ================== and statement.

[Dosi-Dough]

added whitespace to check doc

[Dosi-Dough]

added white space to check doc.

[Dosi-Dough]

Hi I was wondering if there were any other changes anyone recommends should be made to this revision?

[Dosi-Dough]

@EricWF @JonasToth I was wondering what other changes I should make to submit my check upstream?

[Dosi-Dough]

gentle ping

[Dosi-Dough]

Gentle ping

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
+#include "WrapUniqueCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<StrCatAppendCheck>(
         "abseil-str-cat-append");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
     CheckFactories.registerCheck<TimeSubtractionCheck>(
         "abseil-time-subtraction");
     CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
         "abseil-upgrade-duration-conversions");
-  }
+    CheckFactories.registerCheck<WrapUniqueCheck>(
+        "abseil-wrap-unique");
 };
 
 // Register the AbseilModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AbseilModule> X("abseil-module",
                                                     "Add Abseil checks.");
 
 } // namespace abseil
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the AbseilModule.
 volatile int AbseilModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/abseil/CMakeLists.txt

add_clang_library(clangTidyAbseilModule
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
   TimeSubtractionCheck.cpp
   UpgradeDurationConversionsCheck.cpp
+  WrapUniqueCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/abseil/WrapUniqueCheck.h

+//===--- UseAutoForRangeCheck.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_WRAPUNIQUECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_WRAPUNIQUECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Check for instances of factory functions, which use a non-public constructor,
+/// that returns a std::unique_ptr<T>. Then recommends using 
+/// absl::wrap_unique(new T(...))
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-wrap-unique.html
+class WrapUniqueCheck : public ClangTidyCheck {
+private:
+ // std::string getArgs(const SourceManager *SM, const CallExpr *MemExpr);
+
+public:
+  WrapUniqueCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_WRAPUNIQUECHECK_H

clang-tidy/abseil/WrapUniqueCheck.cpp

+//===--- WrapUniqueCheck.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 "WrapUniqueCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <string>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+static std::string getArgs(const SourceManager *SM, const CallExpr *MemExpr) {
+  llvm::StringRef ArgRef = Lexer::getSourceText(
+      CharSourceRange::getCharRange(MemExpr->getSourceRange()), *SM,
+      LangOptions());
+
+  return (!ArgRef.empty()) ? ArgRef.str() + ")" : "()";
+}
+
+void WrapUniqueCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxMemberCallExpr(
+          callee(cxxMethodDecl(
+              hasName("reset"),
+              ofClass(cxxRecordDecl(hasName("std::unique_ptr"), decl())))),
+          has(memberExpr(has(declRefExpr()))),
+          has(callExpr(has(implicitCastExpr(has(declRefExpr()))))),
+          hasArgument(0, callExpr().bind("callExpr")))
+          .bind("facCons"),
+      this);
+
+  Finder->addMatcher(
+      cxxConstructExpr(anyOf(hasParent(decl().bind("cons_decl")), anything()),
+                       hasType(cxxRecordDecl(hasName("std::unique_ptr"))),
+                       has(callExpr().bind("FC_call")))
+          .bind("upfc"),
+      this);
+}
+
+void WrapUniqueCheck::check(const MatchFinder::MatchResult &Result) {
+  // gets the instance of factory constructor
+  const SourceManager *SM = Result.SourceManager;
+  const auto *FacExpr = Result.Nodes.getNodeAs<CXXMemberCallExpr>("facCons");
+  const auto *Cons = Result.Nodes.getNodeAs<CXXConstructExpr>("upfc");
+  if (FacExpr) {
+    const auto *CExpr = Result.Nodes.getNodeAs<CallExpr>("callExpr");
+    std::string DiagText = "Perfer absl::WrapUnique for resetting unique_ptr";
+
+    const Expr *ObjectArg = FacExpr->getImplicitObjectArgument();
+    SourceLocation Target = ObjectArg->getExprLoc();
+    llvm::StringRef ObjName =
+        Lexer::getSourceText(CharSourceRange::getCharRange(
+                                 ObjectArg->getBeginLoc(),
+                                 FacExpr->getExprLoc().getLocWithOffset(-1)),
+                             *SM, LangOptions());
+
+    std::string NewText =
+        ObjName.str() + " = absl::WrapUnique(" + getArgs(SM, CExpr) + ")";
+
+    diag(Target, DiagText) << FixItHint::CreateReplacement(
+        CharSourceRange::getTokenRange(Target, FacExpr->getEndLoc()), NewText);
+  }
+
+  if (Cons) {
+    if (Cons->isListInitialization())
+      return;
+
+    const auto *FcCall = Result.Nodes.getNodeAs<CallExpr>("FC_call");
+    const auto *ConsDecl = Result.Nodes.getNodeAs<Decl>("cons_decl");
+    std::string DiagText = "Perfer absl::WrapUnique to constructing unique_ptr";
+
+    llvm::StringRef NameRef = Lexer::getSourceText(
+        CharSourceRange::getCharRange(Cons->getBeginLoc(),
+                                      Cons->getParenOrBraceRange().getBegin()),
+        *SM, LangOptions());
+
+    std::string Left = (ConsDecl) ? "auto " + NameRef.str() + " = " : "";
+    std::string NewText =
+        Left + "absl::WrapUnique(" + getArgs(SM, FcCall) + ")";
+    SourceLocation Target =
+        (ConsDecl) ? ConsDecl->getBeginLoc() : Cons->getExprLoc();
+
+    diag(Target, DiagText) << FixItHint::CreateReplacement(
+        CharSourceRange::getTokenRange(Target, Cons->getEndLoc()), NewText);
+  }
+}
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New :doc:`abseil-duration-addition
   <clang-tidy/checks/abseil-duration-addition>` check.
 
   Checks for cases where addition should be performed in the ``absl::Time``
   domain.
 
 - New :doc:`abseil-duration-conversion-cast
   <clang-tidy/checks/abseil-duration-conversion-cast>` check.
 
   Checks for casts of ``absl::Duration`` conversion functions, and recommends
   the right conversion function instead.
 
 - New :doc:`abseil-duration-unnecessary-conversion
   <clang-tidy/checks/abseil-duration-unnecessary-conversion>` check.
 
   Finds and fixes cases where ``absl::Duration`` values are being converted to
   numeric types and back again.
 
 - New :doc:`abseil-time-subtraction
   <clang-tidy/checks/abseil-time-subtraction>` check.
 
   Finds and fixes ``absl::Time`` subtraction expressions to do subtraction
   in the Time domain instead of the numeric domain.
 
+- New :doc:`abseil-wrap-unique
+  <clang-tidy/checks/abseil-wrap-unique>` check.
+
+  Looks for instances of factory functions which uses a non-public constructor
+  that returns a ``std::unqiue_ptr<T>`` then recommends using 
+  ``absl::wrap_unique(new T(...))``.
+
 - New :doc:`google-readability-avoid-underscore-in-googletest-name
   <clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
   check.
 
   Checks whether there are underscores in googletest test and test case names in
   test macros, which is prohibited by the Googletest FAQ.
 
 - The :doc:`bugprone-argument-comment

docs/clang-tidy/checks/abseil-wrap-unique.rst

+.. title:: clang-tidy - abseil-wrap-unique
+
+abseil-wrap-unique
+==================
+Looks for instances of factory functions which uses a non-public constructor
+that returns a ``std::unqiue_ptr<T>`` then recommends using 
+``absl::wrap_unique(new T(...))``.
+
+.. code-block:: c++
+ 
+  class A {
+  public:
+    static A* NewA() { return new A(); }
+
+  private:
+    A() {}
+  };
+
+  std::unique_ptr<A> a;
+
+  // Original - reset called with a static function returning a std::unqiue_ptr
+  a.reset(A::NewA());
+
+  // Suggested - reset ptr with absl::WrapUnique
+  a = absl::WrapUnique(A::NewA());
+
+  // Original - std::unique_ptr initialized with static function
+  std::unique_ptr<A> b(A::NewA());
+
+  // Suggested - initialize with absl::WrapUnique instead
+  auto b = absl::WrapUnique(A::NewA())
+

docs/clang-tidy/checks/list.rst

.. toctree::
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
    abseil-str-cat-append
    abseil-string-find-startswith
    abseil-time-subtraction
    abseil-upgrade-duration-conversions
+   abseil-wrap-unique
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen
    android-cloexec-inotify-init

test/clang-tidy/abseil-wrap-unique.cpp

+// RUN: %check_clang_tidy %s abseil-wrap-unique %t
+
+namespace std {
+
+template <typename T>
+class default_delete {};
+
+template <typename type, typename Deleter = std::default_delete<type>>
+class unique_ptr {
+public:
+  unique_ptr() {}
+  unique_ptr(type *ptr) {}
+  unique_ptr(const unique_ptr<type> &t) = delete;
+  unique_ptr(unique_ptr<type> &&t) {}
+  ~unique_ptr() {}
+  type &operator*() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release() { return ptr; }
+  void reset() {}
+  void reset(type *pt) {}
+  void reset(type pt) {}
+  unique_ptr &operator=(unique_ptr &&) { return *this; }
+  template <typename T>
+  unique_ptr &operator=(unique_ptr<T> &&) { return *this; }
+
+private:
+  type *ptr;
+};
+}  // namespace std
+
+class A {
+ public:
+  static A* NewA() {
+    return new A();
+  }
+
+ private:
+  A() {}
+};
+
+class B {
+ public:
+  static B* NewB(int bIn) {
+    return new B();
+  }
+
+ private:
+  B() {}
+};
+
+struct C {
+  int x;
+  int y;
+};
+/*
+std::unique_ptr<A> returnPointer() {
+  return std::unique_ptr<A>(A::NewA());
+}
+*/
+void positives() {
+  std::unique_ptr<A> a;
+  a.reset(A::NewA());
+  //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: prefer absl::WrapUnique for resetting unique_ptr [abseil-wrap-unique]
+  //CHECK-FIXES: a = absl::WrapUnique(A::NewA())
+  
+  std::unique_ptr<A> b(A::NewA());
+  //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: Perfer absl::WrapUnique to constructing unique_ptr [abseil-wrap-unique]
+  //CHECK-FIXES: auto b = absl::WrapUnique(A::NewA())
+
+  int cIn;
+  std::unique_ptr<B> c(B::NewB(cIn));
+  //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: Perfer absl::WrapUnique to constructing unique_ptr [abseil-wrap-unique]
+  //CHECK-FIXES: auto c = absl::WrapUnique(B::NewB(cIn))
+
+  int dIn;
+  std::unique_ptr<B> d;
+  d.reset(B::NewB(dIn));
+  //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: prefer absl::WrapUnique for resetting unique_ptr [abseil-wrap-unique]
+  //CHECK-FIXES: d = absl::WrapUnique(B::NewB(dIn))
+  
+  auto e = std::unique_ptr<A>(A::NewA());
+  //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: prefer absl::WrapUnique for resetting unique_ptr [abseil-wrap-unique]
+  //CHECK-FIXES: e = absl::WrapUnique(A::NewA())
+
+  //std::unique_ptr<int> e(new int[2] {1,2});
+}
+