[clang-tidy] Make modernize-use-using's fix-its more robust

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

@@ -7,30 +7,26 @@
 //===----------------------------------------------------------------------===//
 
 #include "UseUsingCheck.h"
-#include "../utils/LexerUtils.h"
-#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Lex/Lexer.h"
-#include <string>
 
 using namespace clang::ast_matchers;
 namespace {
 
 AST_MATCHER(clang::LinkageSpecDecl, isExternCLinkage) {
   return Node.getLanguage() == clang::LinkageSpecLanguageIDs::C;
 }
+
 } // namespace
 
 namespace clang::tidy::modernize {
 
 static constexpr llvm::StringLiteral ExternCDeclName = "extern-c-decl";
-static constexpr llvm::StringLiteral ParentDeclName = "parent-decl";
-static constexpr llvm::StringLiteral TagDeclName = "tag-decl";
 static constexpr llvm::StringLiteral TypedefName = "typedef";
-static constexpr llvm::StringLiteral DeclStmtName = "decl-stmt";
 
 UseUsingCheck::UseUsingCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
@@ -47,183 +43,75 @@ void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
       typedefDecl(
           unless(isInstantiated()),
           optionally(hasAncestor(
-              linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))),
-          anyOf(hasParent(decl().bind(ParentDeclName)),
-                hasParent(declStmt().bind(DeclStmtName))))
+              linkageSpecDecl(isExternCLinkage()).bind(ExternCDeclName))))
           .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())),
-                      anyOf(hasParent(decl().bind(ParentDeclName)),
-                            hasParent(declStmt().bind(DeclStmtName)))),
-                // We want the parent of the ClassTemplateDecl, not the parent
-                // of the specialization.
-                classTemplateSpecializationDecl(hasAncestor(classTemplateDecl(
-                    anyOf(hasParent(decl().bind(ParentDeclName)),
-                          hasParent(declStmt().bind(DeclStmtName))))))))
-          .bind(TagDeclName),
-      this);
 }
 
 void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
-  const auto *ParentDecl = Result.Nodes.getNodeAs<Decl>(ParentDeclName);
-
-  if (!ParentDecl) {
-    const auto *ParentDeclStmt = Result.Nodes.getNodeAs<DeclStmt>(DeclStmtName);
-    if (ParentDeclStmt) {
-      if (ParentDeclStmt->isSingleDecl())
-        ParentDecl = ParentDeclStmt->getSingleDecl();
-      else
-        ParentDecl =
-            ParentDeclStmt->getDeclGroup().getDeclGroup()
-                [ParentDeclStmt->getDeclGroup().getDeclGroup().size() - 1];
-    }
-  }
-
-  if (!ParentDecl)
-    return;
-
-  const SourceManager &SM = *Result.SourceManager;
-  const LangOptions &LO = getLangOpts();
-
-  // 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>(TagDeclName);
-  if (MatchedTagDecl) {
-    // 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.
-    if (MatchedTagDecl->isThisDeclarationADefinition())
-      LastTagDeclRanges[ParentDecl] = MatchedTagDecl->getSourceRange();
-    return;
-  }
-
-  const auto *MatchedDecl = Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
-  if (MatchedDecl->getLocation().isInvalid())
+  const auto &MatchedDecl = *Result.Nodes.getNodeAs<TypedefDecl>(TypedefName);
+  if (MatchedDecl.getLocation().isInvalid())
     return;
 
   const auto *ExternCDecl =
       Result.Nodes.getNodeAs<LinkageSpecDecl>(ExternCDeclName);
   if (ExternCDecl && IgnoreExternC)
     return;
 
-  SourceLocation StartLoc = MatchedDecl->getBeginLoc();
-
-  if (StartLoc.isMacroID() && IgnoreMacros)
-    return;
-
   static constexpr llvm::StringLiteral UseUsingWarning =
       "use 'using' instead of 'typedef'";
 
-  // Warn at StartLoc but do not fix if there is macro or array.
-  if (MatchedDecl->getUnderlyingType()->isArrayType() || StartLoc.isMacroID()) {
-    diag(StartLoc, UseUsingWarning);
+  if (MatchedDecl.getBeginLoc().isMacroID()) {
+    // Warn but do not fix if there is a macro.
+    if (!IgnoreMacros)
+      diag(MatchedDecl.getBeginLoc(), UseUsingWarning);
     return;
   }
 
-  const TypeLoc TL = MatchedDecl->getTypeSourceInfo()->getTypeLoc();
-
-  auto [Type, QualifierStr] = [MatchedDecl, this, &TL, &SM,
-                               &LO]() -> std::pair<std::string, std::string> {
-    SourceRange TypeRange = TL.getSourceRange();
-
-    // Function pointer case, get the left and right side of the identifier
-    // without the identifier.
-    if (TypeRange.fullyContains(MatchedDecl->getLocation())) {
-      const auto RangeLeftOfIdentifier = CharSourceRange::getCharRange(
-          TypeRange.getBegin(), MatchedDecl->getLocation());
-      const auto RangeRightOfIdentifier = CharSourceRange::getCharRange(
-          Lexer::getLocForEndOfToken(MatchedDecl->getLocation(), 0, SM, LO),
-          Lexer::getLocForEndOfToken(TypeRange.getEnd(), 0, SM, LO));
-      const std::string VerbatimType =
-          (Lexer::getSourceText(RangeLeftOfIdentifier, SM, LO) +
-           Lexer::getSourceText(RangeRightOfIdentifier, SM, LO))
-              .str();
-      return {VerbatimType, ""};
-    }
-
-    StringRef ExtraReference = "";
-    if (MainTypeEndLoc.isValid() && TypeRange.fullyContains(MainTypeEndLoc)) {
-      // Each type introduced in a typedef can specify being a reference or
-      // pointer type seperately, so we need to sigure out if the new using-decl
-      // needs to be to a reference or pointer as well.
-      const SourceLocation Tok = utils::lexer::findPreviousAnyTokenKind(
-          MatchedDecl->getLocation(), SM, LO, tok::TokenKind::star,
-          tok::TokenKind::amp, tok::TokenKind::comma,
-          tok::TokenKind::kw_typedef);
-
-      ExtraReference = Lexer::getSourceText(
-          CharSourceRange::getCharRange(Tok, Tok.getLocWithOffset(1)), SM, LO);
-
-      if (ExtraReference != "*" && ExtraReference != "&")
-        ExtraReference = "";
-
-      TypeRange.setEnd(MainTypeEndLoc);
-    }
-    return {
-        Lexer::getSourceText(CharSourceRange::getTokenRange(TypeRange), SM, LO)
-            .str(),
-        ExtraReference.str()};
-  }();
-  StringRef Name = MatchedDecl->getName();
-  SourceRange ReplaceRange = MatchedDecl->getSourceRange();
+  const SourceManager &SM = *Result.SourceManager;
+  const LangOptions &LO = getLangOpts();
 
   // 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() ||
-      (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.str();
-    MainTypeEndLoc = TL.getEndLoc();
+  const Token TokenBeforeName =
+      *Lexer::findPreviousToken(MatchedDecl.getLocation(), SM, LO,
+                                /*IncludeComments=*/false);
+  const SourceRange RemovalRange = {
+      TokenBeforeName.getEndLoc(),
+      Lexer::getLocForEndOfToken(MatchedDecl.getLocation(), 1, SM, LO)};
+  if (NextTypedefStartsANewSequence) {
+    auto Diag = diag(MatchedDecl.getBeginLoc(), UseUsingWarning)
+                << FixItHint::CreateInsertion(
+                       MatchedDecl.getBeginLoc(),
+                       ("using " + MatchedDecl.getName() + " =").str())
+                << FixItHint::CreateRemoval(RemovalRange);
+
+    SmallString<128> Scratch;
+    if (Lexer::getSpelling(MatchedDecl.getBeginLoc(), Scratch, SM, LO) ==
+        "typedef")
+      Diag << FixItHint::CreateRemoval(MatchedDecl.getBeginLoc());
+
+    Scratch.resize(0);
+    if (Lexer::getSpelling(TokenBeforeName.getLocation(), Scratch, SM, LO) ==
+        "typedef")
+      Diag << FixItHint::CreateRemoval(TokenBeforeName.getLocation());
+
+    FirstTypedefName = MatchedDecl.getName();
   } 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 == FirstTypedefType && !QualifierStr.empty())
-      Type = FirstTypedefName;
-  }
-
-  if (!ReplaceRange.getEnd().isMacroID()) {
-    const SourceLocation::IntTy Offset =
-        MatchedDecl->getFunctionType() ? 0 : Name.size();
-    LastReplacementEnd = ReplaceRange.getEnd().getLocWithOffset(Offset);
+    diag(LastCommaOrSemi, UseUsingWarning)
+        << FixItHint::CreateReplacement(
+               LastCommaOrSemi,
+               (";\nusing " + MatchedDecl.getName() + " = " + FirstTypedefName)
+                   .str())
+        << FixItHint::CreateRemoval(RemovalRange);
   }
 
-  auto Diag = diag(ReplaceRange.getBegin(), UseUsingWarning);
-
-  // If typedef contains a full tag declaration, extract its full text.
-  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), SM, LO));
-    if (Type.empty())
-      return;
-  }
-
-  std::string Replacement = (Using + Name + " = " + Type + QualifierStr).str();
-  Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
+  const Token CommaOrSemi = *Lexer::findNextToken(
+      MatchedDecl.getEndLoc(), SM, LO, /*IncludeComments=*/false);
+  NextTypedefStartsANewSequence = CommaOrSemi.isNot(tok::TokenKind::comma);
+  LastCommaOrSemi = CommaOrSemi.getLocation();
 }
+
 } // namespace clang::tidy::modernize

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

@@ -18,15 +18,11 @@ namespace clang::tidy::modernize {
 /// 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;
   const bool IgnoreExternC;
-  SourceLocation LastReplacementEnd;
-  llvm::DenseMap<const Decl *, SourceRange> LastTagDeclRanges;
-
-  std::string FirstTypedefType;
-  std::string FirstTypedefName;
-  SourceLocation MainTypeEndLoc;
+  bool NextTypedefStartsANewSequence = true;
+  StringRef FirstTypedefName;
+  SourceLocation LastCommaOrSemi;
 
 public:
   UseUsingCheck(StringRef Name, ClangTidyContext *Context);

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

@@ -1988,7 +1988,7 @@ TEST(Diagnostics, Tags) {
                         withTag(DiagnosticTag::Deprecated))));
 
   Test = Annotations(R"cpp(
-    $typedef[[typedef int INT]];
+    $typedef[[typedef]] int INT;
   )cpp");
   TU.Code = Test.code();
   TU.ClangTidyProvider = addTidyChecks("modernize-use-using");
@@ -2002,7 +2002,7 @@ TEST(Diagnostics, Tags) {
 TEST(Diagnostics, TidyDiagsArentAffectedFromWerror) {
   TestTU TU;
   TU.ExtraArgs = {"-Werror"};
-  Annotations Test(R"cpp($typedef[[typedef int INT]]; // error-ok)cpp");
+  Annotations Test(R"cpp($typedef[[typedef]] int INT; // error-ok)cpp");
   TU.Code = Test.code().str();
   TU.ClangTidyProvider = addTidyChecks("modernize-use-using");
   EXPECT_THAT(

clang-tools-extra/docs/ReleaseNotes.rst

@@ -102,6 +102,10 @@ New check aliases
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`modernize-use-using
+  <clang-tidy/checks/modernize/use-using>` check by removing many incorrect
+  fix-its and providing fix-its where before there was only a warning.
+
 Removed checks
 ^^^^^^^^^^^^^^
 

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

@@ -9,11 +9,6 @@ CODE;
 // CHECK-FIXES: CODE;
 
 struct Foo;
-#define Bar Baz
-typedef Foo Bar;
-// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: #define Bar Baz
-// CHECK-FIXES: using Baz = Foo;
 
 #define TYPEDEF typedef
 TYPEDEF Foo Bak;