Cover remaining tinyformat usages in CheckFormatSpecifiers #30999

l0rinc · 2024-09-29T18:54:13Z

The current string formatter couldn't validate every string format template that we were using.
Extended it with dynamic widths, fixed a number parsing bug that could go over the string's content and added a %n validation.

DrahtBot · 2024-09-29T18:54:16Z

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

No conflicts as of last run.

maflcko

d49dfaf looks correct, but I am not sure the others.

maflcko · 2024-09-30T06:35:48Z

src/test/util_string_tests.cpp

-// Helper to allow compile-time sanity checks while providing the number of
-// args directly. Normally PassFmt<sizeof...(Args)> would be used.
-template <unsigned NumArgs>
-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)


in cba51f2: Not sure about turning the passing one into a runtime check from a compile-time check. Previously it was trivial to compile a single unit (this test) to sanity check the parser, as well as check the compile failures for various errors, by simply looking at the compile output. Also, the code was as close as possible to the real code, serving as documentation of how to use it.

Now, checking the passing cases requires not only compiling, but also linking and executing the test. Also, triggering a compile error to see that it works and to see how it looks is harder.

I understand you want better error messages on failure, but changing the passing cases isn't required for that.

Valid complaint, I've added a consteval ValidFormatSpecifiers local delegate and split the valid tests from the BOOST_CHECK_EXCEPTIONs - this way failures still show the correct lines both for the valid and invalid tests.

What is the benefit of ValidFormatSpecifiers over the existing PassFmt, other than dropping the code coverage stats?

Seems fine to update the comment below to say // Execute compile-time check again at run-time to get code coverage stats., but not sure about dropping it.

I've added the comment back, that's indeed important context.
Compared to PassFmt I found the ValidFormatSpecifiers to be more specific (I'm not a fan of abbrvs and // comments).
Don't have strong preference here, I can be convinced to rename it back, if you do.

Compared to PassFmt I found the ValidFormatSpecifiers to be more specific (I'm not a fan of abbrvs and // comments).

I don't care about naming, so if you want to rename PassFmt to something else, this is fine. However, the // comment isn't useless: It explains that the goal of this helper function is to be close as possible to the real code (and document the only difference). I found that useful as a single compilation unit that serves as a close proxy to the real code, with almost the same compile-time error messages (and behavior).

I understand you want better error messages on failure, but changing the behavior of the passing cases isn't required for that.

Generally, if there isn't a reason to change something, it is better to leave the code as-is, because it was most likely intentionally written in that way.

src/util/string.h

maflcko · 2024-09-30T06:50:07Z

src/util/string.h

- *
- * @note Counting of `*` dynamic width and precision fields (such as `%*c`,
- * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
- * they are not used in the codebase. Usage of these fields is not counted and
- * can lead to run-time exceptions. Code wanting to use the `*` specifier can
- * side-step this struct and call tinyformat directly.


Why remove this comment? format("'%1$*3$s %2$-*3$s'", "hi", "w", 12) is still unsupported and parsed incorrectly at compile time.

Added back the part that I think is relevant, let me know if you'd like me to rewrite it.

Those are dynamic width fields, so I still don't understand why you remove that from the comment.

Because we do have some dynamic width support for the values that were used in the codebase.
But I've reverted the original comment (but deleted %*c which is a compile-time failure now).

* Renamed `Detail_CheckNumFormatSpecifiers` to `CheckFormatSpecifiers` since we're checking more than the number of parameters * Moved it out of `ConstevalFormatString` to make it easier to test * Inline `FailFmtWithError` (and rename `PassFmt` to `ValidFormatSpecifiers`) in tests to provide better errors messages on failure (e.g. line number)

They were used in bitcoin-cli

It's not supported in tinyformat: https://github.com/bitcoin/bitcoin/blob/master/src/tinyformat.h#L843-L845

maflcko

As mentioned previously, it looks like there is one correct commit. However, I have a hard time seeing how the others are useful in a great picture, given that some of them are incomplete anyway.

maflcko · 2024-10-01T07:34:49Z

src/test/util_string_tests.cpp

-// Helper to allow compile-time sanity checks while providing the number of
-// args directly. Normally PassFmt<sizeof...(Args)> would be used.
-template <unsigned NumArgs>
-inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)


Compared to PassFmt I found the ValidFormatSpecifiers to be more specific (I'm not a fan of abbrvs and // comments).

I don't care about naming, so if you want to rename PassFmt to something else, this is fine. However, the // comment isn't useless: It explains that the goal of this helper function is to be close as possible to the real code (and document the only difference). I found that useful as a single compilation unit that serves as a close proxy to the real code, with almost the same compile-time error messages (and behavior).

I understand you want better error messages on failure, but changing the behavior of the passing cases isn't required for that.

Generally, if there isn't a reason to change something, it is better to leave the code as-is, because it was most likely intentionally written in that way.

maflcko · 2024-10-01T07:44:24Z

src/util/string.h

+ * @note Counting of `*` dynamic width and precision fields (such as
 * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
 * they are not used in the codebase. Usage of these fields is not counted and


not sure about implementing a random and specific subset of * in specifiers. I think it is easier to either fully support them, or not at all. But having developers read the parser to understand which subset they are allowed to use may be causing more frustration than solving any real issue.

not implemented to minimize code complexity as long as they are not used in the codebase

I've implemented the part that was used, not a "random subset"

maflcko · 2024-10-01T07:48:02Z

src/util/string.h

 * `%2$*3$d`, `%.*f`) is not implemented to minimize code complexity as long as
 * they are not used in the codebase. Usage of these fields is not counted and
 * can lead to run-time exceptions. Code wanting to use the `*` specifier can
 * side-step this struct and call tinyformat directly.
 */
+constexpr static void CheckFormatSpecifiers(std::string_view str, unsigned num_params)


Not sure about moving this out. This will break the doxygen comment above. Also, it drops the "detail-namespace".

Generally, I think that test-only code should follow the real code, not the other way round. As long as real code is testable, optimizing other parts of the unit tests doesn't seem too useful, especially if it breaks the existing construct and documentation.

l0rinc · 2024-10-01T08:15:45Z

I'm closing it for lack of interest, feel free to cherry-pick changes to other PRs

DrahtBot added the Tests label Sep 29, 2024

l0rinc changed the title ~~test: streamline CheckFormatSpecifiers testability~~ Cover remaining tinyformat usages in CheckFormatSpecifiers Sep 29, 2024

l0rinc marked this pull request as ready for review September 29, 2024 20:59

DrahtBot mentioned this pull request Sep 29, 2024

log: Enforce trailing newline #30929

Merged

maflcko reviewed Sep 30, 2024

View reviewed changes

stickies-v mentioned this pull request Sep 30, 2024

tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error #30928

Closed

l0rinc force-pushed the l0rinc/ConstevalFOrmatString branch from 23f2887 to 44aa62e Compare September 30, 2024 11:04

l0rinc added 5 commits September 30, 2024 14:05

test: Unify CheckFormatSpecifiers error messages

f701cca

CheckFormatSpecifiers shouldn't iterate beyond string bounds

51c56e8

Implement dynamic width validation in CheckFormatSpecifiers

49fc242

They were used in bitcoin-cli

Prohibit %n usages in format

6e4935d

It's not supported in tinyformat: https://github.com/bitcoin/bitcoin/blob/master/src/tinyformat.h#L843-L845

l0rinc force-pushed the l0rinc/ConstevalFOrmatString branch from 44aa62e to 6e4935d Compare September 30, 2024 12:11

maflcko reviewed Oct 1, 2024

View reviewed changes

l0rinc closed this Oct 1, 2024

hodlinator mentioned this pull request Oct 30, 2024

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

Merged

Cover remaining tinyformat usages in CheckFormatSpecifiers #30999

Cover remaining tinyformat usages in CheckFormatSpecifiers #30999

Uh oh!

Conversation

l0rinc commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Oct 1, 2024

Uh oh!

Uh oh!

l0rinc commented Sep 29, 2024 •

edited

Loading

DrahtBot commented Sep 29, 2024 •

edited

Loading

l0rinc Sep 30, 2024 •

edited

Loading

l0rinc Sep 30, 2024 •

edited

Loading

l0rinc Sep 30, 2024 •

edited

Loading