diff --git a/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp b/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp index 8f8c152b5de86..16a55b3ab1a12 100644 --- a/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp +++ b/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp @@ -19,7 +19,10 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Lex/Lexer.h" +#include "clang/Tooling/FixIt.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/Support/ErrorHandling.h" using namespace clang::ast_matchers; @@ -28,7 +31,7 @@ namespace clang::tidy::llvm_check { namespace { AST_MATCHER(Expr, isMacroID) { return Node.getExprLoc().isMacroID(); } AST_MATCHER_P(OverloadExpr, hasAnyUnresolvedName, ArrayRef, Names) { - auto DeclName = Node.getName(); + const DeclarationName DeclName = Node.getName(); if (!DeclName.isIdentifier()) return false; const IdentifierInfo *II = DeclName.getAsIdentifierInfo(); @@ -36,34 +39,53 @@ AST_MATCHER_P(OverloadExpr, hasAnyUnresolvedName, ArrayRef, Names) { } } // namespace -static constexpr StringRef FunctionNames[] = { +static constexpr StringRef CastFunctionNames[] = { "cast", "cast_or_null", "cast_if_present", "dyn_cast", "dyn_cast_or_null", "dyn_cast_if_present"}; +static constexpr StringRef IsaFunctionNames[] = {"isa", "isa_and_nonnull", + "isa_and_present"}; + void RedundantCastingCheck::registerMatchers(MatchFinder *Finder) { auto IsInLLVMNamespace = hasDeclContext( namespaceDecl(hasName("llvm"), hasDeclContext(translationUnitDecl()))); - auto AnyCalleeName = + auto AnyCastCalleeName = allOf(unless(isMacroID()), unless(cxxMemberCallExpr()), - callee(expr(ignoringImpCasts( - declRefExpr( - to(namedDecl(hasAnyName(FunctionNames), IsInLLVMNamespace)), - templateArgumentLocCountIs(1)) - .bind("callee"))))); - auto AnyCalleeNameInUninstantiatedTemplate = + callee(expr(declRefExpr(to(namedDecl(hasAnyName(CastFunctionNames), + IsInLLVMNamespace)), + templateArgumentLocCountIs(1)) + .bind("callee")))); + auto AnyCastCalleeNameInUninstantiatedTemplate = allOf( + unless(isMacroID()), unless(cxxMemberCallExpr()), + callee(expr(unresolvedLookupExpr(hasAnyUnresolvedName(CastFunctionNames), + templateArgumentLocCountIs(1)) + .bind("callee")))); + Finder->addMatcher( + callExpr(AnyCastCalleeName, argumentCountIs(1), + optionally( + hasParent(callExpr(AnyCastCalleeName).bind("parent_cast")))) + .bind("call"), + this); + + auto AnyIsaCalleeName = allOf(unless(isMacroID()), unless(cxxMemberCallExpr()), - callee(expr(ignoringImpCasts( - unresolvedLookupExpr(hasAnyUnresolvedName(FunctionNames), - templateArgumentLocCountIs(1)) - .bind("callee"))))); - Finder->addMatcher(callExpr(AnyCalleeName, argumentCountIs(1), - optionally(hasParent( - callExpr(AnyCalleeName).bind("parent_cast")))) - .bind("call"), - this); + callee(expr(declRefExpr(to(namedDecl(hasAnyName(IsaFunctionNames), + IsInLLVMNamespace)), + hasAnyTemplateArgumentLoc(anything())) + .bind("callee")))); + Finder->addMatcher( + callExpr(AnyIsaCalleeName, argumentCountIs(1)).bind("call"), this); + + auto AnyIsaCalleeNameInUninstantiatedTemplate = allOf( + unless(isMacroID()), unless(cxxMemberCallExpr()), + callee(expr(unresolvedLookupExpr(hasAnyUnresolvedName(IsaFunctionNames), + hasAnyTemplateArgumentLoc(anything())) + .bind("callee")))); Finder->addMatcher( callExpr( - AnyCalleeNameInUninstantiatedTemplate, argumentCountIs(1), + anyOf(AnyCastCalleeNameInUninstantiatedTemplate, + AnyIsaCalleeNameInUninstantiatedTemplate), + argumentCountIs(1), optionally(hasAncestor( namespaceDecl(hasName("llvm"), hasParent(translationUnitDecl())) .bind("llvm_ns")))) @@ -90,15 +112,14 @@ static bool isLLVMNamespace(NestedNameSpecifier NNS) { } void RedundantCastingCheck::check(const MatchFinder::MatchResult &Result) { - const auto &Nodes = Result.Nodes; + const BoundNodes &Nodes = Result.Nodes; const auto *Call = Nodes.getNodeAs("call"); - CanQualType RetTy; + ArrayRef TArgLocs; std::string FuncName; if (const auto *ResolvedCallee = Nodes.getNodeAs("callee")) { + TArgLocs = ResolvedCallee->template_arguments(); const auto *F = cast(ResolvedCallee->getDecl()); - RetTy = stripPointerOrReference(F->getReturnType()) - ->getCanonicalTypeUnqualified(); FuncName = F->getName(); } else if (const auto *UnresolvedCallee = Nodes.getNodeAs("callee")) { @@ -107,77 +128,100 @@ void RedundantCastingCheck::check(const MatchFinder::MatchResult &Result) { const auto *CallerNS = Nodes.getNodeAs("llvm_ns"); if (!IsExplicitlyLLVM && !CallerNS) return; - auto TArg = UnresolvedCallee->template_arguments()[0].getArgument(); - if (TArg.getKind() != TemplateArgument::Type) - return; - RetTy = TArg.getAsType()->getCanonicalTypeUnqualified(); + TArgLocs = UnresolvedCallee->template_arguments(); FuncName = UnresolvedCallee->getName().getAsString(); } else { - llvm_unreachable(""); + llvm_unreachable("unexpected callee kind"); + } + + llvm::SmallVector TargetTypes; + for (const TemplateArgumentLoc &TArgLoc : TArgLocs) { + const TemplateArgument TArg = TArgLoc.getArgument(); + if (TArg.getKind() == TemplateArgument::Type) { + const CanQualType TargetTy = + TArg.getAsType()->getCanonicalTypeUnqualified(); + TargetTypes.emplace_back(TargetTy); + } else if (TArg.getKind() == TemplateArgument::Pack) { + for (const TemplateArgument &E : TArg.pack_elements()) { + if (E.getKind() != TemplateArgument::Type) + return; + TargetTypes.emplace_back(E.getAsType()->getCanonicalTypeUnqualified()); + } + } else { + llvm_unreachable("unexpected template argument"); + } } - const auto *Arg = Call->getArg(0); + const Expr *Arg = Call->getArg(0); const QualType ArgTy = Arg->getType(); const QualType ArgPointeeTy = stripPointerOrReference(ArgTy); const CanQualType FromTy = ArgPointeeTy->getCanonicalTypeUnqualified(); const auto *FromDecl = FromTy->getAsCXXRecordDecl(); - const auto *RetDecl = RetTy->getAsCXXRecordDecl(); - const bool IsDerived = - FromDecl && RetDecl && FromDecl->isDerivedFrom(RetDecl); - if (FromTy != RetTy && !IsDerived) + const bool IsIsa = StringRef(FuncName).starts_with("isa"); + if (!IsIsa && TargetTypes.size() != 1) return; - QualType ParentTy; - if (const auto *ParentCast = Nodes.getNodeAs("parent_cast")) { - ParentTy = ParentCast->getType(); - } else { - // IgnoreUnlessSpelledInSource prevents matching implicit casts - const TraversalKindScope TmpTraversalKind(*Result.Context, TK_AsIs); - for (const DynTypedNode Parent : Result.Context->getParents(*Call)) { - if (const auto *ParentCastExpr = Parent.get()) { - ParentTy = ParentCastExpr->getType(); - break; + for (const CanQualType TargetTy : TargetTypes) { + const auto *RetDecl = TargetTy->getAsCXXRecordDecl(); + const bool IsDerived = + FromDecl && RetDecl && FromDecl->isDerivedFrom(RetDecl); + if (FromTy != TargetTy && !IsDerived) + continue; + + if (IsIsa) { + diag(Call->getExprLoc(), "call to '%0' always succeeds") << FuncName; + } else { + QualType ParentTy; + if (const auto *ParentCast = Nodes.getNodeAs("parent_cast")) { + ParentTy = ParentCast->getType(); + } else { + // IgnoreUnlessSpelledInSource prevents matching implicit casts + const TraversalKindScope TmpTraversalKind(*Result.Context, TK_AsIs); + for (const DynTypedNode Parent : Result.Context->getParents(*Call)) { + if (const auto *ParentCastExpr = Parent.get()) { + ParentTy = ParentCastExpr->getType(); + break; + } + } } + if (!ParentTy.isNull()) { + const CXXRecordDecl *ParentDecl = ParentTy->getAsCXXRecordDecl(); + if (FromDecl && ParentDecl) { + CXXBasePaths Paths(/*FindAmbiguities=*/true, + /*RecordPaths=*/false, + /*DetectVirtual=*/false); + const bool IsDerivedFromParent = + FromDecl && ParentDecl && + FromDecl->isDerivedFrom(ParentDecl, Paths); + // For the following case a direct `cast(d)` would be ambiguous: + // struct A {}; + // struct B : A {}; + // struct C : A {}; + // struct D : B, C {}; + // So we should not warn for `A *a = cast(d)`. + if (IsDerivedFromParent && + Paths.isAmbiguous(ParentTy->getCanonicalTypeUnqualified())) + return; + } + } + + diag(Call->getExprLoc(), "redundant use of '%0'") + << FuncName + << tooling::fixit::createReplacement(*Call, *Arg, *Result.Context); } + // printing the canonical type for a template parameter prints as e.g. + // 'type-parameter-0-0' + const QualType DiagFromTy(ArgPointeeTy->getUnqualifiedDesugaredType(), 0); + diag( + Arg->getExprLoc(), + "source expression has%select{| pointee}0 type %1%select{|, which is a " + "subtype of %3}2", + DiagnosticIDs::Note) + << Arg->getSourceRange() << ArgTy->isPointerType() << DiagFromTy + << (FromTy != TargetTy) << TargetTy; + return; } - if (!ParentTy.isNull()) { - const CXXRecordDecl *ParentDecl = ParentTy->getAsCXXRecordDecl(); - if (FromDecl && ParentDecl) { - CXXBasePaths Paths(/*FindAmbiguities=*/true, - /*RecordPaths=*/false, - /*DetectVirtual=*/false); - const bool IsDerivedFromParent = - FromDecl && ParentDecl && FromDecl->isDerivedFrom(ParentDecl, Paths); - // For the following case a direct `cast(d)` would be ambiguous: - // struct A {}; - // struct B : A {}; - // struct C : A {}; - // struct D : B, C {}; - // So we should not warn for `A *a = cast(d)`. - if (IsDerivedFromParent && - Paths.isAmbiguous(ParentTy->getCanonicalTypeUnqualified())) - return; - } - } - - auto GetText = [&](SourceRange R) { - return Lexer::getSourceText(CharSourceRange::getTokenRange(R), - *Result.SourceManager, getLangOpts()); - }; - StringRef ArgText = GetText(Arg->getSourceRange()); - diag(Call->getExprLoc(), "redundant use of '%0'") - << FuncName - << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText); - // printing the canonical type for a template parameter prints as e.g. - // 'type-parameter-0-0' - const QualType DiagFromTy(ArgPointeeTy->getUnqualifiedDesugaredType(), 0); - diag(Arg->getExprLoc(), - "source expression has%select{| pointee}0 type %1%select{|, which is a " - "subtype of %3}2", - DiagnosticIDs::Note) - << Arg->getSourceRange() << ArgTy->isPointerType() << DiagFromTy - << (FromTy != RetTy) << RetTy; } } // namespace clang::tidy::llvm_check diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 78a7ff6ff7561..dcdede6229e8e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -130,7 +130,8 @@ New checks Points out uses of ``cast<>``, ``dyn_cast<>`` and their ``or_null`` variants that are unnecessary because the argument already is of the target type, or a - derived type thereof. + derived type thereof. Also does similar analysis for calls to ``isa<>`` that + always return ``true``. - New :doc:`llvm-type-switch-case-types ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/llvm/redundant-casting.rst b/clang-tools-extra/docs/clang-tidy/checks/llvm/redundant-casting.rst index 84596b26ee382..10aaf67892bd7 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/llvm/redundant-casting.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/llvm/redundant-casting.rst @@ -3,9 +3,9 @@ llvm-redundant-casting ====================== -Points out uses of ``cast<>``, ``dyn_cast<>`` and their ``or_null`` variants -that are unnecessary because the argument already is of the target type, or a -derived type thereof. +Points out uses of ``cast<>``, ``dyn_cast<>``, ``isa<>`` and their ``or_null`` +variants that are unnecessary because the argument already is of the target +type, or a derived type thereof. .. code-block:: c++ @@ -23,6 +23,13 @@ derived type thereof. // replaced by: A y = b; + struct C : public A {}; + C c; + // Finds: + bool r1 = isa(a) // always true + bool r2 = isa(b) // always true + bool r3 = isa(c) // always true + Supported functions: - ``llvm::cast`` - ``llvm::cast_or_null`` @@ -30,3 +37,6 @@ Supported functions: - ``llvm::dyn_cast`` - ``llvm::dyn_cast_or_null`` - ``llvm::dyn_cast_if_present`` + - ``llvm::isa`` + - ``llvm::isa_and_nonnull`` + - ``llvm::isa_and_present`` diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-casting.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-casting.cpp index 933cbadc51701..2b6e76b009423 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-casting.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-casting.cpp @@ -24,7 +24,7 @@ CAST_FUNCTION(cast_or_null) CAST_FUNCTION(cast_if_present) CAST_FUNCTION(dyn_cast_or_null) CAST_FUNCTION(dyn_cast_if_present) -} +} // namespace llvm struct A {}; struct B : A {}; @@ -247,6 +247,17 @@ void testCastCRTPUpcast(L& value) { (void)a24; } +namespace mlir { +using llvm::cast; +void testCastUsing(A& value) { + A& a30 = cast(value); + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: redundant use of 'cast' [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:20: note: source expression has type 'A' + // CHECK-FIXES: A& a30 = value; + (void)a30; +} +} + CAST_FUNCTION(cast) CAST_FUNCTION(dyn_cast) diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-isa.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-isa.cpp new file mode 100644 index 0000000000000..592b5f09dcdde --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-isa.cpp @@ -0,0 +1,178 @@ +// RUN: %check_clang_tidy -std=c++17-or-later %s llvm-redundant-casting %t -- -- -fno-delayed-template-parsing + +namespace llvm { +#define ISA_FUNCTION(name) \ +template \ +[[nodiscard]] inline bool name(const From &Val) { \ + return true; \ +} \ +template \ +[[nodiscard]] inline bool name(const From *Val) { \ + return true; \ +} + +ISA_FUNCTION(isa) +ISA_FUNCTION(isa_and_present) +ISA_FUNCTION(isa_and_nonnull) +} // namespace llvm + +struct A {}; +struct B : A {}; +A &getA(); + +void testIsa(A& value) { + bool b1 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:26: note: source expression has type 'A' + (void)b1; +} + +void testIsaAndPresent(A& value) { + bool b2 = llvm::isa_and_present(value); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: call to 'isa_and_present' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:38: note: source expression has type 'A' + (void)b2; +} + +void testIsaAndNonnull(A& value) { + bool b3 = llvm::isa_and_nonnull(value); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: call to 'isa_and_nonnull' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:38: note: source expression has type 'A' + (void)b3; +} + +void testIsaNonDeclRef() { + bool b4 = llvm::isa((getA())); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:26: note: source expression has type 'A' + (void)b4; +} + +void testUpcast(B& value) { + bool b5 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:26: note: source expression has type 'B', which is a subtype of 'A' + (void)b5; +} + +void testDowncast(A& value) { + bool b6 = llvm::isa(value); + (void)b6; +} + +namespace llvm { +void testIsaInLLVM(A& value) { + bool b7 = isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:20: note: source expression has type 'A' + (void)b7; +} +} // namespace llvm + +void testIsaPointer(A* value) { + bool b8 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:26: note: source expression has pointee type 'A' + (void)b8; +} + +template +void testIsaTemplateIgnore(T* value) { + bool b9 = llvm::isa(value); + (void)b9; +} +template void testIsaTemplateIgnore(A *value); + +template +struct testIsaTemplateIgnoreStruct { + void method(T* value) { + bool b10 = llvm::isa(value); + (void)b10; + } +}; + +void call(testIsaTemplateIgnoreStruct s, A *a) { + s.method(a); +} + +template +void testIsaTemplateTrigger1(T* value) { + bool b11 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:27: note: source expression has pointee type 'T' + (void)b11; +} + +template +void testIsaTemplateTrigger2(A* value, T other) { + bool b12 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:27: note: source expression has pointee type 'A' + (void)b12; (void)other; +} + +void testIsaConst(const A& value) { + bool b13 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:27: note: source expression has type 'A' + (void)b13; +} + +void testIsaConstPointer(const A* value) { + bool b14 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:27: note: source expression has pointee type 'A' + (void)b14; +} + +void testIsaMulti(A& value) { + bool b15 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:32: note: source expression has type 'A' + (void)b15; +} + +void testIsaMultiUpcast(B& value) { + bool b16 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:35: note: source expression has type 'B', which is a subtype of 'A' + (void)b16; +} + +template +void testIsaTemplateMulti(const T* value) { + bool b17 = llvm::isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:30: note: source expression has pointee type 'T' + (void)b17; +} + +namespace llvm { +namespace magic { +template +void testIsaImplicitlyLLVMUnresolve(T* value) { + bool b18 = isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:21: note: source expression has pointee type 'T' + (void)b18; +} +} // namespace magic +} // namespace llvm + +namespace mlir { +using llvm::isa; +void testIsaUsing(A& value) { + bool b20 = isa(value); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: call to 'isa' always succeeds [llvm-redundant-casting] + // CHECK-MESSAGES: :[[@LINE-2]]:21: note: source expression has type 'A' + (void)b20; +} +} + +ISA_FUNCTION(isa) + +void testIsaNonLLVM(A& value) { + bool b19 = isa(value); + (void)b19; +} +