[clang-tidy] use specified type verbatim in modernize-use-using fix (#113837)

Previously, the implementation used the printed type, which contains expanded macro arguments, deletes comments, and removes function argument names from the alias declaration. Instead, this check can be more surgical and use the actual written type verbatim. Fixes #33760 Fixes #37846 Fixes #41685 Fixes #83568 Fixes #95716 Fixes #97009
2024-12-21 13:06:30 +01:00
parent b9a2658a3e
commit f77152d9a4
4 changed files with 123 additions and 20 deletions
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -7,9 +7,14 @@
 //===----------------------------------------------------------------------===//

 #include "UseUsingCheck.h"
-#include "clang/AST/ASTContext.h"
+#include "../utils/LexerUtils.h"
 #include "clang/AST/DeclGroup.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 {
@@ -83,6 +88,9 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
  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);
@@ -119,14 +127,51 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
    return;
  }

-  PrintingPolicy PrintPolicy(getLangOpts());
-  PrintPolicy.SuppressScope = true;
-  PrintPolicy.ConstantArraySizeAsWritten = true;
-  PrintPolicy.UseVoidForZeroParams = false;
-  PrintPolicy.PrintInjectedClassNameWithArguments = false;
+  const TypeLoc TL = MatchedDecl->getTypeSourceInfo()->getTypeLoc();

-  std::string Type = MatchedDecl->getUnderlyingType().getAsString(PrintPolicy);
-  std::string Name = MatchedDecl->getNameAsString();
+  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();

  // typedefs with multiple comma-separated definitions produce multiple
@@ -143,7 +188,8 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
    // 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;
+    FirstTypedefName = Name.str();
+    MainTypeEndLoc = TL.getEndLoc();
  } else {
    // This is additional TypedefDecl in a comma-separated typedef declaration.
    // Start replacement *after* prior replacement and separate with semicolon.
@@ -153,10 +199,10 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
    // 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 (Type == FirstTypedefType && !QualifierStr.empty())
+      Type = FirstTypedefName;
  }
+
  if (!ReplaceRange.getEnd().isMacroID()) {
    const SourceLocation::IntTy Offset =
        MatchedDecl->getFunctionType() ? 0 : Name.size();
@@ -171,13 +217,12 @@ void UseUsingCheck::check(const MatchFinder::MatchResult &Result) {
      LastTagDeclRange->second.isValid() &&
      ReplaceRange.fullyContains(LastTagDeclRange->second)) {
    Type = std::string(Lexer::getSourceText(
-        CharSourceRange::getTokenRange(LastTagDeclRange->second),
-        *Result.SourceManager, getLangOpts()));
+        CharSourceRange::getTokenRange(LastTagDeclRange->second), SM, LO));
    if (Type.empty())
      return;
  }

-  std::string Replacement = Using + Name + " = " + Type;
+  std::string Replacement = (Using + Name + " = " + Type + QualifierStr).str();
  Diag << FixItHint::CreateReplacement(ReplaceRange, Replacement);
 }
 } // namespace clang::tidy::modernize
--- a/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseUsingCheck.h
@@ -26,6 +26,7 @@ class UseUsingCheck : public ClangTidyCheck {

  std::string FirstTypedefType;
  std::string FirstTypedefName;
+  SourceLocation MainTypeEndLoc;

 public:
  UseUsingCheck(StringRef Name, ClangTidyContext *Context);
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -317,6 +317,9 @@ Changes in existing checks
  member function calls too and to only expand macros starting with ``PRI``
  and ``__PRI`` from ``<inttypes.h>`` in the format string.

+- Improved :doc:`modernize-use-using
+  <clang-tidy/checks/modernize/use-using>` check by not expanding macros.
+
 - Improved :doc:`performance-avoid-endl
  <clang-tidy/checks/performance/avoid-endl>` check to use ``std::endl`` as
  placeholder when lexer cannot get source text.
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-using.cpp
@@ -80,7 +80,7 @@ typedef Test<my_class *> another;

 typedef int* PInt;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
-// CHECK-FIXES: using PInt = int *;
+// CHECK-FIXES: using PInt = int*;

 typedef int bla1, bla2, bla3;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -112,7 +112,7 @@ TYPEDEF Foo Bak;
 typedef FOO Bam;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: #define FOO Foo
-// CHECK-FIXES: using Bam = Foo;
+// CHECK-FIXES: using Bam = FOO;

 typedef struct Foo Bap;
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
@@ -247,7 +247,7 @@ typedef Q<T{0 < 0}.b> Q3_t;

 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]>>>;
+// CHECK-FIXES: using Nested_t = TwoArgTemplate<TwoArgTemplate<int, Q<T{0 < 0}.b> >, S<(0 < 0), Q<b[0 < 0]> > >;

 template <typename a>
 class TemplateKeyword {
@@ -265,12 +265,12 @@ 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]>>>>
+// 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-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: 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;
@@ -383,3 +383,57 @@ namespace ISSUE_72179
  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: use 'using' instead of 'typedef' [modernize-use-using]
  // CHECK-FIXES: const auto foo4 = [](int a){using d = int;};
 }
+
+
+typedef int* int_ptr, *int_ptr_ptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-MESSAGES: :[[@LINE-2]]:21: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using int_ptr = int*;
+// CHECK-FIXES-NEXT: using int_ptr_ptr = int_ptr*;
+
+#ifndef SpecialMode
+#define SomeMacro(x) x
+#else
+#define SomeMacro(x) SpecialType
+#endif
+
+class SomeMacro(GH33760) { };
+
+typedef void(SomeMacro(GH33760)::* FunctionType)(float, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using FunctionType = void(SomeMacro(GH33760)::* )(float, int);
+
+#define CDECL __attribute((cdecl))
+
+// GH37846 & GH41685
+typedef void (CDECL *GH37846)(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using GH37846 = void (CDECL *)(int);
+
+typedef void (__attribute((cdecl)) *GH41685)(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using GH41685 = void (__attribute((cdecl)) *)(int);
+
+namespace GH83568 {
+  typedef int(*name)(int arg1, int arg2);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using name = int(*)(int arg1, int arg2);
+}
+
+#ifdef FOO
+#define GH95716 float
+#else
+#define GH95716 double
+#endif
+
+typedef GH95716 foo;
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using foo = GH95716;
+
+namespace GH97009 {
+  typedef double PointType[3];
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef' [modernize-use-using]
+  typedef bool (*Function)(PointType, PointType);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef' [modernize-use-using]
+// CHECK-FIXES: using Function = bool (*)(PointType, PointType);
+}