[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,71 @@ 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 typedef 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 separately, so we need to figure 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;
+ for (const SourceLocation Loc :
+ {MatchedDecl.getBeginLoc(), TokenBeforeName.getLocation()})
+ if (Lexer::getSpelling(Loc, Scratch, SM, LO) == "typedef")
+ Diag << FixItHint::CreateRemoval(Loc);
+
+ 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:
 /// https://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

@@ -315,6 +315,10 @@ Changes in existing checks
  when the format string is converted to a different type by an implicit
  constructor call.
 
+- 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.
+
 - Improved :doc:`performance-unnecessary-copy-initialization
  <clang-tidy/checks/performance/unnecessary-copy-initialization>` by printing
  the type of the diagnosed variable.

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;