Merge bitcoin/bitcoin#31061: refactor: Check translatable format strings at compile-time

fanquake · fanquake · commit 712cab3a8f8a · 2025-01-15T15:47:00.000Z
fa3efb5 refactor: Introduce struct to hold a runtime format string (MarcoFalke) fa6adb0 lint: Remove unused and broken format string linter (MarcoFalke) fadc6b9 refactor: Check translatable format strings at compile-time (MarcoFalke) fa1d5ac refactor: Use TranslateFn type consistently (MarcoFalke) eeee6cf refactor: Delay translation of _() literals (MarcoFalke) Pull request description: All translatable format strings are fixed. This change surfaces errors in them at compile-time. The implementation achieves this by allowing to delay the translation (or `std::string` construction) that previously happened in `_()` by returning a new type from this function. The new type can be converted to `bilingual_str` where needed. This can be tested by adding a format string error in an original string literal and observing a new compile-time failure. Fixes bitcoin/bitcoin#30530 ACKs for top commit: stickies-v: re-ACK fa3efb5 ryanofsky: Code review ACK fa3efb5. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant `inline` keyword Tree-SHA512: 28fa1db11e85935d998031347bd519675d75c171c8323b0ed6cdd0b628c95250bb86b30876946cc48840ded541e95b8a152696f9f2b13a5f28f5673228ee0509
diff --git a/src/banman.cpp b/src/banman.cpp
@@ -30,7 +30,7 @@ void BanMan::LoadBanlist()
 {
     LOCK(m_banned_mutex);
 
-    if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);
+    if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…"));
 
     const auto start{SteadyClock::now()};
     if (m_ban_db.Read(m_banned)) {
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
@@ -50,7 +50,7 @@ using util::ToString;
 // just use a plain system_clock.
 using CliClock = std::chrono::system_clock;
 
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
 static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
@@ -41,7 +41,7 @@ static bool fCreateBlank;
 static std::map<std::string,UniValue> registers;
 static const int CONTINUE_EXECUTION=-1;
 
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 static void SetupBitcoinTxArgs(ArgsManager &argsman)
 {
diff --git a/src/bitcoin-util.cpp b/src/bitcoin-util.cpp
@@ -26,7 +26,7 @@
 
 static const int CONTINUE_EXECUTION=-1;
 
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 static void SetupBitcoinUtilArgs(ArgsManager &argsman)
 {
diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp
@@ -26,7 +26,7 @@
 
 using util::Join;
 
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 static void SetupWalletToolArgs(ArgsManager& argsman)
 {
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
@@ -34,7 +34,7 @@
 
 using node::NodeContext;
 
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 #if HAVE_DECL_FORK
 
diff --git a/src/clientversion.cpp b/src/clientversion.cpp
@@ -95,7 +95,7 @@ std::string LicenseInfo()
            strprintf(_("The source code is available from %s."), URL_SOURCE_CODE).translated +
            "\n" +
            "\n" +
-           _("This is experimental software.").translated + "\n" +
+           _("This is experimental software.") + "\n" +
            strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s"), "COPYING", "<https://opensource.org/licenses/MIT>").translated +
            "\n";
 }
diff --git a/src/init.cpp b/src/init.cpp
@@ -1244,8 +1244,8 @@ static ChainstateLoadResult InitAndLoadChainstate(
             _("Error reading from database, shutting down."),
             "", CClientUIInterface::MSG_ERROR);
     };
-    uiInterface.InitMessage(_("Loading block index…").translated);
-    auto catch_exceptions = [](auto&& f) {
+    uiInterface.InitMessage(_("Loading block index…"));
+    auto catch_exceptions = [](auto&& f) -> ChainstateLoadResult {
         try {
             return f();
         } catch (const std::exception& e) {
@@ -1255,7 +1255,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
     };
     auto [status, error] = catch_exceptions([&] { return LoadChainstate(chainman, cache_sizes, options); });
     if (status == node::ChainstateLoadStatus::SUCCESS) {
-        uiInterface.InitMessage(_("Verifying blocks…").translated);
+        uiInterface.InitMessage(_("Verifying blocks…"));
         if (chainman.m_blockman.m_have_pruned && options.check_blocks > MIN_BLOCKS_TO_KEEP) {
             LogWarning("pruned datadir may not have more than %d blocks; only checking available blocks\n",
                        MIN_BLOCKS_TO_KEEP);
@@ -1418,7 +1418,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
         // Initialize addrman
         assert(!node.addrman);
-        uiInterface.InitMessage(_("Loading P2P addresses…").translated);
+        uiInterface.InitMessage(_("Loading P2P addresses…"));
         auto addrman{LoadAddrman(*node.netgroupman, args)};
         if (!addrman) return InitError(util::ErrorString(addrman));
         node.addrman = std::move(*addrman);
@@ -1703,7 +1703,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
         if (chainman.m_blockman.m_blockfiles_indexed) {
             LOCK(cs_main);
             for (Chainstate* chainstate : chainman.GetAll()) {
-                uiInterface.InitMessage(_("Pruning blockstore…").translated);
+                uiInterface.InitMessage(_("Pruning blockstore…"));
                 chainstate->PruneAndFlush();
             }
         }
@@ -1999,7 +1999,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     // ChainstateManager's active tip.
     SetRPCWarmupFinished();
 
-    uiInterface.InitMessage(_("Done loading").translated);
+    uiInterface.InitMessage(_("Done loading"));
 
     for (const auto& client : node.chain_clients) {
         client->start(scheduler);
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
@@ -1,10 +1,11 @@
 // Copyright (c) 2022 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
+#include <util/translation.h>
 
 #include <functional>
 #include <string>
 
 // Define G_TRANSLATION_FUN symbol in libbitcoinkernel library so users of the
 // library aren't required to export this symbol
-extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+extern const TranslateFn G_TRANSLATION_FUN{nullptr};
diff --git a/src/net.cpp b/src/net.cpp
@@ -3296,7 +3296,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
     }
 
     if (m_client_interface) {
-        m_client_interface->InitMessage(_("Starting network threads…").translated);
+        m_client_interface->InitMessage(_("Starting network threads…"));
     }
 
     fAddressesInitialized = true;
diff --git a/src/qt/main.cpp b/src/qt/main.cpp
@@ -13,7 +13,7 @@
 #include <string>
 
 /** Translate string to current locale using Qt. */
-extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](const char* psz) {
+extern const TranslateFn G_TRANSLATION_FUN = [](const char* psz) {
     return QCoreApplication::translate("bitcoin-core", psz).toStdString();
 };
 
diff --git a/src/test/fuzz/strprintf.cpp b/src/test/fuzz/strprintf.cpp
@@ -14,11 +14,16 @@
 #include <string>
 #include <vector>
 
+template <typename... Args>
+void fuzz_fmt(const std::string& fmt, const Args&... args)
+{
+    (void)tfm::format(tfm::RuntimeFormat{fmt}, args...);
+}
+
 FUZZ_TARGET(str_printf)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
     const std::string format_string = fuzzed_data_provider.ConsumeRandomLengthString(64);
-    const bilingual_str bilingual_string{format_string, format_string};
 
     const int digits_in_format_specifier = std::count_if(format_string.begin(), format_string.end(), IsDigit);
 
@@ -52,28 +57,22 @@ FUZZ_TARGET(str_printf)
         CallOneOf(
             fuzzed_data_provider,
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32));
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeRandomLengthString(32).c_str());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<signed char>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<unsigned char>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<char>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<char>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<char>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeBool());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeBool());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeBool());
             });
     } catch (const tinyformat::format_error&) {
     }
@@ -98,36 +97,28 @@ FUZZ_TARGET(str_printf)
         CallOneOf(
             fuzzed_data_provider,
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeFloatingPoint<float>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeFloatingPoint<double>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<int16_t>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<uint16_t>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<int32_t>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<uint32_t>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<int64_t>());
             },
             [&] {
-                (void)strprintf(format_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
-                (void)tinyformat::format(bilingual_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
+                fuzz_fmt(format_string, fuzzed_data_provider.ConsumeIntegral<uint64_t>());
             });
     } catch (const tinyformat::format_error&) {
     }
diff --git a/src/test/fuzz/util/wallet.h b/src/test/fuzz/util/wallet.h
@@ -46,7 +46,7 @@ struct FuzzedWallet {
 
         for (const std::string& desc_fmt : DESCS) {
             for (bool internal : {true, false}) {
-                const auto descriptor{(strprintf)(desc_fmt, "[5aa9973a/66h/4h/2h]" + seed_insecure, int{internal})};
+                const auto descriptor{strprintf(tfm::RuntimeFormat{desc_fmt}, "[5aa9973a/66h/4h/2h]" + seed_insecure, int{internal})};
 
                 FlatSigningProvider keys;
                 std::string error;
diff --git a/src/test/translation_tests.cpp b/src/test/translation_tests.cpp
@@ -9,13 +9,26 @@
 
 BOOST_AUTO_TEST_SUITE(translation_tests)
 
+static TranslateFn translate{[](const char * str) {  return strprintf("t(%s)", str);  }};
+
+// Custom translation function _t(), similar to _() but internal to this test.
+consteval auto _t(util::TranslatedLiteral str)
+{
+    str.translate_fn = &translate;
+    return str;
+}
+
 BOOST_AUTO_TEST_CASE(translation_namedparams)
 {
     bilingual_str arg{"original", "translated"};
-    bilingual_str format{"original [%s]", "translated [%s]"};
-    bilingual_str result{strprintf(format, arg)};
+    bilingual_str result{strprintf(_t("original [%s]"), arg)};
     BOOST_CHECK_EQUAL(result.original, "original [original]");
-    BOOST_CHECK_EQUAL(result.translated, "translated [translated]");
+    BOOST_CHECK_EQUAL(result.translated, "t(original [translated])");
+
+    util::TranslatedLiteral arg2{"original", &translate};
+    bilingual_str result2{strprintf(_t("original [%s]"), arg2)};
+    BOOST_CHECK_EQUAL(result2.original, "original [original]");
+    BOOST_CHECK_EQUAL(result2.translated, "t(original [t(original)])");
 }
 
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -72,7 +72,7 @@ using node::LoadChainstate;
 using node::RegenerateCommitments;
 using node::VerifyLoadedChainstate;
 
-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+const TranslateFn G_TRANSLATION_FUN{nullptr};
 
 constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes a space to catch possible path escape issues.
 /** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
@@ -16,7 +16,7 @@ template <unsigned NumArgs>
 void TfmFormatZeroes(const std::string& fmt)
 {
     std::apply([&](auto... args) {
-        (void)tfm::format(fmt, args...);
+        (void)tfm::format(tfm::RuntimeFormat{fmt}, args...);
     }, std::array<int, NumArgs>{});
 }
 
diff --git a/src/tinyformat.h b/src/tinyformat.h
@@ -142,6 +142,7 @@ namespace tfm = tinyformat;
 //------------------------------------------------------------------------------
 // Implementation details.
 #include <algorithm>
+#include <attributes.h> // Added for Bitcoin Core
 #include <iostream>
 #include <sstream>
 #include <stdexcept> // Added for Bitcoin Core
@@ -179,13 +180,19 @@ namespace tfm = tinyformat;
 
 namespace tinyformat {
 
+// Added for Bitcoin Core. Similar to std::runtime_format from C++26.
+struct RuntimeFormat {
+    const std::string& fmt; // Not a string view, because tinyformat requires a c_str
+    explicit RuntimeFormat(LIFETIMEBOUND const std::string& str) : fmt{str} {}
+};
+
 // Added for Bitcoin Core. Wrapper for checking format strings at compile time.
-// Unlike ConstevalFormatString this supports std::string for runtime string
-// formatting without compile time checks.
+// Unlike ConstevalFormatString this supports RunTimeFormat-wrapped std::string
+// for runtime string formatting without compile time checks.
 template <unsigned num_params>
 struct FormatStringCheck {
     consteval FormatStringCheck(const char* str) : fmt{util::ConstevalFormatString<num_params>{str}.fmt} {}
-    FormatStringCheck(const std::string& str) : fmt{str.c_str()} {}
+    FormatStringCheck(LIFETIMEBOUND const RuntimeFormat& run) : fmt{run.fmt.c_str()} {}
     FormatStringCheck(util::ConstevalFormatString<num_params> str) : fmt{str.fmt} {}
     operator const char*() { return fmt; }
     const char* fmt;
diff --git a/src/util/translation.h b/src/util/translation.h
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py
diff --git a/test/lint/run-lint-format-strings.py b/test/lint/run-lint-format-strings.py

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ void BanMan::LoadBanlist()`
`30`	`30`	`{`
`31`	`31`	`LOCK(m_banned_mutex);`
`32`	`32`
`33`		`- if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…").translated);`
	`33`	`+ if (m_client_interface) m_client_interface->InitMessage(_("Loading banlist…"));`
`34`	`34`
`35`	`35`	`const auto start{SteadyClock::now()};`
`36`	`36`	`if (m_ban_db.Read(m_banned)) {`
Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,7 @@ static bool fCreateBlank;`
`41`	`41`	`static std::map<std::string,UniValue> registers;`
`42`	`42`	`static const int CONTINUE_EXECUTION=-1;`
`43`	`43`
`44`		`-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;`
	`44`	`+const TranslateFn G_TRANSLATION_FUN{nullptr};`
`45`	`45`
`46`	`46`	`static void SetupBitcoinTxArgs(ArgsManager &argsman)`
`47`	`47`	`{`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@`
`26`	`26`
`27`	`27`	`static const int CONTINUE_EXECUTION=-1;`
`28`	`28`
`29`		`-const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;`
	`29`	`+const TranslateFn G_TRANSLATION_FUN{nullptr};`
`30`	`30`
`31`	`31`	`static void SetupBitcoinUtilArgs(ArgsManager &argsman)`
`32`	`32`	`{`
Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ std::string LicenseInfo()`
`95`	`95`	`strprintf(_("The source code is available from %s."), URL_SOURCE_CODE).translated +`
`96`	`96`	`"\n" +`
`97`	`97`	`"\n" +`
`98`		`- _("This is experimental software.").translated + "\n" +`
	`98`	`+ _("This is experimental software.") + "\n" +`
`99`	`99`	`strprintf(_("Distributed under the MIT software license, see the accompanying file %s or %s"), "COPYING", "<https://opensource.org/licenses/MIT>").translated +`
`100`	`100`	`"\n";`
`101`	`101`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3296,7 +3296,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)`
`3296`	`3296`	`}`
`3297`	`3297`
`3298`	`3298`	`if (m_client_interface) {`
`3299`		`- m_client_interface->InitMessage(_("Starting network threads…").translated);`
	`3299`	`+ m_client_interface->InitMessage(_("Starting network threads…"));`
`3300`	`3300`	`}`
`3301`	`3301`
`3302`	`3302`	`fAddressesInitialized = true;`