Merge pull request #9269 from JosJuice/fmt-positional-checks

Common: Assert that translatable strings use positional arguments

Source/Core/Common/FormatUtil.h

@@ -38,4 +38,39 @@ constexpr std::size_t CountFmtReplacementFields(std::string_view s)
 static_assert(CountFmtReplacementFields("") == 0);
 static_assert(CountFmtReplacementFields("{} test {:x}") == 2);
 static_assert(CountFmtReplacementFields("{} {{}} test {{{}}}") == 2);
+
+constexpr bool ContainsNonPositionalArguments(std::string_view s)
+{
+  for (std::size_t i = 0; i < s.size(); ++i)
+  {
+    if (s[i] != '{' || i + 1 == s.size())
+      continue;
+
+    const char next = s[i + 1];
+
+    // If the opening brace is followed by another brace, what we have is
+    // an escaped brace, not a replacement field.
+    if (next == '{')
+    {
+      // Skip the second brace.
+      // This ensures that e.g. {{{}}} is counted correctly: when the first brace character
+      // is read and detected as being part of an '{{' escape sequence, the second character
+      // is skipped so the most inner brace (the third character) is not detected
+      // as the end of an '{{' pair.
+      ++i;
+    }
+    else if (next == '}' || next == ':')
+    {
+      return true;
+    }
+  }
+  return false;
+}
+
+static_assert(!ContainsNonPositionalArguments(""));
+static_assert(ContainsNonPositionalArguments("{}"));
+static_assert(!ContainsNonPositionalArguments("{0}"));
+static_assert(ContainsNonPositionalArguments("{:x}"));
+static_assert(!ContainsNonPositionalArguments("{0:x}"));
+static_assert(!ContainsNonPositionalArguments("{0} {{}} test {{{1}}}"));
 }  // namespace Common

Source/Core/Common/MsgHandler.h

@@ -102,6 +102,16 @@ std::string FmtFormatT(const char* string, Args&&... args)
                                                     ##__VA_ARGS__);                                \
   }()
 
+#define GenericAlertFmtT(yes_no, style, format, ...)                                               \
+  [&] {                                                                                            \
+    static_assert(!Common::ContainsNonPositionalArguments(format),                                 \
+                  "Translatable strings must use positional arguments (e.g. {0} instead of {})");  \
+    /* Use a macro-like name to avoid shadowing warnings */                                        \
+    constexpr auto GENERIC_ALERT_FMT_N = Common::CountFmtReplacementFields(format);                \
+    return Common::MsgAlertFmt<GENERIC_ALERT_FMT_N>(yes_no, style, FMT_STRING(format),             \
+                                                    ##__VA_ARGS__);                                \
+  }()
+
 #define SuccessAlertFmt(format, ...)                                                               \
   GenericAlertFmt(false, Common::MsgType::Information, format, ##__VA_ARGS__)
 
@@ -119,16 +129,16 @@ std::string FmtFormatT(const char* string, Args&&... args)
 
 // Use these macros (that do the same thing) if the message should be translated.
 #define SuccessAlertFmtT(format, ...)                                                              \
-  GenericAlertFmt(false, Common::MsgType::Information, format, ##__VA_ARGS__)
+  GenericAlertFmtT(false, Common::MsgType::Information, format, ##__VA_ARGS__)
 
 #define PanicAlertFmtT(format, ...)                                                                \
-  GenericAlertFmt(false, Common::MsgType::Warning, format, ##__VA_ARGS__)
+  GenericAlertFmtT(false, Common::MsgType::Warning, format, ##__VA_ARGS__)
 
 #define PanicYesNoFmtT(format, ...)                                                                \
-  GenericAlertFmt(true, Common::MsgType::Warning, format, ##__VA_ARGS__)
+  GenericAlertFmtT(true, Common::MsgType::Warning, format, ##__VA_ARGS__)
 
 #define AskYesNoFmtT(format, ...)                                                                  \
-  GenericAlertFmt(true, Common::MsgType::Question, format, ##__VA_ARGS__)
+  GenericAlertFmtT(true, Common::MsgType::Question, format, ##__VA_ARGS__)
 
 #define CriticalAlertFmtT(format, ...)                                                             \
-  GenericAlertFmt(false, Common::MsgType::Critical, format, ##__VA_ARGS__)
+  GenericAlertFmtT(false, Common::MsgType::Critical, format, ##__VA_ARGS__)

Source/Core/Core/HW/DVD/DVDInterface.cpp

@@ -1209,7 +1209,7 @@ void ExecuteCommand(ReplyType reply_type)
   default:
     ERROR_LOG_FMT(DVDINTERFACE, "Unknown command {:#010x} (Buffer {:#010x}, {:#x})", s_DICMDBUF[0],
                   s_DIMAR, s_DILENGTH);
-    PanicAlertFmtT("Unknown DVD command {:08x} - fatal error", s_DICMDBUF[0]);
+    PanicAlertFmtT("Unknown DVD command {0:08x} - fatal error", s_DICMDBUF[0]);
     SetDriveError(DriveError::InvalidCommand);
     interrupt_type = DIInterruptType::DEINT;
     break;

Source/Core/Core/HW/DVD/DVDThread.cpp

@@ -344,7 +344,7 @@ static void FinishRead(u64 id, s64 cycles_late)
   DVDInterface::DIInterruptType interrupt;
   if (buffer.size() != request.length)
   {
-    PanicAlertFmtT("The disc could not be read (at {:#x} - {:#x}).", request.dvd_offset,
+    PanicAlertFmtT("The disc could not be read (at {0:#x} - {1:#x}).", request.dvd_offset,
                    request.dvd_offset + request.length);
 
     DVDInterface::SetDriveError(DVDInterface::DriveError::BlockOOB);

Source/Core/VideoCommon/CommandProcessor.cpp

@@ -525,7 +525,7 @@ void SetCpClearRegister()
 void HandleUnknownOpcode(u8 cmd_byte, void* buffer, bool preprocess)
 {
   // TODO(Omega): Maybe dump FIFO to file on this error
-  PanicAlertFmtT("GFX FIFO: Unknown Opcode ({:#04x} @ {}, {}).\n"
+  PanicAlertFmtT("GFX FIFO: Unknown Opcode ({0:#04x} @ {1}, {2}).\n"
                  "This means one of the following:\n"
                  "* The emulated GPU got desynced, disabling dual core can help\n"
                  "* Command stream corrupted by some spurious memory bug\n"

Source/Core/VideoCommon/ImageWrite.cpp

@@ -51,7 +51,7 @@ bool TextureToPng(const u8* data, int row_stride, const std::string& filename, i
   File::IOFile fp(filename, "wb");
   if (!fp.IsOpen())
   {
-    PanicAlertFmtT("Screenshot failed: Could not open file \"{}\" (error {})", filename, errno);
+    PanicAlertFmtT("Screenshot failed: Could not open file \"{0}\" (error {1})", filename, errno);
     goto finalise;
   }
 

Source/Core/VideoCommon/RenderBase.cpp

@@ -1666,7 +1666,7 @@ bool Renderer::StartFrameDumpToImage(const FrameDump::FrameData&)
     std::string filename = GetFrameDumpNextImageFileName();
     if (File::Exists(filename))
     {
-      if (!AskYesNoFmtT("Frame dump image(s) '{}' already exists. Overwrite?", filename))
+      if (!AskYesNoFmtT("Frame dump image(s) '{0}' already exists. Overwrite?", filename))
         return false;
     }
   }