Extend the check to detect patterns like 'ptr.get() == nullptr'

Summary:
Extend the check to detect patterns like 'ptr.get() == nullptr'
It detects == and != when any argument is a ptr.get() and the other is a
nullptr.
Only supports standard smart pointer types std::unique_ptr and std::shared_ptr.
Does not support the case 'ptr.get() == other.get()' yet.

Reviewers: djasper

CC: cfe-commits, alexfh

Differential Revision: http://llvm-reviews.chandlerc.com/D3294

llvm-svn: 205854

clang-tools-extra/clang-tidy/misc/RedundantSmartptrGet.cpp

@@ -16,8 +16,19 @@ using namespace clang::ast_matchers;
 namespace clang {
 namespace tidy {
 
-void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) {
+namespace {
+internal::Matcher<Expr> callToGet(internal::Matcher<Decl> OnClass) {
+  return memberCallExpr(
+             on(expr(anyOf(hasType(OnClass),
+                           hasType(qualType(pointsTo(decl(OnClass).bind(
+                               "ptr_to_ptr")))))).bind("smart_pointer")),
+             callee(methodDecl(hasName("get")))).bind("redundant_get");
+}
+
+void registerMatchersForGetArrowStart(MatchFinder *Finder,
+                                      MatchFinder::MatchCallback *Callback) {
   const auto QuacksLikeASmartptr = recordDecl(
+      recordDecl().bind("duck_typing"),
       has(methodDecl(hasName("operator->"),
                      returns(qualType(pointsTo(type().bind("op->Type")))))),
       has(methodDecl(hasName("operator*"),
@@ -25,35 +36,50 @@ void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) {
       has(methodDecl(hasName("get"),
                      returns(qualType(pointsTo(type().bind("getType")))))));
 
-  const auto CallToGet =
-      memberCallExpr(on(expr(hasType(recordDecl(QuacksLikeASmartptr)))
-                            .bind("smart_pointer")),
-                     callee(methodDecl(hasName("get")))).bind("redundant_get");
-
-  const auto ArrowCallToGet =
-      memberCallExpr(
-          on(expr(hasType(qualType(pointsTo(recordDecl(QuacksLikeASmartptr)))))
-                 .bind("smart_pointer")),
-          callee(methodDecl(hasName("get")))).bind("redundant_get");
-
   // Catch 'ptr.get()->Foo()'
-  Finder->addMatcher(
-      memberExpr(isArrow(), hasObjectExpression(ignoringImpCasts(CallToGet))),
-      this);
+  Finder->addMatcher(memberExpr(expr().bind("memberExpr"), isArrow(),
+                                hasObjectExpression(ignoringImpCasts(
+                                    callToGet(QuacksLikeASmartptr)))),
+                     Callback);
 
-  // Catch '*ptr.get()'
+  // Catch '*ptr.get()' or '*ptr->get()'
   Finder->addMatcher(
-      unaryOperator(hasOperatorName("*"), hasUnaryOperand(CallToGet)), this);
+      unaryOperator(hasOperatorName("*"),
+                    hasUnaryOperand(callToGet(QuacksLikeASmartptr))),
+      Callback);
+}
 
-  // Catch '*ptr->get()'
+void registerMatchersForGetEquals(MatchFinder *Finder,
+                                  MatchFinder::MatchCallback *Callback) {
+  // This one is harder to do with duck typing.
+  // The operator==/!= that we are looking for might be member or non-member,
+  // might be on global namespace or found by ADL, might be a template, etc.
+  // For now, lets keep a list of known standard types.
+
+  const auto IsAKnownSmartptr = recordDecl(
+      anyOf(hasName("::std::unique_ptr"), hasName("::std::shared_ptr")));
+
+  // Matches against nullptr.
   Finder->addMatcher(
-      unaryOperator(hasOperatorName("*"), hasUnaryOperand(ArrowCallToGet))
-          .bind("ptr_to_ptr"),
-      this);
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                     hasEitherOperand(ignoringImpCasts(nullPtrLiteralExpr())),
+                     hasEitherOperand(callToGet(IsAKnownSmartptr))),
+      Callback);
+  // TODO: Catch ptr.get() == other_ptr.get()
+}
+
+
+}  // namespace
+
+void RedundantSmartptrGet::registerMatchers(MatchFinder *Finder) {
+  registerMatchersForGetArrowStart(Finder, this);
+  registerMatchersForGetEquals(Finder, this);
 }
 
 namespace {
 bool allReturnTypesMatch(const MatchFinder::MatchResult &Result) {
+  if (Result.Nodes.getNodeAs<Decl>("duck_typing") == nullptr)
+    return true;
   // Verify that the types match.
   // We can't do this on the matcher because the type nodes can be different,
   // even though they represent the same type. This difference comes from how
@@ -71,14 +97,20 @@ bool allReturnTypesMatch(const MatchFinder::MatchResult &Result) {
 void RedundantSmartptrGet::check(const MatchFinder::MatchResult &Result) {
   if (!allReturnTypesMatch(Result)) return;
 
-  bool IsPtrToPtr = Result.Nodes.getNodeAs<Expr>("ptr_to_ptr") != nullptr;
+  bool IsPtrToPtr = Result.Nodes.getNodeAs<Decl>("ptr_to_ptr") != nullptr;
+  bool IsMemberExpr = Result.Nodes.getNodeAs<Expr>("memberExpr") != nullptr;
   const Expr *GetCall = Result.Nodes.getNodeAs<Expr>("redundant_get");
   const Expr *Smartptr = Result.Nodes.getNodeAs<Expr>("smart_pointer");
 
+  if (IsPtrToPtr && IsMemberExpr) {
+    // Ignore this case (eg. Foo->get()->DoSomething());
+    return;
+  }
+
   StringRef SmartptrText = Lexer::getSourceText(
       CharSourceRange::getTokenRange(Smartptr->getSourceRange()),
       *Result.SourceManager, LangOptions());
-  // Replace *foo->get() with **foo, and foo.get() with foo.
+  // Replace foo->get() with *foo, and foo.get() with foo.
   std::string Replacement = Twine(IsPtrToPtr ? "*" : "", SmartptrText).str();
   diag(GetCall->getLocStart(), "Redundant get() call on smart pointer.")
       << FixItHint::CreateReplacement(GetCall->getSourceRange(), Replacement);

clang-tools-extra/test/clang-tidy/check_clang_tidy_fix.sh

@@ -0,0 +1,12 @@
+#!/bin/sh
+#
+# Run clang-tidy in fix mode and verify the result.
+
+INPUT_FILE=$1
+CHECK_TO_RUN=$2
+TEMPORARY_FILE=$3.cpp
+
+grep -Ev "// *[A-Z-]+:" ${INPUT_FILE} > ${TEMPORARY_FILE}
+clang-tidy ${TEMPORARY_FILE} -fix --checks=${CHECK_TO_RUN} \
+    --disable-checks="" -- --std=c++11
+FileCheck -input-file=${TEMPORARY_FILE} ${INPUT_FILE}

clang-tools-extra/test/clang-tidy/check_clang_tidy_output.sh

@@ -0,0 +1,10 @@
+#!/bin/sh
+#
+# Run clang-tidy and verify its command line output.
+
+INPUT_FILE=$1
+CHECK_TO_RUN=$2
+
+clang-tidy --checks=${CHECK_TO_RUN} --disable-checks="" ${INPUT_FILE} \
+    -- --std=c++11 \
+  | FileCheck ${INPUT_FILE}

clang-tools-extra/test/clang-tidy/redundant-smartptr-get-fix.cpp

@@ -1,6 +1,6 @@
-// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: clang-tidy %t.cpp -fix -checks=misc-redundant-smartptr-get --
-// RUN: FileCheck -input-file=%t.cpp %s
+// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-redundant-smartptr-get %t
+
+#include <memory>
 
 struct Bar {
   void Do();
@@ -39,4 +39,11 @@ void Positive() {
   int_ptr ip;
   int i = *ip.get();
   // CHECK: int i = *ip;
+
+  std::unique_ptr<int> uu;
+  std::shared_ptr<double> *ss;
+  bool bb = uu.get() == nullptr;
+  // CHECK: bool bb = uu == nullptr;
+  bb = nullptr != ss->get();
+  // CHECK: bb = nullptr != *ss;
 }

clang-tools-extra/test/clang-tidy/redundant-smartptr-get.cpp

@@ -1,22 +1,8 @@
-// RUN: clang-tidy --checks=misc-redundant-smartptr-get %s -- | FileCheck %s
+// RUN: $(dirname %s)/check_clang_tidy_output.sh %s misc-redundant-smartptr-get
 
 // CHECK-NOT: warning
 
-namespace std {
-
-template <typename T>
-struct MakeRef {
-  typedef T& type;
-};
-
-template <typename T>
-struct unique_ptr {
-  T* get();
-  T* operator->();
-  // This simulates libstdc++'s implementation of unique_ptr.
-  typename MakeRef<T>::type operator*();
-};
-}  // namespace std
+#include <memory>
 
 struct int_ptr {
   int* get();
@@ -65,6 +51,14 @@ void Positive() {
   int i = *ip.get();
   // CHECK: :[[@LINE-1]]:12: warning: Redundant get() call on smart pointer.
   // CHECK: int i = *ip.get();
+
+  bool bb = u.get() == nullptr;
+  // CHECK: :[[@LINE-1]]:13: warning: Redundant get() call on smart pointer.
+  // CHECK: u.get() == nullptr;
+  std::shared_ptr<double> *sp;
+  bb = nullptr != sp->get();
+  // CHECK: :[[@LINE-1]]:19: warning: Redundant get() call on smart pointer.
+  // CHECK: nullptr != sp->get();
 }
 
 // CHECK-NOT: warning
@@ -77,4 +71,9 @@ void Negative() {
   Fail2().get()->Do();
   const Bar& b = *Fail1().get();
   (*Fail2().get()).Do();
+
+  int_ptr ip;
+  bool bb = std::unique_ptr<int>().get() == NULL;
+  bb = ip.get() == nullptr;
+  bb = u->get() == NULL;
 }