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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
