Deal better with a macro that surrounds the whole call

We don't really want to stop enclosing an entire call to a printf-like
function to stop the check from replacing the call since that makes the
check ineffective when using testing frameworks such as Catch2.

Author: mikecrowe

clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp

@@ -216,7 +216,7 @@ FormatStringConverter::FormatStringConverter(
   }
 
   if (const std::optional<StringRef> MaybeMacroName =
-          formatStringContainsUnreplaceableMacro(FormatExpr, SM, PP);
+          formatStringContainsUnreplaceableMacro(Call, FormatExpr, SM, PP);
       MaybeMacroName) {
     conversionNotPossible(
         ("format string contains unreplaceable macro '" + *MaybeMacroName + "'")
@@ -243,30 +243,43 @@ FormatStringConverter::FormatStringConverter(
 
 std::optional<StringRef>
 FormatStringConverter::formatStringContainsUnreplaceableMacro(
-    const StringLiteral *FormatExpr, SourceManager &SM, Preprocessor &PP) {
+    const CallExpr *Call, const StringLiteral *FormatExpr, SourceManager &SM,
+    Preprocessor &PP) {
+  // If a macro invocation surrounds the entire call then we don't want that to
+  // inhibit conversion. The whole format string will appear to come from that
+  // macro, as will the function call.
+  std::optional<StringRef> MaybeSurroundingMacroName;
+  if (SourceLocation BeginCallLoc = Call->getBeginLoc();
+      BeginCallLoc.isMacroID())
+    MaybeSurroundingMacroName =
+        Lexer::getImmediateMacroName(BeginCallLoc, SM, PP.getLangOpts());
+
   for (auto I = FormatExpr->tokloc_begin(), E = FormatExpr->tokloc_end();
        I != E; ++I) {
     const SourceLocation &TokenLoc = *I;
     if (TokenLoc.isMacroID()) {
       const StringRef MacroName =
           Lexer::getImmediateMacroName(TokenLoc, SM, PP.getLangOpts());
 
-      // glibc uses __PRI64_PREFIX and __PRIPTR_PREFIX to define the prefixes
-      // for types that change size so we must look for multiple prefixes.
-      if (!MacroName.starts_with("PRI") && !MacroName.starts_with("__PRI"))
-        return MacroName;
-
-      const SourceLocation TokenSpellingLoc = SM.getSpellingLoc(TokenLoc);
-      const OptionalFileEntryRef MaybeFileEntry =
-          SM.getFileEntryRefForID(SM.getFileID(TokenSpellingLoc));
-      if (!MaybeFileEntry)
-        return MacroName;
-
-      HeaderSearch &HS = PP.getHeaderSearchInfo();
-      // Check if the file is a system header
-      if (!isSystem(HS.getFileDirFlavor(*MaybeFileEntry)) ||
-          llvm::sys::path::filename(MaybeFileEntry->getName()) != "inttypes.h")
-        return MacroName;
+      if (MaybeSurroundingMacroName != MacroName) {
+        // glibc uses __PRI64_PREFIX and __PRIPTR_PREFIX to define the prefixes
+        // for types that change size so we must look for multiple prefixes.
+        if (!MacroName.starts_with("PRI") && !MacroName.starts_with("__PRI"))
+          return MacroName;
+
+        const SourceLocation TokenSpellingLoc = SM.getSpellingLoc(TokenLoc);
+        const OptionalFileEntryRef MaybeFileEntry =
+            SM.getFileEntryRefForID(SM.getFileID(TokenSpellingLoc));
+        if (!MaybeFileEntry)
+          return MacroName;
+
+        HeaderSearch &HS = PP.getHeaderSearchInfo();
+        // Check if the file is a system header
+        if (!isSystem(HS.getFileDirFlavor(*MaybeFileEntry)) ||
+            llvm::sys::path::filename(MaybeFileEntry->getName()) !=
+                "inttypes.h")
+          return MacroName;
+      }
     }
   }
   return std::nullopt;

clang-tools-extra/clang-tidy/utils/FormatStringConverter.h

@@ -112,7 +112,8 @@ class FormatStringConverter
   void appendFormatText(StringRef Text);
   void finalizeFormatText();
   static std::optional<StringRef>
-  formatStringContainsUnreplaceableMacro(const StringLiteral *FormatExpr,
+  formatStringContainsUnreplaceableMacro(const CallExpr *CallExpr,
+                                         const StringLiteral *FormatExpr,
                                          SourceManager &SM, Preprocessor &PP);
   bool conversionNotPossible(std::string Reason) {
     ConversionNotPossibleReason = std::move(Reason);

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

@@ -152,4 +152,30 @@ std::string StrFormat_macros() {
   // Needs a __PRI prefix so that we get as far as looking for where the macro comes from
   auto s11 = absl::StrFormat(" macro from command line %" __PRI_CMDLINE_MACRO, s);
   // CHECK-MESSAGES: [[@LINE-1]]:14: warning: unable to use 'std::format' instead of 'StrFormat' because format string contains unreplaceable macro '__PRI_CMDLINE_MACRO' [modernize-use-std-format]
+
+  // We ought to be able to fix this since the macro surrounds the whole call
+  // and therefore can't change the format string independently. This is
+  // required to be able to fix calls inside Catch2 macros for example.
+#define SURROUND_ALL(x) x
+  auto s12 = SURROUND_ALL(absl::StrFormat("Macro surrounding entire invocation %" PRIu64, u64));
+  // CHECK-MESSAGES: [[@LINE-1]]:27: warning: use 'std::format' instead of 'StrFormat' [modernize-use-std-format]
+  // CHECK-FIXES: auto s12 = SURROUND_ALL(std::format("Macro surrounding entire invocation {}", u64));
+
+  // But having that surrounding macro shouldn't stop us ignoring an
+  // unreplaceable macro elsewhere.
+  auto s13 = SURROUND_ALL(absl::StrFormat("Macro surrounding entire invocation with unreplaceable macro %" PRI_FMT_MACRO, s));
+  // CHECK-MESSAGES: [[@LINE-1]]:27: warning: unable to use 'std::format' instead of 'StrFormat' because format string contains unreplaceable macro 'PRI_FMT_MACRO' [modernize-use-std-format]
+
+  // At the moment at least the check will replace occurrences where the
+  // function name is the result of expanding a macro.
+#define SURROUND_FUNCTION_NAME(x) absl:: x
+  auto s14 = SURROUND_FUNCTION_NAME(StrFormat)("Hello %d", 4442);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: use 'std::format' instead of 'StrFormat' [modernize-use-std-format]
+  // CHECK-FIXES: auto s14 = std::format("Hello {}", 4442);
+
+  // We can't safely fix occurrences where the macro may affect the format
+  // string differently in different builds.
+#define SURROUND_FORMAT(x) "!" x
+  auto s15 = absl::StrFormat(SURROUND_FORMAT("Hello %d"), 4443);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: unable to use 'std::format' instead of 'StrFormat' because format string contains unreplaceable macro 'SURROUND_FORMAT' [modernize-use-std-format]
 }

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

@@ -1613,4 +1613,30 @@ void macro_expansion(const char *s)
   // Needs a __PRI prefix so that we get as far as looking for where the macro comes from
   printf(" macro from command line %" __PRI_CMDLINE_MACRO, s);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unable to use 'std::print' instead of 'printf' because format string contains unreplaceable macro '__PRI_CMDLINE_MACRO' [modernize-use-std-print]
+
+  // We ought to be able to fix this since the macro surrounds the whole call
+  // and therefore can't change the format string independently. This is
+  // required to be able to fix calls inside Catch2 macros for example.
+#define SURROUND_ALL(x) x
+  SURROUND_ALL(printf("Macro surrounding entire invocation %" PRIu64, u64));
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: SURROUND_ALL(std::print("Macro surrounding entire invocation {}", u64));
+
+  // But having that surrounding macro shouldn't stop us ignoring an
+  // unreplaceable macro elsewhere.
+  SURROUND_ALL(printf("Macro surrounding entire invocation with unreplaceable macro %" PRI_FMT_MACRO, s));
+  // CHECK-MESSAGES: [[@LINE-1]]:16: warning: unable to use 'std::print' instead of 'printf' because format string contains unreplaceable macro 'PRI_FMT_MACRO' [modernize-use-std-print]
+
+  // At the moment at least the check will replace occurrences where the
+  // function name is the result of expanding a macro.
+#define SURROUND_FUNCTION_NAME(x) x
+  SURROUND_FUNCTION_NAME(printf)("Hello %d", 4442);
+  // CHECK-MESSAGES: [[@LINE-1]]:26: warning: use 'std::print' instead of 'printf' [modernize-use-std-print]
+  // CHECK-FIXES: std::print("Hello {}", 4442);
+
+  // We can't safely fix occurrences where the macro may affect the format
+  // string differently in different builds.
+#define SURROUND_FORMAT(x) "!" x
+  printf(SURROUND_FORMAT("Hello %d"), 4443);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unable to use 'std::print' instead of 'printf' because format string contains unreplaceable macro 'SURROUND_FORMAT' [modernize-use-std-print]
 }