lint: Converting lint-assertions.sh to lint-assertions.py

hiagopdutra commented at 8:18 pm on April 14, 2022: contributor

This PR is converting test/lint/lint-assertions.sh to test/lint/lint-assertions.py. It’s an item of #24783.

DrahtBot added the label Tests on Apr 14, 2022

in test/lint/lint-assertions.py:42 in 501e75534c outdated

37+    ], "Assertions should not have side effects:")
38+
39+    # Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
40+    # is undesirable to crash the whole program. See: src/util/check.h
41+    # src/rpc/server.cpp is excluded from this check since it's mostly meta-code.
42+    exit_code += exec_cmd([

Eunoia1729 commented at 0:03 am on April 15, 2022:

0    exit_code |= exec_cmd([

in test/lint/lint-assertions.py:11 in 501e75534c outdated

 6+#
 7+# Check for assertions with obvious side effects.
 8+
 9+import sys
10+
11+from subprocess import check_output, CalledProcessError

Eunoia1729 commented at 0:08 am on April 15, 2022:

nit: the code will more clearer if we import subprocess and then make calls using subprocess.check_output and so on.

Eunoia1729 commented at 0:09 am on April 15, 2022: contributor

concept ACK

in test/lint/lint-assertions.py:20 in f8b3dfd90f outdated

15+        output = subprocess.check_output(command, universal_newlines=True, encoding="utf8")
16+        print(error_msg)
17+        print(output)
18+        return 1
19+    except subprocess.CalledProcessError as ex1:
20+        if ex1.returncode > 1:

laanwj commented at 7:58 am on April 15, 2022:

Why check for a return code larger than 1 here? The original script doesn’t check return codes at all. As we’ve discussed in other linter reviews, it’s fine to leave out this except and let a CalledProcessError escalate. This only hapens if git fails.

hiagopdutra commented at 2:29 pm on April 15, 2022:

When the output of grep is empty the return code will be 1, it in the bash script it don’t stop the execution because it in a $(), that will get only output ignoring the exit code. The idea here is expect the CalledProcessError.returncode equal 1 to have sure if command output is empty.

laanwj commented at 9:23 pm on April 15, 2022:

OK, that makes sense. Do you know if this behavior of git grep is documented anywhere? The manual page doesn’t mention anything about return codes.

Edit: apparently this is expected behavior because grep does it:

0EXIT STATUS
1       Normally the exit status is 0 if a line is selected, 1 if no lines were selected, and 2 if an error  occurred.
2       However,  if  the  -q  or  --quiet or --silent is used and a line is selected, the exit status is 0 even if an
3       error occurred.

in test/lint/lint-assertions.py:15 in f8b3dfd90f outdated

10+import subprocess
11+
12+
13+def exec_cmd(command: [], error_msg: ""):
14+    try:
15+        output = subprocess.check_output(command, universal_newlines=True, encoding="utf8")

laanwj commented at 7:59 am on April 15, 2022:

You should check the output for being non-empty, like in the original script.

hiagopdutra commented at 2:29 pm on April 15, 2022:

If it prints the output, so it isn’t empty.

in test/lint/lint-assertions.py:13 in f8b3dfd90f outdated

 8+
 9+import sys
10+import subprocess
11+
12+
13+def exec_cmd(command: [], error_msg: ""):

laanwj commented at 8:01 am on April 15, 2022:

Would prefer not to have this wrapper and use subprocess.check_output directly below where relevant. The amount of shared code is minimal, no need to define a function.

hiagopdutra commented at 2:30 pm on April 15, 2022:

I did it because of the try catch needed to check the return code when it is empty.

laanwj commented at 9:37 pm on April 15, 2022:

Fair enough. Though this logic is specific to git grep. It makes sense to make it a git_grep wrapper specifically.

hiagopdutra requested review from laanwj on Apr 19, 2022

KevinMusgrave commented at 8:04 pm on April 20, 2022: contributor

Tested ACK 1e0a9ad1cedafe10a1e8743a4eda1d64cef5fe2c

Before:

 0Assertions should not have side effects:
 1
 2src/addrman.cpp:            assert(--nIds != nNew); // this means nNew was wrong, oh ow
 3src/bench/ccoins_caching.cpp:        assert(success = 2);
 4src/bench/checkblock.cpp:        assert(rewound ++);
 5src/bench/verify_script.cpp:        assert(success += 1);
 6src/node/coin.cpp:    assert(node.mempool -= 1);
 7CHECK_NONFATAL(condition) should be used instead of assert for RPC code.
 8
 9src/rpc/blockchain.cpp:77:    assert(blockindex);
10src/rpc/rawtransaction.cpp:469:            assert(false);

After:

0Assertions should not have side effects:
1src/addrman.cpp:            assert(--nIds != nNew); // this means nNew was wrong, oh ow
2src/bench/ccoins_caching.cpp:        assert(success = 2);
3src/bench/checkblock.cpp:        assert(rewound ++);
4src/bench/verify_script.cpp:        assert(success += 1);
5src/node/coin.cpp:    assert(node.mempool -= 1);
6
7CHECK_NONFATAL(condition) should be used instead of assert for RPC code.
8src/rpc/blockchain.cpp:77:    assert(blockindex);
9src/rpc/rawtransaction.cpp:469:            assert(false);

KevinMusgrave commented at 8:06 pm on April 20, 2022: contributor

I guess you should squash your commits, see: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Porting lint-assertions.sh to lint-assertions.py 172c2333f0

hiagopdutra force-pushed on Apr 22, 2022

laanwj commented at 5:46 pm on April 25, 2022: member

Tested ACK 172c2333f03aecb4c347c791537e13c296adbde2

laanwj merged this on Apr 25, 2022

laanwj closed this on Apr 25, 2022

hiagopdutra deleted the branch on Apr 25, 2022

sidhujag referenced this in commit 80390805ab on Apr 26, 2022

DrahtBot locked this on Apr 25, 2023

lint: Converting lint-assertions.sh to lint-assertions.py #24856