test: Prove+document ConstevalFormatString/tinyformat parity

hodlinator commented at 10:11 pm on September 19, 2024: contributor

Makes unequivocally clear for which type of format strings we do have parity between the two, and which we (currently) don’t.

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

refactor test: Profit from using namespace 14b5554ef4

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

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30929 (log: Enforce trailing newline at compile time by maflcko)
#30921 (test: generalize HasReason and use it in FailFmtWithError by l0rinc)

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.

DrahtBot added the label Tests on Sep 19, 2024

hodlinator force-pushed on Sep 19, 2024

DrahtBot added the label CI failed on Sep 19, 2024

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.

DrahtBot removed the label CI failed on Sep 19, 2024

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!

in src/test/util_string_tests.cpp:19 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.

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).

hodlinator marked this as a draft on Sep 20, 2024

hodlinator force-pushed on Sep 20, 2024

in src/test/util_string_tests.cpp:26 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.

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 force-pushed on Sep 24, 2024

test: Prove+document ConstevalFormatString/tinyformat parity

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

a911efc6e8

hodlinator force-pushed on Sep 24, 2024

hodlinator commented at 8:20 pm on September 24, 2024: contributor

(Just added co-authorship of maflcko in latest push from 071342d4c7627200323d8f22cc37ea022b2e9d69 -> a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b).

in src/test/util_string_tests.cpp:33 in a911efc6e8

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.

maflcko approved

DrahtBot added the label CI failed on Sep 25, 2024

DrahtBot added the label Needs rebase on Sep 27, 2024

DrahtBot commented at 11:05 am on September 27, 2024: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

test: Prove+document ConstevalFormatString/tinyformat parity #30933

Code Coverage

Reviews

Conflicts