[clang-tidy] Fix PR#44528 'modernize-use-using and enums'

[f00kat]

Summary

Add 'enumDecl' matcher for 'enum' cases. Change a bit logic in 'check' method and add test for this case.

[njames93]

If you are using diff to create a patch, use -U999999 to get the full context of the patch

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

274

Can you put a new line here.

[f00kat]

Add new line in the end of file checkers/modernize-use-using.cpp

[aaron.ballman]

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

28

It looks like there's a backtick in the comment rather than a quotation mark, that should probably be matcher's instead. Also, instead of struct/enums, I think it should be tag declarations (unions count, for instance).

30–31

Rather than matching on these, I think it would make more sense to add a new matcher for tagDecl(). It can be local to the check for now, or you can add it to the AST matchers header as a separate patch and then base this patch off that work.

[f00kat]

I have changed the code to work with tagDecl. It`s work fine on simple test cases such as

typedef enum { ea, eb } EnumT;

But not work with

#include <string>
typedef enum { ea, eb } EnumT;

It is not related with new tagDecl matcher. I simplify this case:

1. Create file Header.h
2. Header.h

typedef size_t sometype;

3. main.cpp

#include "Header.h"
using EnumT = enum { ea, eb };

It`s happens because in class UseUsingCheck we have variable LastReplacementEnd and it may contains SourceLocation for another file.
First time AST matcher trigger on file Header.h and we remember SourceLocation for code "typedef size_t sometype".
Then AST matcher trigger on "using EnumT = enum { ea, eb };" in main.cpp and we trying to use the value of LastReplacementEnd storing SourceLocation for Header.h.

So this 'if case' does not execute

if (ReplaceRange.getBegin().isMacroID() ||
   ReplaceRange.getBegin() >= LastReplacementEnd) {

execute this one

// This is additional TypedefDecl in a comma-separated typedef declaration.
// Start replacement *after* prior replacement and separate with semicolon.
ReplaceRange.setBegin(LastReplacementEnd);

Simple fix

if (ReplaceRange.getBegin().isMacroID() ||
   (Result.SourceManager->getFileID(ReplaceRange.getBegin()) != Result.SourceManager->getFileID(LastReplacementEnd)) || \\ new check
   (ReplaceRange.getBegin() >= LastReplacementEnd)) {

But maybe I wrong. I don`t know API well enough.

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

28

Ok

30–31

"Add new AST Matcher" - it's the Jedi level :)
Where can I find some manual that describe this? Or maybe some example(git commit).

"you can add it to the AST matchers header as a separate patch and then base this patch off that work."
So the sequencing will be:

1. I create another new patch with new AST matcher 'tagDecl'
2. Wait until you reviewed it
3. Merge to master
4. Pull latest master
5. Modify code and update this patch

?

30–31

I add new TagDecl matcher in this patch https://reviews.llvm.org/D73464. Check please.

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

274

Yeah

[f00kat]

1. Apply new tagDecl matcher
2. Trying to fix some bug

[aaron.ballman]

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

76–78

Be sure to run clang-format over the patch; this looks well beyond the usual 80-col limit. You can also drop the unnecessary parens around the sub expressions.

Also, a comment explaining why this code is needed would help future folks as would a test case showing what this corrects.

[f00kat]

1. clang-format
2. Add comment for additional "files IDs equals" check
3. Add test

[aaron.ballman]

LGTM! I'm happy to commit on your behalf once the minor nits are fixed up.

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

73

that ranges -> that the ranges
belongs -> belong to
overlap -> overlapping

[f00kat]

Clean up

[f00kat]

Sorry for my english. Sometimes it's more difficult than bug fixing

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

76–78

I left the brackets because they seem to increase readability. Or not? :)

[aaron.ballman]

In D73090#1854031, @f00kat wrote:

Sorry for my english. Sometimes it's more difficult than bug fixing

It's not a problem at all, that's why we help one another out spotting those things! :-)

I've commit on your behalf in 6423ae417e17611c4ee529f5848e839e6d9cb795

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

76–78

We don't typically use them unless there's a need, but it doesn't harm readability to keep them, so this is fine.

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;
+  SourceRange LastTagDeclRange;
   std::string FirstTypedefType;
   std::string FirstTypedefName;
 
 public:
   UseUsingCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override {
     Options.store(Opts, "IgnoreMacros", IgnoreMacros);
   }

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.
+  // This matcher used to find tag declarations in source code within typedefs.
   // They appear in the AST just *prior* to the typedefs.
-  Finder->addMatcher(cxxRecordDecl(unless(isImplicit())).bind("struct"), this);
+  Finder->addMatcher(tagDecl(unless(isImplicit())).bind("tagdecl"), this);
 }
 
 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();
+  const auto *MatchedTagDecl = Result.Nodes.getNodeAs<TagDecl>("tagdecl");
+  if (MatchedTagDecl) {
+    LastTagDeclRange = MatchedTagDecl->getSourceRange();
     return;
   }
 
   const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>("typedef");
   if (MatchedDecl->getLocation().isInvalid())
     return;
 
   SourceLocation StartLoc = MatchedDecl->getBeginLoc();
void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
   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.
+  // But also we need to check that the ranges belong to the same file because
+  // different files may contain overlapping ranges.
   std::string Using = "using ";
   if (ReplaceRange.getBegin().isMacroID() ||
-      ReplaceRange.getBegin() >= LastReplacementEnd) {
+      (Result.SourceManager->getFileID(ReplaceRange.getBegin()) !=
+       Result.SourceManager->getFileID(LastReplacementEnd)) ||
+      (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)) {
+  // 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(LastCxxDeclRange),
+        Lexer::getSourceText(CharSourceRange::getTokenRange(LastTagDeclRange),
                              *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/test/clang-tidy/checkers/Inputs/modernize-use-using/modernize-use-using.h

+#ifndef MODERNIZE_USE_USING_H
+#define MODERNIZE_USE_USING_H
+
+typedef int mytype;
+
+#endif

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

-// RUN: %check_clang_tidy %s modernize-use-using %t
+// RUN: %check_clang_tidy %s modernize-use-using %t -- -- -I %S/Inputs/modernize-use-using/
 
 typedef int Type;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
 // CHECK-FIXES: using Type = int;
 
 typedef long LL;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using LL = long;

 // 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*;
+
+typedef enum { ea1, eb1 } EnumT1;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT1 = enum { ea1, eb1 };
+
+#include "modernize-use-using.h"
+
+typedef enum { ea2, eb2 } EnumT2_CheckTypedefImpactFromAnotherFile;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
+// CHECK-FIXES: using EnumT2_CheckTypedefImpactFromAnotherFile = enum { ea2, eb2 };
+