[clang-tidy] Fix some false negatives in readability-redundant-typename

[localspook]

#169166 reported some false positives in this check. They stemmed from the fact that it assumed TypedefTypeLocs and TagTypeLocs couldn't be dependent. Turns out, this incorrect assumption leads to some false negatives as well: https://godbolt.org/z/K6EvfrE6a. This PR fixes those. (I restructured the code because the "straightforward" fix would have introduced a bit too much duplication).

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Victor Chernyakin (localspook)

Changes

#169166 reported some false positives in this check. They stemmed from the fact that it assumed TypedefTypeLocs and TagTypeLocs couldn't be dependent. Turns out, this incorrect assumption leads to some false negatives as well: https://godbolt.org/z/K6EvfrE6a. This PR fixes those. (I restructured the code a bit because the "straightforward" fix would have introduced a bit too much duplication).

---

Full diff: https://github.com/llvm/llvm-project/pull/171947.diff

2 Files Affected:

• (modified) clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp (+30-34)
• (modified) clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp (+19-3)

diff --git a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
index 5f2519ce9d5c3..0816625b1937d 100644
--- a/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/RedundantTypenameCheck.cpp
@@ -18,9 +18,9 @@ using namespace clang::ast_matchers;
 namespace clang::tidy::readability {
 
 void RedundantTypenameCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(typeLoc(unless(hasAncestor(decl(isInstantiated()))))
-                         .bind("nonDependentTypeLoc"),
-                     this);
+  Finder->addMatcher(
+      typeLoc(unless(hasAncestor(decl(isInstantiated())))).bind("typeLoc"),
+      this);
 
   if (!getLangOpts().CPlusPlus20)
     return;
@@ -44,38 +44,34 @@ void RedundantTypenameCheck::registerMatchers(MatchFinder *Finder) {
 }
 
 void RedundantTypenameCheck::check(const MatchFinder::MatchResult &Result) {
+  const TypeLoc TL = [&] {
+    if (const auto *TL = Result.Nodes.getNodeAs<TypeLoc>("typeLoc"))
+      return TL->getType()->isDependentType() ? TypeLoc() : *TL;
+
+    auto TL = *Result.Nodes.getNodeAs<TypeLoc>("dependentTypeLoc");
+    while (const TypeLoc Next = TL.getNextTypeLoc())
+      TL = Next;
+    return TL;
+  }();
+
+  if (TL.isNull())
+    return;
+
   const SourceLocation ElaboratedKeywordLoc = [&] {
-    if (const auto *NonDependentTypeLoc =
-            Result.Nodes.getNodeAs<TypeLoc>("nonDependentTypeLoc")) {
-      if (NonDependentTypeLoc->getType()->isDependentType())
-        return SourceLocation();
-
-      if (const auto TL = NonDependentTypeLoc->getAs<TypedefTypeLoc>())
-        return TL.getElaboratedKeywordLoc();
-
-      if (const auto TL = NonDependentTypeLoc->getAs<TagTypeLoc>())
-        return TL.getElaboratedKeywordLoc();
-
-      if (const auto TL = NonDependentTypeLoc
-                              ->getAs<DeducedTemplateSpecializationTypeLoc>())
-        return TL.getElaboratedKeywordLoc();
-
-      if (const auto TL =
-              NonDependentTypeLoc->getAs<TemplateSpecializationTypeLoc>())
-        return TL.getElaboratedKeywordLoc();
-    } else {
-      TypeLoc InnermostTypeLoc =
-          *Result.Nodes.getNodeAs<TypeLoc>("dependentTypeLoc");
-      while (const TypeLoc Next = InnermostTypeLoc.getNextTypeLoc())
-        InnermostTypeLoc = Next;
-
-      if (const auto TL = InnermostTypeLoc.getAs<DependentNameTypeLoc>())
-        return TL.getElaboratedKeywordLoc();
-
-      if (const auto TL =
-              InnermostTypeLoc.getAs<TemplateSpecializationTypeLoc>())
-        return TL.getElaboratedKeywordLoc();
-    }
+    if (const auto CastTL = TL.getAs<TypedefTypeLoc>())
+      return CastTL.getElaboratedKeywordLoc();
+
+    if (const auto CastTL = TL.getAs<TagTypeLoc>())
+      return CastTL.getElaboratedKeywordLoc();
+
+    if (const auto CastTL = TL.getAs<DeducedTemplateSpecializationTypeLoc>())
+      return CastTL.getElaboratedKeywordLoc();
+
+    if (const auto CastTL = TL.getAs<TemplateSpecializationTypeLoc>())
+      return CastTL.getElaboratedKeywordLoc();
+
+    if (const auto CastTL = TL.getAs<DependentNameTypeLoc>())
+      return CastTL.getElaboratedKeywordLoc();
 
     return SourceLocation();
   }();
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp
index e8fcd9bcd5731..96bd7b6412724 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/redundant-typename.cpp
@@ -271,13 +271,21 @@ WHOLE_DECLARATION_IN_MACRO;
 template <typename T> struct Wrapper {};
 template <typename T>
 struct ClassWrapper {
-    using R = T;
-    Wrapper<R> f();
+  using R = T;
+  Wrapper<R> f();
+  R g();
 };
 
 template <typename T>
 Wrapper<typename ClassWrapper<T>::R> ClassWrapper<T>::f() {
-    return {};
+  return {};
+}
+
+template <typename T>
+typename ClassWrapper<T>::R ClassWrapper<T>::g() {
+// CHECK-MESSAGES-20: :[[@LINE-1]]:1: warning: redundant 'typename' [readability-redundant-typename]
+// CHECK-FIXES-20: ClassWrapper<T>::R ClassWrapper<T>::g() {
+  return {};
 }
 
 template <typename T> struct StructWrapper {};
@@ -285,9 +293,17 @@ template <typename T>
 class ClassWithNestedStruct {
   struct Nested {};
   StructWrapper<Nested> f();
+  Nested g();
 };
 
 template <typename T>
 StructWrapper<typename ClassWithNestedStruct<T>::Nested> ClassWithNestedStruct<T>::f() {
   return {};
 }
+
+template <typename T>
+typename ClassWithNestedStruct<T>::Nested ClassWithNestedStruct<T>::g() {
+// CHECK-MESSAGES-20: :[[@LINE-1]]:1: warning: redundant 'typename' [readability-redundant-typename]
+// CHECK-FIXES-20: ClassWithNestedStruct<T>::Nested ClassWithNestedStruct<T>::g() {
+  return {};
+}

[zeyi2]

Thanks for the fix