[clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals

Summary:
Looks at conditionals and finds cases of ``cast<>``, which will
assert rather than return a null pointer, and ``dyn_cast<>`` where
the return value is not captured. Additionally, finds cases that
match the pattern ``var.foo() && isa<X>(var.foo())``, where the
method is called twice and could be expensive.

.. code-block:: c++

  // Finds cases like these:
  if (auto x = cast<X>(y)) <...>
  if (cast<X>(y)) <...>

  // But not cases like these:
  if (auto f = cast<Z>(y)->foo()) <...>
  if (cast<Z>(y)->foo()) <...>

Reviewers: alexfh, rjmccall, hokein, aaron.ballman, JonasToth

Reviewed By: aaron.ballman

Subscribers: xbolva00, Eugene.Zelenko, mgorny, xazax.hun, cfe-commits

Tags: #clang-tools-extra, #clang

Differential Revision: https://reviews.llvm.org/D59802

llvm-svn: 359142

clang-tools-extra/clang-tidy/llvm/CMakeLists.txt

@@ -4,6 +4,7 @@ add_clang_library(clangTidyLLVMModule
   HeaderGuardCheck.cpp
   IncludeOrderCheck.cpp
   LLVMTidyModule.cpp
+  PreferIsaOrDynCastInConditionalsCheck.cpp
   TwineLocalCheck.cpp
 
   LINK_LIBS

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

@@ -12,6 +12,7 @@
 #include "../readability/NamespaceCommentCheck.h"
 #include "HeaderGuardCheck.h"
 #include "IncludeOrderCheck.h"
+#include "PreferIsaOrDynCastInConditionalsCheck.h"
 #include "TwineLocalCheck.h"
 
 namespace clang {
@@ -25,6 +26,8 @@ public:
     CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order");
     CheckFactories.registerCheck<readability::NamespaceCommentCheck>(
         "llvm-namespace-comment");
+    CheckFactories.registerCheck<PreferIsaOrDynCastInConditionalsCheck>(
+        "llvm-prefer-isa-or-dyn-cast-in-conditionals");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");
   }
 };

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp

@@ -0,0 +1,135 @@
+//===--- PreferIsaOrDynCastInConditionalsCheck.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 "PreferIsaOrDynCastInConditionalsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace ast_matchers {
+AST_MATCHER(Expr, isMacroID) { return Node.getExprLoc().isMacroID(); }
+} // namespace ast_matchers
+
+namespace tidy {
+namespace llvm {
+
+void PreferIsaOrDynCastInConditionalsCheck::registerMatchers(
+    MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  auto Condition = hasCondition(implicitCastExpr(has(
+      callExpr(
+          allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                anyOf(callee(namedDecl(hasName("cast"))),
+                      callee(namedDecl(hasName("dyn_cast")).bind("dyn_cast")))))
+          .bind("call"))));
+
+  auto Any = anyOf(
+      has(declStmt(containsDeclaration(
+          0,
+          varDecl(hasInitializer(
+              callExpr(allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                             callee(namedDecl(hasName("cast")))))
+                  .bind("assign")))))),
+      Condition);
+
+  auto CallExpression =
+      callExpr(
+          allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                allOf(callee(namedDecl(anyOf(hasName("isa"), hasName("cast"),
+                                             hasName("cast_or_null"),
+                                             hasName("dyn_cast"),
+                                             hasName("dyn_cast_or_null")))
+                                 .bind("func")),
+                      hasArgument(0, anyOf(declRefExpr().bind("arg"),
+                                           cxxMemberCallExpr().bind("arg"))))))
+          .bind("rhs");
+
+  Finder->addMatcher(
+      stmt(anyOf(ifStmt(Any), whileStmt(Any), doStmt(Condition),
+                 binaryOperator(
+                     allOf(unless(isExpansionInFileMatching(
+                               "llvm/include/llvm/Support/Casting.h")),
+                           hasOperatorName("&&"),
+                           hasLHS(implicitCastExpr().bind("lhs")),
+                           hasRHS(anyOf(implicitCastExpr(has(CallExpression)),
+                                        CallExpression))))
+                     .bind("and"))),
+      this);
+}
+
+void PreferIsaOrDynCastInConditionalsCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("assign")) {
+    SourceLocation StartLoc = MatchedDecl->getCallee()->getExprLoc();
+    SourceLocation EndLoc =
+        StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+
+    diag(MatchedDecl->getBeginLoc(),
+         "cast<> in conditional will assert rather than return a null pointer")
+        << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc),
+                                        "dyn_cast");
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<CallExpr>("call")) {
+    SourceLocation StartLoc = MatchedDecl->getCallee()->getExprLoc();
+    SourceLocation EndLoc =
+        StartLoc.getLocWithOffset(StringRef("cast").size() - 1);
+
+    StringRef Message =
+        "cast<> in conditional will assert rather than return a null pointer";
+    if (Result.Nodes.getNodeAs<NamedDecl>("dyn_cast"))
+      Message = "return value from dyn_cast<> not used";
+
+    diag(MatchedDecl->getBeginLoc(), Message)
+        << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =
+                 Result.Nodes.getNodeAs<BinaryOperator>("and")) {
+    const auto *LHS = Result.Nodes.getNodeAs<ImplicitCastExpr>("lhs");
+    const auto *RHS = Result.Nodes.getNodeAs<CallExpr>("rhs");
+    const auto *Arg = Result.Nodes.getNodeAs<Expr>("arg");
+    const auto *Func = Result.Nodes.getNodeAs<NamedDecl>("func");
+
+    assert(LHS && "LHS is null");
+    assert(RHS && "RHS is null");
+    assert(Arg && "Arg is null");
+    assert(Func && "Func is null");
+
+    StringRef LHSString(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(LHS->getSourceRange()),
+        *Result.SourceManager, getLangOpts()));
+
+    StringRef ArgString(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(Arg->getSourceRange()),
+        *Result.SourceManager, getLangOpts()));
+
+    if (ArgString != LHSString)
+      return;
+
+    StringRef RHSString(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(RHS->getSourceRange()),
+        *Result.SourceManager, getLangOpts()));
+
+    std::string Replacement("isa_and_nonnull");
+    Replacement += RHSString.substr(Func->getName().size());
+
+    diag(MatchedDecl->getBeginLoc(),
+         "isa_and_nonnull<> is preferred over an explicit test for null "
+         "followed by calling isa<>")
+        << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
+                                                    MatchedDecl->getEndLoc()),
+                                        Replacement);
+  }
+}
+
+} // namespace llvm
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h

@@ -0,0 +1,64 @@
+//===--- PreferIsaOrDynCastInConditionalsCheck.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_LLVM_AVOIDCASTINCONDITIONALCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDCASTINCONDITIONALCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace llvm {
+
+/// \brief Looks at conditionals and finds and replaces cases of ``cast<>``, which will
+/// assert rather than return a null pointer, and ``dyn_cast<>`` where
+/// the return value is not captured.  Additionally, finds and replaces cases that match the
+/// pattern ``var && isa<X>(var)``, where ``var`` is evaluated twice.
+///
+/// Finds cases like these:
+/// \code
+///  if (auto x = cast<X>(y)) {}
+///  // is replaced by:
+///  if (auto x = dyn_cast<X>(y)) {}
+///
+///  if (cast<X>(y)) {}
+///  // is replaced by:
+///  if (isa<X>(y)) {}
+///
+///  if (dyn_cast<X>(y)) {}
+///  // is replaced by:
+///  if (isa<X>(y)) {}
+///
+///  if (var && isa<T>(var)) {}
+///  // is replaced by:
+///  if (isa_and_nonnull<T>(var.foo())) {}
+/// \endcode
+///
+///  // Other cases are ignored, e.g.:
+/// \code
+///  if (auto f = cast<Z>(y)->foo()) {}
+///  if (cast<Z>(y)->foo()) {}
+///  if (X.cast(y)) {}
+/// \endcode
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.html
+class PreferIsaOrDynCastInConditionalsCheck : public ClangTidyCheck {
+public:
+  PreferIsaOrDynCastInConditionalsCheck(StringRef Name,
+                                        ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace llvm
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDCASTINCONDITIONALCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -142,6 +142,15 @@ Improvements to clang-tidy
   <clang-tidy/checks/modernize-use-override>` now supports `OverrideSpelling`
   and `FinalSpelling` options.
 
+- New :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals
+  <clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals>` check.
+
+  Looks at conditionals and finds and replaces cases of ``cast<>``,
+  which will assert rather than return a null pointer, and
+  ``dyn_cast<>`` where the return value is not captured. Additionally,
+  finds and replaces cases that match the pattern ``var &&
+  isa<X>(var)``, where ``var`` is evaluated twice.
+
 - New :doc:`openmp-exception-escape
   <clang-tidy/checks/openmp-exception-escape>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -178,6 +178,7 @@ Clang-Tidy Checks
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
+   llvm-prefer-isa-or-dyn-cast-in-conditionals
    llvm-twine-local
    misc-definitions-in-headers
    misc-misplaced-const

clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.rst

@@ -0,0 +1,34 @@
+.. title:: clang-tidy - llvm-prefer-isa-or-dyn-cast-in-conditionals
+
+llvm-prefer-isa-or-dyn-cast-in-conditionals
+===========================================
+
+Looks at conditionals and finds and replaces cases of ``cast<>``,
+which will assert rather than return a null pointer, and
+``dyn_cast<>`` where the return value is not captured. Additionally,
+finds and replaces cases that match the pattern ``var &&
+isa<X>(var)``, where ``var`` is evaluated twice.
+
+.. code-block:: c++
+
+  // Finds these:
+  if (auto x = cast<X>(y)) {}
+  // is replaced by:
+  if (auto x = dyn_cast<X>(y)) {}
+
+  if (cast<X>(y)) {}
+  // is replaced by:
+  if (isa<X>(y)) {}
+
+  if (dyn_cast<X>(y)) {}
+  // is replaced by:
+  if (isa<X>(y)) {}
+
+  if (var && isa<T>(var)) {}
+  // is replaced by:
+  if (isa_and_nonnull<T>(var.foo())) {}
+
+  // Other cases are ignored, e.g.:
+  if (auto f = cast<Z>(y)->foo()) {}
+  if (cast<Z>(y)->foo()) {}
+  if (X.cast(y)) {}

clang-tools-extra/test/clang-tidy/llvm-prefer-isa-or-dyn-cast-in-conditionals.cpp

@@ -0,0 +1,132 @@
+// RUN: %check_clang_tidy %s llvm-prefer-isa-or-dyn-cast-in-conditionals %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+  X *cast(Y*);
+  bool baz(Y*);
+};
+
+template <class X, class Y>
+bool isa(Y *);
+template <class X, class Y>
+X *cast(Y *);
+template <class X, class Y>
+X *dyn_cast(Y *);
+template <class X, class Y>
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: cast<> in conditional will assert rather than return a null pointer [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (auto x = dyn_cast<X>(y))
+
+  while (auto x = cast<X>(y))
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
+  // CHECK-FIXES: while (auto x = dyn_cast<X>(y))
+
+  if (cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: cast<> in conditional
+  // CHECK-FIXES: if (isa<X>(y))
+
+  while (cast<X>(y))
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa<X>(y))
+
+  do {
+    break;
+  } while (cast<X>(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa<X>(y));
+
+  if (dyn_cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: return value from dyn_cast<> not used [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (isa<X>(y))
+
+  while (dyn_cast<X>(y))
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa<X>(y))
+
+  do {
+    break;
+  } while (dyn_cast<X>(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa<X>(y));
+
+  if (y && isa<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred over an explicit test for null followed by calling isa<> [llvm-prefer-isa-or-dyn-cast-in-conditionals]
+  // CHECK-FIXES: if (isa_and_nonnull<X>(y))
+
+  if (z->bar() && isa<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  if (z->bar() && cast<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  if (z->bar() && dyn_cast<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null<Y>(z->bar()))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
+
+  bool b = z->bar() && cast<Y>(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: isa_and_nonnull<> is preferred
+  // CHECK-FIXES: bool b = isa_and_nonnull<Y>(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = cast<Z>(y)->foo())
+    return true;
+  if (auto x = z->cast(y))
+    return true;
+  while (auto x = cast<Z>(y)->foo())
+    break;
+  if (cast<Z>(y)->foo())
+    return true;
+  if (z->cast(y))
+    return true;
+  while (cast<Z>(y)->foo())
+    break;
+  if (y && cast<X>(z->bar()))
+    return true;
+  if (z && cast<Z>(y)->foo())
+    return true;
+  bool b2 = y && cast<X>(z);
+  if(z->cast(y))
+    return true;
+  if (z->baz(cast<Y>(z)))
+    return true;
+
+#define CAST(T, Obj) cast<T>(Obj)
+#define AUTO_VAR_CAST(X, Y, Z) auto X = cast<Y>(Z)
+#define ISA(T, Obj) isa<T>(Obj)
+#define ISA_OR_NULL(T, Obj) Obj &&isa<T>(Obj)
+
+  // Macros don't trigger warning.
+  if (auto x = CAST(X, y))
+    return true;
+  if (AUTO_VAR_CAST(x, X, z))
+    return true;
+  if (z->bar() && ISA(Y, z->bar()))
+    return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+    return true;
+
+  return false;
+}