[clang-tidy] Only expand <inttypes.h> macros in modernize-use-std-format/print (#97911)

mikecrowe · web-flow · commit a199fb122998 · 2024-10-08T22:03:58.000+02:00
Expanding all macros in the printf/absl::StrFormat format string before
conversion could easily break code if those macros are expanded change
their definition between builds. It's important for this check to expand
the &lt;inttypes.h&gt; PRI macros though, so let's ensure that the presence of
any other macros in the format string causes the check to emit a warning
and not perform any conversion.
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.cpp
@@ -44,6 +44,7 @@ void UseStdFormatCheck::registerPPCallbacks(const SourceManager &SM,
                                             Preprocessor *PP,
                                             Preprocessor *ModuleExpanderPP) {
   IncludeInserter.registerPreprocessor(PP);
+  this->PP = PP;
 }
 
 void UseStdFormatCheck::registerMatchers(MatchFinder *Finder) {
@@ -75,9 +76,9 @@ void UseStdFormatCheck::check(const MatchFinder::MatchResult &Result) {
 
   utils::FormatStringConverter::Configuration ConverterConfig;
   ConverterConfig.StrictMode = StrictMode;
-  utils::FormatStringConverter Converter(Result.Context, StrFormat,
-                                         FormatArgOffset, ConverterConfig,
-                                         getLangOpts());
+  utils::FormatStringConverter Converter(
+      Result.Context, StrFormat, FormatArgOffset, ConverterConfig,
+      getLangOpts(), *Result.SourceManager, *PP);
   const Expr *StrFormatCall = StrFormat->getCallee();
   if (!Converter.canApply()) {
     diag(StrFormat->getBeginLoc(),
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.h b/clang-tools-extra/clang-tidy/modernize/UseStdFormatCheck.h
@@ -44,6 +44,7 @@ class UseStdFormatCheck : public ClangTidyCheck {
   StringRef ReplacementFormatFunction;
   utils::IncludeInserter IncludeInserter;
   std::optional<StringRef> MaybeHeaderToInclude;
+  Preprocessor *PP = nullptr;
 };
 
 } // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.cpp
@@ -68,6 +68,7 @@ void UseStdPrintCheck::registerPPCallbacks(const SourceManager &SM,
                                            Preprocessor *PP,
                                            Preprocessor *ModuleExpanderPP) {
   IncludeInserter.registerPreprocessor(PP);
+  this->PP = PP;
 }
 
 static clang::ast_matchers::StatementMatcher
@@ -131,7 +132,8 @@ void UseStdPrintCheck::check(const MatchFinder::MatchResult &Result) {
   ConverterConfig.StrictMode = StrictMode;
   ConverterConfig.AllowTrailingNewlineRemoval = true;
   utils::FormatStringConverter Converter(
-      Result.Context, Printf, FormatArgOffset, ConverterConfig, getLangOpts());
+      Result.Context, Printf, FormatArgOffset, ConverterConfig, getLangOpts(),
+      *Result.SourceManager, *PP);
   const Expr *PrintfCall = Printf->getCallee();
   const StringRef ReplacementFunction = Converter.usePrintNewlineFunction()
                                             ? ReplacementPrintlnFunction
diff --git a/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h b/clang-tools-extra/clang-tidy/modernize/UseStdPrintCheck.h
@@ -36,6 +36,7 @@ class UseStdPrintCheck : public ClangTidyCheck {
   }
 
 private:
+  Preprocessor *PP;
   bool StrictMode;
   std::vector<StringRef> PrintfLikeFunctions;
   std::vector<StringRef> FprintfLikeFunctions;
diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.cpp
@@ -18,6 +18,7 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/FixIt.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Debug.h"
@@ -195,11 +196,10 @@ static bool castMismatchedIntegerTypes(const CallExpr *Call, bool StrictMode) {
   return false;
 }
 
-FormatStringConverter::FormatStringConverter(ASTContext *ContextIn,
-                                             const CallExpr *Call,
-                                             unsigned FormatArgOffset,
-                                             const Configuration ConfigIn,
-                                             const LangOptions &LO)
+FormatStringConverter::FormatStringConverter(
+    ASTContext *ContextIn, const CallExpr *Call, unsigned FormatArgOffset,
+    const Configuration ConfigIn, const LangOptions &LO, SourceManager &SM,
+    Preprocessor &PP)
     : Context(ContextIn), Config(ConfigIn),
       CastMismatchedIntegerTypes(
           castMismatchedIntegerTypes(Call, ConfigIn.StrictMode)),
@@ -208,11 +208,22 @@ FormatStringConverter::FormatStringConverter(ASTContext *ContextIn,
   assert(ArgsOffset <= NumArgs);
   FormatExpr = llvm::dyn_cast<StringLiteral>(
       Args[FormatArgOffset]->IgnoreImplicitAsWritten());
+
   if (!FormatExpr || !FormatExpr->isOrdinary()) {
     // Function must have a narrow string literal as its first argument.
     conversionNotPossible("first argument is not a narrow string literal");
     return;
   }
+
+  if (const std::optional<StringRef> MaybeMacroName =
+          formatStringContainsUnreplaceableMacro(Call, FormatExpr, SM, PP);
+      MaybeMacroName) {
+    conversionNotPossible(
+        ("format string contains unreplaceable macro '" + *MaybeMacroName + "'")
+            .str());
+    return;
+  }
+
   PrintfFormatString = FormatExpr->getString();
 
   // Assume that the output will be approximately the same size as the input,
@@ -230,6 +241,50 @@ FormatStringConverter::FormatStringConverter(ASTContext *ContextIn,
   finalizeFormatText();
 }
 
+std::optional<StringRef>
+FormatStringConverter::formatStringContainsUnreplaceableMacro(
+    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());
+
+      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;
+}
+
 void FormatStringConverter::emitAlignment(const PrintfSpecifier &FS,
                                           std::string &FormatSpec) {
   ConversionSpecifier::Kind ArgKind = FS.getConversionSpecifier().getKind();
diff --git a/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h b/clang-tools-extra/clang-tidy/utils/FormatStringConverter.h
@@ -40,7 +40,8 @@ class FormatStringConverter
 
   FormatStringConverter(ASTContext *Context, const CallExpr *Call,
                         unsigned FormatArgOffset, Configuration Config,
-                        const LangOptions &LO);
+                        const LangOptions &LO, SourceManager &SM,
+                        Preprocessor &PP);
 
   bool canApply() const { return ConversionNotPossibleReason.empty(); }
   const std::string &conversionNotPossibleReason() const {
@@ -110,6 +111,10 @@ class FormatStringConverter
 
   void appendFormatText(StringRef Text);
   void finalizeFormatText();
+  static std::optional<StringRef>
+  formatStringContainsUnreplaceableMacro(const CallExpr *CallExpr,
+                                         const StringLiteral *FormatExpr,
+                                         SourceManager &SM, Preprocessor &PP);
   bool conversionNotPossible(std::string Reason) {
     ConversionNotPossibleReason = std::move(Reason);
     return false;
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -198,11 +198,13 @@ Changes in existing checks
 
 - Improved :doc:`modernize-use-std-format
   <clang-tidy/checks/modernize/use-std-format>` check to support replacing
-  member function calls too.
+  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-std-print
   <clang-tidy/checks/modernize/use-std-print>` check to support replacing
-  member function calls too.
+  member function calls too and to only expand macros starting with ``PRI``
+  and ``__PRI`` from ``<inttypes.h>`` in the format string.
 
 - Improved :doc:`performance-avoid-endl
   <clang-tidy/checks/performance/avoid-endl>` check to use ``std::endl`` as
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-format.rst
@@ -23,7 +23,8 @@ into:
 
 The check uses the same format-string-conversion algorithm as
 `modernize-use-std-print <../modernize/use-std-print.html>`_ and its
-shortcomings are described in the documentation for that check.
+shortcomings and behaviour in combination with macros are described in the
+documentation for that check.
 
 Options
 -------
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-std-print.rst
@@ -24,8 +24,15 @@ into:
   std::println(stderr, "The {} is {:3}", description, value);
 
 If the `ReplacementPrintFunction` or `ReplacementPrintlnFunction` options
-are left, or assigned to their default values then this check is only
-enabled with `-std=c++23` or later.
+are left at or set to their default values then this check is only enabled
+with `-std=c++23` or later.
+
+Macros starting with ``PRI`` and ``__PRI`` from `<inttypes.h>` are
+expanded, escaping is handled and adjacent strings are concatenated to form
+a single ``StringLiteral`` before the format string is converted. Use of
+any other macros in the format string will cause a warning message to be
+emitted and no conversion will be performed. The converted format string
+will always be a single string literal.
 
 The check doesn't do a bad job, but it's not perfect. In particular:
 
@@ -34,13 +41,10 @@ The check doesn't do a bad job, but it's not perfect. In particular:
   possible.
 
 - At the point that the check runs, the AST contains a single
-  ``StringLiteral`` for the format string and any macro expansion, token
-  pasting, adjacent string literal concatenation and escaping has been
-  handled. Although it's possible for the check to automatically put the
-  escapes back, they may not be exactly as they were written (e.g.
-  ``"\x0a"`` will become ``"\n"`` and ``"ab" "cd"`` will become
-  ``"abcd"``.) This is helpful since it means that the ``PRIx`` macros from
-  ``<inttypes.h>`` are removed correctly.
+  ``StringLiteral`` for the format string where escapes have been expanded.
+  The check tries to reconstruct escape sequences, they may not be the same
+  as they were written (e.g. ``"\x41\x0a"`` will become ``"A\n"`` and
+  ``"ab" "cd"`` will become ``"abcd"``.)
 
 - It supports field widths, precision, positional arguments, leading zeros,
   leading ``+``, alignment and alternative forms.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/inttypes.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/inttypes.h
@@ -21,40 +21,46 @@ typedef __UINT32_TYPE__ uint32_t;
 typedef __UINT16_TYPE__ uint16_t;
 typedef __UINT8_TYPE__ uint8_t;
 
-#define PRIdMAX "lld"
-#define PRId64 "lld"
+#if __WORDSIZE == 64
+# define __PRI64_PREFIX	"l"
+#else
+# define __PRI64_PREFIX	"ll"
+#endif
+
+#define PRIdMAX __PRI64_PREFIX "d"
+#define PRId64 __PRI64_PREFIX "d"
 #define PRId32 "d"
 #define PRId16 "hd"
 #define PRId8  "hhd"
 
-#define PRIiMAX "lli"
-#define PRIi64 "lli"
+#define PRIiMAX __PRI64_PREFIX "i"
+#define PRIi64 __PRI64_PREFIX "i"
 #define PRIi32 "i"
 #define PRIi16 "hi"
 #define PRIi8  "hhi"
 
-#define PRIiFAST64 "lli"
+#define PRIiFAST64 __PRI64_PREFIX "i"
 #define PRIiFAST32 "i"
 #define PRIiFAST16 "hi"
 #define PRIiFAST8  "hhi"
 
-#define PRIiLEAST64 "lli"
+#define PRIiLEAST64 __PRI64_PREFIX "i"
 #define PRIiLEAST32 "i"
 #define PRIiLEAST16 "hi"
 #define PRIiLEAST8  "hhi"
 
-#define PRIuMAX "llu"
-#define PRIu64 "llu"
+#define PRIuMAX __PRI64_PREFIX "u"
+#define PRIu64 __PRI64_PREFIX "u"
 #define PRIu32 "u"
 #define PRIu16 "hu"
 #define PRIu8  "hhu"
 
-#define PRIuFAST64 "llu"
+#define PRIuFAST64 __PRI64_PREFIX "u"
 #define PRIuFAST32 "u"
 #define PRIuFAST16 "hu"
 #define PRIuFAST8  "hhu"
 
-#define PRIuLEAST64 "llu"
+#define PRIuLEAST64 __PRI64_PREFIX "u"
 #define PRIuLEAST32 "u"
 #define PRIuLEAST16 "hu"
 #define PRIuLEAST8  "hhu"
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-std-format.cpp
@@ -1,13 +1,18 @@
 // RUN: %check_clang_tidy \
 // RUN:   -std=c++20 %s modernize-use-std-format %t -- \
 // RUN:   -config="{CheckOptions: {StrictMode: true}}" \
-// RUN:   -- -isystem %clang_tidy_headers
+// RUN:   -- -isystem %clang_tidy_headers \
+// RUN:      -DPRI_CMDLINE_MACRO="\"s\"" \
+// RUN:      -D__PRI_CMDLINE_MACRO="\"s\""
 // RUN: %check_clang_tidy \
 // RUN:   -std=c++20 %s modernize-use-std-format %t -- \
 // RUN:   -config="{CheckOptions: {StrictMode: false}}" \
-// RUN:   -- -isystem %clang_tidy_headers
+// RUN:   -- -isystem %clang_tidy_headers \
+// RUN:      -DPRI_CMDLINE_MACRO="\"s\"" \
+// RUN:      -D__PRI_CMDLINE_MACRO="\"s\""
 #include <string>
 // CHECK-FIXES: #include <format>
+#include <inttypes.h>
 
 namespace absl
 {
@@ -102,18 +107,74 @@ std::string StrFormat_macros() {
   // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use 'std::format' instead of 'StrFormat' [modernize-use-std-format]
   // CHECK-FIXES: std::format("Hello {}", 42);
 
-  // The format string is replaced even though it comes from a macro, this
-  // behaviour is required so that that <inttypes.h> macros are replaced.
-#define FORMAT_STRING "Hello %s"
-  auto s2 = absl::StrFormat(FORMAT_STRING, 42);
-  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use 'std::format' instead of 'StrFormat' [modernize-use-std-format]
-  // CHECK-FIXES: std::format("Hello {}", 42);
-
   // Arguments that are macros aren't replaced with their value, even if they are rearranged.
 #define VALUE 3.14159265358979323846
 #define WIDTH 10
 #define PRECISION 4
   auto s3 = absl::StrFormat("Hello %*.*f", WIDTH, PRECISION, VALUE);
   // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use 'std::format' instead of 'StrFormat' [modernize-use-std-format]
   // CHECK-FIXES: std::format("Hello {:{}.{}f}", VALUE, WIDTH, PRECISION);
+
+  const uint64_t u64 = 42;
+  const uint32_t u32 = 32;
+  std::string s;
+
+  auto s4 = absl::StrFormat("Replaceable macro at end %" PRIu64, u64);
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use 'std::format' instead of 'StrFormat' [modernize-use-std-format]
+  // CHECK-FIXES: std::format("Replaceable macro at end {}", u64);
+
+  auto s5 = absl::StrFormat("Replaceable macros in middle %" PRIu64 " %" PRIu32 "\n", u64, u32);
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: use 'std::format' instead of 'StrFormat' [modernize-use-std-format]
+  // CHECK-FIXES: std::format("Replaceable macros in middle {} {}\n", u64, u32);
+
+// These need PRI and __PRI prefixes so that the check get as far as looking for
+// where the macro comes from.
+#define PRI_FMT_MACRO "s"
+#define __PRI_FMT_MACRO "s"
+
+  auto s6 = absl::StrFormat("Unreplaceable macro at end %" PRI_FMT_MACRO, s.c_str());
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: unable to use 'std::format' instead of 'StrFormat' because format string contains unreplaceable macro 'PRI_FMT_MACRO' [modernize-use-std-format]
+
+  auto s7 = absl::StrFormat(__PRI_FMT_MACRO " Unreplaceable macro at beginning %s", s);
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: unable to use 'std::format' instead of 'StrFormat' because format string contains unreplaceable macro '__PRI_FMT_MACRO' [modernize-use-std-format]
+
+  auto s8 = absl::StrFormat("Unreplacemable macro %" PRI_FMT_MACRO " in the middle", s);
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: unable to use 'std::format' instead of 'StrFormat' because format string contains unreplaceable macro 'PRI_FMT_MACRO' [modernize-use-std-format]
+
+  auto s9 = absl::StrFormat("First macro is replaceable %" PRIu64 " but second one is not %" __PRI_FMT_MACRO, u64, s);
+  // CHECK-MESSAGES: [[@LINE-1]]:13: warning: unable to use 'std::format' instead of 'StrFormat' because format string contains unreplaceable macro '__PRI_FMT_MACRO' [modernize-use-std-format]
+
+  // Needs a PRI prefix so that we get as far as looking for where the macro comes from
+  auto s10 = 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]
+
+  // 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]
 }
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

Original file line number	Diff line number	Diff line change
`@@ -36,6 +36,7 @@ class UseStdPrintCheck : public ClangTidyCheck {`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`private:`
	`39`	`+ Preprocessor *PP;`
`39`	`40`	`bool StrictMode;`
`40`	`41`	`std::vector<StringRef> PrintfLikeFunctions;`
`41`	`42`	`std::vector<StringRef> FprintfLikeFunctions;`