clang-tidy: modernize-use-using work with multi-argument templates

[poelmanc]

Summary

clang-tidy's modernize-use-using feature is great! But if it finds any commas that are not within parentheses, it won't create a fix. That means it won't change lines like:
typedef std::pair<int, int> Point;
to
using Point = std::pair<int, int>;
or even:
typedef std::map<std::string, Foo> MyMap;
typedef std::vector<int,MyCustomAllocator<int>> MyVector;

This patch allows the fix to apply to lines with commas if they are within parentheses or angle brackets that were not themselves within parentheses. Three tests included, latter due to jonathanmeier's reply.

(Also one additional line auto Diag = diag... was accidentally clang-formatted by my editor but it seemed like a clear improvement so I left it in.)

[jonathanmeier]

With non-type template parameters we can have expressions inside template arguments, including comparison operators like <, <=, > and >=, which lets us write typedefs like this:

template <bool B>
struct S {};

typedef S<(0 < 0)> S_t, *S_p;

Unfortunately, for this example your patch breaks the check in the tok::comma case, which should abort the removal when there are multiple declarations in the declaration chain. It thinks the comma is still part of the template argument, since it expects a matching end template angle bracket for the less than operator which was erroneously interpreted as the start of a template argument.

With the -fix option, clang-tidy produces the following invalid using declaration for the example above:

using S_t = S<(0 < 0)>, *S_p;

[poelmanc]

Wow, thanks for the super-quick testing, feedback and a new test case! I added a slightly enhanced version of your test case to the modernize-use-using.cpp test file and got it passing by treating tok::less and tok::right as AngleBrackets only when ParenLevel == 0. See updated patch above. Holler if you think of any other stressing cases!

P.S. What do we think of actually handling the comma cases? (A) that's probably hard, and (B) as a C++ programmer I don't use them myself, so would we expand:

typedef S<(0 < 0)> S_t, *S_p;

to:

using S_t = S<(0 < 0)>;
using S_p = S_t*;

?

[jonathanmeier]

Unfortunately, only considering parenthesis (l_paren, r_paren) is not sufficient. We need to consider all the nesting tokens, including brackets (l_square, r_square) and braces (l_brace, r_brace), since the Standard says (C++ 17 Draft N4659, Section 17.2/3):

[...] When parsing a template-argument-list, the first non-nested > is taken as the ending delimiter rather than a greater-than operator. [...]

Example with brackets that fails with your updated patch:

template <bool B>
struct S {};

constexpr bool b[1] = { true };

typedef S<b[0 < 0]> S_t, *S_p;

The following is an example with braces inside a template argument:

template <bool B>
struct S {};

struct T {
  constexpr T(bool) {}
  
  static constexpr bool b = true;  
};

typedef S<T{ 0 < 0 }.b> S_t, *S_p;

Note though, that the current code aborts upon encountering braces to avoid removing a typedef struct {...} T; case and therefore, isn't even able to handle a single declaration chain with braces in a template argument, such as typedef S<T{ 0 < 0 }.b> S_t;.

As to handling the comma cases:
(A) It is certainly not straightforward, since typedef declaration chains with multiple declarations are internally split up into separate declarations and the checker is called for each of these declarations individually.
(B) Your expansion would be valid, however, I think it might be easier to implement the expansion to

using S_t = S<(0 < 0)>;
using S_p = S<(0 < 0)>*;

[jonathanmeier]

Also note that your enhanced version of my first example actually works with your initial patch, since the two comparison operators cancel each other out in the counting logic.

[poelmanc]

Thanks for the stressing test cases. I reverted to your original test case with a single >, added a separate one with a single <, and expanded the final case to have a non-balanced number of > and <. I added all your new cases, with variations for non-fixed (multiple typedef) and fixed (single typedef) examples.

To make the braces example pass, we now only abandon the fix when encountering a non-nested open brace.

[jonathanmeier]

Thanks, this looks much better now. I think there's no need to track nesting of parenthesis, brackets and braces separately, so we can collapse ParenLevel, BraceLevel and SquareBracketLevel into a single NestingLevel, which simplifies all the conditions quite a bit.

Also consider adding a few more testcases, especially with nested templates and multiple template arguments (the reason for this patch, after all! ;-) ). You can easily test (different numbers of) multiple template arguments by just adding a few more template parameters with default arguments to e.g. TwoArgTemplate, S or Q.

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

82

change to If there is a non-nested comma we have two or more declarations in this chain.

[poelmanc]

Nice catch, that simplifies the code quite a bit! I added two more nested, complex multiple-template-argument tests and am happy to add more.

(We could do a case fallthrough after tok::l_brace, though some linters warn about them.)

[poelmanc]

Sorry added one more test at the end to make sure a multi-typedef with all that Variadic stuff still doesn't get changed to using.

[jonathanmeier]

Nice! I don't have commit access, so we'll need someone else have another look. @alexfh, since you previously worked on this, would you mind taking a look at this patch?

[poelmanc]

Hi @alexfh, @jonathanmeier has reviewed my pull request but lacks commit access. It changes ~30 lines of code isolated to modernize-use-using.cpp and adds ~60 lines of tests. If you have time I'd greatly appreciate it if you could provide any feedback or commit it. Alternatively can you suggest someone else who can review it? Thanks!

[poelmanc]

Rebase to latest master (tests moved into new "checkers" subdirectory.)

[poelmanc]

Checked "Done". (I addressed @jonathanmeier's comment feedback with a previous update but forgot to check the box!)

I welcome any more feedback. Thanks.

[aaron.ballman]

I'm a bit worried that this manual parsing technique will need fixing again in the future, but I think this is at least a reasonable incremental improvement.

LGTM with a minor nit.

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

54

Might as well use ++NestingLevel given that you don't care about the result anyway. Similar below.

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

197

I was going to suggest another test involving attributes (which can have arbitrary expressions as arguments to the attribute), but then I discovered attributes in Clang are broken in the place where it would be an issue for this check anyway. :-D (I filed https://bugs.llvm.org/show_bug.cgi?id=43939 to address the attribute issue.)

[poelmanc]

Thanks @aaron.ballman, I don't have commit access so will someone else commit this?

To address the minor nit, should I upload a new patch with post-increment/post-decrement changed to pre-increment/pre-decrement? (Does uploading changes undo the "Ready to Land" status?)

[aaron.ballman]

In D67460#1739062, @poelmanc wrote:

Thanks @aaron.ballman, I don't have commit access so will someone else commit this?

I can commit it for you when I get back into the office mid-next week, unless someone else wants to commit it on your behalf first.

To address the minor nit, should I upload a new patch with post-increment/post-decrement changed to pre-increment/pre-decrement? (Does uploading changes undo the "Ready to Land" status?)

Yes, please upload a new patch. It may change the review state in Phab, but unless it's a substantive change (which this is not), we do not require additional review/acceptance before landing (unless someone clicks the "Request Changes" option in the meantime).

[poelmanc]

Changed post-increment/decrement to pre-increment/decrement.

[poelmanc]

Done, thanks.

[poelmanc]

In D67460#1737529, @aaron.ballman wrote:

I'm a bit worried that this manual parsing technique will need fixing again in the future, but I think this is at least a reasonable incremental improvement.

Thanks and I agree. Your comments encouraged me to take another stab at improving things. See D70270, a whole new approach that removes manual parsing in favor of AST processing and successfully converts many more cases from typedef to using.

I didn't want to clobber this patch with that one in case any fatal flaws are discovered with the new approach. We can commit this one first - getting improved handling of commas and getting additional test cases - and treat D70270 as that next incremental improvement. Or we can keep this on hold until D70270 is worked out.

Thanks again for all your feedback and help!

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

static bool CheckRemoval(SourceManager &SM, SourceLocation StartLoc,
   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 ParenLevel = 0;
+  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:
-    case tok::r_brace:
-      // This might be the `typedef struct {...} T;` case.
+      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:
-      ParenLevel++;
+    case tok::l_square:
+      ++NestingLevel;
       break;
+    case tok::r_brace:
     case tok::r_paren:
-      ParenLevel--;
+    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 (ParenLevel == 0) {
-        // If there is comma and we are not between open parenthesis then it is
-        // two or more declarations in this chain.
+      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:
void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
   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'");
+  auto Diag = diag(StartLoc, "use 'using' instead of 'typedef'");
 
   // do not fix if there is macro or array
   if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID())
     return;
 
   if (CheckRemoval(SM, StartLoc, Context)) {
     auto printPolicy = PrintingPolicy(getLangOpts());
     printPolicy.SuppressScope = true;

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

 
 class D : public C<D> {
   void f() override { super::f(); }
 };
 class E : public C<E> {
   void f() override { super::f(); }
 };
 }
+
+template <typename T1, typename T2>
+class TwoArgTemplate {
+  typedef TwoArgTemplate<T1, T2> self;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+  // 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;
+
+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;
+
+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;
+
+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;
+
+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;