[clang-tidy] abseil-auto-make-unique

[hugoeg]

Summary

warns to use 'auto' to avoid repeating the type name and fixes the issue

Replace:

std::unique_ptr<Foo> x = make_unique<Foo>(...);

with:

auto x = make_unique<Foo>(...);

[lebedev.ri]

1. Please always upload all patches with full context.
2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

[hugoeg]

what do you mean by "with full context"?

[lebedev.ri]

In D50852#1202781, @hugoeg wrote:

what do you mean by "with full context"?

-U99999 when generating the diff.

[hugoeg]

In D50852#1202774, @lebedev.ri wrote:

1. Please always upload all patches with full context.
2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

since this check checks for absl::make_unique primarily
if we move it to the general check, it'd be weird to check for absl::make_unique
right now our main goal is to release checks tailored specifically to abseil users, so if we can we would like to release this check separately in the abseil directory

[lebedev.ri]

In D50852#1203009, @hugoeg wrote:

In D50852#1202774, @lebedev.ri wrote:

1. Please always upload all patches with full context.
2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

since this check checks for absl::make_unique primarily
if we move it to the general check, it'd be weird to check for absl::make_unique

Why do you think it would be weird?
That list should/would be a user-configurable config option anyway.

right now our main goal is to release checks tailored specifically to abseil users, so if we can we would like to release this check separately in the abseil directory

Just checking, was there some memo i missed that abseil-related checks are exempt from all the usual guidelines?
Because this check is really not abseil-library-specific.

[JonasToth]

In D50852#1202774, @lebedev.ri wrote:

1. Please always upload all patches with full context.
2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

I checked fast and modernize-use-auto does not catch this case.

Just checking, was there some memo i missed that abseil-related checks are exempt from all the usual guidelines?

Because this check is really not abseil-library-specific.

I agree that this check is general for make_... templates. It should be either an addition to modernize-use-auto or readability- or so. What we do for such cases, where the check is generally useful and specific to a guideline at the same time is aliasing.

You can take a look at hicpp- module where most checks are actually aliases.

[JonasToth]

clang-tidy/abseil/AutoMakeUniqueCheck.cpp

21

Please clang-format, return on next line.

23

clang::ast_matchers is used already at line 14. No need to add this line.

27

This sentence misses something. inbetween what?

34

The same rules apply for make_shared.

make_pair would be interesting as well, are there are standard make_ templates?

51

Formatting and Naming conventions.

I think the following is more readable:

if (const auto* Unique = dyn_cast<ClassTemplateSpecializationDecl>(var...)) {
  // stuff with var
}

return nullptr;

57

Naming conventions.

64

You don't need the ast_matchers namespace here.

Please follow the naming convention inside the function.

69

What exactly is the issue here in GCC? Please make this comment more explanatory and maybe add a FIXME to work around the issue (depending on the nature of the issue)

docs/ReleaseNotes.rst

63

Please add release notes.

docs/clang-tidy/checks/abseil-auto-make-unique.rst

11

Please add documentation that the 'Abstract Factory' use case is resolved correctly.

test/clang-tidy/abseil-auto-make-unique.cpp

34

How does the code look with multiple variable declarations?

Please add a test for

std::unique_ptr<int> x = absl::make_unique<int>(), y = absl::make_unique<int>(), z;

73

lets consider a 3 level class hierarchy.

struct A { virtual void Something(); };
struct B : A { void Something() override; };
struct C : B { void Something() override; };

std::unique_ptr<B> b_ptr = make_unique<B>(), c_ptr = make_unique<C>();
std::unique_ptr<B> c_ptr2 = make_unique<C>(), b_ptr2 = make_unique<B>();

What is the behaviour? I expect that these places break when transformed. To avoid you can check the VarDecl isSingleDecl() (or similar, i forgot the exact name) and only emit a fixit if it is.
Doing type transformations for the multi-definitions is tricky.

[JonasToth]

test/clang-tidy/abseil-auto-make-unique.cpp

73

isSingleDecl() is a member of DeclStmt.

[zinovy.nis]

In D50852#1202774, @lebedev.ri wrote:

1. Please always upload all patches with full context.
2. There already is modernize-use-auto. Does it handle this case? Then this should be just an alias to that check. Else, i think it would be best to extend that one, and still alias.

First of all, thanks, Hugo, for your check!

My +1 for extending modernize-use-auto as currently it doesn't support cases your check supports.
Extending because replacing

std::unique_ptr< int >y = std::make_unique<int>();

with

auto y = std::make_unique<int>();

is logically the same as replacing

long long int *y = new long long int (42);

with

auto* y = new long long int (42);

what modernize-use-auto does.

So your check should extend the existing one and not create one more check IMHO.

[zinovy.nis]

clang-tidy/abseil/AutoMakeUniqueCheck.cpp

21

auto first appeared in C++11, so you may use getLangOpts().CPlusPlus11
And std::make_unique is a part of C++14, so even `getLangOpts().CPlusPlus14.

66

You may move this definition after the next if+return.

docs/clang-tidy/checks/abseil-auto-make-unique.rst

10

MakeUnique<Foo>? Maybe std/abseil::make_unique?

[lebedev.ri]

clang-tidy/abseil/AutoMakeUniqueCheck.cpp

21

I agree on the auto part, but it really shouldn't care any further than that.
You can write custom yournamespace::MakeUnique<> in C++11.
(As i have already wrote, the list if MakeUnique functions-to-be-searched should be a config option.)

[lebedev.ri]

Marking both of these as "changes requested" to highlight the similarity of issues in them.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AutoMakeUniqueCheck.h"
 #include "StringFindStartswithCheck.h"
 
 namespace clang {

 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AutoMakeUniqueCheck>(
+        "abseil-auto-make-unique");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
   }

clang-tidy/abseil/AutoMakeUniqueCheck.h

+//===--- AutoMakeUniqueCheck.h - clang-tidy----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_AUTOMAKEUNIQUECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_AUTOMAKEUNIQUECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Replace:
+///   std::unique_ptr<Foo> x = make_unique<Foo>(...);
+/// with:
+///   auto x = make_unique<Foo>(...);
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-auto-make-unique.html
+class AutoMakeUniqueCheck : public ClangTidyCheck {
+public:
+  AutoMakeUniqueCheck(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_AUTOMAKEUNIQUECHECK_H

clang-tidy/abseil/AutoMakeUniqueCheck.cpp

+//===--- AutoMakeUniqueCheck.cpp - clang-tidy------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "AutoMakeUniqueCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void AutoMakeUniqueCheck::registerMatchers(MatchFinder* finder) {
+  if (!getLangOpts().CPlusPlus) return;
+
+  using clang::ast_matchers::isTemplateInstantiation;
+  auto is_instantiation = decl(anyOf(cxxRecordDecl(isTemplateInstantiation()),
+                                     varDecl(isTemplateInstantiation()),
+                                     functionDecl(isTemplateInstantiation())));
+  // There should be no additional expressions inbetween.
+  // E.g. this statement contains implicitCastExpr which makes it not eligible:
+  // std::unique_ptr<Base> p = make_unique<Derived>();
+  finder->addMatcher(
+      varDecl(has(exprWithCleanups(has(cxxConstructExpr(
+                  has(ignoringParenImpCasts(cxxBindTemporaryExpr(
+                      has(ignoringParenImpCasts(callExpr(callee(functionDecl(
+                          hasAnyName("absl::MakeUnique", "absl::make_unique",
+                                     "gtl::MakeUnique", "std::make_unique"),
+                          decl().bind("make_unique_decl"))))))))))))),
+              hasType(classTemplateSpecializationDecl(
+                  hasName("std::unique_ptr"),
+                  hasTemplateArgument(
+                      1, refersToType(qualType(hasDeclaration(cxxRecordDecl(
+                             hasName("std::default_delete")))))))),
+              unless(hasType(autoType())),
+              unless(anyOf(is_instantiation, hasAncestor(is_instantiation))))
+          .bind("var_decl"),
+      this);
+}
+
+const Type* GetUniquePtrType(const VarDecl* var) {
+  const auto* unique = dyn_cast_or_null<ClassTemplateSpecializationDecl>(
+      var->getType()->getAsCXXRecordDecl());
+  if (!unique) return nullptr;
+  QualType type = unique->getTemplateArgs().get(0).getAsType();
+  return type->getUnqualifiedDesugaredType();
+}
+
+const Type* GetMakeUniqueType(const FunctionDecl* make_unique_decl) {
+  const auto& template_arg =
+      make_unique_decl->getTemplateSpecializationInfo()->TemplateArguments->get(
+          0);
+  return template_arg.getAsType()->getUnqualifiedDesugaredType();
+}
+
+void AutoMakeUniqueCheck::check(
+    const ast_matchers::MatchFinder::MatchResult& result) {
+  const auto* var_decl = result.Nodes.getNodeAs<VarDecl>("var_decl");
+  const auto* make_unique_decl =
+      result.Nodes.getNodeAs<FunctionDecl>("make_unique_decl");
+  if (var_decl->isOutOfLine()) {
+    // "auto Struct::field = make_unique<...>();" doesn't work in GCC.
+    return;
+  }
+  if (GetUniquePtrType(var_decl) != GetMakeUniqueType(make_unique_decl)) {
+    // The variable and the make_unique expression do not share the same type.
+    // Ignore this.
+    return;
+  }
+
+  diag(var_decl->getTypeSpecStartLoc(),
+       "use 'auto' to avoid repeating the type name")
+      << FixItHint::CreateReplacement(
+             clang::CharSourceRange::getCharRange(
+                 var_decl->getTypeSpecStartLoc(), var_decl->getLocation()),
+             "auto ");
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tidy/abseil/CMakeLists.txt

 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AutoMakeUniqueCheck.cpp
   StringFindStartswithCheck.cpp
 
   LINK_LIBS

docs/ReleaseNotes.rst

 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`abseil-auto-make-unique
+  <clang-tidy/checks/abseil-auto-make-unique>` check.
+
+  FIXME: add release notes.
+
 - New :doc:`readability-magic-numbers
   <clang-tidy/checks/readability-magic-numbers>` check.
 

docs/clang-tidy/checks/abseil-auto-make-unique.rst

+.. title:: clang-tidy - abseil-auto-make-unique
+
+abseil-auto-make-unique
+=======================
+
+The check suggests replacing:
+
+.. code-block:: c++
+
+  std::unique_ptr<Foo> x = MakeUnique<Foo>(...);
+
+with
+
+.. code-block:: c++
+
+  auto x = MakeUnique<Foo>(...);
+
+The initializer already spells the type, so it can be safely omitted from the
+declaration while making the code more readable.

docs/clang-tidy/checks/list.rst

 =================
 
 .. toctree::
+   abseil-auto-make-unique
    abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4

test/clang-tidy/abseil-auto-make-unique.cpp

+// RUN: %check_clang_tidy %s abseil-auto-make-unique %t
+
+namespace std {
+template <typename T>
+struct default_delete {};
+
+template <typename T, typename D = default_delete<T>>
+class unique_ptr {
+ public:
+  unique_ptr();
+  ~unique_ptr();
+  explicit unique_ptr(T*);
+  template <typename U, typename E>
+  unique_ptr(unique_ptr<U, E>&&);
+};
+template <typename T, typename... Args>
+unique_ptr<T> make_unique(Args&&...);
+}  // namespace std
+
+namespace absl {
+template <typename T, typename... Args>
+std::unique_ptr<T> MakeUnique(Args&&...);
+template <typename T, typename... Args>
+std::unique_ptr<T> make_unique(Args&&...);
+}  // namespace absl
+using absl::make_unique;
+
+typedef int integer;
+
+struct Base {};
+struct Derived : public Base {};
+
+void Primitive() {
+  std::unique_ptr<int> x = absl::make_unique<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto x = absl::make_unique<int>();
+
+  std::unique_ptr< int >y = absl::make_unique<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto y = absl::make_unique<int>();
+
+  const std::unique_ptr<int> z = absl::make_unique<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: const auto z = absl::make_unique<int>();
+
+  std::unique_ptr<const int> t = absl::make_unique<int>();
+}
+
+void Typedefs() {
+  std::unique_ptr<int> x = absl::make_unique<integer>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto x = absl::make_unique<integer>();
+
+  std::unique_ptr<integer> y = absl::make_unique<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto y = absl::make_unique<int>();
+
+  std::unique_ptr<integer> z = absl::make_unique<integer>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto z = absl::make_unique<integer>();
+}
+
+void Class() {
+  std::unique_ptr<Base> base = make_unique<Base>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto base = make_unique<Base>();
+
+  std::unique_ptr<Base> base2(make_unique<Base>());
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto base2(make_unique<Base>());
+
+  // Different type. No change.
+  std::unique_ptr<Base> z = make_unique<Derived>();
+  std::unique_ptr<Base> z2(make_unique<Derived>());
+}
+
+template <typename A, typename B>
+void f() {
+  std::unique_ptr<A> x = make_unique<B>();
+}
+
+void Negatives() {
+  // Different deleter. No change.
+  struct MyDeleter {};
+  std::unique_ptr<int, MyDeleter> z3 = make_unique<int>();
+  std::unique_ptr<int, MyDeleter> z4(make_unique<int>());
+
+  f<int, int>();
+}
+
+std::unique_ptr<int> global_var = make_unique<int>();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'auto' to avoid repeating the type name
+// CHECK-FIXES: auto global_var = make_unique<int>();
+
+struct Struct {
+  static std::unique_ptr<int> static_field;
+};
+// This code with "auto" replaced doesn't compile in GCC.
+std::unique_ptr<int> Struct::static_field = make_unique<int>();
+
+void FunctionWithStatic() {
+  static std::unique_ptr<int> static_var = make_unique<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto static_var = make_unique<int>();
+}
+
+void OtherNames() {
+  std::unique_ptr<int> x = absl::MakeUnique<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto x = absl::MakeUnique<int>();
+
+  std::unique_ptr<int> y = std::make_unique<int>();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'auto' to avoid repeating the type name
+  // CHECK-FIXES: auto y = std::make_unique<int>();
+}