[clang-tidy] Fix c_str() removal and cast addition when re-ordering a…

…rguments The modernize-use-std-print check would get confused if it had to re-order field-width and precision arguments at the same time as adding casts or removing calls to c_str(). Fix this by tracking the argument indices and combining c_str() removal with argument re-ordering. Add missing test cases to lit check. Fixes #64033 Reviewed By: PiotrZSL Differential Revision: https://reviews.llvm.org/D156616
llvm · Aug 28, 2023 · c6207f6 · c6207f6
1 parent 9c760ca
commit c6207f6
Show file tree

Hide file tree

Showing 3 changed files with 128 additions and 39 deletions.
diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
@@ -356,7 +356,8 @@ void FormatStringConverter::maybeRotateArguments(const PrintfSpecifier &FS) {
     ArgRotates.emplace_back(FS.getArgIndex() + ArgsOffset, ArgCount);
 }
 
-void FormatStringConverter::emitStringArgument(const Expr *Arg) {
+void FormatStringConverter::emitStringArgument(unsigned ArgIndex,
+                                               const Expr *Arg) {
   // If the argument is the result of a call to std::string::c_str() or
   // data() with a return type of char then we can remove that call and
   // pass the std::string directly. We don't want to do so if the return
@@ -386,7 +387,7 @@ void FormatStringConverter::emitStringArgument(const Expr *Arg) {
     // printf is happy to print signed char and unsigned char strings, but
     // std::format only likes char strings.
     if (Pointee->isCharType() && !isRealCharType(Pointee))
-      ArgFixes.emplace_back(Arg, "reinterpret_cast<const char *>(");
+      ArgFixes.emplace_back(ArgIndex, "reinterpret_cast<const char *>(");
   }
 }
 
@@ -409,7 +410,7 @@ bool FormatStringConverter::emitIntegerArgument(
       if (const std::optional<std::string> MaybeCastType =
               castTypeForArgument(ArgKind, ET->getDecl()->getIntegerType()))
         ArgFixes.emplace_back(
-            Arg, (Twine("static_cast<") + *MaybeCastType + ">(").str());
+            ArgIndex, (Twine("static_cast<") + *MaybeCastType + ">(").str());
       else
         return conversionNotPossible(
             (Twine("argument ") + Twine(ArgIndex) + " has unexpected enum type")
@@ -423,7 +424,7 @@ bool FormatStringConverter::emitIntegerArgument(
     if (const std::optional<std::string> MaybeCastType =
             castTypeForArgument(ArgKind, ArgType))
       ArgFixes.emplace_back(
-          Arg, (Twine("static_cast<") + *MaybeCastType + ">(").str());
+          ArgIndex, (Twine("static_cast<") + *MaybeCastType + ">(").str());
     else
       return conversionNotPossible(
           (Twine("argument ") + Twine(ArgIndex) + " cannot be cast to " +
@@ -447,7 +448,7 @@ bool FormatStringConverter::emitType(const PrintfSpecifier &FS, const Expr *Arg,
   ConversionSpecifier::Kind ArgKind = FS.getConversionSpecifier().getKind();
   switch (ArgKind) {
   case ConversionSpecifier::Kind::sArg:
-    emitStringArgument(Arg);
+    emitStringArgument(FS.getArgIndex() + ArgsOffset, Arg);
     break;
   case ConversionSpecifier::Kind::cArg:
     // The type must be "c" to get a character unless the type is exactly
@@ -466,7 +467,8 @@ bool FormatStringConverter::emitType(const PrintfSpecifier &FS, const Expr *Arg,
     const clang::QualType &ArgType = Arg->getType();
     // std::format knows how to format void pointers and nullptrs
     if (!ArgType->isNullPtrType() && !ArgType->isVoidPointerType())
-      ArgFixes.emplace_back(Arg, "static_cast<const void *>(");
+      ArgFixes.emplace_back(FS.getArgIndex() + ArgsOffset,
+                            "static_cast<const void *>(");
     break;
   }
   case ConversionSpecifier::Kind::xArg:
@@ -673,6 +675,15 @@ void FormatStringConverter::appendFormatText(const StringRef Text) {
   }
 }
 
+static std::string withoutCStrReplacement(const BoundNodes &CStrRemovalMatch,
+                                          ASTContext &Context) {
+  const auto *Arg = CStrRemovalMatch.getNodeAs<Expr>("arg");
+  const auto *Member = CStrRemovalMatch.getNodeAs<MemberExpr>("member");
+  const bool Arrow = Member->isArrow();
+  return Arrow ? utils::fixit::formatDereference(*Arg, Context)
+               : tooling::fixit::getText(*Arg, Context).str();
+}
+
 /// Called by the check when it is ready to apply the fixes.
 void FormatStringConverter::applyFixes(DiagnosticBuilder &Diag,
                                        SourceManager &SM) {
@@ -683,41 +694,67 @@ void FormatStringConverter::applyFixes(DiagnosticBuilder &Diag,
         StandardFormatString);
   }
 
-  for (const auto &[Arg, Replacement] : ArgFixes) {
-    SourceLocation AfterOtherSide =
-        Lexer::findNextToken(Arg->getEndLoc(), SM, LangOpts)->getLocation();
-
-    Diag << FixItHint::CreateInsertion(Arg->getBeginLoc(), Replacement)
-         << FixItHint::CreateInsertion(AfterOtherSide, ")");
-  }
-
-  for (const auto &Match : ArgCStrRemovals) {
-    const auto *Call = Match.getNodeAs<CallExpr>("call");
-    const auto *Arg = Match.getNodeAs<Expr>("arg");
-    const auto *Member = Match.getNodeAs<MemberExpr>("member");
-    const bool Arrow = Member->isArrow();
-    const std::string ArgText =
-        Arrow ? utils::fixit::formatDereference(*Arg, *Context)
-              : tooling::fixit::getText(*Arg, *Context).str();
-    if (!ArgText.empty())
-      Diag << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText);
-  }
-
   // ArgCount is one less than the number of arguments to be rotated.
   for (auto [ValueArgIndex, ArgCount] : ArgRotates) {
     assert(ValueArgIndex < NumArgs);
     assert(ValueArgIndex > ArgCount);
 
-    // First move the value argument to the right place.
-    Diag << tooling::fixit::createReplacement(*Args[ValueArgIndex - ArgCount],
-                                              *Args[ValueArgIndex], *Context);
+    // First move the value argument to the right place. But if there's a
+    // pending c_str() removal then we must do that at the same time.
+    if (const auto CStrRemovalMatch =
+            std::find_if(ArgCStrRemovals.cbegin(), ArgCStrRemovals.cend(),
+                         [ArgStartPos = Args[ValueArgIndex]->getBeginLoc()](
+                             const BoundNodes &Match) {
+                           // This c_str() removal corresponds to the argument
+                           // being moved if they start at the same location.
+                           const Expr *CStrArg = Match.getNodeAs<Expr>("arg");
+                           return ArgStartPos == CStrArg->getBeginLoc();
+                         });
+        CStrRemovalMatch != ArgCStrRemovals.end()) {
+      const std::string ArgText =
+          withoutCStrReplacement(*CStrRemovalMatch, *Context);
+      assert(!ArgText.empty());
+
+      Diag << FixItHint::CreateReplacement(
+          Args[ValueArgIndex - ArgCount]->getSourceRange(), ArgText);
+
+      // That c_str() removal is now dealt with, so we don't need to do it again
+      ArgCStrRemovals.erase(CStrRemovalMatch);
+    } else
+      Diag << tooling::fixit::createReplacement(*Args[ValueArgIndex - ArgCount],
+                                                *Args[ValueArgIndex], *Context);
 
     // Now shift down the field width and precision (if either are present) to
     // accommodate it.
     for (size_t Offset = 0; Offset < ArgCount; ++Offset)
       Diag << tooling::fixit::createReplacement(
           *Args[ValueArgIndex - Offset], *Args[ValueArgIndex - Offset - 1],
           *Context);
+
+    // Now we need to modify the ArgFix index too so that we fix the right
+    // argument. We don't need to care about the width and precision indices
+    // since they never need fixing.
+    for (auto &ArgFix : ArgFixes) {
+      if (ArgFix.ArgIndex == ValueArgIndex)
+        ArgFix.ArgIndex = ValueArgIndex - ArgCount;
+    }
+  }
+
+  for (const auto &[ArgIndex, Replacement] : ArgFixes) {
+    SourceLocation AfterOtherSide =
+        Lexer::findNextToken(Args[ArgIndex]->getEndLoc(), SM, LangOpts)
+            ->getLocation();
+
+    Diag << FixItHint::CreateInsertion(Args[ArgIndex]->getBeginLoc(),
+                                       Replacement, true)
+         << FixItHint::CreateInsertion(AfterOtherSide, ")", true);
+  }
+
+  for (const auto &Match : ArgCStrRemovals) {
+    const auto *Call = Match.getNodeAs<CallExpr>("call");
+    const std::string ArgText = withoutCStrReplacement(Match, *Context);
+    if (!ArgText.empty())
+      Diag << FixItHint::CreateReplacement(Call->getSourceRange(), ArgText);
   }
 }
 } // namespace clang::tidy::utils
diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
@@ -64,7 +64,16 @@ class FormatStringConverter
   std::string StandardFormatString;
 
   /// Casts to be used to wrap arguments to retain printf compatibility.
-  std::vector<std::tuple<const Expr *, std::string>> ArgFixes;
+  struct ArgumentFix {
+    unsigned ArgIndex;
+    std::string Fix;
+
+    // We currently need this for emplace_back. Roll on C++20.
+    explicit ArgumentFix(unsigned ArgIndex, std::string Fix)
+        : ArgIndex(ArgIndex), Fix(std::move(Fix)) {}
+  };
+
+  std::vector<ArgumentFix> ArgFixes;
   std::vector<clang::ast_matchers::BoundNodes> ArgCStrRemovals;
 
   // Argument rotations to cope with the fact that std::print puts the value to
@@ -77,7 +86,7 @@ class FormatStringConverter
   void emitAlternativeForm(const PrintfSpecifier &FS, std::string &FormatSpec);
   void emitFieldWidth(const PrintfSpecifier &FS, std::string &FormatSpec);
   void emitPrecision(const PrintfSpecifier &FS, std::string &FormatSpec);
-  void emitStringArgument(const Expr *Arg);
+  void emitStringArgument(unsigned ArgIndex, const Expr *Arg);
   bool emitIntegerArgument(ConversionSpecifier::Kind ArgKind, const Expr *Arg,
                            unsigned ArgIndex, std::string &FormatSpec);
 

diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-print.cpp
@@ -14,6 +14,12 @@
 #include <string.h>
 #include <string>
 
+template <typename T>
+struct iterator {
+  T *operator->();
+  T &operator*();
+};
+
 void printf_simple() {
   printf("Hello");
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
@@ -1121,11 +1127,32 @@ void printf_precision() {
   // CHECK-FIXES: std::println("Hello {:.5}", 'G');
 }
 
-void printf_field_width_and_precision() {
+void printf_field_width_and_precision(const std::string &s1, const std::string &s2, const std::string &s3)
+{
   printf("width only:%*d width and precision:%*.*f precision only:%.*f\n", 3, 42, 4, 2, 3.14159265358979323846, 5, 2.718);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("width only:{:{}} width and precision:{:{}.{}f} precision only:{:.{}f}", 42, 3, 3.14159265358979323846, 4, 2, 2.718, 5);
 
+  const unsigned int ui1 = 42, ui2 = 43, ui3 = 44;
+  printf("casts width only:%*d width and precision:%*.*d precision only:%.*d\n", 3, ui1, 4, 2, ui2, 5, ui3);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES-NOTSTRICT: std::println("casts width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", ui1, 3, ui2, 4, 2, ui3, 5);
+  // CHECK-FIXES-STRICT: std::println("casts width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", static_cast<int>(ui1), 3, static_cast<int>(ui2), 4, 2, static_cast<int>(ui3), 5);
+
+  printf("c_str removal width only:%*s width and precision:%*.*s precision only:%.*s\n", 3, s1.c_str(), 4, 2, s2.c_str(), 5, s3.c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println("c_str removal width only:{:>{}} width and precision:{:>{}.{}} precision only:{:.{}}", s1, 3, s2, 4, 2, s3, 5);
+
+  const std::string *ps1 = &s1, *ps2 = &s2, *ps3 = &s3;
+  printf("c_str() removal pointer width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, ps1->c_str(), 4, 2, ps2->c_str(), 5, ps3->c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println("c_str() removal pointer width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *ps1, 3, *ps2, 4, 2, *ps3, 5);
+
+  iterator<std::string> is1, is2, is3;
+  printf("c_str() removal iterator width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, is1->c_str(), 4, 2, is2->c_str(), 5, is3->c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println("c_str() removal iterator width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *is1, 3, *is2, 4, 2, *is3, 5);
+
   printf("width and precision positional:%1$*2$.*3$f after\n", 3.14159265358979323846, 4, 2);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("width and precision positional:{0:{1}.{2}f} after", 3.14159265358979323846, 4, 2);
@@ -1134,9 +1161,13 @@ void printf_field_width_and_precision() {
   printf("width only:%3$*1$d width and precision:%4$*1$.*2$f precision only:%5$.*2$f\n", width, precision, 42, 3.1415926, 2.718);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
   // CHECK-FIXES: std::println("width only:{2:{0}} width and precision:{3:{0}.{1}f} precision only:{4:.{1}f}", width, precision, 42, 3.1415926, 2.718);
+
+  printf("c_str removal width only:%3$*1$s width and precision:%4$*1$.*2$s precision only:%5$.*2$s\n", width, precision, s1.c_str(), s2.c_str(), s3.c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println("c_str removal width only:{2:>{0}} width and precision:{3:>{0}.{1}} precision only:{4:.{1}}", width, precision, s1, s2, s3);
 }
 
-void fprintf_field_width_and_precision() {
+void fprintf_field_width_and_precision(const std::string &s1, const std::string &s2, const std::string &s3) {
   fprintf(stderr, "width only:%*d width and precision:%*.*f precision only:%.*f\n", 3, 42, 4, 2, 3.14159265358979323846, 5, 2.718);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
   // CHECK-FIXES: std::println(stderr, "width only:{:{}} width and precision:{:{}.{}f} precision only:{:.{}f}", 42, 3, 3.14159265358979323846, 4, 2, 2.718, 5);
@@ -1145,10 +1176,28 @@ void fprintf_field_width_and_precision() {
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
   // CHECK-FIXES: std::println(stderr, "width and precision positional:{0:{1}.{2}f} after", 3.14159265358979323846, 4, 2);
 
+  fprintf(stderr, "c_str removal width only:%*s width and precision:%*.*s precision only:%.*s\n", 3, s1.c_str(), 4, 2, s2.c_str(), 5, s3.c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println(stderr, "c_str removal width only:{:>{}} width and precision:{:>{}.{}} precision only:{:.{}}", s1, 3, s2, 4, 2, s3, 5);
+
+  const std::string *ps1 = &s1, *ps2 = &s2, *ps3 = &s3;
+  fprintf(stderr, "c_str() removal pointer width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, ps1->c_str(), 4, 2, ps2->c_str(), 5, ps3->c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println(stderr, "c_str() removal pointer width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *ps1, 3, *ps2, 4, 2, *ps3, 5);
+
+  iterator<std::string> is1, is2, is3;
+  fprintf(stderr, "c_str() removal iterator width only:%-*s width and precision:%-*.*s precision only:%-.*s\n", 3, is1->c_str(), 4, 2, is2->c_str(), 5, is3->c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println(stderr, "c_str() removal iterator width only:{:{}} width and precision:{:{}.{}} precision only:{:.{}}", *is1, 3, *is2, 4, 2, *is3, 5);
+
   const int width = 10, precision = 3;
   fprintf(stderr, "width only:%3$*1$d width and precision:%4$*1$.*2$f precision only:%5$.*2$f\n", width, precision, 42, 3.1415926, 2.718);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
   // CHECK-FIXES: std::println(stderr, "width only:{2:{0}} width and precision:{3:{0}.{1}f} precision only:{4:.{1}f}", width, precision, 42, 3.1415926, 2.718);
+
+  fprintf(stderr, "c_str removal width only:%3$*1$s width and precision:%4$*1$.*2$s precision only:%5$.*2$s\n", width, precision, s1.c_str(), s2.c_str(), s3.c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: use 'std::println' instead of 'fprintf' [modernize-use-std-print]
+  // CHECK-FIXES: std::println(stderr, "c_str removal width only:{2:>{0}} width and precision:{3:>{0}.{1}} precision only:{4:.{1}}", width, precision, s1, s2, s3);
 }
 
 void printf_alternative_form() {
@@ -1497,12 +1546,6 @@ void fprintf_string_pointer_cstr(const std::string *s1) {
   // CHECK-FIXES: std::print(stderr, "fprintf string pointer c_str {}", *s1);
 }
 
-template <typename T>
-struct iterator {
-  T *operator->();
-  T &operator*();
-};
-
 void printf_iterator_cstr(iterator<std::string> i1, iterator<std::string> i2)
 {
   printf("printf iterator c_str %s %s\n", i1->c_str(), i2->data());