[clang-tidy] modernize-use-using uses AST and now supports struct def…

…intions and multiple types in a typedef

Summary:
It now handles `typedef`s that include comma-separated multiple types, and handles embedded struct definitions, which previously could not be automatically converted.

For example, with this patch `modernize-use-using` now can convert:

typedef struct { int a; } R_t, *R_p;

to:

using R_t = struct { int a; };
using R_p = R_t*;

`-ast-dump` showed that the `CXXRecordDecl` definitions and multiple `TypedefDecl`s come consecutively in the tree, so `check()` stores information between calls to determine when it is receiving a second or additional `TypedefDecl` within a single `typedef`, or when the current `TypedefDecl` refers to an embedded `CXXRecordDecl` like a `struct`.

Reviewers: alexfh, aaron.ballman

Patch by: poelmanc

Subscribers: riccibruno, sammccall, cfe-commits, aaron.ballman

Tags: clang-tools-extra, clang

Differential Revision: https://reviews.llvm.org/D70270

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

@@ -25,107 +25,88 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
     return;
   Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
                      this);
+  // This matcher used to find structs defined in source code within typedefs.
+  // They appear in the AST just *prior* to the typedefs.
+  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
 }
 
-// Checks if 'typedef' keyword can be removed - we do it only if
-// it is the only declaration in a declaration chain.
-static bool CheckRemoval(SourceManager &SM, SourceLocation StartLoc,
-                         ASTContext &Context) {
-  assert(StartLoc.isFileID() && "StartLoc must not be in a macro");
-  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(StartLoc);
-  StringRef File = SM.getBufferData(LocInfo.first);
-  const char *TokenBegin = File.data() + LocInfo.second;
-  Lexer DeclLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
-                  File.begin(), TokenBegin, File.end());
-
-  Token Tok;
-  int NestingLevel = 0; // Parens, braces, and square brackets
-  int AngleBracketLevel = 0;
-  bool FoundTypedef = false;
-
-  while (!DeclLexer.LexFromRawLexer(Tok) && !Tok.is(tok::semi)) {
-    switch (Tok.getKind()) {
-    case tok::l_brace:
-      if (NestingLevel == 0 && AngleBracketLevel == 0) {
-        // At top level, this might be the `typedef struct {...} T;` case.
-        // Inside parens, square brackets, or angle brackets it's not.
-        return false;
-      }
-      ++NestingLevel;
-      break;
-    case tok::l_paren:
-    case tok::l_square:
-      ++NestingLevel;
-      break;
-    case tok::r_brace:
-    case tok::r_paren:
-    case tok::r_square:
-      --NestingLevel;
-      break;
-    case tok::less:
-      // If not nested in paren/brace/square bracket, treat as opening angle bracket.
-      if (NestingLevel == 0)
-        ++AngleBracketLevel;
-      break;
-    case tok::greater:
-      // Per C++ 17 Draft N4659, Section 17.2/3
-      //   https://timsong-cpp.github.io/cppwp/n4659/temp.names#3:
-      // "When parsing a template-argument-list, the first non-nested > is
-      // taken as the ending delimiter rather than a greater-than operator."
-      // If not nested in paren/brace/square bracket, treat as closing angle bracket.
-      if (NestingLevel == 0)
-        --AngleBracketLevel;
-      break;
-    case tok::comma:
-      if (NestingLevel == 0 && AngleBracketLevel == 0) {
-        // If there is a non-nested comma we have two or more declarations in this chain.
-        return false;
-      }
-      break;
-    case tok::raw_identifier:
-      if (Tok.getRawIdentifier() == "typedef") {
-        FoundTypedef = true;
-      }
-      break;
-    default:
-      break;
-    }
+void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
+  // 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 *MatchedCxxRecordDecl =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("struct");
+  if (MatchedCxxRecordDecl) {
+    LastCxxDeclRange = MatchedCxxRecordDecl->getSourceRange();
+    return;
   }
 
-  // Sanity check against weird macro cases.
-  return FoundTypedef;
-}
-
-void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>("typedef");
   if (MatchedDecl->getLocation().isInvalid())
     return;
 
-  auto &Context = *Result.Context;
-  auto &SM = *Result.SourceManager;
-
   SourceLocation StartLoc = MatchedDecl->getBeginLoc();
 
   if (StartLoc.isMacroID() && IgnoreMacros)
     return;
 
-  auto Diag = diag(StartLoc, "use 'using' instead of 'typedef'");
+  static const char *UseUsingWarning = "use 'using' instead of 'typedef'";
 
-  // do not fix if there is macro or array
-  if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID())
+  // Warn at StartLoc but do not fix if there is macro or array.
+  if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
+    diag(StartLoc, UseUsingWarning);
     return;
+  }
 
-  if (CheckRemoval(SM, StartLoc, Context)) {
-    auto printPolicy = PrintingPolicy(getLangOpts());
-    printPolicy.SuppressScope = true;
-    printPolicy.ConstantArraySizeAsWritten = true;
-    printPolicy.UseVoidForZeroParams = false;
-
-    Diag << FixItHint::CreateReplacement(
-        MatchedDecl->getSourceRange(),
-        "using " + MatchedDecl->getNameAsString() + " = " +
-            MatchedDecl->getUnderlyingType().getAsString(printPolicy));
+  auto printPolicy = PrintingPolicy(getLangOpts());
+  printPolicy.SuppressScope = true;
+  printPolicy.ConstantArraySizeAsWritten = true;
+  printPolicy.UseVoidForZeroParams = false;
+
+  std::string Type = MatchedDecl->getUnderlyingType().getAsString(printPolicy);
+  std::string Name = MatchedDecl->getNameAsString();
+  SourceRange ReplaceRange = MatchedDecl->getSourceRange();
+
+  // typedefs with multiple comma-separated definitions produce multiple
+  // consecutive TypedefDecl nodes whose SourceRanges overlap. Each range starts
+  // at the "typedef" and then continues *across* previous definitions through
+  // the end of the current TypedefDecl definition.
+  std::string Using = "using ";
+  if (ReplaceRange.getBegin().isMacroID() ||
+      ReplaceRange.getBegin() >= LastReplacementEnd) {
+    // This is the first (and possibly the only) TypedefDecl in a typedef. Save
+    // Type and Name in case we find subsequent TypedefDecl's in this typedef.
+    FirstTypedefType = Type;
+    FirstTypedefName = Name;
+  } else {
+    // This is additional TypedefDecl in a comma-separated typedef declaration.
+    // Start replacement *after* prior replacement and separate with semicolon.
+    ReplaceRange.setBegin(LastReplacementEnd);
+    Using = ";\nusing ";
+
+    // If this additional TypedefDecl's Type starts with the first TypedefDecl's
+    // type, make this using statement refer back to the first type, e.g. make
+    // "typedef int Foo, *Foo_p;" -> "using Foo = int;\nusing Foo_p = Foo*;"
+    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 struct/class declaration, extract its full text.
+  if (LastCxxDeclRange.isValid() && ReplaceRange.fullyContains(LastCxxDeclRange)) {
+    bool Invalid;
+    Type =
+        Lexer::getSourceText(CharSourceRange::getTokenRange(LastCxxDeclRange),
+                             *Result.SourceManager, getLangOpts(), &Invalid);
+    if (Invalid)
+      return;
+  }
+
+  std::string Replacement = Using + Name + " = " + Type;
+  Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
 }
 
 } // namespace modernize

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

@@ -22,6 +22,10 @@ namespace modernize {
 class UseUsingCheck : public ClangTidyCheck {
 
   const bool IgnoreMacros;
+  SourceLocation LastReplacementEnd;
+  SourceRange LastCxxDeclRange;
+  std::string FirstTypedefType;
+  std::string FirstTypedefName;
 
 public:
   UseUsingCheck(StringRef Name, ClangTidyContext *Context);

clang-tools-extra/docs/ReleaseNotes.rst

@@ -166,6 +166,10 @@ Improvements to clang-tidy
   Finds types that could be made trivially-destructible by removing out-of-line
   defaulted destructor declarations.
 
+- The :doc:`modernize-use-using
+  <clang-tidy/checks/modernize-use-using>` check now converts typedefs containing
+  struct definitions and multiple comma-separated types.
+
 - Improved :doc:`bugprone-posix-return
   <clang-tidy/checks/bugprone-posix-return>` check.
 

clang-tools-extra/docs/clang-tidy/checks/modernize-use-using.rst

@@ -14,6 +14,8 @@ Before:
   class Class{};
   typedef void (Class::* MyPtrType)() const;
 
+  typedef struct { int a; } R_t, *R_p;
+
 After:
 
 .. code-block:: c++
@@ -23,6 +25,9 @@ After:
   class Class{};
   using MyPtrType = void (Class::*)() const;
 
+  using R_t = struct { int a; };
+  using R_p = R_t*;
+
 This check requires using C++11 or higher to run.
 
 Options

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

@@ -84,7 +84,11 @@ typedef int* PInt;
 
 typedef int bla1, bla2, bla3;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef int bla1, bla2, bla3;
+// CHECK-MESSAGES: :[[@LINE-2]]:17: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-3]]:23: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using bla1 = int;
+// CHECK-FIXES-NEXT: using bla2 = int;
+// CHECK-FIXES-NEXT: using bla3 = int;
 
 #define CODE typedef int INT
 
@@ -136,16 +140,16 @@ int typedef Bax;
 
 typedef struct Q1 { int a; } S1;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef struct Q1 { int a; } S1;
+// CHECK-FIXES: using S1 = struct Q1 { int a; };
 typedef struct { int b; } S2;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef struct { int b; } S2;
+// CHECK-FIXES: using S2 = struct { int b; };
 struct Q2 { int c; } typedef S3;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: struct Q2 { int c; } typedef S3;
+// CHECK-FIXES: using S3 = struct Q2 { int c; };
 struct { int d; } typedef S4;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: struct { int d; } typedef S4;
+// CHECK-FIXES: using S4 = struct { int d; };
 
 namespace my_space {
   class my_cclass {};
@@ -196,11 +200,15 @@ struct S {};
 
 typedef S<(0 > 0), int> S_t, *S_p;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef S<(0 > 0), int> S_t, *S_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:28: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using S_t = S<(0 > 0), int>;
+// CHECK-FIXES-NEXT: using S_p = S_t*;
 
 typedef S<(0 < 0), int> S2_t, *S2_p;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef S<(0 < 0), int> S2_t, *S2_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:29: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using S2_t = S<(0 < 0), int>;
+// CHECK-FIXES-NEXT: using S2_p = S2_t*;
 
 typedef S<(0 > 0 && (3 > 1) && (1 < 1)), int> S3_t;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -213,7 +221,9 @@ constexpr bool b[1] = {true};
 
 typedef Q<b[0 < 0]> Q_t, *Q_p;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef Q<b[0 < 0]> Q_t, *Q_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Q_t = Q<b[0 < 0]>;
+// CHECK-FIXES-NEXT: using Q_p = Q_t*;
 
 typedef Q<b[0 < 0]> Q2_t;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -227,7 +237,9 @@ struct T {
 
 typedef Q<T{0 < 0}.b> Q3_t, *Q3_p;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef Q<T{0 < 0}.b> Q3_t, *Q3_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:27: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Q3_t = Q<T{0 < 0}.b>;
+// CHECK-FIXES-NEXT: using Q3_p = Q3_t*;
 
 typedef Q<T{0 < 0}.b> Q3_t;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -246,4 +258,12 @@ typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0
 
 typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > > Variadic_t, *Variadic_p;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > > Variadic_t, *Variadic_p;
+// CHECK-MESSAGES: :[[@LINE-2]]:103: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using Variadic_t = Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > >;
+// CHECK-FIXES-NEXT: using Variadic_p = Variadic_t*;
+
+typedef struct { int a; } R_t, *R_p;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using R_t = struct { int a; };
+// CHECK-FIXES-NEXT: using R_p = R_t*;

clang/include/clang/Basic/SourceLocation.h

@@ -230,6 +230,11 @@ class SourceRange {
     return B != X.B || E != X.E;
   }
 
+  // Returns true iff other is wholly contained within this range.
+  bool fullyContains(const SourceRange &other) const {
+    return B <= other.B && E >= other.E;
+  }
+
   void print(raw_ostream &OS, const SourceManager &SM) const;
   std::string printToString(const SourceManager &SM) const;
   void dump(const SourceManager &SM) const;