lint: Remove string exclusion from locale check

maflcko commented at 8:01 am on May 7, 2025: member

The exclusion isn’t needed. In fact, it prevents detection of "bla" + wrong().

For example, the following is not detected:

 0diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
 1index 1c2951deee..c1209013e5 100644
 2--- a/src/wallet/rpc/addresses.cpp
 3+++ b/src/wallet/rpc/addresses.cpp
 4@@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
 5 RPCHelpMan keypoolrefill()
 6 {
 7     return RPCHelpMan{"keypoolrefill",
 8-                "\nFills the keypool."+
 9+                "\nRefills each descriptor keypool in the wallet up to the specified number of new keys.\n"
10+                "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"), each with " + std::to_string(DEFAULT_KEYPOOL_SIZE) + " entries.\n" +
11         HELP_REQUIRING_PASSPHRASE,
12                 {
13                     {"newsize", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%u, or as set by -keypool", DEFAULT_KEYPOOL_SIZE)}, "The new keypool size"},

Fix the script by detecting it.

DrahtBot commented at 8:01 am on May 7, 2025: 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/32434.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj, rkrux, w0xlt

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

DrahtBot added the label Tests on May 7, 2025

laanwj commented at 10:18 am on May 7, 2025: member

The exclusion isn’t needed. In fact, it prevents detection of “bla” + wrong().

i wonder if the same is true for the comment exclusion. Would it detect /* bla */ wrong().

rkrux commented at 11:05 am on May 7, 2025: contributor

The exclusion isn’t needed. In fact, it prevents detection of “bla” + wrong().

i wonder if the same is true for the comment exclusion. Would it detect /* bla */ wrong().

It seems to.

0➜  bitcoin git:(keypoolrefill_docs) ✗ ./test/lint/lint-locale-dependence.py
1The locale dependent function std::to_string(...) appears to be used:
2src/wallet/rpc/addresses.cpp:                "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"), each with " + std::to_string(DEFAULT_KEYPOOL_SIZE) + " entries.\n" +
3src/wallet/rpc/addresses.cpp:                    {"newsize", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%u, or as set by -keypool", /* bla */ std::to_string(DEFAULT_KEYPOOL_SIZE))}, "The new keypool size"},
4
5Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. Please avoid using locale-dependent functions if possible.
6
7Advice not applicable in this specific case? Add an exception by updating the ignore list in ./test/lint/lint-locale-dependence.py

rkrux commented at 11:07 am on May 7, 2025: contributor

Maybe can mention the to_string function in the docs as well? It’s already a part of the linter.

https://github.com/bitcoin/bitcoin/blob/59d3e4ed34eb55cb40928d524cb0bd5e183ed85a/doc/developer-notes.md?plain=1#L1022-L1042

lint: Remove string exclusion from locale check

The string exclusion would fail to detect `"bla" + wrong()`.

Also, remove /* */ comment exclusion, which would fail to detect stuff
like `/* bla */ wrong()`.

Instead, require the function to be called by adding \\( to the regex.

Finally, also remove the section in the dev notes, because:

* It was outdated and missing some functions such as std::to_string in
  the list.
* The maintenance overhead of having to update two places is fragile and
  questionable.
* Many other linters are also not mentioned in the dev notes, even
  though they are important.
* A dev (and CI) is more likely to run the linters than to read the dev
  notes.
* The dev notes are more than 1000 lines of dense information. It would
  be easier to digest if they focused on the important stuff that is not
  checked by automated tools.

fa24fdcb7f

maflcko force-pushed on May 7, 2025

maflcko commented at 11:25 am on May 7, 2025: member

The exclusion isn’t needed. In fact, it prevents detection of “bla” + wrong().

i wonder if the same is true for the comment exclusion. Would it detect /* bla */ wrong().

Yes, I can see it may happen with named args. Fixed.

Though, Regex isn’t really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy

Maybe can mention the to_string function in the docs as well? It’s already a part of the linter.

I removed it, as per the rationale in the commit message. (There are many more missing than just this one)

laanwj commented at 11:49 am on May 7, 2025: member

Code review ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b.

Though, Regex isn’t really the right tool to parse C++ code. Better would be to write a clang-tidy plugin. See https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools/bitcoin-tidy

Agree. The current script is more of a band-aid, there are likely better ways to check if certain functions aren’t used in C++ code.

What came to mind is “just check the linker output, the linker knows”. However this won’t catch inline / template functions.

rkrux commented at 1:15 pm on May 7, 2025: contributor

ACK fa24fdcb7f474e6959ae4d8fe9759d365c6c021b

I have not hand-checked the regexs but I did run the linter with the changes in #32429 and the errors were correctly caught.

The earlier documentation made me wonder whether the dev is supposed to run this lint manually but I verified that it is a part of the CI as well, just like the other python linters.

+1 on the detailed reasoning in the commit message regarding removal in the docs.

w0xlt commented at 5:57 pm on May 7, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32434/commits/fa24fdcb7f474e6959ae4d8fe9759d365c6c021b

in test/lint/lint-locale-dependence.py:46 in fa24fdcb7f

42@@ -43,6 +43,7 @@
43 
44 KNOWN_VIOLATIONS = [
45     "src/dbwrapper.cpp:.*vsnprintf",
46+    "src/span.h:.*printf",

laanwj commented at 9:30 am on May 8, 2025:

i was worried for a bit here but apparently this appears in a comment only.

fanquake merged this on May 8, 2025

fanquake closed this on May 8, 2025

lint: Remove string exclusion from locale check #32434

Code Coverage & Benchmarks

Reviews