https://github.com/voyager-jhk updated https://github.com/llvm/llvm-project/pull/198085
>From f5c1ffe4e588460b07e2a919220c6fa44523fe87 Mon Sep 17 00:00:00 2001 From: voyager-jhk <[email protected]> Date: Sat, 16 May 2026 20:50:59 +0800 Subject: [PATCH] [clang-tidy] Fix false positive in misc-redundant-expression with type aliases The `misc-redundant-expression` check previously flagged expressions as redundant if their underlying `DeclRefExpr` pointed to the same declaration. This caused false positives when comparing identical values accessed through distinct type aliases (sugared types). This patch uses `printPretty` to compare the source-level spellings of the expressions, ensuring nested name specifiers and explicit template arguments are correctly differentiated. Fixes #145415 --- .../misc/RedundantExpressionCheck.cpp | 348 +++++++++++++++++- clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../redundant-expression-type-aliases.cpp | 29 ++ 3 files changed, 379 insertions(+), 3 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-type-aliases.cpp diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp index 9db297990b274..28789b1ed8a8a 100644 --- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp +++ b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp @@ -24,6 +24,7 @@ #include <cstdint> #include <optional> #include <string> +#include <tuple> using namespace clang::ast_matchers; using namespace clang::tidy::matchers; @@ -44,6 +45,327 @@ static bool incrementWithoutOverflow(const APSInt &Value, APSInt &Result) { return Value < Result; } +static bool areEquivalentExpr(const Expr *Left, const Expr *Right); + +static bool areEquivalentType(QualType Left, QualType Right); +static bool areEquivalentNestedNameSpecifiers(NestedNameSpecifier Left, + NestedNameSpecifier Right); +static bool areEquivalentNestedNameSpecifierLocs(NestedNameSpecifierLoc Left, + NestedNameSpecifierLoc Right); +static bool areEquivalentTemplateNames(TemplateName Left, TemplateName Right); +static bool areEquivalentTemplateArguments(const TemplateArgument &Left, + const TemplateArgument &Right); + +static bool areSameDecl(const Decl *Left, const Decl *Right) { + if (!Left || !Right) + return !Left && !Right; + return Left->getCanonicalDecl() == Right->getCanonicalDecl(); +} + +static bool areEquivalentTemplateNames(TemplateName Left, TemplateName Right) { + if (Left.getKind() != Right.getKind()) + return false; + + NestedNameSpecifier LeftQualifier; + bool LeftHasTemplateKeyword = false; + std::tie(LeftQualifier, LeftHasTemplateKeyword) = + Left.getQualifierAndTemplateKeyword(); + NestedNameSpecifier RightQualifier; + bool RightHasTemplateKeyword = false; + std::tie(RightQualifier, RightHasTemplateKeyword) = + Right.getQualifierAndTemplateKeyword(); + if (LeftHasTemplateKeyword != RightHasTemplateKeyword || + !areEquivalentNestedNameSpecifiers(LeftQualifier, RightQualifier)) + return false; + + switch (Left.getKind()) { + case TemplateName::Template: + case TemplateName::UsingTemplate: + case TemplateName::QualifiedTemplate: + return areSameDecl(Left.getAsTemplateDecl(/*IgnoreDeduced=*/false), + Right.getAsTemplateDecl(/*IgnoreDeduced=*/false)) && + areSameDecl(Left.getAsUsingShadowDecl(), + Right.getAsUsingShadowDecl()); + case TemplateName::DependentTemplate: { + const auto *LeftDependent = Left.getAsDependentTemplateName(); + const auto *RightDependent = Right.getAsDependentTemplateName(); + return LeftDependent->getName() == RightDependent->getName(); + } + case TemplateName::SubstTemplateTemplateParm: { + const auto *LeftSubst = Left.getAsSubstTemplateTemplateParm(); + const auto *RightSubst = Right.getAsSubstTemplateTemplateParm(); + return LeftSubst->getAssociatedDecl() == RightSubst->getAssociatedDecl() && + LeftSubst->getIndex() == RightSubst->getIndex() && + LeftSubst->getPackIndex() == RightSubst->getPackIndex() && + LeftSubst->getFinal() == RightSubst->getFinal() && + areEquivalentTemplateNames(LeftSubst->getReplacement(), + RightSubst->getReplacement()); + } + case TemplateName::SubstTemplateTemplateParmPack: + case TemplateName::OverloadedTemplate: + case TemplateName::AssumedTemplate: + case TemplateName::DeducedTemplate: + return Left == Right; + } + llvm_unreachable("Unhandled TemplateName kind"); +} + +static bool areEquivalentTemplateArguments(const TemplateArgument &Left, + const TemplateArgument &Right) { + if (Left.getKind() != Right.getKind()) + return false; + + switch (Left.getKind()) { + case TemplateArgument::Null: + return true; + case TemplateArgument::Type: + return areEquivalentType(Left.getAsType(), Right.getAsType()); + case TemplateArgument::Declaration: + return areSameDecl(Left.getAsDecl(), Right.getAsDecl()) && + areEquivalentType(Left.getParamTypeForDecl(), + Right.getParamTypeForDecl()); + case TemplateArgument::NullPtr: + return areEquivalentType(Left.getNullPtrType(), Right.getNullPtrType()); + case TemplateArgument::Integral: + return Left.getAsIntegral() == Right.getAsIntegral() && + areEquivalentType(Left.getIntegralType(), Right.getIntegralType()); + case TemplateArgument::StructuralValue: + return Left.structurallyEquals(Right); + case TemplateArgument::Template: + return areEquivalentTemplateNames(Left.getAsTemplate(), + Right.getAsTemplate()); + case TemplateArgument::TemplateExpansion: + return Left.getNumTemplateExpansions() == + Right.getNumTemplateExpansions() && + areEquivalentTemplateNames(Left.getAsTemplateOrTemplatePattern(), + Right.getAsTemplateOrTemplatePattern()); + case TemplateArgument::Expression: + return areEquivalentExpr(Left.getAsExpr(), Right.getAsExpr()); + case TemplateArgument::Pack: { + ArrayRef<TemplateArgument> LeftPack = Left.pack_elements(); + ArrayRef<TemplateArgument> RightPack = Right.pack_elements(); + if (LeftPack.size() != RightPack.size()) + return false; + for (unsigned I = 0, E = LeftPack.size(); I != E; ++I) + if (!areEquivalentTemplateArguments(LeftPack[I], RightPack[I])) + return false; + return true; + } + } + llvm_unreachable("Unhandled TemplateArgument kind"); +} + +static bool +areEquivalentTemplateArgumentLocs(const TemplateArgumentLoc &Left, + const TemplateArgumentLoc &Right) { + if (Left.getArgument().getKind() != Right.getArgument().getKind()) + return false; + + switch (Left.getArgument().getKind()) { + case TemplateArgument::Type: { + const TypeSourceInfo *LeftInfo = Left.getTypeSourceInfo(); + const TypeSourceInfo *RightInfo = Right.getTypeSourceInfo(); + if (!LeftInfo || !RightInfo) + return LeftInfo == RightInfo && + areEquivalentTemplateArguments(Left.getArgument(), + Right.getArgument()); + return areEquivalentType(LeftInfo->getTypeLoc().getType(), + RightInfo->getTypeLoc().getType()); + } + case TemplateArgument::Expression: + return areEquivalentExpr(Left.getSourceExpression(), + Right.getSourceExpression()); + case TemplateArgument::Declaration: + if (!Left.getLocInfo().isTrivial() || !Right.getLocInfo().isTrivial()) + return areEquivalentExpr(Left.getSourceDeclExpression(), + Right.getSourceDeclExpression()); + return areEquivalentTemplateArguments(Left.getArgument(), + Right.getArgument()); + case TemplateArgument::NullPtr: + if (!Left.getLocInfo().isTrivial() || !Right.getLocInfo().isTrivial()) + return areEquivalentExpr(Left.getSourceNullPtrExpression(), + Right.getSourceNullPtrExpression()); + return areEquivalentTemplateArguments(Left.getArgument(), + Right.getArgument()); + case TemplateArgument::Integral: + if (!Left.getLocInfo().isTrivial() || !Right.getLocInfo().isTrivial()) + return areEquivalentExpr(Left.getSourceIntegralExpression(), + Right.getSourceIntegralExpression()); + return areEquivalentTemplateArguments(Left.getArgument(), + Right.getArgument()); + case TemplateArgument::StructuralValue: + if (!Left.getLocInfo().isTrivial() || !Right.getLocInfo().isTrivial()) + return areEquivalentExpr(Left.getSourceStructuralValueExpression(), + Right.getSourceStructuralValueExpression()); + return areEquivalentTemplateArguments(Left.getArgument(), + Right.getArgument()); + case TemplateArgument::Template: + case TemplateArgument::TemplateExpansion: + return areEquivalentNestedNameSpecifierLocs( + Left.getTemplateQualifierLoc(), + Right.getTemplateQualifierLoc()) && + areEquivalentTemplateArguments(Left.getArgument(), + Right.getArgument()); + case TemplateArgument::Null: + case TemplateArgument::Pack: + return areEquivalentTemplateArguments(Left.getArgument(), + Right.getArgument()); + } + llvm_unreachable("Unhandled TemplateArgument kind"); +} + +static bool areEquivalentNestedNameSpecifiers(NestedNameSpecifier Left, + NestedNameSpecifier Right) { + if (!Left || !Right) + return !Left && !Right; + if (Left.getKind() != Right.getKind()) + return false; + + switch (Left.getKind()) { + case NestedNameSpecifier::Kind::Null: + case NestedNameSpecifier::Kind::Global: + return true; + case NestedNameSpecifier::Kind::Namespace: { + NamespaceAndPrefix LeftNamespace = Left.getAsNamespaceAndPrefix(); + NamespaceAndPrefix RightNamespace = Right.getAsNamespaceAndPrefix(); + return LeftNamespace.Namespace == RightNamespace.Namespace && + areEquivalentNestedNameSpecifiers(LeftNamespace.Prefix, + RightNamespace.Prefix); + } + case NestedNameSpecifier::Kind::Type: + return areEquivalentType(QualType(Left.getAsType(), 0), + QualType(Right.getAsType(), 0)); + case NestedNameSpecifier::Kind::MicrosoftSuper: + return areSameDecl(Left.getAsMicrosoftSuper(), Right.getAsMicrosoftSuper()); + } + llvm_unreachable("Unhandled NestedNameSpecifier kind"); +} + +static bool areEquivalentNestedNameSpecifierLocs(NestedNameSpecifierLoc Left, + NestedNameSpecifierLoc Right) { + if (!Left || !Right) + return !Left && !Right; + + NestedNameSpecifier LeftSpecifier = Left.getNestedNameSpecifier(); + NestedNameSpecifier RightSpecifier = Right.getNestedNameSpecifier(); + if (LeftSpecifier.getKind() != RightSpecifier.getKind()) + return false; + + switch (LeftSpecifier.getKind()) { + case NestedNameSpecifier::Kind::Null: + case NestedNameSpecifier::Kind::Global: + return true; + case NestedNameSpecifier::Kind::Namespace: { + NamespaceAndPrefixLoc LeftNamespace = Left.getAsNamespaceAndPrefix(); + NamespaceAndPrefixLoc RightNamespace = Right.getAsNamespaceAndPrefix(); + return LeftNamespace.Namespace == RightNamespace.Namespace && + areEquivalentNestedNameSpecifierLocs(LeftNamespace.Prefix, + RightNamespace.Prefix); + } + case NestedNameSpecifier::Kind::Type: { + TypeLoc LeftLoc = Left.getAsTypeLoc(); + TypeLoc RightLoc = Right.getAsTypeLoc(); + if (LeftLoc && RightLoc) + return areEquivalentType(LeftLoc.getType(), RightLoc.getType()); + return !LeftLoc && !RightLoc && + areEquivalentType(QualType(LeftSpecifier.getAsType(), 0), + QualType(RightSpecifier.getAsType(), 0)); + } + case NestedNameSpecifier::Kind::MicrosoftSuper: + return areSameDecl(LeftSpecifier.getAsMicrosoftSuper(), + RightSpecifier.getAsMicrosoftSuper()); + } + llvm_unreachable("Unhandled NestedNameSpecifier kind"); +} + +static bool areEquivalentType(QualType Left, QualType Right) { + if (Left.isNull() || Right.isNull()) + return Left.isNull() && Right.isNull(); + if (Left.getLocalCVRQualifiers() != Right.getLocalCVRQualifiers()) + return false; + + Left = Left.getLocalUnqualifiedType(); + Right = Right.getLocalUnqualifiedType(); + if (Left == Right) + return true; + + const Type *LeftType = Left.getTypePtr(); + const Type *RightType = Right.getTypePtr(); + if (LeftType->getTypeClass() != RightType->getTypeClass()) + return false; + + switch (LeftType->getTypeClass()) { + case Type::Builtin: + return LeftType == RightType; + case Type::Typedef: { + const auto *LeftTypedef = cast<TypedefType>(LeftType); + const auto *RightTypedef = cast<TypedefType>(RightType); + return LeftTypedef->getKeyword() == RightTypedef->getKeyword() && + areSameDecl(LeftTypedef->getDecl(), RightTypedef->getDecl()) && + areEquivalentNestedNameSpecifiers(LeftTypedef->getQualifier(), + RightTypedef->getQualifier()); + } + case Type::Using: { + const auto *LeftUsing = cast<UsingType>(LeftType); + const auto *RightUsing = cast<UsingType>(RightType); + return LeftUsing->getKeyword() == RightUsing->getKeyword() && + areSameDecl(LeftUsing->getDecl(), RightUsing->getDecl()) && + areEquivalentNestedNameSpecifiers(LeftUsing->getQualifier(), + RightUsing->getQualifier()); + } + case Type::TemplateSpecialization: { + const auto *LeftTemplate = cast<TemplateSpecializationType>(LeftType); + const auto *RightTemplate = cast<TemplateSpecializationType>(RightType); + ArrayRef<TemplateArgument> LeftArguments = + LeftTemplate->template_arguments(); + ArrayRef<TemplateArgument> RightArguments = + RightTemplate->template_arguments(); + if (LeftArguments.size() != RightArguments.size()) + return false; + for (unsigned I = 0, E = LeftArguments.size(); I != E; ++I) + if (!areEquivalentTemplateArguments(LeftArguments[I], RightArguments[I])) + return false; + return LeftTemplate->getKeyword() == RightTemplate->getKeyword() && + areEquivalentTemplateNames(LeftTemplate->getTemplateName(), + RightTemplate->getTemplateName()); + } + case Type::Pointer: + return areEquivalentType(cast<PointerType>(LeftType)->getPointeeType(), + cast<PointerType>(RightType)->getPointeeType()); + case Type::LValueReference: + case Type::RValueReference: + return areEquivalentType(cast<ReferenceType>(LeftType)->getPointeeType(), + cast<ReferenceType>(RightType)->getPointeeType()); + case Type::Paren: + return areEquivalentType(cast<ParenType>(LeftType)->getInnerType(), + cast<ParenType>(RightType)->getInnerType()); + case Type::ConstantArray: { + const auto *LeftArray = cast<ConstantArrayType>(LeftType); + const auto *RightArray = cast<ConstantArrayType>(RightType); + return LeftArray->getSize() == RightArray->getSize() && + LeftArray->getSizeModifier() == RightArray->getSizeModifier() && + LeftArray->getIndexTypeCVRQualifiers() == + RightArray->getIndexTypeCVRQualifiers() && + areEquivalentType(LeftArray->getElementType(), + RightArray->getElementType()); + } + case Type::Record: + case Type::Enum: + case Type::InjectedClassName: { + const auto *LeftTag = cast<TagType>(LeftType); + const auto *RightTag = cast<TagType>(RightType); + return LeftTag->getKeyword() == RightTag->getKeyword() && + areSameDecl(LeftTag->getDecl(), RightTag->getDecl()) && + areEquivalentNestedNameSpecifiers(LeftTag->getQualifier(), + RightTag->getQualifier()); + } + default: + if (Left != Left.getCanonicalType() || Right != Right.getCanonicalType()) + return false; + return Left.getCanonicalType() == Right.getCanonicalType(); + } +} + static bool areEquivalentExpr(const Expr *Left, const Expr *Right) { if (!Left || !Right) return !Left && !Right; @@ -97,9 +419,29 @@ static bool areEquivalentExpr(const Expr *Left, const Expr *Right) { return false; return cast<DependentScopeDeclRefExpr>(Left)->getQualifier() == cast<DependentScopeDeclRefExpr>(Right)->getQualifier(); - case Stmt::DeclRefExprClass: - return cast<DeclRefExpr>(Left)->getDecl() == - cast<DeclRefExpr>(Right)->getDecl(); + case Stmt::DeclRefExprClass: { + const auto *L = cast<DeclRefExpr>(Left); + const auto *R = cast<DeclRefExpr>(Right); + + if (L->getDecl() != R->getDecl() || L->getFoundDecl() != R->getFoundDecl()) + return false; + + if (!areEquivalentNestedNameSpecifierLocs(L->getQualifierLoc(), + R->getQualifierLoc())) + return false; + + if (L->hasExplicitTemplateArgs() != R->hasExplicitTemplateArgs()) + return false; + if (L->hasExplicitTemplateArgs()) { + if (L->getNumTemplateArgs() != R->getNumTemplateArgs()) + return false; + for (unsigned I = 0, E = L->getNumTemplateArgs(); I != E; ++I) + if (!areEquivalentTemplateArgumentLocs(L->getTemplateArgs()[I], + R->getTemplateArgs()[I])) + return false; + } + return true; + } case Stmt::MemberExprClass: return cast<MemberExpr>(Left)->getMemberDecl() == cast<MemberExpr>(Right)->getMemberDecl(); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 89fb1684bba7c..c4f9b86bdd87c 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -500,6 +500,11 @@ Changes in existing checks <clang-tidy/checks/misc/multiple-inheritance>` by avoiding false positives when virtual inheritance causes concrete bases to be counted more than once. +- Improved :doc:`misc-redundant-expression + <clang-tidy/checks/misc/redundant-expression>` check to avoid false + positives when comparing expressions that are structurally identical but + use different type aliases. + - Improved :doc:`misc-throw-by-value-catch-by-reference <clang-tidy/checks/misc/throw-by-value-catch-by-reference>` check: diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-type-aliases.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-type-aliases.cpp new file mode 100644 index 0000000000000..5ebbfd518d48c --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/misc/redundant-expression-type-aliases.cpp @@ -0,0 +1,29 @@ +// RUN: %check_clang_tidy -std=c++17-or-later %s misc-redundant-expression %t + +namespace std { +template <class T, int N> struct array {}; +template <class T> struct tuple_size; +template <class T, int N> struct tuple_size<array<T, N>> { + static constexpr int value = N; +}; +template <class T> constexpr int tuple_size_v = tuple_size<T>::value; +} // namespace std + +using MonthArray = std::array<int, 12>; +using ZodiacArray = std::array<int, 12>; + +void test() { + // 1. False positive cases (Should NOT warn): + bool b1 = std::tuple_size<MonthArray>::value == std::tuple_size<ZodiacArray>::value; + bool b2 = std::tuple_size_v<MonthArray> == std::tuple_size_v<ZodiacArray>; + + // 2. True positive cases (Must warn): + bool b3 = std::tuple_size<MonthArray>::value == std::tuple_size<MonthArray>::value; + // CHECK-MESSAGES: :[[@LINE-1]]:48: warning: both sides of operator are equivalent [misc-redundant-expression] + + // 3. Multiline true positive case: + bool b4 = std::tuple_size_v< + MonthArray> == + std::tuple_size_v<MonthArray>; + // CHECK-MESSAGES: :[[@LINE-2]]:29: warning: both sides of operator are equivalent [misc-redundant-expression] +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
