Enhance MakeSmartPtrCheck for work with class hierarchies

[ZaMaZaN4iK]

Summary

In this patch I've tried to fix some issue which was reported by user. We have this code:

#include <memory>

struct Base{
    virtual ~Base() = default;
    virtual void onTest() {};
};

struct Derived : Base {
    void onTest() override {

    }
};

class A {
    std::unique_ptr<Base> base_;
public:
    A() {
        base_ = std::unique_ptr<Base>(new Derived); // Missing warning
    }
};

void foo()
{
    A a;
}

So I've added one more check for this specific case.

NB: by this change clang-tidy can report some breaking changes, e.g.:

// Types will be different
auto origin = std::unique_ptr<Base>(new Derived);
auto modified = std::make_unique<Derived>();

Suggestions are welcomed.

[hokein]

Thanks for the contribution.

I believe your patch is addressing the FIXME in https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/clang-tidy/modernize-make-unique.cpp#L194, could you please run the test ninja check-clang-tools to make sure all check tests passed?

NB: by this change clang-tidy can report some breaking changes, e.g.:

// Types will be different
auto origin = std::unique_ptr<Base>(new Derived);
auto modified = std::make_unique<Derived>();

This new behavior seems too aggressive, it may break compilation, if there are other code statements like origin.reset(new Base()) after auto origin. I think we should be conservative about this case, but for the case like std::unique_ptr<Base> origin = std::unique_ptr<Base>(new Derived);, the new behavior is safe.

[ZaMaZaN4iK]

@hokein Is there any policy about the question "Which changes can clang-tidy check suggest?" . Should clang-tidy check suggest only "safe" fixes and warn only in "safe" cases?

[aaron.ballman]

In D56301#1346425, @ZaMaZaN4iK wrote:

@hokein Is there any policy about the question "Which changes can clang-tidy check suggest?" . Should clang-tidy check suggest only "safe" fixes and warn only in "safe" cases?

In general, clang-tidy can have a higher false positive rater for diagnostics than the static analyzer or the compiler proper, but check authors should strive for a reasonably low false positive rate (where "reasonable" is a bit subjective). However, fix-its should have nearly zero false positives because it's a very bad user experience otherwise ("your code is wrong, but press this button and we'll make it better; oops, that code is wrong too now, sorry!"). If you can lower the FP rate without undue burden for the check, that's generally the preferred approach.

[ZaMaZaN4iK]

Can anyone help me with writing a checker for such code:

auto ptr = std::unique_ptr<Base>(new Derived);

I tried a lot of different combinations of matchers, but no one didn't match this line. Without such checker there is no chance, I think, fix current smart check in right way.

[JonasToth]

In D56301#1347488, @ZaMaZaN4iK wrote:

Can anyone help me with writing a checker for such code:

auto ptr = std::unique_ptr<Base>(new Derived);

I tried a lot of different combinations of matchers, but no one didn't match this line. Without such checker there is no chance, I think, fix current smart check in right way.

Do you use clang-query to prototype your matcher?

*EDIT* Sorry, I didn't fully read the revision before, I will think about something in a moment.

[JonasToth]

Please add test-cases that demonstrate your change in behaviour. Especially your problematic cases that could be a false positive/result in wrong transformation shuold be in the tests with explicit annotation what happens/nothing bad happens.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

128

I think getTypePtr() is redundant here, as getAllocatedType() seems to be a QualType. You can just use -> on a QualType to go to the underying Type

130

Comment with caution: The inheritance could be hidden behind typedefs, I think that would not be resolved with your current code. Please provide some tests to make it easier to reason about what happens.

clang-tidy/modernize/MakeSmartPtrCheck.cpp

 
 namespace {
 
 constexpr char StdMemoryHeader[] = "memory";
 constexpr char ConstructorCall[] = "constructorCall";
 constexpr char ResetCall[] = "resetCall";
 constexpr char NewExpression[] = "newExpression";
 
-std::string GetNewExprName(const CXXNewExpr *NewExpr,
-                           const SourceManager &SM,
+std::string GetNewExprName(const CXXNewExpr *NewExpr, const SourceManager &SM,
                            const LangOptions &Lang) {
   StringRef WrittenName = Lexer::getSourceText(
       CharSourceRange::getTokenRange(
           NewExpr->getAllocatedTypeSourceInfo()->getTypeLoc().getSourceRange()),
       SM, Lang);
   if (NewExpr->isArray()) {
     return (WrittenName + "[]").str();
   }
   return WrittenName.str();
 }
 
 } // namespace
 
 const char MakeSmartPtrCheck::PointerType[] = "pointerType";
 
-MakeSmartPtrCheck::MakeSmartPtrCheck(StringRef Name,
-                                     ClangTidyContext* Context,
+MakeSmartPtrCheck::MakeSmartPtrCheck(StringRef Name, ClangTidyContext *Context,
                                      StringRef MakeSmartPtrFunctionName)
     : ClangTidyCheck(Name, Context),
       IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
           Options.getLocalOrGlobal("IncludeStyle", "llvm"))),
       MakeSmartPtrFunctionHeader(
           Options.get("MakeSmartPtrFunctionHeader", StdMemoryHeader)),
       MakeSmartPtrFunctionName(
           Options.get("MakeSmartPtrFunction", MakeSmartPtrFunctionName)),
Inserter = llvm::make_unique<utils::IncludeInserter>(
         Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle);
     Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
   }
 }
 
 void MakeSmartPtrCheck::registerMatchers(ast_matchers::MatchFinder *Finder) {
   if (!isLanguageVersionSupported(getLangOpts()))
     return;
-
   // Calling make_smart_ptr from within a member function of a type with a
   // private or protected constructor would be ill-formed.
   auto CanCallCtor = unless(has(ignoringImpCasts(
       cxxConstructExpr(hasDeclaration(decl(unless(isPublic())))))));
 
   Finder->addMatcher(
       cxxBindTemporaryExpr(has(ignoringParenImpCasts(
           cxxConstructExpr(
               hasType(getSmartPointerTypeMatcher()), argumentCountIs(1),
-              hasArgument(0,
-                          cxxNewExpr(hasType(pointsTo(qualType(hasCanonicalType(
-                                         equalsBoundNode(PointerType))))),
-                                     CanCallCtor)
-                              .bind(NewExpression)),
+              hasArgument(
+                  0,
+                  cxxNewExpr(anyOf(hasType(pointsTo(qualType(hasCanonicalType(
+                                   hasType(pointsTo(cxxRecordDecl(anyOf(
+                                       isClass(), isStruct()))))),
               unless(isInTemplateInstantiation()))
               .bind(ConstructorCall)))),
       this);
 
   Finder->addMatcher(
       cxxMemberCallExpr(
           thisPointerType(getSmartPointerTypeMatcher()),
           callee(cxxMethodDecl(hasName("reset"))),
           hasArgument(0, cxxNewExpr(CanCallCtor).bind(NewExpression)),
           unless(isInTemplateInstantiation()))
           .bind(ResetCall),
       this);
 }
 
 void MakeSmartPtrCheck::check(const MatchFinder::MatchResult &Result) {
   // 'smart_ptr' refers to 'std::shared_ptr' or 'std::unique_ptr' or other
   // pointer, 'make_smart_ptr' refers to 'std::make_shared' or
   // 'std::make_unique' or other function that creates smart_ptr.
-
   SourceManager &SM = *Result.SourceManager;
   const auto *Construct =
       Result.Nodes.getNodeAs<CXXConstructExpr>(ConstructorCall);
   const auto *Reset = Result.Nodes.getNodeAs<CXXMemberCallExpr>(ResetCall);
   const auto *Type = Result.Nodes.getNodeAs<QualType>(PointerType);
   const auto *New = Result.Nodes.getNodeAs<CXXNewExpr>(NewExpression);
 
   if (New->getNumPlacementArgs() != 0)
     return;
   // Skip when this is a new-expression with `auto`, e.g. new auto(1)
   if (New->getType()->getPointeeType()->getContainedAutoType())
     return;
 
+  // Skip classes from different hierarchies
+  const auto *NewAllocatedRecord =
+      New->getAllocatedType().getTypePtr()->getAsCXXRecordDecl();
+  const auto *OriginalPointerRecord = Type->getTypePtr()->getAsCXXRecordDecl();
+  if (NewAllocatedRecord != nullptr && OriginalPointerRecord != nullptr &&
+      !NewAllocatedRecord->isDerivedFrom(OriginalPointerRecord))
+    return;
   // Be conservative for cases where we construct an array without any
   // initalization.
   // For example,
   //    P.reset(new int[5]) // check fix: P = make_unique<int []>(5)
   //
   // The fix of the check has side effect, it introduces default initialization
   // which maybe unexpected and cause performance regression.
   if (New->isArray() && !New->hasInitializer())
     return;
   if (Construct)
     checkConstruct(SM, Result.Context, Construct, Type, New);
   else if (Reset)
     checkReset(SM, Result.Context, Reset, New);
 }
 
 void MakeSmartPtrCheck::checkConstruct(SourceManager &SM, ASTContext *Ctx,
                                        const CXXConstructExpr *Construct,
                                        const QualType *Type,
                                        const CXXNewExpr *New) {
   SourceLocation ConstructCallStart = Construct->getExprLoc();
   bool InMacro = ConstructCallStart.isMacroID();
-
   if (InMacro && IgnoreMacros) {
     return;
   }
 
   bool Invalid = false;
   StringRef ExprStr = Lexer::getSourceText(
       CharSourceRange::getCharRange(
           ConstructCallStart, Construct->getParenOrBraceRange().getBegin()),
void MakeSmartPtrCheck::checkConstruct(SourceManager &SM, ASTContext *Ctx,
   // Find the location of the template's left angle.
   size_t LAngle = ExprStr.find("<");
   SourceLocation ConstructCallEnd;
   if (LAngle == StringRef::npos) {
     // If the template argument is missing (because it is part of the alias)
     // we have to add it back.
     ConstructCallEnd = ConstructCallStart.getLocWithOffset(ExprStr.size());
     Diag << FixItHint::CreateInsertion(
-        ConstructCallEnd,
-        "<" + GetNewExprName(New, SM, getLangOpts()) + ">");
+        ConstructCallEnd, "<" + GetNewExprName(New, SM, getLangOpts()) + ">");
   } else {
     ConstructCallEnd = ConstructCallStart.getLocWithOffset(LAngle);
   }
 
   Diag << FixItHint::CreateReplacement(
       CharSourceRange::getCharRange(ConstructCallStart, ConstructCallEnd),
       MakeSmartPtrFunctionName);
 
bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder &Diag,
   SourceLocation NewStart = NewRange.getBegin();
   SourceLocation NewEnd = NewRange.getEnd();
 
   // Skip when the source location of the new expression is invalid.
   if (NewStart.isInvalid() || NewEnd.isInvalid())
     return false;
 
   std::string ArraySizeExpr;
-  if (const auto* ArraySize = New->getArraySize()) {
+  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
if (const auto *CE = New->getConstructExpr()) {
       if (HasListIntializedArgument(CE))
         return false;
     }
     if (ArraySizeExpr.empty()) {
       SourceRange InitRange = New->getDirectInitRange();
       Diag << FixItHint::CreateRemoval(
           SourceRange(NewStart, InitRange.getBegin()));
       Diag << FixItHint::CreateRemoval(SourceRange(InitRange.getEnd(), NewEnd));
-    }
-    else {
+    } 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;
   }