clang-tidy: Enable misc-no-recursion #29690
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
dergoegge
force-pushed
the
2024-03-ct-no-recursion
branch
3 times, most recently
from
6f29b80 to
263f9c8
Compare
dergoegge
changed the title
It would be good to list at least one example in this codebase. |
A simple search for recursion on github turned up a few things:
|
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
Generally concept ACK, discouraging and making recursion explicit seems like the right thing to do. Would be nice (but I don't think possible) if this could be done at compile time instead of with clang-tidy, so devs find out quicker. Now, they might spend a whole lotta time implementing something, only to find out from CI that they shouldn't, and then potentially have to quite radically change their approach. Not a very common problem, I'd suspect though.
|
Probably useful to mention this in developer notes? E.g.: diff --git a/doc/developer-notes.md b/doc/developer-notes.md
index 89c13600eb..afc1e10a0b 100644
--- a/doc/developer-notes.md
+++ b/doc/developer-notes.md
@@ -115,6 +115,8 @@ code.
Use `reinterpret_cast` and `const_cast` as appropriate.
- Prefer [`list initialization ({})`](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-list) where possible.
For example `int x{0};` instead of `int x = 0;` or `int x(0);`
+ - Recursive functions are generally discouraged, and checked for by clang-tidy. When recursiveness
+ is the best approach, use `NOLINTNEXTLINE(misc-no-recursion)` to suppress the check.
For function calls a namespace should be specified explicitly, unless such functions have been declared within it.
Otherwise, [argument-dependent lookup](https://en.cppreference.com/w/cpp/language/adl), also known as ADL, could be |
dergoegge
force-pushed
the
2024-03-ct-no-recursion
branch
from
263f9c8 to
0c9db2b
Compare
|
Thanks @stickies-v! added your dev note suggestion |
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
crACK 0c9db2b1bb41fe1b00844cd21f226a57aeaddc53
I ran clang-tidy on master with misc-no-recursion and verified that the exceptions made correspond with where clang-tidy warned for me, with the exception of DescribeWalletAddressVisitor::ProcessSubScript (and the operator() overloads), which mine missed completely. I suspect this is because of the std::visit indirection. (LLVM 17.0.6)
The main potential risk with this PR imo is increasing developer frustration/friction, so to avoid that I would quickly bring up this PR on the next Thursday IRC meeting so people are aware and can surface concerns early?
dergoegge
force-pushed
the
2024-03-ct-no-recursion
branch
from
0c9db2b to
ca34f36
Compare
I don't agree with this as a general statement and I don't think any developer who has worked with a functional language for a few years would. I think bugs are just as likely to occur in loops and you will also find a lot of examples for that. Recursion is a very basic tool in software development and discouraging it in general feels wrong. A developer who is experienced with it should be able to use it if they think it's the right tool for the job. |
I agree that this is perhaps subjective. I've amended the description.
To me it seems like programmers are unlikely to blame recursion for bugs, when recursion is their primary tool. We are not working with a functional language in this repository, so there is no strict need for recursion.
Stack overflows caused by recursion is a specific class of bugs that can be avoided by simply not using recursion. Not using loops is obviously not an option in this repository. However, clang-tidy checks to avoid bugprone loop patterns might make sense in the same spirit as discouraging recursion, e.g. https://clang.llvm.org/extra/clang-tidy/checks/bugprone/infinite-loop.html.
I'm not proposing a ban on recursion. A experienced programmer can still choose to use recursion and suppress the linter. |
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
ACK ca34f36c326ee8e54f170a61e86ed74e3cb04da1
Personally, I'm in favour of discouraging recursion and making it explicit when used. It very much still should be used where it makes most sense, and that's still possible without much overhead.
fjahr
left a comment
fjahr
left a comment
There was a problem hiding this comment.
(Also adding my comment from IRC in case the discussion continues here) I think it’s not going to be doing much for us because it’s not like inexperienced developers include recursion in their PRs by accident very often. But since this also won’t affect ~99% all PRs anyway, I won’t stand in the way if enough others see value in this.
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
Concept ACK, as I think we agree that we want to avoid unintentional recursion. I think it's useful to have all of our recursive functions enumerated.
|
Concept ACK. I think tracking where any recursion currently is, is useful, as well as being alerted to new usage in the future. |
dergoegge
force-pushed
the
2024-03-ct-no-recursion
branch
from
ca34f36 to
d9536e6
Compare
|
Changed the dev note. Ready for review. |
sedited
left a comment
sedited
left a comment
There was a problem hiding this comment.
ACK d9536e6e423ef7384a1869f3ac8a9cf7f503ed6f
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
Co-authored-by: stickies-v <[email protected]> Co-authored-by: Gloria Zhao <[email protected]>
dergoegge
force-pushed
the
2024-03-ct-no-recursion
branch
from
d9536e6 to
78407b9
Compare
|
Rebased due to conflict |
|
ACK 78407b9 |
# Conflicts: # doc/developer-notes.md # src/wallet/scriptpubkeyman.cpp
9 participants
Recursion is a frequent source of stack overflow bugs. Secondly, introduction of recursion can be non-obvious.
This PR proposes to use the clang-tidy
misc-no-recursioncheck to make introduction of new recursion obvious. We don't make use of recursion a lot in our code base but there are a few places that need suppressions anyway (mostly the descriptor and univalue/rpc code).