[clang-tidy] Fix behavior of `modernize-use-using` with nested structs/unions

[fwolff]

Summary

Fixes issue #50334.

[whisperity]

clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h

26–27

Considering the pointer is just a number that hashes well, is there a reason to use the plain std::map? It is generally discouraged.

clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp

305–308

Just for the sake of testing a new logic, it would be beneficial to add a "more complex" nested example. At least one example where there are two sibling nests, and one where there is an additional level of nesting.

[fwolff]

Thanks for your suggestions @whisperity! I have fixed both of them now.

[whisperity]

hasParent() is the direct upwards traversal, right? I remember some discussion about matchers like that being potentially costly. But I can't find any description of this, so I guess it's fine, rewriting the matcher to have the opposite logic (decl(hasDescendant(...the-real-matcher-here...)).bind("blah")) would just make everything ugly for no tangible benefit.

clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp

33

(Nit: Once we have multiple uses of an identifier, it's better to hoist it to a definition and reuse it that way, to prevent future typos introducing arcane issues. Generally, a static constexpr llvm::StringLiteral in the TU's global scope, is sufficient for that.)

54–56

(Style: Elid braces.)

130–139

(Nit: if the source ranges are invalid, a default constructed StringRef is returned, which is empty and converts to a default-constructed std::string.)

[fwolff]

Thanks for your comments @whisperity! They should all be fixed now.

[fwolff]

Rebased and ping @whisperity

[fwolff]

Ping @aaron.ballman.

[aaron.ballman]

LGTM aside from a nit.

clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp

39–48

I recall that using hasParent() and hasAncestor() tend to be expensive because they walk up the entire AST to the TU level, and so they can also match in surprising places. However, I can't seem to devise a test case that will fail, so I think this is probably okay.

clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp

306

We don't typically use clang-format comments (especially not in tests; those are almost never formatted anyway). Same below.

clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h

 /// Check finds typedefs and replaces it with usings.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-using.html
 class UseUsingCheck : public ClangTidyCheck {
 
   const bool IgnoreMacros;
   SourceLocation LastReplacementEnd;
-  SourceRange LastTagDeclRange;
+  llvm::DenseMap<const Decl *, SourceRange> LastTagDeclRanges;
+
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
 public:
   UseUsingCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus11;
   }

clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
+static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
+static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
+static constexpr llvm::StringLiteral TypedefName = "typedef";
+
 UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
 
 void UseUsingCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreMacros", IgnoreMacros);
 }
 
 void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
-  Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
-                     this);
-  // This matcher used to find tag declarations in source code within typedefs.
-  // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(tagDecl(unless(isImplicit())).bind("tagdecl"), this);
+  Finder->addMatcher(typedefDecl(unless(isInstantiated()),
+                                 hasParent(decl().bind(ParentDeclName)))
+                         .bind(TypedefName),
+                     this);
+
+  // This matcher is used to find tag declarations in source code within
+  // typedefs. They appear in the AST just *prior* to the typedefs.
+  Finder->addMatcher(
+      tagDecl(
+          anyOf(allOf(unless(anyOf(isImplicit(),
+                                   classTemplateSpecializationDecl())),
+                      hasParent(decl().bind(ParentDeclName))),
+                // We want the parent of the ClassTemplateDecl, not the parent
+                // of the specialization.
+                classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
+                    hasParent(decl().bind(ParentDeclName)))))))
+          .bind(TagDeclName),
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *ParentDecl = Result.Nodes.getNodeAs<Decl>(ParentDeclName);
+  if (!ParentDecl)
+    return;
+
   // Match CXXRecordDecl only to store the range of the last non-implicit full
   // declaration, to later check whether it's within the typdef itself.
-  const auto *MatchedTagDecl = Result.Nodes.getNodeAs<TagDecl>("tagdecl");
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs<TagDecl>(TagDeclName);
   if (MatchedTagDecl) {
-    LastTagDeclRange = MatchedTagDecl->getSourceRange();
+    // It is not sufficient to just track the last TagDecl that we've seen,
+    // because if one struct or union is nested inside another, the last TagDecl
+    // before the typedef will be the nested one (PR#50990). Therefore, we also
+    // keep track of the parent declaration, so that we can look up the last
+    // TagDecl that is a sibling of the typedef in the AST.
+    LastTagDeclRanges[ParentDecl] = MatchedTagDecl->getSourceRange();
     return;
   }
 
-  const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>("typedef");
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
   if (MatchedDecl->getLocation().isInvalid())
     return;
 
   SourceLocation StartLoc = MatchedDecl->getBeginLoc();
 
   if (StartLoc.isMacroID() && IgnoreMacros)
     return;
 
if (Type.size() > FirstTypedefType.size() &&
         Type.substr(0, FirstTypedefType.size()) == FirstTypedefType)
       Type = FirstTypedefName + Type.substr(FirstTypedefType.size() + 1);
   }
   if (!ReplaceRange.getEnd().isMacroID())
     LastReplacementEnd = ReplaceRange.getEnd().getLocWithOffset(Name.size());
 
   auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
 
   // If typedef contains a full tag declaration, extract its full text.
-  if (LastTagDeclRange.isValid() &&
-      ReplaceRange.fullyContains(LastTagDeclRange)) {
-    bool Invalid;
-    Type = std::string(
-        Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
-                             *Result.SourceManager, getLangOpts(), &Invalid));
-    if (Invalid)
+  auto LastTagDeclRange = LastTagDeclRanges.find(ParentDecl);
+  if (LastTagDeclRange != LastTagDeclRanges.end() &&
+      LastTagDeclRange->second.isValid() &&
+      ReplaceRange.fullyContains(LastTagDeclRange->second)) {
+    Type = std::string(Lexer::getSourceText(
+        CharSourceRange::getTokenRange(LastTagDeclRange->second),
+        *Result.SourceManager, getLangOpts()));
+    if (Type.empty())
       return;
   }
 
   std::string Replacement = Using + Name + " = " + Type;
   Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
 }
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp

 };
 
 template <int>
 struct InjectedClassNameWithUnnamedArgument {
   typedef InjectedClassNameWithUnnamedArgument b;
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
   // CHECK-FIXES: using b = InjectedClassNameWithUnnamedArgument;
 };
+
+typedef struct { int a; union { int b; }; } PR50990;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990 = struct { int a; union { int b; }; };
+
+typedef struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; } PR50990_nested;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_nested = struct { struct { int a; struct { struct { int b; } c; int d; } e; } f; int g; };
+
+typedef struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; } PR50990_siblings;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using PR50990_siblings = struct { struct { int a; } b; union { int c; float d; struct { int e; }; }; struct { double f; } g; };