llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Henrik G. Olsson (hnrklssn) <details> <summary>Changes</summary> Warns when performing a dynamic type check that is always true, either because the dynamic type is the same as the static type, or because the static type derives the dynamic type. Supported functions: - isa - isa_and_present - isa_and_nonnull Related PR: https://github.com/llvm/llvm-project/pull/189274 --- Patch is 20.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/191081.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp (+128-72) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+2-1) - (modified) clang-tools-extra/docs/clang-tidy/checks/llvm/redundant-casting.rst (+13-3) - (modified) clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-casting.cpp (+1-1) - (added) clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-isa.cpp (+169) ``````````diff diff --git a/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp b/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp index 8f8c152b5de86..e1162b6a5d59f 100644 --- a/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp +++ b/clang-tools-extra/clang-tidy/llvm/RedundantCastingCheck.cpp @@ -20,6 +20,8 @@ #include "clang/ASTMatchers/ASTMatchers.h" #include "clang/Lex/Lexer.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/Support/ErrorHandling.h" using namespace clang::ast_matchers; @@ -36,34 +38,57 @@ AST_MATCHER_P(OverloadExpr, hasAnyUnresolvedName, ArrayRef<StringRef>, 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)) + declRefExpr(to(namedDecl(hasAnyName(CastFunctionNames), + IsInLLVMNamespace)), + templateArgumentLocCountIs(1)) .bind("callee"))))); - auto AnyCalleeNameInUninstantiatedTemplate = + auto AnyCastCalleeNameInUninstantiatedTemplate = allOf(unless(isMacroID()), unless(cxxMemberCallExpr()), callee(expr(ignoringImpCasts( - unresolvedLookupExpr(hasAnyUnresolvedName(FunctionNames), + unresolvedLookupExpr(hasAnyUnresolvedName(CastFunctionNames), templateArgumentLocCountIs(1)) .bind("callee"))))); - Finder->addMatcher(callExpr(AnyCalleeName, argumentCountIs(1), - optionally(hasParent( - callExpr(AnyCalleeName).bind("parent_cast")))) - .bind("call"), - this); + 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( + 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(ignoringImpCasts( + 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")))) @@ -93,12 +118,28 @@ void RedundantCastingCheck::check(const MatchFinder::MatchResult &Result) { const auto &Nodes = Result.Nodes; const auto *Call = Nodes.getNodeAs<CallExpr>("call"); - CanQualType RetTy; + llvm::SmallVector<CanQualType, 4> TargetTypes; std::string FuncName; if (const auto *ResolvedCallee = Nodes.getNodeAs<DeclRefExpr>("callee")) { const auto *F = cast<FunctionDecl>(ResolvedCallee->getDecl()); - RetTy = stripPointerOrReference(F->getReturnType()) - ->getCanonicalTypeUnqualified(); + const auto *TArgs = F->getTemplateSpecializationArgs(); + if (TArgs->size() != 2) + return; + + TemplateArgument TArg = TArgs->get(0); + if (TArg.getKind() == TemplateArgument::Type) { + CanQualType TargetTy = TArg.getAsType()->getCanonicalTypeUnqualified(); + TargetTypes.emplace_back(TargetTy); + } else if (TArg.getKind() == TemplateArgument::Pack) { + for (const auto &E : TArg.pack_elements()) { + if (E.getKind() != TemplateArgument::Type) + return; + TargetTypes.emplace_back(E.getAsType()->getCanonicalTypeUnqualified()); + } + } else { + llvm_unreachable("unexpected template argument"); + } + FuncName = F->getName(); } else if (const auto *UnresolvedCallee = Nodes.getNodeAs<UnresolvedLookupExpr>("callee")) { @@ -107,14 +148,16 @@ void RedundantCastingCheck::check(const MatchFinder::MatchResult &Result) { const auto *CallerNS = Nodes.getNodeAs<NamedDecl>("llvm_ns"); if (!IsExplicitlyLLVM && !CallerNS) return; - auto TArg = UnresolvedCallee->template_arguments()[0].getArgument(); - if (TArg.getKind() != TemplateArgument::Type) - return; - RetTy = TArg.getAsType()->getCanonicalTypeUnqualified(); + for (const auto &TArgLoc : UnresolvedCallee->template_arguments()) { + const TemplateArgument TArg = TArgLoc.getArgument(); + if (TArg.getKind() != TemplateArgument::Type) + return; + TargetTypes.emplace_back(TArg.getAsType()->getCanonicalTypeUnqualified()); + } FuncName = UnresolvedCallee->getName().getAsString(); } else { - llvm_unreachable(""); + llvm_unreachable("unexpected callee kind"); } const auto *Arg = Call->getArg(0); @@ -122,62 +165,75 @@ void RedundantCastingCheck::check(const MatchFinder::MatchResult &Result) { 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<Expr>("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<CastExpr>()) { - ParentTy = ParentCastExpr->getType(); - break; + for (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<Expr>("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<CastExpr>()) { + 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<A>(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<C>(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 != 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<A>(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<C>(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 <clang-tidy/checks/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>(a) // always true + bool r2 = isa<A>(b) // always true + bool r3 = isa<B, C>(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..5955c8a6870a6 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 {}; 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..bb6941b81a712 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/llvm/redundant-isa.cpp @@ -0,0 +1,169 @@ +// RUN: %check_clang_tidy -std=c++17 %s llvm-redundant-casting %t -- -- -fno-delayed-template-parsing + +namespace llvm { +#define ISA_FUNCTION(name) \ +template <typename ...To, typename From> \ +[[nodiscard]] inline bool name(const From &Val) { \ + return true; \ +} \ +template <typename ...To, typename From> \ +[[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<A>(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<A>(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<A>(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<A>((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<A>(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<B>(value); + (void)b6; +} + +namespace llvm { +void testIsaInLLVM(A& value) { + bool b7 = isa<A>(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<A>(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 <typename T> +void testIsaTemplateIgnore(T* value) { + bool b9 = llvm::isa<A>(value); + (void)b9; +} +template void testIsaTemplateIgnore<A>(A *value); + +template <typename T> +struct testIsaTemplateIgnoreStruct { + void method(T* value) { + bool b10 = llvm::isa<A>(value); + (void)b10; + } +}; + +void call(testIsaTemplateIgnoreStruct<A> s, A *a) { + s.method(a); +} + +template <typename T> +void testIsaTemplateTrigger1(T* value) { + bool b11 = llvm::isa<T>(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 <typename T> +void testIsaTemplateTrigger2(A* value, T other) { + bool b12 = llvm::isa<A>(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<A>(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<A>(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<int, A>(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<int, A, B>(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 <typename T> +void testIsaTemplateMulti(const T* value) { + bool b17 = llvm::isa<A, T>(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 <typename T> +void testIsaImplicitlyLLVMUnresolve(T* value) { + bool b18 = isa<T>(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 + + +ISA_FUNCTION(isa) + +void testIsaNonLLVM(A& value) { + bool b19 = isa<A>(value); + (void)b19; +... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/191081 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
