[clang-tools-extra] 2ec6b3b - [Clang-Tidy] Handle nested-name-specifier in "llvm-prefer-isa-or-dyn-cast-in-conditionals" (#155982)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Sep 7 19:19:56 PDT 2025
Author: Yanzuo Liu
Date: 2025-09-08T10:19:52+08:00
New Revision: 2ec6b3bceda275b4146bb229668e38797d6afe62
URL: https://github.com/llvm/llvm-project/commit/2ec6b3bceda275b4146bb229668e38797d6afe62
DIFF: https://github.com/llvm/llvm-project/commit/2ec6b3bceda275b4146bb229668e38797d6afe62.diff
LOG: [Clang-Tidy] Handle nested-name-specifier in "llvm-prefer-isa-or-dyn-cast-in-conditionals" (#155982)
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
Added:
Modified:
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
index 4c138bcc564d8..cb289af46ea44 100644
--- 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
index 62916906f96fe..bd757851abe02 100644
--- 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
index 88e4b643004fc..6b4c917f6c599 100644
--- 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,14 +9,20 @@ 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))
@@ -24,6 +30,26 @@ bool foo(Y *y, Z *z) {
// 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
More information about the cfe-commits
mailing list