tinyformat: Add compile-time checking for literal format strings #31174

nit in 97dd5fe5128592332c83998825bbeda063815120: in the commit message: Missing needed *in the* next commit.

Also, it would be good to add some failing test cases for this stuff.

For example, "%1" should fail, because terminate called after throwing an instance of 'tinyformat::format_error' what(): tinyformat: Conversion spec incorrectly terminated by end of string. See https://godbolt.org/z/1eehh1oTv . Also see 51c56e8b38033b96fb3930c2bcba3add2047d324

Other failing test cases to possibly add: "%*", "%+*", "%.*", ...

Also, some more "fancy" passing test cases:

• PassFmt<3>("'%1$- 0+*3$.*2$f'"); (space and 0 are ignored, confusing, but valid)
• PassFmt<3>("'%- 0+*.*f'"); (same, without pos specs)

re: #31174 (review)

Thanks, I added the suggested test cases. The suggested tests that "should fail" didn't actually fail with 97dd5fe5128592332c83998825bbeda063815120 because it accepted \0 as a valid specifier character, so I added an extra check to prevent that. I also added extra code to consume digits after . otherwise format strings like "%1.2" would be accepted treating 2 as the specifier.

Nit in 1d16d6e6bac994fed7c695f530b9984edcd290bd: I don't really see why this is useful. Maybe I am missing something, but everything compiles by just moving the two members to the derived struct:

diff --git a/src/tinyformat.h b/src/tinyformat.h
index 56c25d8f7f..6227cdeca0 100644
--- a/src/tinyformat.h
+++ b/src/tinyformat.h
@@ -179,21 +179,16 @@ namespace tfm = tinyformat;
 
 namespace tinyformat {
 
-// Added for Bitcoin Core. Wrapper type for format strings to allow compile-time checks.
-struct FormatString {
-    const char* fmt;
-    operator const char*() { return fmt; }
-};
-
 // 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.
 template <unsigned num_params>
-struct FormatStringCheck : FormatString {
-    consteval FormatStringCheck(const char* str) : FormatString{util::ConstevalFormatString<num_params>{str}.fmt} {}
-    FormatStringCheck(const std::string& str) : FormatString{str.c_str()} {}
-    FormatStringCheck(FormatString str) : FormatString{str} {}
-    FormatStringCheck(util::ConstevalFormatString<num_params> str) : FormatString{str.fmt} {}
+struct FormatStringCheck {
+    consteval FormatStringCheck(const char* str) : fmt{util::ConstevalFormatString<num_params>{str}.fmt} {}
+    FormatStringCheck(const std::string& str) : fmt{str.c_str()} {}
+    FormatStringCheck(util::ConstevalFormatString<num_params> str) : fmt{str.fmt} {}
+    const char* fmt;
+    operator const char*() { return fmt; }
 };
 
 // Added for Bitcoin Core

re: #31174 (review)

Thanks, applied patch. The reason for having a FormatString class was to provide a cleaner escape from compile-time checks strprintf(FormatString{"%*s"}, width, str) before the first commit was implemented. But it's no longer necessary after that commit.

we still have unbounded increment without checking the end (I though we've fixed this already, maybe it got stuck in the comments...)

re: #31174 (review)

we still have unbounded increment without checking the end (I though we've fixed this already, maybe it got stuck in the comments...)

Thanks. I fixed this by just switching the string type to const char* instead of string_view since tinyformat already assumes the string is null terminated.

I think it would be possible to write a clean version of this code that used string_view, but it would have to be structured differently to avoid the need for it < str.end() checks everywhere.

Found the PR thanks to @hodlinator, this is how I handled that case: https://github.com/bitcoin/bitcoin/pull/30999/files#diff-71badc1cc71ba46244f7841a088251bb294265f4fe9e662c0ad6b15be540eee4R60

We could a test case for the incomplete trailing number (or any other test that seems relevant here): https://github.com/bitcoin/bitcoin/pull/30999/files#diff-718d0d85269ec81595bd9f9181eea3a74b20244b07f14c546e3e07520b2b5f82R81

Isn't that done here? https://github.com/bitcoin/bitcoin/blob/ad37073f2e6ab1f39a59109692f84cc85809f53e/src/test/util_string_tests.cpp#L90

It is indeed now, thanks for checking.

                }

re: #31174 (review)

Thanks, updated

Should this be removed?

re: #31174 (review)

Should this be removed?

I think it's useful to document the format of the specifier, and it's still true that the remainder of the specifier (length and type) is not checked. Happy to update this if there's a specific suggestion, but I think the comment is still helpful and accurate so would not want to remove it.

As you say, only the length and type are no longer checked. This PR now implements rudimentary checking of flags, width and precision.

            // Length and type in "[flags][width][.precision][length]type"
            // is not checked. Parsing continues with the next '%'.

re: #31174 (review)

As you say, only the length and type are no longer checked. This PR now implements rudimentary checking of flags, width and precision.

Thanks, applied suggestion

(One could expect that at least one precision-digit was required after '.' but it is not in tinyformat so this behavior is consistent).

re: #31174 (review)

(One could expect that at least one precision-digit was required after '.' but it is not in tinyformat so this behavior is consistent).

That's good to know, I was just trying to make the parsing as simple as possible, but this syntax does seem to be commonly accepted (it works in python too) and I added test cases for it following your other suggestion

Why not use FormatStringCheck<sizeof...(Args)> in printf and printfln directly below as well?

re: #31174 (review)

Why not use FormatStringCheck<sizeof...(Args)> in printf and printfln directly below as well?

No reason, added now.

Could also add

PassFmt<1>("%.f");

maybe grouped with the <1> calls above. Confirmed tinyformat allows for it, see how the success status returned by parseWidthOrPrecision is ignored: https://github.com/bitcoin/bitcoin/blob/e53829d3952c6ed275507a66e77636aad67d106b/src/tinyformat.h#L778-L788

re: #31174 (review)

PassFmt<1>("%.f");

Nice suggestion, added two cases: %5.f and %.f below %5.2f

Could add new failing cases here as well?

FailFmtWithError<2>("%2$*$d", err_0_pos);
FailFmtWithError<2>("%2$*0$d", err_0_pos);
FailFmtWithError<3>("%3$*2$.*$f", err_0_pos);
FailFmtWithError<3>("%3$*2$.*0$f", err_0_pos);

re: #31174 (review)

Thanks, added

Same:

FailFmtWithError<2>("%2$*d", err_mix);
FailFmtWithError<2>("%*2$d", err_mix);
FailFmtWithError<2>("%.*3$d", err_mix);
FailFmtWithError<2>("%2$.*d", err_mix);

Same (below):

    FailFmtWithError<1>("%*c", err_num);

re: #31174 (review)

Thanks, added

Given that we have two separate number "parsers" (one that keeps the result and one that throws it away), we might as well extract number parsing to a local lambda like you did with the other ones.

<details> <summary>Diff</summary>

diff --git a/src/util/string.h b/src/util/string.h
--- a/src/util/string.h	(revision ecc5cb9a89c6b001df839675b23d8fc1f7ac69ba)
+++ b/src/util/string.h	(date 1731267170701)
@@ -45,14 +45,16 @@
                 continue;
             }
 
+            auto parse_number = [&] {
+                unsigned num{0};
+                for (; '0' <= *it && *it <= '9'; ++it) {
+                    num = num * 10 + (*it - '0');
+                }
+                return num;
+            };
+
             auto add_arg = [&] {
-                unsigned maybe_num{0};
-                while ('0' <= *it && *it <= '9') {
-                    maybe_num *= 10;
-                    maybe_num += *it - '0';
-                    ++it;
-                }
-
+                unsigned maybe_num = parse_number();
                 if (*it == '$') {
                     ++it;
                     // Positional specifier, like %8$s
@@ -75,7 +77,7 @@
                     ++it;
                     add_arg();
                 } else {
-                    while ('0' <= *it && *it <= '9') ++it;
+                    parse_number();
                 }
             };

</details>

re: #31174 (review)

we might as well extract number parsing to a local lambda like you did with the other ones.

This seems reasonable but I"m not sure it's clearer, and it does make the diff bigger replacing the maybe_num code that doesn't have to change currently. Happy to apply this suggestion if other reviews think it is a good idea.

@hodlinator, @maflcko, what do you think? I can ack without this as well, but I'd prefer reducing duplication.

I think while ('0' <= *it && *it <= '9') ++it; is fine. It is pretty standard self-explanatory code. I don't think a one-line while loop needs to be de-duplicated. Also, I like that the diff is minimal as-is now.

These are all here to check if we're inside a format string, but we don't have a %%, right? Could we maybe simplify that to something like:

            if (*it != '%' || *(++it) == '%') continue; // Skip escaped %%

?

re: #31174 (review)

Nice suggestion! Applied

In C++23 this could be a simple .contains, but even in C++20 we should be able to group the flags to something like:

            while ("#0- +"sv.find(*it) != std::string_view::npos) ++it;

(we could even extract the flag in which case we could get rid of the comment)

Time to return to C89? ;)

            while (strchr("#0- +", *it)) ++it;

I thought of that, but not sure how to make it work, I'm getting:

note: non-constexpr function 'strchr' cannot be used in a constant expression

Still rebooting :brain: for this week, sorry for the noise.

re: #31174 (review)

I can apply the "#0- +"sv.find change if others like it, but to me it seems less readable and only a little shorter.

it could be more readable and a bit shorter if we extract the flags as a variable and delete the comment stating the same - what do you think?

My preference would be to leave style-only nits to a follow-up, especially given that they will be temporary anyway until C++23. This pull request is basically ready for two weeks now, with more than 50 style-only or test-only comments. Unless there are any real issues or bugs with the code, and a foce-push needs to happen anyway, I don't really see the value of asking reviewers to go through more comments and code changes, some of which don't even compile.

Checked and failures seem to be validated successfully from command line, but - unlike the previous versions - doesn't seem to be shown in the IDE... Weird :/

are the extra surrounding ' deliberate? If so, what do they mean?

re: #31174 (review)

I'm pretty sure they must be accidental. These cases came from #31174 (review), and I just pasted them without noticing the single quotes. Can remove if the PR is updated again.

The ' are used to denote the begin and the end of the string, which would otherwise not be possible, because trailing spaces can normally not be seen when printing. They are not needed in this test and they are a leftover when I tested this against tinyformat at runtime.