test: Prove+document ConstevalFormatString/tinyformat parity #30933

pull hodlinator wants to merge 4 commits into bitcoin:master from hodlinator:2024/09/tinyformat_consteval_parity changing 2 files +40 −4
  1. hodlinator commented at 10:11 pm on September 19, 2024: contributor

    Clarifies and puts the extent of parity under test.

    Broken out from #30546 based on #30546 (review) and #30546 (review).

  2. DrahtBot commented at 10:11 pm on September 19, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30933.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, maflcko, ryanofsky

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
    • #31061 (refactor: Check translatable format strings at compile-time by maflcko)
    • #29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Tests on Sep 19, 2024
  4. hodlinator force-pushed on Sep 19, 2024
  5. DrahtBot added the label CI failed on Sep 19, 2024
  6. DrahtBot commented at 10:24 pm on September 19, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30403435103

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  7. DrahtBot removed the label CI failed on Sep 19, 2024
  8. in src/test/util_string_tests.cpp:23 in 99db776a6d outdated 
    18 {
19-    // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20-    decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
21+    // Prove parity with tinyformat
22+    switch (NumArgs) {
23+    case 0:
    maflcko commented at 6:20 am on September 20, 2024:

    style nit (feel free to ignore): I suspect you could reduce verbosity and having to hand-format all of this by using tuple_cat, see #30546 (review)

    This should also ensure that all cases are covered and wouldn’t require to change the tests vectors.

    hodlinator commented at 9:34 am on September 20, 2024:

    Not sure it would reduce verbosity that much, but would definitely consider switching if someone took the time to provide a diff.

    Compiler warnings are triggered for missing cases.

    maflcko commented at 9:51 am on September 20, 2024:

    This should also ensure that all cases are covered and wouldn’t require to change the tests vectors.

    If you don’t want to take it, that is fine. However, it would be good to at least terminate in the default case instead of silently passing. Otherwise it is unclear that all cases are covered.

    hodlinator commented at 11:36 am on September 20, 2024:

    While posting this PR I was aware of your tuple_cat suggestion. I am not against it on principle but TBH I haven’t had a clear reason to struggle through that corner of C++ and don’t see your suggestion compelling enough to work through it. Your suggestion in that comment is close to compiling but doesn’t handle sending in one arg less for the failure cases. A motivation behind this PR is to prove parity in a clear way, and I’m worried it might obfuscate it. Happy to learn more from a diff.

    Added default cases in dbbc2e52284a151a33d553b4a4032503906a4774, agree it’s more robust.

    maflcko commented at 11:45 am on September 20, 2024:
    Thanks, it is fine. I may take a look next week to recover the diff from when I wrote this earlier this month, but this minor style nit certainly isn’t important at all.
    hodlinator commented at 8:04 pm on September 24, 2024:
    Gave it another go in the latest push (071342d4c7627200323d8f22cc37ea022b2e9d69) and your direction seems to be working fine. Happy to see the linter changes go!
  9. in src/test/util_string_tests.cpp:20 in 99db776a6d outdated 
    12@@ -13,17 +13,50 @@ BOOST_AUTO_TEST_SUITE(util_string_tests)
13 // Helper to allow compile-time sanity checks while providing the number of
14 // args directly. Normally PassFmt<sizeof...(Args)> would be used.
15 template <unsigned NumArgs>
16-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
17+inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
18 {
19-    // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20-    decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
    maflcko commented at 6:21 am on September 20, 2024:
    not sure about removing this. Apart from Wunused, it is also required to be detected in coverage reports (which will obviously omit consteval stuff)
    hodlinator commented at 9:39 am on September 20, 2024:
    Good point about coverage! Error paths at runtime should be covered by FailFmtWithError, but happy path might only be run at compile time. Will bring it back.
    hodlinator commented at 10:16 am on September 20, 2024:
    Done in 2955b1a1f3eec934960e880963a09b359d828721.
  10. in test/lint/run-lint-format-strings.py:23 in 99db776a6d outdated 
    14@@ -15,6 +15,12 @@
15 FALSE_POSITIVES = [
16     ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS).translated, COPYRIGHT_HOLDERS_SUBSTITUTION)"),
17     ("src/test/translation_tests.cpp", "strprintf(format, arg)"),
18+    ("src/test/util_string_tests.cpp", "tfm::format(fmt.fmt)"),
19+    ("src/test/util_string_tests.cpp", 'tfm::format(fmt.fmt, "foo", "bar")'),
20+    ("src/test/util_string_tests.cpp", 'tfm::format(fmt.fmt, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11")'),
21+    ("src/test/util_string_tests.cpp", 'tfm::format(fmt.fmt, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12")'),
22+    ("src/test/util_string_tests.cpp", "tfm::format(fmt.fmt, a, b)"),
23+    ("src/test/util_string_tests.cpp", "tfm::format(fmt.fmt, a, b, c)"),
    maflcko commented at 6:22 am on September 20, 2024:
    Not sure about adding those. The whole point of the previous pull requests was to remove this linter (https://github.com/bitcoin/bitcoin/issues/30530). Now having follow-ups (https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547) that make it harder, albeit minimally, seems a step in the wrong direction.
    hodlinator commented at 9:35 am on September 20, 2024:
    I’ll be happy to remove them once the linter is no longer run on CI.
    maflcko commented at 9:48 am on September 20, 2024:

    I’ll be happy to remove them once the linter is no longer run on CI.

    That seems like extra churn and merge conflicts for no good reason. It should be trivial to avoid adding lines to this file. For example a simple #define tfm_fmt tfm::format in the test (and using it) should avoid merge conflicts with future changes, or at least make them one-line conflicts at most.

    hodlinator commented at 10:04 am on September 20, 2024:

    Current approach:

    • Linter is deleted by another PR - merge conflict has obvious resolution.

    Suggested approach:

    • Adding #define’s to temporarily work around a linter that is about to be removed, only to clean it up in another change seems like extra churn.

    Regardless:

    • Possible need to call other tfm::*format* function based on #30928.

    Maybe I’m missing something.

    I’m not in a hurry to get this merged before other PRs in this area. Happy to move this to draft for now so other PRs can merge and deal with churn myself before undrafting.

    maflcko commented at 10:26 am on September 20, 2024:

    Yes, exactly. Depending on how #30928 (review) turns out, this test-only pull will have to be adjusted.

    There are already 4 different follow-ups, all of which explicitly or implicitly conflict with each other.

    I don’t really want to open a 5th one to remove the linter first.

    The more pull requests exist that explicitly or implicitly conflict with each other, the more review will be wasted, because if one of them is merged, all others will have to be rebased and re-reviewed. I understand that it isn’t always possible to avoid conflicts, but this whole changeset around formatting should be low priority enough to not require wasting (re)-review.

    hodlinator commented at 8:06 pm on September 24, 2024:
    (Latest push 071342d4c7627200323d8f22cc37ea022b2e9d69 doesn’t touch linter - resolving).
  11. hodlinator marked this as a draft on Sep 20, 2024
  12. hodlinator force-pushed on Sep 20, 2024
  13. hodlinator force-pushed on Sep 20, 2024
  14. in src/test/util_string_tests.cpp:27 in dbbc2e5228 outdated 
    12@@ -13,17 +13,57 @@ BOOST_AUTO_TEST_SUITE(util_string_tests)
13 // Helper to allow compile-time sanity checks while providing the number of
14 // args directly. Normally PassFmt<sizeof...(Args)> would be used.
15 template <unsigned NumArgs>
16-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
17+inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
18 {
19-    // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20+    // Exercise happy paths at run-time for code coverage metrics.
    l0rinc commented at 2:22 pm on September 20, 2024:

    I’m not sure I understand why we need this.

    Why not make them all runtime instead, why are we “testing” compile time behavior here at all, aren’t we already doing that in the rest of the source code?

    It seems to me the whole situation would simplify a lot if we would only test runtime behavior here.

    hodlinator commented at 8:10 pm on September 24, 2024:
    constexpr functions are not guaranteed to be able to evaluate at compile time for all possible parameters, so exercising that path in the test still provides some value IMO.
  15. in src/test/util_string_tests.cpp:22 in dbbc2e5228 outdated 
    19-    // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
20+    // Exercise happy paths at run-time for code coverage metrics.
21     decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
22+
23+    // Prove parity with tinyformat
24+    switch (NumArgs) {
    l0rinc commented at 3:05 pm on September 20, 2024:

    I like the idea of testing the validator together with the implementation!

    But I’m not in love with the current approach.

    Why are we testing PassFmt<1>("%02d"); with foo - I don’t think it helps with understanding how formatter works.

    Also PassFmt<1>("%s") made sense when we were only validating the number of args, but we’ve extended it since, I think we should extend examples with the actual parameters, e.g.

    • PassFmt("%s", "test");
    • PassFmt("%02d", 42);
    • PassFmt("%12$s %2$s %1$s", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12");

    In which case each example would document the format that we’re exercising more specifically.

    It would also obviate the parameter count by examples, would all be runtime (seems to me that would simplify a lot) and comparison would simply depend on the behavior of tfm::format, e.g. validation would be something like:

     0template <typename... Args>
 1inline bool ValidateFmt(const char* fmt, const char* expected_error, const Args&... args)
 2{
 3    try {
 4        tfm::format(fmt, args...);
 5        ConstevalFormatString<sizeof...(Args)>::Detail_CheckNumFormatSpecifiers(fmt);
 6        return true;
 7    } catch (const tfm::format_error&) {
 8        using ErrType = const char*;
 9        auto check_throw = [expected_error](const ErrType& str) { return str == expected_error; };
10        BOOST_CHECK_EXCEPTION(ConstevalFormatString<sizeof...(Args)>::Detail_CheckNumFormatSpecifiers(fmt), ErrType, check_throw);
11        return false;
12    }
13}
14
15template <typename... Args>
16void PassFmt(const char* fmt, const Args&... args) { BOOST_CHECK(ValidateFmt(fmt, nullptr, args...)); }
17
18template <typename... Args>
19void FailFmt(const char* fmt, const char* expected_error, const Args&... args) { BOOST_CHECK(!ValidateFmt(fmt, expected_error, args...)); }

    and usage would be something like

    0    PassFmt("%%");
1    PassFmt("%s", "test");
2
3    auto err_num{"Format specifier count must match the argument count!"};
4    FailFmt("%s", err_num);
5    FailFmt("%s", err_num, "test", "extra");
    hodlinator commented at 8:36 pm on September 24, 2024:
    I like parts of your direction here, and have taken the time to try it out in 2fb2e72f45a1c81d036515861713371fec2bbe2c. My aim is to retain the tests of leaving out one arg though (which your suggestion doesn’t include). I’m not enough of a variadic template arg magician to figure out how to skip the last argument, which forces me to still lean on maflcko’s tuple_cat approach, leaving it feeling somewhat half-baked.
    l0rinc commented at 12:54 pm on September 25, 2024:

    My aim is to retain the tests of leaving out one arg though (which your suggestion doesn’t include)

    I didn’t include it since I though a single instance of those is enough in the tests.

    I would prefer having concrete typed examples over retesting -1 and +1 args (for which explicit examples should likely suffice)

    maflcko commented at 1:39 pm on September 25, 2024:
    Yeah, seems fine to drop the Args-1, because tinyformat doesn’t have to throw on invalid stuff anyway (it does not for many other “invalid” things). Seems fine to just test the happy case.
    hodlinator commented at 11:27 pm on December 3, 2024:

    I prefer retaining negative tests to increase certainty and would rather not do the PassFmt<1>("%s") -> PassFmt("%s", "test") refactor in this PR if that’s okay with you.

    Edit: I double-checked that tinyformat errors on both too many and too few conversion/format specifiers, and agree the negative case isn’t necessary - removed in latest push.

  16. hodlinator force-pushed on Sep 24, 2024
  17. hodlinator force-pushed on Sep 24, 2024
  18. hodlinator commented at 8:20 pm on September 24, 2024: contributor
    (Just added co-authorship of maflcko in latest push from 071342d4c7627200323d8f22cc37ea022b2e9d69 -> a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b).
  19. in src/test/util_string_tests.cpp:36 in a911efc6e8 outdated 
    30+
31+    // Prove parity with tinyformat
32+    TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs>{}));
33+    if constexpr (NumArgs > 0) {
34+        BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs - 1>{})), tfm::format_error);
35+    }
    maflcko commented at 8:24 pm on September 24, 2024: 
    0    {
1        BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs + 1>{})), tfm::format_error);
2    }

    style nit?

    But let’s wait for CI first. There is a good chance that tuple_cat doesn’t compile on Windows or one of the older compilers :sweat_smile:

    hodlinator commented at 8:48 pm on September 24, 2024:

    Surely that’s more than style? :)

    Would be nice to have both -1 and +1. Tried out your suggestion but tinyformat fails to throw in 8 of the cases. For example in "%12$s 999$s %2$s" with 13 arguments, only using the 12th is not considered an error.

    maflcko commented at 8:52 pm on September 24, 2024:
    Ah right, tinyformat may accept “invalid” input. Feel free to resolve.
  20. maflcko approved
  21. DrahtBot added the label CI failed on Sep 25, 2024
  22. DrahtBot added the label Needs rebase on Sep 27, 2024
  23. maflcko commented at 12:08 pm on October 2, 2024: member

    Are you still working on this?

    I am asking now, because there shouldn’t be any conflicts after rebase, I think.

  24. hodlinator commented at 2:00 pm on October 2, 2024: contributor

    @maflcko

    Are you still working on this?

    I’m waiting on #30928 #31174 before un-drafting this.

  25. in src/test/util_string_tests.cpp:41 in a911efc6e8 outdated 
    37+inline void PassFmtIncorrect(ConstevalFormatString<WrongNumArgs> fmt)
38+{
39+    // Disprove parity with tinyformat
40+    static_assert(WrongNumArgs != CorrectArgs);
41+    TfmF(fmt.fmt, std::tuple_cat(std::array<int, CorrectArgs>{}));
42+    BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, WrongNumArgs>{})), tfm::format_error);
    ryanofsky commented at 10:57 pm on November 4, 2024:

    In commit “test: Prove+document ConstevalFormatString/tinyformat parity” (a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b)

    The name TfmF doesn’t have any clear meaning to me, and usage of the function is also complicated by the need to use tuple_cat everywhere. I think the code would be clearer if the function were less generic and did not require passing a tuple. Maybe would suggest:

     0--- a/src/test/util_string_tests.cpp
 1+++ b/src/test/util_string_tests.cpp
 2@@ -10,12 +10,12 @@ using namespace util;
 3 
 4 BOOST_AUTO_TEST_SUITE(util_string_tests)
 5 
 6-template <typename... Tt>
 7-void TfmF(const char* fmt, const std::tuple<Tt...>& t)
 8+template <unsigned NumArgs>
 9+std::string FormatZeroes(const char* fmt)
10 {
11-    std::apply([fmt](const Tt&... ta){
12-        tfm::format(fmt, ta...);
13-    }, t);
14+    return std::apply([fmt](auto... args) {
15+        return tfm::format(fmt, args...);
16+    }, std::tuple_cat(std::array<int, NumArgs>{}));
17 }
18 
19 // Helper to allow compile-time sanity checks while providing the number of
20@@ -27,9 +27,11 @@ inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
21     decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
22 
23     // Prove parity with tinyformat
24-    TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs>{}));
25+    FormatZeroes<NumArgs>(fmt.fmt);
26+
27+    // Make sure tinyformat would throw if an argument is missing
28     if constexpr (NumArgs > 0) {
29-        BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs - 1>{})), tfm::format_error);
30+        BOOST_CHECK_THROW(FormatZeroes<NumArgs - 1>(fmt.fmt), tfm::format_error);
31     }
32 }
33 template <unsigned WrongNumArgs, unsigned CorrectArgs>
34@@ -37,8 +39,8 @@ inline void PassFmtIncorrect(ConstevalFormatString<WrongNumArgs> fmt)
35 {
36     // Disprove parity with tinyformat
37     static_assert(WrongNumArgs != CorrectArgs);
38-    TfmF(fmt.fmt, std::tuple_cat(std::array<int, CorrectArgs>{}));
39-    BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, WrongNumArgs>{})), tfm::format_error);
40+    FormatZeroes<CorrectArgs>(fmt.fmt);
41+    BOOST_CHECK_THROW(FormatZeroes<WrongNumArgs>(fmt.fmt), tfm::format_error);
42 }
43 template <unsigned WrongNumArgs>
44 inline void FailFmtWithError(std::string_view wrong_fmt, std::string_view error)
    hodlinator commented at 11:14 pm on December 3, 2024:
    Thanks! Ended up naming it TfmFormatZeroes in latest push to highlight the tinyformat part being essential.
  26. in src/test/util_string_tests.cpp:83 in a911efc6e8 outdated 
    79@@ -58,9 +80,9 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
80     // The `*` specifier behavior is unsupported and can lead to runtime
81     // errors when used in a ConstevalFormatString. Please refer to the
82     // note in the ConstevalFormatString docs.
83-    PassFmt<1>("%*c");
84-    PassFmt<2>("%2$*3$d");
85-    PassFmt<1>("%.*f");
86+    PassFmtIncorrect<1, 2>("%*c");
    ryanofsky commented at 11:00 pm on November 4, 2024:

    In commit “test: Prove+document ConstevalFormatString/tinyformat parity” (a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b)

    Maybe drop WrongNumArgs and just pass CorrectArgs? Unless I’m missing something it seems like PassFmtIncorrect could choose the wrong number of args arbitrarily like PassFmt does.

    hodlinator commented at 11:16 pm on December 3, 2024:
    The known cases where I was using PassFmtIncorrect have now become valid PassFmt thanks to your #31174. PassFmtIncorrect is no more.
  27. ryanofsky approved
  28. ryanofsky commented at 11:05 pm on November 4, 2024: contributor
    Code review ACK a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b. I didn’t dig into previous discussion and it seems possible another approach to extending the tests might have advantages over this one. But the code change looks good and seems like an easy way of leveraging existing test cases to check for compatibility with tinyformat.
  29. hodlinator force-pushed on Dec 3, 2024
  30. hodlinator commented at 11:18 pm on December 3, 2024: contributor

    So, I was a bit late reacting to the merge of #31174, but here we are - rebased and ready for review again.

    Added a commit regarding non-parity of "%n" as suggested in #31174 (comment).

  31. hodlinator marked this as ready for review on Dec 3, 2024
  32. hodlinator force-pushed on Dec 3, 2024
  33. DrahtBot removed the label Needs rebase on Dec 4, 2024
  34. DrahtBot removed the label CI failed on Dec 4, 2024
  35. maflcko approved
  36. maflcko commented at 7:20 am on December 4, 2024: member
    lgtm, but it would be better to merge the conflicting non-draft pull with two acks first. This will also avoid having to touch some lines twice, which are touched here.
  37. in src/test/util_string_tests.cpp:128 in 2c82ee3219 outdated 
    121@@ -110,6 +122,12 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
122     FailFmtWithError<2>("%1$*2$", err_term);
123     FailFmtWithError<2>("%1$.*2$", err_term);
124     FailFmtWithError<2>("%1$9.*2$", err_term);
125+
126+    // Non-parity between tinyformat and ConstevalFormatString
127+    int n{};
128+    BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, n), tfm::format_error,
    l0rinc commented at 8:52 am on December 4, 2024:

    👍 for checking %n as well!

    nit: we might inline 0 here:

    0    BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, 0), tfm::format_error,
    hodlinator commented at 7:41 pm on December 5, 2024:
    Taken in latest push.
  38. in src/test/util_string_tests.cpp:32 in 2c82ee3219 outdated 
    29+    // Exercise happy paths at run-time for code coverage metrics.
30     decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
31+
32+    // Prove parity with tinyformat, which will throw if the number of
33+    // conversion specifiers is incorrect.
34+    TfmFormatZeroes<NumArgs>(fmt.fmt);
    l0rinc commented at 9:01 am on December 4, 2024:

    I was wondering how it will behave with %c which would add a \0 which could end the const char* prematurely, but since we have a string it seems to work correctly.

    Are there any checks we could do with the result here? Or make TfmFormatZeroes void if we don’t need the actual result. Maybe something like this:

    0    auto result{TfmFormatZeroes<NumArgs>(fmt.fmt)};
1    BOOST_CHECK_EQUAL(result.empty(), std::string_view{fmt.fmt}.empty());
    ryanofsky commented at 3:35 pm on December 4, 2024:

    re: #30933 (review)

    I think suggested check would be broken in the case of strprintf("%.0s", 0) where result should be empty (because it is printing the first 0 characters of a string) even though the format string empty. It has hard to think of a reliable check that could be done here that would also be meaningful.

    l0rinc commented at 3:48 pm on December 4, 2024:
    We can void the method in that case
    hodlinator commented at 7:58 pm on December 5, 2024:
    (void)ed in latest push and added commit testing "%c".
  39. in src/test/util_string_tests.cpp:15 in 2c82ee3219 outdated 
    10@@ -11,18 +11,30 @@ using namespace util;
11 
12 BOOST_AUTO_TEST_SUITE(util_string_tests)
13 
14+template <unsigned NumArgs>
15+std::string TfmFormatZeroes(const char* fmt)
    l0rinc commented at 9:01 am on December 4, 2024:
    Is it coincidental that every single format type accepts 0 as a valid substitution?
    hodlinator commented at 7:55 pm on December 5, 2024:
    tfm::format(std::string{"%s"}, 1.6f) results in "1.6" instead of an error, so tinyformat isn’t too picky about types.
    l0rinc commented at 10:03 am on December 6, 2024:
    So %n is the only one that doesn’t accept zeroes, right?
    hodlinator commented at 1:31 pm on December 6, 2024:
    Have not fuzzed tinyformat but that seems like a good guess.
  40. in src/test/util_string_tests.cpp:31 in db081bd660 outdated 
    27-    // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
28+    // Exercise happy paths at run-time for code coverage metrics.
29     decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt);
30+
31+    // Prove parity with tinyformat, which will throw if the number of
32+    // conversion specifiers is incorrect.
    ryanofsky commented at 3:09 pm on December 4, 2024:

    In commit “test: Prove+document ConstevalFormatString/tinyformat parity” (db081bd66015f7899696e6eb4a76052446685b64)

    Can there be a test here or somewhere in this file that verifies calls to format really do throw if the number of specifiers is wrong? I am concerned there could be a future change like #30928 where tinyformat errors start being handled differently and these tests become broken without someone noticing.

    hodlinator commented at 7:56 pm on December 5, 2024:
    Fine idea. Felt a bit empty after I removed the negation test following discussion above. Added belt & suspenders test in later push.
  41. in src/test/util_string_tests.cpp:130 in 2c82ee3219 outdated 
    121@@ -122,6 +122,12 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
122     FailFmtWithError<2>("%1$*2$", err_term);
123     FailFmtWithError<2>("%1$.*2$", err_term);
124     FailFmtWithError<2>("%1$9.*2$", err_term);
125+
126+    // Non-parity between tinyformat and ConstevalFormatString
    ryanofsky commented at 3:22 pm on December 4, 2024:

    In commit “test: Document non-parity between tinyformat and ConstevalFormatstring for “%n”” (2c82ee3219ee186b9d3998a129bff1bc809894d3):

    Another case where this is not parity is when a noninteger type is used as a width like strprintf("%*s", "hi", "hi"), which throws a tinyformat::format_error exception “tinyformat: Cannot convert from argument type to integer for use as variable width or precision”. A test could be added for this as well.

    hodlinator commented at 7:58 pm on December 5, 2024:
    Added together with strprintf("%.*s", "hi", "hi")… (I guess it should technically be %.*f but tinyformat doesn’t care, might change iff I re-touch).
  42. ryanofsky approved
  43. ryanofsky commented at 3:36 pm on December 4, 2024: contributor
    Code review ACK 2c82ee3219ee186b9d3998a129bff1bc809894d3
  44. ryanofsky commented at 3:47 pm on December 4, 2024: contributor

    lgtm, but it would be better to merge the conflicting non-draft pull with two acks first.

    This is apparently #31295, which looks ready for merge

  45. DrahtBot added the label Needs rebase on Dec 5, 2024
  46. hodlinator force-pushed on Dec 5, 2024
  47. hodlinator force-pushed on Dec 5, 2024
  48. DrahtBot added the label CI failed on Dec 5, 2024
  49. DrahtBot commented at 2:59 pm on December 5, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33979284676

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  50. DrahtBot removed the label Needs rebase on Dec 5, 2024
  51. DrahtBot removed the label CI failed on Dec 5, 2024
  52. maflcko commented at 7:20 pm on December 5, 2024: member

    review ACK 8e7b252d8adb2821b946bd9052f1a707c4edea4f 🔏

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 8e7b252d8adb2821b946bd9052f1a707c4edea4f 🔏
3YEFop/SgVPN4plP8ZSV4N79ZGj/2ySsY/oG6RmO4QgkHBsA1pFiqOgX9y8+BjXJ88F+fUv4ErGkaKKct3s7MDA==
  53. DrahtBot requested review from ryanofsky on Dec 5, 2024
  54. in src/test/util_string_tests.cpp:126 in 4352596f41 outdated 
    121@@ -110,6 +122,12 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
122     FailFmtWithError<2>("%1$*2$", err_term);
123     FailFmtWithError<2>("%1$.*2$", err_term);
124     FailFmtWithError<2>("%1$9.*2$", err_term);
125+
126+    // Belt & suspenders for tinyformat behavior
    ryanofsky commented at 7:33 pm on December 5, 2024:

    In commit “test: Prove+document ConstevalFormatString/tinyformat parity” (4352596f4167d3e0c1d39ee46c6d5096f0228c61)

    I don’t think this is a belt and suspenders check (assuming belt and suspenders means redundant with another check here). Would suggest a more actionable comment like: // Ensure that tinyformat will throw if format string contains wrong number of specifiers. PassFmt relies on this to verify tinyformat successfully formats the strings, and will need to be updated if tinyformat is changed not to throw on failure.

    hodlinator commented at 1:44 pm on December 6, 2024:
    Thanks! Taken.
  55. in src/test/util_string_tests.cpp:32 in 4352596f41 outdated 
    27     // Execute compile-time check again at run-time to get code coverage stats
28     util::detail::CheckNumFormatSpecifiers<NumArgs>(fmt.fmt);
29+
30+    // Prove parity with tinyformat, which will throw if the number of
31+    // conversion specifiers is incorrect.
32+    TfmFormatZeroes<NumArgs>(fmt.fmt);
    ryanofsky commented at 7:42 pm on December 5, 2024:

    In commit “test: Prove+document ConstevalFormatString/tinyformat parity” (4352596f4167d3e0c1d39ee46c6d5096f0228c61)

    Adding BOOST_CHECK_NO_THROW could be nice to make intent clearer

    hodlinator commented at 1:47 pm on December 6, 2024:
    And here I was being proud about having added BOOST_CHECK_NO_THROW to your suggested non-parity checks at the bottom. :) Good point, also added to the original CheckNumFormatSpecifiers-call in PassFmt as I ended up touching that line.
  56. in src/test/util_string_tests.cpp:137 in f71933c82b outdated 
    130+    BOOST_CHECK_NO_THROW(util::detail::CheckNumFormatSpecifiers<2>("%*s"));
131+    BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%*s"}, "hi", "hi"), tfm::format_error,
132+        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
133+    BOOST_CHECK_NO_THROW(util::detail::CheckNumFormatSpecifiers<2>("%.*s"));
134+    BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%.*s"}, "hi", "hi"), tfm::format_error,
135+        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
    ryanofsky commented at 7:50 pm on December 5, 2024:

    In commit “test: Document non-parity between tinyformat and ConstevalFormatstring” (f71933c82b14d2a2e619d02f25d60a1432c3093d):

    Repeating each case two different ways makes this harder to understand. Would suggest consolidating:

    0    // Non-parity between tinyformat and ConstevalFormatString
1    // tinyformat throws but ConstevalFormatString does not
2    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%n"}, 0), tfm::format_error,
3        HasReason{"tinyformat: %n conversion spec not supported"});
4    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%*s"}, "hi", "hi"), tfm::format_error,
5        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
6    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%.*s"}, "hi", "hi"), tfm::format_error,
7        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
    hodlinator commented at 1:50 pm on December 6, 2024:

    I think it is valid to have the CheckNumFormatSpecifiers calls to prove they work.

    In the latest push I’ve at least removed the util::detail:: parts to decrease noise which happily makes the format strings line up.

    ryanofsky commented at 2:42 pm on December 6, 2024:

    re: #30933 (review)

    I think it is valid to have the CheckNumFormatSpecifiers calls to prove they work.

    Agree it’s good to have these but ConstevalFormatString is calling CheckNumFormatSpecifiers internally. So suggested version should make these cases easier to read, easier to extend in the future, more consistent with each other, and not lose any test coverage (actually increasing it a little)

    hodlinator commented at 8:50 pm on December 6, 2024:
    To be honest, I didn’t notice that you substituted in ConstevalFormatString instead of std::string. A GitHub suggestion might have made it stand out more. I did visit the optician earlier in the year… anyway. Taken in latest push.
  57. in src/test/util_string_tests.cpp:30 in 4352596f41 outdated 
    25 inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
26 {
27     // Execute compile-time check again at run-time to get code coverage stats
28     util::detail::CheckNumFormatSpecifiers<NumArgs>(fmt.fmt);
29+
30+    // Prove parity with tinyformat, which will throw if the number of
    ryanofsky commented at 8:03 pm on December 5, 2024:

    In commit “test: Prove+document ConstevalFormatString/tinyformat parity” (4352596f4167d3e0c1d39ee46c6d5096f0228c61)

    Not sure this is really parity, since it only shows that tinyformat doesn’t throw when ConstevalFormatString doesn’t throw, not that tinyformat throws when ConstevalFormatString does (which we probably don’t care about). A more literal comment like “Make sure tinyformat doesn’t throw if ConstevalFormat didn’t throw” might make it more obvious what benefit of this check is.

    hodlinator commented at 1:52 pm on December 6, 2024:

    Updated with more verbose comment, let me know if you think it’s okay.

    0// If ConstevalFormatString didn't throw above, make sure tinyformat doesn't
1// throw either for the same format string and parameter count combination.
2// Proves that we have some extent of protection from runtime errors
3// (tinyformat may still throw for some type mismatches).
    ryanofsky commented at 2:44 pm on December 6, 2024:

    re: #30933 (review)

    Updated with more verbose comment, let me know if you think it’s okay.

    Looks good! Very clear

  58. ryanofsky approved
  59. ryanofsky commented at 8:14 pm on December 5, 2024: contributor
    Code review ACK 8e7b252d8adb2821b946bd9052f1a707c4edea4f
  60. hodlinator force-pushed on Dec 6, 2024
  61. hodlinator commented at 2:00 pm on December 6, 2024: contributor

    Latest push:

    • Adds using util::detail::CheckNumFormatSpecifiers;, decreasing noise in the rest of the changes.
    • Adds BOOST_CHECK_NO_THROW to both CheckNumFormatSpecifiers and tinyformat calls in PassFmt.
    • Makes comments more descriptive/useful.
    • Adds context to commit message of the last commit.

    Also updated PR summary: “Makes unequivocally clear the extent of parity.” -> “Clarifies and puts the extent of parity under test.”

  62. in src/test/util_string_tests.cpp:29 in dace90d07e outdated 
    18-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
19+inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
20 {
21     // Execute compile-time check again at run-time to get code coverage stats
22-    util::detail::CheckNumFormatSpecifiers<NumArgs>(fmt.fmt);
23+    BOOST_CHECK_NO_THROW(CheckNumFormatSpecifiers<NumArgs>(fmt.fmt));
    l0rinc commented at 2:02 pm on December 6, 2024:
    👍 for BOOST_CHECK_NO_THROW, documents the reason for the dangling call a lot better
  63. in src/test/util_string_tests.cpp:26 in dace90d07e outdated 
    14 
15 // Helper to allow compile-time sanity checks while providing the number of
16 // args directly. Normally PassFmt<sizeof...(Args)> would be used.
17 template <unsigned NumArgs>
18-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
19+inline void PassFmt(ConstevalFormatString<NumArgs> fmt)
    l0rinc commented at 2:13 pm on December 6, 2024:

    nit: given that PassFmt is local to util_string_tests, I think static would make more sense here (and the other helpers which sometimes have inline and sometimes nothing):

    0static void PassFmt(ConstevalFormatString<NumArgs> fmt)
    hodlinator commented at 3:18 pm on December 6, 2024:
    They are also template functions, so implicitly inline, and arguably local already. Holding off on this for now.
    l0rinc commented at 3:38 pm on December 6, 2024:
    I find it inconsistent that PassFmt and FailFmtWithError are marked inline, but the new TfmFormatZeroes is not. I’m fine with not making them static, but we could at least remove the redundant inlines - not critical, I won’t block, would just prefer consistency.
    hodlinator commented at 8:52 pm on December 6, 2024:
    Removed inlines as I had to push again anyway. Consistency++
    maflcko commented at 9:11 am on December 7, 2024:

    They are also template functions, so implicitly inline

    Just as a note. I don’t think this is generally true. For example, explicit template specialization isn’t implicitly inline. This is even explicitly mentioned in the standard, see https://timsong-cpp.github.io/cppwp/temp.expl.spec#12

    You can get a link failure with a diff like so:

     0diff --git a/src/node/chainstatemanager_args.cpp b/src/node/chainstatemanager_args.cpp
 1index 0ac96c5514..96bc418819 100644
 2--- a/src/node/chainstatemanager_args.cpp
 3+++ b/src/node/chainstatemanager_args.cpp
 4@@ -21,6 +21,18 @@
 5 #include <chrono>
 6 #include <string>
 7 
 8+template <class A>
 9+void Afoo(const A& a)
10+{
11+    std::cout << "one" << std::endl;
12+}
13+
14+template <>
15+void Afoo<int>(const int& a)
16+{
17+    std::cout << "specialized for int" << std::endl;
18+}
19+
20 namespace node {
21 util::Result<void> ApplyArgsManOptions(const ArgsManager& args, ChainstateManager::Options& opts)
22 {
23diff --git a/src/node/coins_view_args.cpp b/src/node/coins_view_args.cpp
24index 5d55143e83..3a0a7a5e40 100644
25--- a/src/node/coins_view_args.cpp
26+++ b/src/node/coins_view_args.cpp
27@@ -7,6 +7,17 @@
28 #include <common/args.h>
29 #include <txdb.h>
30 
31+template <class A>
32+void Afoo(const A& a)
33+{
34+    std::cout << "one" << std::endl;
35+}
36+template <>
37+void Afoo<int>(const int& a)
38+{
39+    std::cout << "specialized for int" << std::endl;
40+}
41+
42 namespace node {
43 void ReadCoinsViewArgs(const ArgsManager& args, CoinsViewOptions& options)
44 {
  64. in src/test/util_string_tests.cpp:142 in 2951532a2c outdated 
    139+        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
140+
141+    // Ensure that tinyformat will throws if format string contains wrong number
142+    // of specifiers. PassFmt relies on this to verify tinyformat successfully
143+    // formats the strings, and will need to be updated if tinyformat is changed
144+    // not to throw on failure.
    maflcko commented at 2:13 pm on December 6, 2024:

    nit in 920782a8531a3dae7423ca304cc1fe2ec5ee46bf: possible typo. Also could remove the note that the test may need to be updated if the underlying tested module is changed (This should be obvious). Even more so, given that tinyformat likely won’t be changed before its removal with C++26.

    0    // Ensure that tinyformat throws if format string contains wrong number
1    // of specifiers. PassFmt relies on this to verify tinyformat successfully
2    // formats the strings.
    ryanofsky commented at 2:57 pm on December 6, 2024:

    re: #30933 (review)

    Good catch on the typo, and suggestion is ok, but disagree with:

    Also could remove the note that the test may need to be updated if the underlying tested module is changed (This should be obvious). Even more so, given that tinyformat likely won’t be changed before its removal with C++26.

    I agree it should not be necessary to say that changes to tinyformat could break assumptions made by a tinyformat_tests.cpp test file, but it doesn’t seem obvious that a change to tinyformat would break assumptions made by util_string_tests.cpp tests. And there is an open PR which could break this in #30928 commit 49f73071a8ec4131595090a619d6198a5f8b7c3c, so it’s not just a theoretical concern. Fine to change this, but I think it’s better if the comment clearly explains why this check is here.

  65. in src/test/util_string_tests.cpp:141 in 2951532a2c outdated 
    136+        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
137+    BOOST_CHECK_NO_THROW(CheckNumFormatSpecifiers<2>("%.*s"));
138+    BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%.*s"}, "hi", "hi"), tfm::format_error,
139+        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
140+
141+    // Ensure that tinyformat will throws if format string contains wrong number
    l0rinc commented at 2:14 pm on December 6, 2024: 
    0    // Ensure that tinyformat will throw if format string contains wrong number
  66. maflcko approved
  67. maflcko commented at 2:14 pm on December 6, 2024: member

    nothing blocking, just a nit.

    re-ACK 2951532a2ca09ac8ebdea066fa20826fdb6fa8e5 💼

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 2951532a2ca09ac8ebdea066fa20826fdb6fa8e5 💼
3LJt7P3harWblu2dLt4t2t8Z3yPelpL/kKq/KB7AoO45V3vTgMr8HXd6YbZiCCx5Y3BTDTIlVohJmss9B0+l9DA==
  68. DrahtBot requested review from ryanofsky on Dec 6, 2024
  69. in src/test/util_string_tests.cpp:144 in 920782a853 outdated 
    129+    // Ensure that tinyformat will throws if format string contains wrong number
130+    // of specifiers. PassFmt relies on this to verify tinyformat successfully
131+    // formats the strings, and will need to be updated if tinyformat is changed
132+    // not to throw on failure.
133+    BOOST_CHECK_EXCEPTION(TfmFormatZeroes<2>("%s"), tfm::format_error,
134+        HasReason{"tinyformat: Not enough conversion specifiers in format string"});
    l0rinc commented at 2:16 pm on December 6, 2024:

    nit: don’t feel strongly about it, just noting that HasReason can do partial matches, we can also check something shorter like:

    0        HasReason{"Not enough conversion specifiers"});

    (which would enable this being a one-liner)

    hodlinator commented at 3:21 pm on December 6, 2024:
    Thanks for the suggestion, it’s a trade-off, holding off on changing this.
  70. in src/test/util_string_tests.cpp:1 in 116b5895b1 outdated
    l0rinc commented at 2:18 pm on December 6, 2024:

    commit message typo:

    0- For "%n", which is supposed to write to the argument for printf.
    hodlinator commented at 3:16 pm on December 6, 2024:
    Cheers, taken.
  71. in src/test/util_string_tests.cpp:21 in 2951532a2c outdated 
    16+void TfmFormatZeroes(const char* fmt)
17+{
18+    std::apply([fmt](auto... args) {
19+        (void)tfm::format(std::string{fmt}, args...);
20+    }, std::tuple_cat(std::array<int, NumArgs>{}));
21+}
    l0rinc commented at 2:26 pm on December 6, 2024:

    nit: we could avoid [fmt] via [&] here, make it static and give it a std::string param directly:

    0template <unsigned NumArgs>
1static void TfmFormatZeroes(const std::string& fmt)
2{
3    std::apply([&](auto... args) { tfm::format(fmt, args...); }, std::array<int, NumArgs>{});
4}
    hodlinator commented at 3:23 pm on December 6, 2024:

    Thanks, taken in broad strokes.

    Temporarily dded

    0BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
1{
2+    TfmFormatZeroes<10>("blah")

    together with

    0void TfmFormatZeroes(const std::string& fmt)
1{
2    std::apply([&](auto... args) {
3+        fprintf(stderr, "ARG COUNT: %d", sizeof...(args));
4+        assert(false);
5        (void)tfm::format(fmt, args...);
6    }, std::array<int, NumArgs>{});
7}

    To verify that tuple_cat was unnecessary.

  72. in test/lint/run-lint-format-strings.py:18 in 2951532a2c outdated 
    14@@ -15,6 +15,8 @@
15 FALSE_POSITIVES = [
16     ("src/clientversion.cpp", "strprintf(_(COPYRIGHT_HOLDERS), COPYRIGHT_HOLDERS_SUBSTITUTION)"),
17     ("src/test/translation_tests.cpp", "strprintf(format, arg)"),
18+    ("src/test/util_string_tests.cpp", 'tfm::format(std::string{"%*s"}, "hi", "hi")'),
    l0rinc commented at 2:27 pm on December 6, 2024:
    What’s the added value for extending this linter as well?
    hodlinator commented at 3:25 pm on December 6, 2024:
    Was getting CI failures, so added these exceptions. I considered extending the linter implementation to handle them correctly, but part of the raison d’étre of ConstevalFormatString is getting further towards being able to remove the linter in the first place.
    l0rinc commented at 8:53 pm on December 6, 2024:

    Ah like these new ones you mean?

    0[20:48:24.756] src/test/util_string_tests.cpp: Expected 0 argument(s) after format string but found 1 argument(s): tfm::format(ConstevalFormatString<2>{"%*s"}, "hi", "hi")
1[20:48:24.756] src/test/util_string_tests.cpp: Expected 0 argument(s) after format string but found 1 argument(s): tfm::format(ConstevalFormatString<2>{"%.*s"}, "hi", "hi")
    hodlinator commented at 9:00 pm on December 6, 2024:

    Exactly :D

    Fixed in latest push. @maflcko I realize you were concerned about touching this linter in #30933 (review), do you have an alternate approach suggestion?

    maflcko commented at 10:43 am on December 7, 2024:

    @maflcko I realize you were concerned about touching this linter in #30933 (comment), do you have an alternate approach suggestion?

    The file should just be deleted. I went ahead and did that in #31061 (comment)

  73. l0rinc changes_requested
  74. ryanofsky approved
  75. ryanofsky commented at 3:02 pm on December 6, 2024: contributor
    Code review ACK 2951532a2ca09ac8ebdea066fa20826fdb6fa8e5 just adding using declaration and improving comments since last reviewe
  76. hodlinator force-pushed on Dec 6, 2024
  77. l0rinc commented at 3:41 pm on December 6, 2024: contributor
    ACK 8ee014020071a60d1bdcdd6c16a7e515c6576fea
  78. DrahtBot requested review from ryanofsky on Dec 6, 2024
  79. DrahtBot requested review from maflcko on Dec 6, 2024
  80. hodlinator force-pushed on Dec 6, 2024
  81. refactor test: Profit from using namespace + using detail function 
    Also adds BOOST_CHECK_NO_THROW() while touching that line, clarifying part of what we are checking for.

Also removed redundant inline from template functions in .cpp file.
    b81a465995
  82. test: Prove+document ConstevalFormatString/tinyformat parity 
    Co-Authored-By: Lőrinc <pap.lorinc@gmail.com>
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
    533013cba2
  83. hodlinator force-pushed on Dec 6, 2024
  84. DrahtBot added the label CI failed on Dec 6, 2024
  85. DrahtBot commented at 8:46 pm on December 6, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34054153748

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  86. test: Document non-parity between tinyformat and ConstevalFormatstring 
    - For "%n", which is supposed to write to the argument for printf.
- For string/integer mismatches of width/precision specifiers.

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
    76cca4aa6f
  87. test: Add missing %c character test 
    Proves tinyformat doesn't trigger an exception for \0 characters.

Co-Authored-By: Lőrinc <pap.lorinc@gmail.com>
    c93bf0e6e2
  88. hodlinator force-pushed on Dec 6, 2024
  89. hodlinator commented at 9:02 pm on December 6, 2024: contributor
    Had to re-push after fully grokking #30933 (review).
  90. in src/test/util_string_tests.cpp:134 in c93bf0e6e2 
    125@@ -110,6 +126,24 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
126     FailFmtWithError<2>("%1$*2$", err_term);
127     FailFmtWithError<2>("%1$.*2$", err_term);
128     FailFmtWithError<2>("%1$9.*2$", err_term);
129+
130+    // Non-parity between tinyformat and ConstevalFormatString.
131+    // tinyformat throws but ConstevalFormatString does not.
132+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%n"}, 0), tfm::format_error,
133+        HasReason{"tinyformat: %n conversion spec not supported"});
134+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%*s"}, "hi", "hi"), tfm::format_error,
    l0rinc commented at 9:08 pm on December 6, 2024:
    nit: personally I’m not a fan of “funny” variable names - can we do “X” or “abc” or “text” or something instead? Not a blocker, just a preference
    hodlinator commented at 9:27 pm on December 6, 2024:

    Good point. Might go with something like this (slightly useful) if I re-touch:

    0    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%*s"}, "one", "two"), tfm::format_error,
  91. in src/test/util_string_tests.cpp:132 in c93bf0e6e2 
    125@@ -110,6 +126,24 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
126     FailFmtWithError<2>("%1$*2$", err_term);
127     FailFmtWithError<2>("%1$.*2$", err_term);
128     FailFmtWithError<2>("%1$9.*2$", err_term);
129+
130+    // Non-parity between tinyformat and ConstevalFormatString.
131+    // tinyformat throws but ConstevalFormatString does not.
132+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%n"}, 0), tfm::format_error,
    l0rinc commented at 9:15 pm on December 6, 2024:
    these are basically TODOs for follow-up PRs, right? Can we add ConstevalFormatString parity check for the FUZZ tests as well (guess that should reveal other cases where it’s misaligned)
    hodlinator commented at 9:37 pm on December 6, 2024:

    I guess a version of ConstevalFormatString/CheckNumFormatSpecifiers() could be checked in a test together with tfm::format, but I’m not sure it would be a fuzz-test, as tfm::format() takes args specified at compile time.

    It could conceivably be a pretty advanced fuzz-test that generated C++ code and compiled it as part of the test, not sure if we have that class of tests already or if this would be new, and if it would be accepted.

    l0rinc commented at 9:45 pm on December 6, 2024:
    Why not validate it during runtime via CheckNumFormatSpecifiers?
    hodlinator commented at 9:49 pm on December 6, 2024:

    CheckNumFormatSpecifiers is currently a template, but sure, could be changed to take the number of specifiers at runtime instead.

    If we want to check parity in a fuzzing context we should probably check tfm::format() as well, with the same input?

    maflcko commented at 11:17 am on December 7, 2024:

    It could conceivably be a pretty advanced fuzz-test that generated C++ code and compiled it as part of the test, not sure if we have that class of tests already or if this would be new, and if it would be accepted.

    I don’t think you need to generate C++ code and compile it. It should be possible to just write it directly. See:

     0#include <test/fuzz/FuzzedDataProvider.h>
 1#include <test/fuzz/fuzz.h>
 2#include <test/fuzz/util.h>
 3#include <tinyformat.h>
 4#include <util/strencodings.h>
 5#include <util/translation.h>
 6
 7#include <algorithm>
 8#include <cstdint>
 9#include <string>
10#include <vector>
11
12template <unsigned NumArgs>
13void Check(const std::string& fmt)
14{
15    try {
16        util::detail::CheckNumFormatSpecifiers<NumArgs>(fmt.c_str());
17        try {
18            std::apply([&](auto... args) { tfm::format(tfm::RuntimeFormat{fmt}, args...); }, std::array<int, NumArgs>{});
19        } catch (const std::exception& e) {
20            std::string_view what{e.what()};
21            if (what == "tinyformat: %n conversion spec not supported") return;
22            std::cout << "mismatch: " << what << std::endl;
23            std::cout << "'" << fmt << "'" << std::endl;
24            assert(false);
25        }
26    } catch (...) {
27        // fine
28    }
29}
30
31FUZZ_TARGET(str_printf)
32{
33    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
34    const std::string format_string = fuzzed_data_provider.ConsumeRandomLengthString(64);
35
36    const int digits_in_format_specifier = std::count_if(format_string.begin(), format_string.end(), IsDigit);
37
38    // Avoid triggering the following crash bug:
39    // * strprintf("%987654321000000:", 1);
40    //
41    // Avoid triggering the following OOM bug:
42    // * strprintf("%.222222200000000$", 1.1);
43    //
44    // Upstream bug report: https://github.com/c42f/tinyformat/issues/70
45    if (format_string.find('%') != std::string::npos && digits_in_format_specifier >= 7) {
46        return;
47    }
48
49    // Avoid triggering the following crash bug:
50    // * strprintf("%1$*1$*", -11111111);
51    //
52    // Upstream bug report: https://github.com/c42f/tinyformat/issues/70
53    if (format_string.find('%') != std::string::npos && format_string.find('$') != std::string::npos && format_string.find('*') != std::string::npos && digits_in_format_specifier > 0) {
54        return;
55    }
56
57    // Avoid triggering the following crash bug:
58    // * strprintf("%.1s", (char*)nullptr);
59    //
60    // (void)strprintf(format_string, (char*)nullptr);
61    //
62    // Upstream bug report: https://github.com/c42f/tinyformat/issues/70
63
64    Check<0>(format_string);
65    Check<1>(format_string);
66    Check<2>(format_string);
67    Check<3>(format_string);
68    Check<4>(format_string);
69    Check<5>(format_string);
70}
    maflcko commented at 8:27 am on December 11, 2024:

    For clarity, running the fuzz test will actually spit out some ways to trick the consteval check, because it doesn’t fully implement tinyformat at compile time. For example, the consteval check doesn’t ignore c99 length modifiers.

     0diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
 1index 340c650fe3..2284e775c6 100644
 2--- a/src/test/util_string_tests.cpp
 3+++ b/src/test/util_string_tests.cpp
 4@@ -129,6 +129,16 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
 5 
 6     // Non-parity between tinyformat and ConstevalFormatString.
 7     // tinyformat throws but ConstevalFormatString does not.
 8+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%h"}, 0), tfm::format_error,
 9+                          HasReason{"tinyformat: Conversion spec incorrectly terminated by end of string"});
10+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%7#%o"}, 0), tfm::format_error,
11+                          HasReason{"tinyformat: Too many conversion specifiers in format string"});
12+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%*9%*"}, 0, 0), tfm::format_error,
13+                          HasReason{"tinyformat: Not enough arguments to read variable width or precision"});
14+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%7#%8$"}, 0), tfm::format_error,
15+                          HasReason{"tinyformat: Positional argument out of range"});
16+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%6#%1$%%"}, 0), tfm::format_error,
17+                          HasReason{"tinyformat: Non-positional argument used after a positional one"});
18     BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%n"}, 0), tfm::format_error,
19         HasReason{"tinyformat: %n conversion spec not supported"});
20     BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%*s"}, "hi", "hi"), tfm::format_error,
    hodlinator commented at 11:21 am on December 11, 2024:

    Didn’t think to lock-down the number of args in that way and still get value from fuzzing. Turned out great!

    Are you going to open a PR for this or what do you suggest as next step?

    maflcko commented at 11:50 am on December 11, 2024:
    I just left a comment for context and clarity, but I won’t be working on this myself. If someone wants to work on this, it is up-for-grabs.
  92. in src/test/util_string_tests.cpp:136 in c93bf0e6e2 
    131+    // tinyformat throws but ConstevalFormatString does not.
132+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<1>{"%n"}, 0), tfm::format_error,
133+        HasReason{"tinyformat: %n conversion spec not supported"});
134+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%*s"}, "hi", "hi"), tfm::format_error,
135+        HasReason{"tinyformat: Cannot convert from argument type to integer for use as variable width or precision"});
136+    BOOST_CHECK_EXCEPTION(tfm::format(ConstevalFormatString<2>{"%.*s"}, "hi", "hi"), tfm::format_error,
    l0rinc commented at 9:16 pm on December 6, 2024:
    nit: strprintf is a bit shorter than tfm::format
    hodlinator commented at 9:29 pm on December 6, 2024:
    Shorter yes, but this is more direct/concrete.
  93. l0rinc approved
  94. l0rinc commented at 9:22 pm on December 6, 2024: contributor

    ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 Changes:

    • Unified templates
    • Adjusted non-parity of tfm::format of ConstevalFormatString

    Would be curious if we could extend this to fuzzing as well - but it can be explored in a different PR as well

  95. DrahtBot removed the label CI failed on Dec 6, 2024
  96. maflcko commented at 8:49 am on December 7, 2024: member

    re-ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 🗜

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 🗜
32bT04GVn2EBycqrMStGo3ZV6cz0SmTdEN2rHArqcWSDe5w8c/2B7Zg6UJUSGT5QzDPVgUPspDPW773Kb5GMiAg==
  97. ryanofsky approved
  98. ryanofsky commented at 10:39 pm on December 9, 2024: contributor
    Code review ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1. Just a few cleanups tweaking function declarations and commit comments and consolidating some test cases since last review.
  99. ryanofsky merged this on Dec 11, 2024
  100. ryanofsky closed this on Dec 11, 2024
  101. Mikaela11 approved
  102. hodlinator deleted the branch on Dec 11, 2024


hodlinator DrahtBot maflcko l0rinc ryanofsky Mikaela11


maflcko

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-01-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me