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

pull hiagopdutra wants to merge 1 commits into bitcoin:master from hiagopdutra:port-assertions-linter changing 2 files +52 −34

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 12:03 AM on April 15, 2022:

    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 12: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 12: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:

EXIT STATUS Normally the exit status is 0 if a line is selected, 1 if no lines were selected, and 2 if an error occurred. However, if the -q or --quiet or --silent is used and a line is selected, the exit status is 0 even if an 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:

Assertions should not have side effects:

src/addrman.cpp:            assert(--nIds != nNew); // this means nNew was wrong, oh ow
src/bench/ccoins_caching.cpp:        assert(success = 2);
src/bench/checkblock.cpp:        assert(rewound ++);
src/bench/verify_script.cpp:        assert(success += 1);
src/node/coin.cpp:    assert(node.mempool -= 1);
CHECK_NONFATAL(condition) should be used instead of assert for RPC code.

src/rpc/blockchain.cpp:77:    assert(blockindex);
src/rpc/rawtransaction.cpp:469:            assert(false);

After:

Assertions should not have side effects:
src/addrman.cpp:            assert(--nIds != nNew); // this means nNew was wrong, oh ow
src/bench/ccoins_caching.cpp:        assert(success = 2);
src/bench/checkblock.cpp:        assert(rewound ++);
src/bench/verify_script.cpp:        assert(success += 1);
src/node/coin.cpp:    assert(node.mempool -= 1);

CHECK_NONFATAL(condition) should be used instead of assert for RPC code.
src/rpc/blockchain.cpp:77:    assert(blockindex);
src/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

Contributors

Review Requested
laanwj

Labels

Tests

Linked (view graph)

#24783 Port lint scripts to Python