lint: Add linter to error on #include <*.cpp> #13301

Files should depend on one another by interface, not by implementation. This checks for quoted includes as well.

With @practicalswift #13288 (comment)

Pending #13288, #13291

Current failures, via travis:

$ if [ "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/lint-all.sh; fi
The following files #include .cpp files:
src/test/blockchain_tests.cpp:#include "rpc/blockchain.cpp"
src/test/torcontrol_tests.cpp:#include <torcontrol.cpp>
^---- failure generated from contrib/devtools/lint-include-cpp.sh

Concept ACK

ACK 177dc4277722c2e3d5c34bab4de3951055510a88

Very nice! :-) Love these small regression tests that make sure we'll never have to think about a given class of otherwise recurring mistakes/issues ever again.

Tiny nit: Could be added to existing lint-includes.sh instead if we want to keep the number of linter files down.

Concept ACK.

Sorry to be skeptical here, but does this really happen enough to merit adding yet another linter?

@laanwj Generally, I'm in favor of more lints as implementing practice in code means that it need not be propagated / enforced by humans - particularly when the difference is easily overlooked by humans and easily detected by code. This case has been violated twice, and overlooked twice by reviewers, so an automatic check seems helpful, if we agree that it should not be violated.

On that point, I agree with MarcoFalke: "Including a cpp file seems extremely wrong." #13230 (review)

That said, I'm not clear how to value the cost of having an extra file in the contrib/devtools directory, or having this command run with each CI run.

Can this be combined with the other include linter, so we don't have too many "duplicate" files?

Can this be combined with the other include linter, so we don't have too many "duplicate" files?

I wouldn't mind separate files as it would allow:

find contrib/devtools -name "lint-*.sh" ! -name lint-all.sh | parallel -u bash :::

However flake8 takes most of the time so no gain for now (hint: s/bash/time above).

Should it just check if the included file is header|header.h|header.hpp?

ACK 2f2a784c7ba189628dfc5d9dbd10d1861ebaadbd

Generally, I'm in favor of more lints as implementing practice in code means that it need not be propagated / enforced by humans

I agree. I don't think I have a problem with it in principle. It's just the proliferation of all kinds of regexp shell scripts that might make the travis build fail, that I'm having problems with. There is a cost to this in maintenance.

I think linters should be used to avoid sneaky problems that might result in bugs. For example one I really like is #13041, which catches potential locale-dependency bugs.

Including a .cpp file is like including a 10000-pound elephant in the room, if reviewers don't catch this we have a really big problem.

Agreed, #13041 is a great one. I mean to cite the fact that this has happened before as evidence that these are tricky, perhaps because it is easy to gloss over includes, particularly in the context of tests. For reference, the PRs where these includes were introduced: #10408 #11748

@laanwj Adding some data: there seems to have been a total of twelve .cpp includes during the project git history:

$ git log -u | grep -E '^\+#include.*\.cpp.$' | cut -b2- | tr '<>' '"' | sort -u
#include "base58_tests.cpp"
#include "base64_tests.cpp"
#include "Checkpoints_tests.cpp"
#include "DoS_tests.cpp"
#include "miner_tests.cpp"
#include "rpc/blockchain.cpp"
#include "script_tests.cpp"
#include "torcontrol.cpp"
#include "transaction_tests.cpp"
#include "uint160_tests.cpp"
#include "uint256_tests.cpp"
#include "util_tests.cpp"

This issue has been independently introduced twice in the code base in two separate PR:s during the last twelve months alone. The PRs (#10408, #11748) were reviewed by very skilled developers before merge.

Thus it empirically seems like an extra pair of very attentive eyes in the form of Travis would be helpful in making sure this 10000-pound elephant doesn't sneak into the code base again :-)

@laanwj Regarding the potential maintenance need for the linting shell scripts: I volunteer to maintain all the linter scripts in the repo if that is of any help. In the event of a linter causing problems just ping me in and I'll fix it quickly! :-)

@laanwj Adding some data: there seems to have been a total of twelve .cpp includes during the project git history:

Even on the desirability there is an alternative perspective, FWIW: at one of my previous employers we maintained a huge system, and including the .c or .cpp was standard practice for private unit tests. This allowed testing internal functions and methods and which are not exposed on the public interface of a compilation unit. (an alternative to this at a slightly higher level was to have a "test interface", but this carries a bit of run time overhead)

Not saying that we should do that here, but it's all too easy to be blanket against something if it offends your aesthetic sensibilities.

Thus it empirically seems like an extra pair of very attentive eyes in the form of Travis would be helpful in making sure this 10000-pound elephant doesn't sneak into the code base again :-)

Heh :)

@laanwj Regarding the potential maintenance need for the linting shell scripts: I volunteer to maintain all the linter scripts in the repo if that is of any help. In the event of a linter causing problems just ping me in and I'll fix it quickly! :-)

Okay! Thanks.

I wouldn't mind separate files as it would allow: find contrib/devtools -name "lint-*.sh" ! -name lint-all.sh | parallel -u bash ::: However flake8 takes most of the time so no gain for now (hint: s/bash/time above).

I highly doubt that a single git grep makes up a significant run-time to justify for the overhead of another script with copyright header, etc.

Oh, I see this was already combined into the existing file. utACK, but needs rebase.

utACK 9d6c9dbb88593dea995072ba812f115a51ea2b4b

Rebased for #13385