[clang-tools-extra] [clang-tidy] Fix some false negatives in `readability-redundant-typename` (PR #171947)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Victor Chernyakin (localspook)

<details>
<summary>Changes</summary>

#<!-- -->169166 reported some false positives in this check. They stemmed from the fact that it assumed `TypedefTypeLoc`s and `TagTypeLoc`s 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
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 {};
+}

``````````

</details>


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