[clang-tidy] Don't generate incorrect fixes for class constructed from list-initialized arguments

[hokein]

Summary

Currently the smart_ptr check (modernize-make-unique) generates the
fixes that cannot compile for cases like below -- because brace list can
not be deduced in make_unique.

struct Bar { int a, b; };
struct Foo { Foo(Bar); };
auto foo = std::unique_ptr<Foo>(new Foo({1, 2}));

[aaron.ballman]

Currently the smart_ptr check (modernize-make-unique) generates the fixes that cannot compile for cases like below -- because brace list can not be deduced in make_unique.

class Bar { int a, b; };
class Foo { Foo(Bar); };
auto foo = std::unique_ptr<Foo>(new Foo({1, 2}));

This code isn't legal in the first place. ;-)

Aside from some small nits, this LGTM.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

287 ↗

(On Diff #174623)

I assume this is expected to be false? Foo{1}

294 ↗

(On Diff #174623)

No need for this lambda (then again, it was in the original code).

295 ↗

(On Diff #174623)

No need to assert this; dyn_cast<> does it for you.

[hokein]

address comments.

[hokein]

In D54704#1304176, @aaron.ballman wrote:

Currently the smart_ptr check (modernize-make-unique) generates the fixes that cannot compile for cases like below -- because brace list can not be deduced in make_unique.

class Bar { int a, b; };
class Foo { Foo(Bar); };
auto foo = std::unique_ptr<Foo>(new Foo({1, 2}));

This code isn't legal in the first place. ;-)

Thanks for the review. Fixed :)

clang-tidy/modernize/MakeSmartPtrCheck.cpp

287 ↗

(On Diff #174623)

Yes, added this sample to the comment.

clang-tools-extra/trunk/clang-tidy/modernize/MakeSmartPtrCheck.cpp

bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
 
   std::string ArraySizeExpr;
   if (const auto* ArraySize = New->getArraySize()) {
     ArraySizeExpr = Lexer::getSourceText(CharSourceRange::getTokenRange(
                                              ArraySize->getSourceRange()),
                                          SM, getLangOpts())
                         .str();
   }
-
+  // Returns true if the given constructor expression has any braced-init-list
+  // argument, e.g.
+  //   Foo({1, 2}, 1) => true
+  //   Foo(Bar{1, 2}) => true
+  //   Foo(1) => false
+  //   Foo{1} => false
+  auto HasListIntializedArgument = [](const CXXConstructExpr *CE) {
+    for (const auto *Arg : CE->arguments()) {
+      if (isa<CXXStdInitializerListExpr>(Arg) || isa<InitListExpr>(Arg))
+        return true;
+      // Check whether we implicitly construct a class from a
+      // std::initializer_list.
+      if (const auto *ImplicitCE =
+              dyn_cast<CXXConstructExpr>(Arg->IgnoreImplicit())) {
+        if (ImplicitCE->isStdInitListInitialization())
+          return true;
+      }
+      return false;
+    }
+    return false;
+  };
   switch (New->getInitializationStyle()) {
   case CXXNewExpr::NoInit: {
     if (ArraySizeExpr.empty()) {
       Diag << FixItHint::CreateRemoval(SourceRange(NewStart, NewEnd));
     } else {
       // New array expression without written initializer:
       //   smart_ptr<Foo[]>(new Foo[5]);
       Diag << FixItHint::CreateReplacement(SourceRange(NewStart, NewEnd),
                                            ArraySizeExpr);
     }
     break;
   }
   case CXXNewExpr::CallInit: {
     // FIXME: Add fixes for constructors with parameters that can be created
     // with a C++11 braced-init-list (e.g. std::vector, std::map).
     // Unlike ordinal cases, braced list can not be deduced in
     // std::make_smart_ptr, we need to specify the type explicitly in the fixes:
     //   struct S { S(std::initializer_list<int>, int); };
     //   struct S2 { S2(std::vector<int>); };
+    //   struct S3 { S3(S2, int); };
     //   smart_ptr<S>(new S({1, 2, 3}, 1));  // C++98 call-style initialization
     //   smart_ptr<S>(new S({}, 1));
     //   smart_ptr<S2>(new S2({1})); // implicit conversion:
     //                               //   std::initializer_list => std::vector
+    //   smart_ptr<S3>(new S3({1, 2}, 3));
     // The above samples have to be replaced with:
     //   std::make_smart_ptr<S>(std::initializer_list<int>({1, 2, 3}), 1);
     //   std::make_smart_ptr<S>(std::initializer_list<int>({}), 1);
     //   std::make_smart_ptr<S2>(std::vector<int>({1}));
+    //   std::make_smart_ptr<S3>(S2{1, 2}, 3);
     if (const auto *CE = New->getConstructExpr()) {
-      for (const auto *Arg : CE->arguments()) {
-        if (isa<CXXStdInitializerListExpr>(Arg)) {
-          return false;
-        }
-        // Check whether we construct a class from a std::initializer_list.
-        // If so, we won't generate the fixes.
-        auto IsStdInitListInitConstructExpr = [](const Expr* E) {
-          assert(E);
-          if (const auto *ImplicitCE = dyn_cast<CXXConstructExpr>(E)) {
-            if (ImplicitCE->isStdInitListInitialization())
-              return true;
-          }
-          return false;
-        };
-        if (IsStdInitListInitConstructExpr(Arg->IgnoreImplicit()))
-          return false;
-      }
-    }
+      if (HasListIntializedArgument(CE))
     if (ArraySizeExpr.empty()) {
       SourceRange InitRange = New->getDirectInitRange();
       Diag << FixItHint::CreateRemoval(
           SourceRange(NewStart, InitRange.getBegin()));
       Diag << FixItHint::CreateRemoval(SourceRange(InitRange.getEnd(), NewEnd));
     }
     else {
       // New array expression with default/value initialization:
       //   smart_ptr<Foo[]>(new int[5]());
       //   smart_ptr<Foo[]>(new Foo[5]());
       Diag << FixItHint::CreateReplacement(SourceRange(NewStart, NewEnd),
                                            ArraySizeExpr);
     }
     break;
   }
   case CXXNewExpr::ListInit: {
     // Range of the substring that we do not want to remove.
     SourceRange InitRange;
     if (const auto *NewConstruct = New->getConstructExpr()) {
-      if (NewConstruct->isStdInitListInitialization()) {
+      if (NewConstruct->isStdInitListInitialization() ||
+          HasListIntializedArgument(NewConstruct)) {
         // FIXME: Add fixes for direct initialization with the initializer-list
         // constructor. Similar to the above CallInit case, the type has to be
         // specified explicitly in the fixes.
         //   struct S { S(std::initializer_list<int>); };
+        //   struct S2 { S2(S, int); };
         //   smart_ptr<S>(new S{1, 2, 3});  // C++11 direct list-initialization
         //   smart_ptr<S>(new S{});  // use initializer-list consturctor
+        //   smart_ptr<S2>()new S2{ {1,2}, 3 }; // have a list-initialized arg
         // The above cases have to be replaced with:
         //   std::make_smart_ptr<S>(std::initializer_list<int>({1, 2, 3}));
         //   std::make_smart_ptr<S>(std::initializer_list<int>({}));
+        //   std::make_smart_ptr<S2>(S{1, 2}, 3);
         return false;
       } else {
         // Direct initialization with ordinary constructors.
         //   struct S { S(int x); S(); };
         //   smart_ptr<S>(new S{5});
         //   smart_ptr<S>(new S{}); // use default constructor
         // The arguments in the initialization list are going to be forwarded to
         // the constructor, so this has to be replaced with:

clang-tools-extra/trunk/test/clang-tidy/modernize-make-unique.cpp

struct H {
   H(std::vector<int> &, double);
   H(MyVector<int>, int);
 };
 
 struct I {
   I(G);
 };
 
+struct J {
+  J(E e, int);
+};
+
 namespace {
 class Foo {};
 } // namespace
 
 namespace bar {
 class Bar {};
 } // namespace bar
 
void initialization(int T, Base b) {
 
   std::unique_ptr<I> PI1 = std::unique_ptr<I>(new I(G({1, 2, 3})));
   // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
   // CHECK-FIXES: std::unique_ptr<I> PI1 = std::make_unique<I>(G({1, 2, 3}));
   PI1.reset(new I(G({1, 2, 3})));
   // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
   // CHECK-FIXES: PI1 = std::make_unique<I>(G({1, 2, 3}));
 
+  std::unique_ptr<J> PJ1 = std::unique_ptr<J>(new J({1, 2}, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
+  // CHECK-FIXES: std::unique_ptr<J> PJ1 = std::unique_ptr<J>(new J({1, 2}, 1));
+  PJ1.reset(new J({1, 2}, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PJ1.reset(new J({1, 2}, 1));
+
+  std::unique_ptr<J> PJ2 = std::unique_ptr<J>(new J(E{1, 2}, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
+  // CHECK-FIXES: std::unique_ptr<J> PJ2 = std::make_unique<J>(E{1, 2}, 1);
+  PJ2.reset(new J(E{1, 2}, 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES: PJ2 = std::make_unique<J>(E{1, 2}, 1);
+
+  std::unique_ptr<J> PJ3 = std::unique_ptr<J>(new J{ {1, 2}, 1 });
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
+  // CHECK-FIXES: std::unique_ptr<J> PJ3 = std::unique_ptr<J>(new J{ {1, 2}, 1 });
+  PJ3.reset(new J{ {1, 2}, 1 });
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES:  PJ3.reset(new J{ {1, 2}, 1 });
+
+  std::unique_ptr<J> PJ4 = std::unique_ptr<J>(new J{E{1, 2}, 1});
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use std::make_unique instead
+  // CHECK-FIXES: std::unique_ptr<J> PJ4 = std::make_unique<J>(E{1, 2}, 1);
+  PJ4.reset(new J{E{1, 2}, 1});
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+  // CHECK-FIXES:  PJ4 = std::make_unique<J>(E{1, 2}, 1);
+
   std::unique_ptr<Foo> FF = std::unique_ptr<Foo>(new Foo());
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning:
   // CHECK-FIXES: std::unique_ptr<Foo> FF = std::make_unique<Foo>();
   FF.reset(new Foo());
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning:
   // CHECK-FIXES: FF = std::make_unique<Foo>();
 
   std::unique_ptr<bar::Bar> BB = std::unique_ptr<bar::Bar>(new bar::Bar());