[Clang-Tidy] Handle nested-name-specifier in "llvm-prefer-isa-or-dyn-cast-in-conditionals" (#155982)

zwuis · web-flow · commit 2ec6b3bceda2 · 2025-09-08T10:19:52.000+08:00
Use `declRefExpr` matcher to match callee so that we can get the `SourceRange` of the identifier of the callee for replacement. Drive-by changes: - Use `hasConditionVariableStatement` matcher to handle `if` statements with init-statement. - Support `for` loops. Fixes #154790
diff --git a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
@@ -11,6 +11,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/Support/FormatVariadic.h"
 
 using namespace clang::ast_matchers;
 
@@ -22,105 +23,104 @@ AST_MATCHER(Expr, isMacroID) { return Node.getExprLoc().isMacroID(); }
 
 void PreferIsaOrDynCastInConditionalsCheck::registerMatchers(
     MatchFinder *Finder) {
-  auto Condition = hasCondition(implicitCastExpr(has(
-      callExpr(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(unless(isMacroID()),
-                                             unless(cxxMemberCallExpr()),
-                                             callee(namedDecl(hasName("cast"))))
-                                        .bind("assign")))))),
-      Condition);
-
-  auto CallExpression =
+  auto AnyCalleeName = [](ArrayRef<StringRef> CalleeName) {
+    return allOf(unless(isMacroID()), unless(cxxMemberCallExpr()),
+                 callee(expr(ignoringImpCasts(
+                     declRefExpr(to(namedDecl(hasAnyName(CalleeName))),
+                                 hasAnyTemplateArgumentLoc(anything()))
+                         .bind("callee")))));
+  };
+
+  auto CondExpr = hasCondition(implicitCastExpr(
+      has(callExpr(AnyCalleeName({"cast", "dyn_cast"})).bind("cond"))));
+
+  auto CondExprOrCondVar =
+      anyOf(hasConditionVariableStatement(containsDeclaration(
+                0, varDecl(hasInitializer(callExpr(AnyCalleeName({"cast"}))))
+                       .bind("var"))),
+            CondExpr);
+
+  auto CallWithBindedArg =
       callExpr(
-
-          unless(isMacroID()), unless(cxxMemberCallExpr()),
-          callee(namedDecl(hasAnyName("isa", "cast", "cast_or_null", "dyn_cast",
-                                      "dyn_cast_or_null"))
-                     .bind("func")),
+          AnyCalleeName(
+              {"isa", "cast", "cast_or_null", "dyn_cast", "dyn_cast_or_null"}),
           hasArgument(0, mapAnyOf(declRefExpr, cxxMemberCallExpr).bind("arg")))
           .bind("rhs");
 
   Finder->addMatcher(
-      traverse(
-          TK_AsIs,
-          stmt(anyOf(
-              ifStmt(Any), whileStmt(Any), doStmt(Condition),
-              binaryOperator(unless(isExpansionInFileMatching(
-                                 "llvm/include/llvm/Support/Casting.h")),
-                             hasOperatorName("&&"),
-                             hasLHS(implicitCastExpr().bind("lhs")),
-                             hasRHS(anyOf(implicitCastExpr(has(CallExpression)),
-                                          CallExpression)))
-                  .bind("and")))),
+      stmt(anyOf(ifStmt(CondExprOrCondVar), forStmt(CondExprOrCondVar),
+                 whileStmt(CondExprOrCondVar), doStmt(CondExpr),
+                 binaryOperator(unless(isExpansionInFileMatching(
+                                    "llvm/include/llvm/Support/Casting.h")),
+                                hasOperatorName("&&"),
+                                hasLHS(implicitCastExpr().bind("lhs")),
+                                hasRHS(ignoringImpCasts(CallWithBindedArg)))
+                     .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);
+  const auto *Callee = Result.Nodes.getNodeAs<DeclRefExpr>("callee");
+
+  assert(Callee && "Callee should be binded if anything is matched");
+
+  // The first and last letter of the identifier
+  //   llvm::cast<T>(x)
+  //         ^  ^
+  //  StartLoc  EndLoc
+  SourceLocation StartLoc = Callee->getLocation();
+  SourceLocation EndLoc = Callee->getNameInfo().getEndLoc();
 
-    diag(MatchedDecl->getBeginLoc(),
+  if (Result.Nodes.getNodeAs<VarDecl>("var")) {
+    diag(StartLoc,
          "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);
-
+  } else if (Result.Nodes.getNodeAs<CallExpr>("cond")) {
     StringRef Message =
         "cast<> in conditional will assert rather than return a null pointer";
-    if (Result.Nodes.getNodeAs<NamedDecl>("dyn_cast"))
+    if (Callee->getDecl()->getName() == "dyn_cast")
       Message = "return value from dyn_cast<> not used";
 
-    diag(MatchedDecl->getBeginLoc(), Message)
+    diag(StartLoc, Message)
         << FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
-  } else if (const auto *MatchedDecl =
-                 Result.Nodes.getNodeAs<BinaryOperator>("and")) {
+  } else if (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()));
+    auto GetText = [&](SourceRange R) {
+      return Lexer::getSourceText(CharSourceRange::getTokenRange(R),
+                                  *Result.SourceManager, getLangOpts());
+    };
 
-    StringRef ArgString(Lexer::getSourceText(
-        CharSourceRange::getTokenRange(Arg->getSourceRange()),
-        *Result.SourceManager, getLangOpts()));
+    const StringRef LHSString = GetText(LHS->getSourceRange());
+    const StringRef ArgString = GetText(Arg->getSourceRange());
 
     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());
+    // It is not clear which is preferred between `isa_and_nonnull` and
+    // `isa_and_present`. See
+    // https://discourse.llvm.org/t/psa-swapping-out-or-null-with-if-present/65018
+    const std::string Replacement = llvm::formatv(
+        "{}isa_and_nonnull{}",
+        GetText(Callee->getQualifierLoc().getSourceRange()),
+        GetText(SourceRange(Callee->getLAngleLoc(), RHS->getEndLoc())));
 
-    diag(MatchedDecl->getBeginLoc(),
+    diag(LHS->getBeginLoc(),
          "isa_and_nonnull<> is preferred over an explicit test for null "
          "followed by calling isa<>")
-        << FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),
-                                                    MatchedDecl->getEndLoc()),
-                                        Replacement);
+        << FixItHint::CreateReplacement(
+               SourceRange(LHS->getBeginLoc(), RHS->getEndLoc()), Replacement);
+  } else {
+    llvm_unreachable(
+        R"(One of "var", "cond" and "and" should be binded if anything is matched)");
   }
 }
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -215,6 +215,16 @@ Changes in existing checks
   adding an option to allow pointer arithmetic via prefix/postfix increment or
   decrement operators.
 
+- Improved :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals
+  <clang-tidy/checks/llvm/prefer-isa-or-dyn-cast-in-conditionals>` check:
+
+  - Fix-it handles callees with nested-name-specifier correctly.
+
+  - ``if`` statements with init-statement (``if (auto X = ...; ...)``) are
+    handled correctly.
+
+  - ``for`` loops are supported.
+
 - Improved :doc:`misc-header-include-cycle
   <clang-tidy/checks/misc/header-include-cycle>` check performance.
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
@@ -9,21 +9,47 @@ struct Z {
   bool baz(Y*);
 };
 
+namespace llvm {
 template <class X, class Y>
 bool isa(Y *);
 template <class X, class Y>
 X *cast(Y *);
 template <class X, class Y>
+X *cast_or_null(Y *);
+template <class X, class Y>
 X *dyn_cast(Y *);
 template <class X, class Y>
 X *dyn_cast_or_null(Y *);
+} // namespace llvm
+
+using namespace llvm;
 
 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))
 
+  if (auto x = ::cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:18: 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))
+
+  if (auto x = llvm::cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:22: 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 = llvm::dyn_cast<X>(y))
+
+  if (auto x = ::llvm::cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:24: 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 = ::llvm::dyn_cast<X>(y))
+
+  for (; auto x = cast<X>(y); )
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
+  // CHECK-FIXES: for (; auto x = dyn_cast<X>(y); )
+
   while (auto x = cast<X>(y))
     break;
   // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
@@ -34,6 +60,16 @@ bool foo(Y *y, Z *z) {
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: cast<> in conditional
   // CHECK-FIXES: if (isa<X>(y))
 
+  if (auto x = cast<X>(y); cast<X>(y))
+    return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:28: 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 = cast<X>(y); isa<X>(y))
+
+  for (; cast<X>(y); )
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
+  // CHECK-FIXES: for (; isa<X>(y); )
+
   while (cast<X>(y))
     break;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
@@ -50,6 +86,11 @@ bool foo(Y *y, Z *z) {
   // 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))
 
+  for (; dyn_cast<X>(y); )
+    break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: for (; isa<X>(y); )
+
   while (dyn_cast<X>(y))
     break;
   // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<> not used
@@ -66,6 +107,21 @@ bool foo(Y *y, Z *z) {
   // 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 (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 (y && llvm::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 (llvm::isa_and_nonnull<X>(y))
+
+  if (y && ::llvm::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 (::llvm::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
@@ -76,6 +132,11 @@ bool foo(Y *y, Z *z) {
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
   // CHECK-FIXES: if (isa_and_nonnull<Y>(z->bar()))
 
+  if (z->bar() && 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()))
+
   if (z->bar() && dyn_cast<Y>(z->bar()))
     return true;
   // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: isa_and_nonnull<> is preferred
