[Clang-Tidy] Handle nested-name-specifier in "llvm-prefer-isa-or-dyn-cast-in-conditionals" #155982
+143
−64
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Yanzuo Liu (zwuis) ChangesUse Drive-by changes:
Fixes #154790 Full diff: https://github.com/llvm/llvm-project/pull/155982.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp b/clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
index 4c138bcc564d8..5e9777248896a 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,95 @@ 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 Condition = hasCondition(implicitCastExpr(
+ has(callExpr(AnyCalleeName({"cast", "dyn_cast"})).bind("cond"))));
+
+ auto Any =
+ anyOf(hasConditionVariableStatement(containsDeclaration(
+ 0, varDecl(hasInitializer(callExpr(AnyCalleeName({"cast"}))))
+ .bind("var"))),
+ Condition);
+
+ 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", "cast_if_present",
+ "dyn_cast", "dyn_cast_or_null",
+ "dyn_cast_if_present"}),
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(Any), forStmt(Any), whileStmt(Any), doStmt(Condition),
+ 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");
+ if (!Callee)
+ return;
+
+ 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()));
+ StringRef LHSString = GetText(LHS->getSourceRange());
+
+ 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());
+ std::string Replacement = llvm::formatv(
+ "{}{}{}", GetText(Callee->getQualifierLoc().getSourceRange()),
+ "isa_and_nonnull",
+ 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);
}
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 06641a602e28f..0829d336932c6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -244,6 +244,19 @@ Changes in existing checks
<clang-tidy/checks/readability/qualified-auto>` check by adding the option
`IgnoreAliasing`, that allows not looking at underlying types of type aliases.
+- 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.
+
+ - ``cast_if_present`` and ``dyn_cast_if_present`` are supported in the case
+ that ``isa_and_nonnull`` is preferred.
+
Removed checks
^^^^^^^^^^^^^^
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..8683926bace11 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,24 @@ 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 *cast_if_present(Y *);
+template <class X, class Y>
X *dyn_cast(Y *);
template <class X, class Y>
X *dyn_cast_or_null(Y *);
+template <class X, class Y>
+X *dyn_cast_if_present(Y *);
+} // namespace llvm
+
+using namespace llvm;
bool foo(Y *y, Z *z) {
if (auto x = cast<X>(y))
@@ -24,6 +34,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 +64,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 +90,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 +111,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 +136,16 @@ 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() && cast_if_present<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
@@ -86,6 +156,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() && dyn_cast_if_present<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());
|
|
Thank you for the improvements of the check! Did you try running on LLVM code to check for any miscodegen/FP? https://clang.llvm.org/extra/clang-tidy/#running-clang-tidy-in-parallel |
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
Outdated
Show resolved
Hide resolved
I just finished checking using |
flovent
reviewed
Aug 31, 2025
| if (z->bar() && cast_if_present<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())) |
There was a problem hiding this comment.
Maybe we should suggest isa_and_present here? instead of hard coded isa_and_nonnull.
There was a problem hiding this comment.
I want to revert this part and add support in a separate PR that
- Keep
_presentand_or_nullas written. - Replace
x && isa<Y>(x)with what configuration files choose (isa_and_nonnullorisa_and_present).
WDYT?
There was a problem hiding this comment.
Keep _present and _or_null as written.
I think that's reasonable, keeping their original style.
Replace x && isa(x) with what configuration files choose (isa_and_nonnull or isa_and_present).
What default option should we use here, i think we don't have cast APIs with present postfix when this check was added to clang-tidy.
There was a problem hiding this comment.
Is it more simple to hard code _present? since _or_null is deprecated. I don't think we should put effort to here.
There was a problem hiding this comment.
What default option should we use here
I have no idea. 🤦 We still don't reach a consense on preferring isa_and_nonnull or isa_and_present.
https://discourse.llvm.org/t/psa-swapping-out-or-null-with-if-present/65018
HerrCai0907
approved these changes
Aug 31, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use
declRefExprmatcher to match callee so that we can get theSourceRangeof the identifier of the callee for replacement.Drive-by changes:
hasConditionVariableStatementmatcher to handleifstatements with init-statement.forloops.cast_if_presentanddyn_cast_if_present.Fixes #154790