clang-tidy: modernize-use-using uses AST and now supports struct defintions and multiple types in a typedef

[poelmanc]

Summary

In D67460#1737529, @aaron.ballman wrote:

I'm a bit worried that this manual parsing technique will need fixing again in the future

That concern plus prior comments and help from clang -ast-dump led me to develop this new approach based entirely on the AST that completely eliminates manual parsing. It now handles typedefs 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 TypedefDecls 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.

This patch fully replaces D67460 though it builds on its extended set of tests. All those tests pass, plus several that previously remained as typedef are now modernized to use using.

[aaron.ballman]

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

30

It's unfortunate that we can't restrict this matcher more -- there tend to be a lot of struct declarations, so I am a bit worried about the performance of this matching on so many of them. At a minimum, I think you can restrict it to non-implicit structs here and simplify the code in check() a bit.

54–56

warn -> Warn
Add a full stop to the end of the comment.

56

Is there a purpose to the Diag local variable, or can it just be removed?

84

Should there be a newline here, to avoid putting all the using declarations on the same line?

[poelmanc]

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

30

Done, thanks!

If there's a way to match only CXXRecordDecls that are immediately followed by a TypedefDecl, that would cut down the matches just to the ones we need. That said, at least the amount of work done on each check() call for the non-implicit CXXRecordDecls is minimal: it just stores its SourceRange and returns.

84

Done. Updated test cases and documentation to reflect this.

[poelmanc]

If there's a way to match only CXXRecordDecls that are immediately followed by a TypedefDecl...

Alternatively, within check() when we get the TypedefDecl, is there any way to navigate up the AST to find its immediately preceding sibling node in the AST and check whether it's a CXXRecordDecl? If so we could eliminate Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this); altogether. I didn't see a way to do that though.

[poelmanc]

Any further feedback or thoughts on this?

[poelmanc]

Update patch to rebase on latest: Changed SourceLocation::contains to SourceLocation::fullyContains, and removed new SourceLocation comparison operators since coincidentally @sammccall added them just days ago.

[aaron.ballman]

LGTM

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 LastCxxDeclRange;
+  std::string FirstTypedefType;
+  std::string FirstTypedefName;
 
 public:
   UseUsingCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
     Options.store(Opts, "IgnoreMacros", IgnoreMacros);
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USE_USING_H

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

UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IgnoreMacros(Options.getLocalOrGlobal("IgnoreMacros", true)) {}
 
 void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
     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;
-    }
-  }
-
-  // Sanity check against weird macro cases.
-  return FoundTypedef;
+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;
   }
 
-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));
+  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
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`performance-no-automatic-move
   Finds local variables that cannot be automatically moved due to constness.
 
 - New :doc:`performance-trivially-destructible
   <clang-tidy/checks/performance-trivially-destructible>` check.
 
   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.
 
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
 - The 'objc-avoid-spinlock' check was renamed to :doc:`darwin-avoid-spinlock
   <clang-tidy/checks/darwin-avoid-spinlock>`

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

 .. title:: clang-tidy - modernize-use-using
 
 modernize-use-using
 ===================
 
 The check converts the usage of ``typedef`` with ``using`` keyword.
 
 Before:
 
 .. code-block:: c++
 
   typedef int variable;
 
   class Class{};
   typedef void (Class::* MyPtrType)() const;
 
+  typedef struct { int a; } R_t, *R_p;
+
 After:
 
 .. code-block:: c++
 
   using variable = int;
 
   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
 -------
 
 .. option:: IgnoreMacros
 
    If set to non-zero, the check will not give warnings inside macros. Default
    is `1`.

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

 // CHECK-FIXES: using another = Test<my_class *>;
 
 typedef int* PInt;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using PInt = int *;
 
 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
 
 CODE;
 // CHECK-FIXES: #define CODE typedef int INT
 // CHECK-FIXES: CODE;
 
 struct Foo;

 // CHECK-FIXES: using Baw = struct Unknown2;
 
 int typedef Bax;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using Bax = int;
 
 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 {};
   typedef my_cclass FuncType;
 // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using FuncType = my_cclass;
 }
 
class TwoArgTemplate {
   // CHECK-FIXES: using self = TwoArgTemplate<T1, T2>;
 };
 
 template <bool B, typename T>
 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'
 // CHECK-FIXES: using S3_t = S<(0 > 0 && (3 > 1) && (1 < 1)), int>;
 
 template <bool B>
 struct Q {};
 
 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'
 // CHECK-FIXES: using Q2_t = Q<b[0 < 0]>;
 
 struct T {
   constexpr T(bool) {}
 
   static constexpr bool b = true;
 };
 
 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'
 // CHECK-FIXES: using Q3_t = Q<T{0 < 0}.b>;
 
 typedef TwoArgTemplate<TwoArgTemplate<int, Q<T{0 < 0}.b> >, S<(0 < 0), Q<b[0 < 0]> > > Nested_t;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using Nested_t = TwoArgTemplate<TwoArgTemplate<int, Q<T{0 < 0}.b> >, S<(0 < 0), Q<b[0 < 0]> > >;
 
 template <typename... Args>
 class Variadic {};
 
 typedef Variadic<Variadic<int, bool, Q<T{0 < 0}.b> >, S<(0 < 0), Variadic<Q<b[0 < 0]> > > > Variadic_t;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: 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]> > > >
 
 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

public:
   bool operator==(const SourceRange &X) const {
     return B == X.B && E == X.E;
   }
 
   bool operator!=(const SourceRange &X) const {
     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;
 };
 
 /// Represents a character-granular source range.
 ///
 /// The underlying SourceRange can either specify the starting/ending character