Re-add dead code detection #21096

flack commented at 7:09 PM on February 6, 2021: contributor

This re-adds unreachable code detection for Python based on vulture.

Effectively, this reverts f4beb4996d27f2cdaf4f0a63e7dc044bf17decce. The difference to the previous version is that this runs with the --min-confidence 100 setting. From https://pypi.org/project/vulture/:

Use --min-confidence 100 to only report code that is guaranteed to be unused within the analyzed files.

So this should avoid the previous issues where static analysis had wrong positives due to the dynamic nature of Python code by only reporting things that are unambiguous (such as code after a return statement). As such, there is not suppressions list.

My motivation was mainly #21081 which would have been caught by this (as can be seen by the CI run failing). This is still marked as draft because #21081 is needed to get the linter to pass. Also, there is a second problem that this found (see https://github.com/bitcoin/bitcoin/pull/19509/files#r571454691). From what I can tell, this is a spurious type comment that could just be removed (or if that line has no side effects it could also be deleted altogether?). I could add a commit here to fix it, but I wanted to see if there is interest in having this linter again in the first place

flack commented at 7:16 PM on February 6, 2021: contributor

meh, turns out clicked the wrong button, and now I can't mark it as Draft anymore... modified the title instead

flack renamed this:
~~Re-add dead code detection~~
[draft] Re-add dead code detection
on Feb 6, 2021

MarcoFalke marked this as a draft on Feb 6, 2021

DrahtBot added the label Tests on Feb 6, 2021

fanquake renamed this:
~~[draft] Re-add dead code detection~~
Re-add dead code detection
on Feb 7, 2021

flack commented at 6:53 PM on February 8, 2021: contributor

#21081 has been merged and the other issue I mentioned has been addressed in #21107, so this should now be ready for review. I also bumped the vulture dependency to 2.3 since it turns out Python 3.5 support is no longer needed

flack marked this as ready for review on Feb 8, 2021

MarcoFalke commented at 7:05 PM on February 8, 2021: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

flack commented at 8:16 PM on February 8, 2021: contributor

@MarcoFalke done

fanquake commented at 3:34 AM on February 9, 2021: member

Seems like at --min-confidence=100 vulture will just report unreachable code. You have to drop it to 90 for it to report unused imports:

diff --git a/test/functional/test_framework/key.py b/test/functional/test_framework/key.py
index e0cbab45c..d0f3472e9 100644
--- a/test/functional/test_framework/key.py
+++ b/test/functional/test_framework/key.py
@@ -14,6 +14,10 @@ import unittest
 
 from .util import modinv
 
+import nonsense
+
 def TaggedHash(tag, data):
     ss = hashlib.sha256(tag.encode('utf-8')).digest()
     ss += ss

bitcoin/test/functional/test_framework/key.py:17: unused import 'nonsense' (90% confidence)

or 60 before it will report unused functions, i.e xor_bytes in #21100:

bitcoin/test/functional/test_framework/key.py:23: unused function 'xor_bytes' (60% confidence)

Looking at the vulture source this makes sense, because the default confidence for basically all checks is 60.

MarcoFalke referenced this in commit d48f9e8ebb on Feb 9, 2021

sidhujag referenced this in commit 59801a48ab on Feb 9, 2021

laanwj commented at 8:38 AM on February 10, 2021: member

NACK on this. I still stand by discussion in #16961. Even having to fine-tune the threshold value just seems a recipe for endless frustration.

flack commented at 10:07 AM on February 10, 2021: contributor

@laanwj I'm not going to spend too much time arguing, but the rationale for #16961 was to avoid false positives. --min-confidence=100 accomplished that, since it is guaranteed to only find dead code (inside of if (false), after return statement or similar). So there isn't any need to fine-tune anything. This is just a little sanity check that even on my five year old laptop completes in less than one second, and it would have caught an instance of xkcd 221 in the taproot tests, which is probably one of the better-reviewed PRs. What's not to like?

in test/lint/lint-python-dead-code.sh:19 in 0a9e5fb9fb outdated

  12 | +    echo "Skipping Python dead code linting since vulture is not installed. Install by running \"pip3 install vulture\""
  13 | +    exit 0
  14 | +fi
  15 | +
  16 | +if ! vulture \
  17 | +    --min-confidence 100 \

MarcoFalke commented at 10:24 AM on February 10, 2021:

Maybe add a comment to say that this must stay at 100 to avoid false positives, which we can't deal with in this repo?

flack commented at 5:35 PM on February 10, 2021:

added note as requested

practicalswift commented at 9:21 PM on February 10, 2021: contributor

Concept ACK

If simply specifying --min-confidence 100 means that the false positives we've seen in the past are avoided then this should be an obvious win, no? :) @laanwj, is your NACK made assuming that --min-confidence 100 is not sufficient to solve the false positive issues we've seen in the past? Not questioning: just trying to understand the reasoning :)

laanwj commented at 3:45 PM on February 11, 2021: member

Okay retracted the NACK if everyone else wants this it's okay with me I just would really prefer if it doesn't come up as issue again.

Re-add dead code detection 3f8776a139

in test/lint/lint-python-dead-code.sh:20 in c0d7ae3ce8 outdated

  15 | +
  16 | +# --min-confidence 100 will only report code that is guaranteed to be unused within the analyzed files.
  17 | +# Any value below 100 introduces the risk of false positives, which would create an unacceptable maintenance burden.
  18 | +if ! vulture \
  19 | +    --min-confidence 100 \
  20 | +    $(git rev-parse --show-toplevel); then

practicalswift commented at 4:18 PM on February 11, 2021:

Should this be $(vulture --min-confidence 100 -- $(git ls-files -- "*.py")) instead to make sure only files in the repo are checked?

flack commented at 10:54 AM on February 12, 2021:

Could be changed if people think that's better. The rev-parse version is what the previous incarnation of the linter had. I was thinking maybe you want to run the linter against a new file before you git add it?

AFAICT, vulture automatically filters by file extension (https://github.com/jendrikseipp/vulture/blob/master/vulture/config.py#L105), so *.py should be implicit

laanwj commented at 11:33 AM on February 12, 2021:

don't think this is a serious issue but, ostensibly a developer could, unwisely, keep their own .py scripts inside the repo directory, which would make no sense to check here, so i think ls-files to only check the committed ones makes sense

flack commented at 8:58 AM on February 13, 2021:

Alright, changed to ls-files as requested

practicalswift commented at 3:20 PM on February 13, 2021: contributor

ACK 3f8776a1391c3978ed66144df15fd9bcb9edd35d

MarcoFalke commented at 2:13 PM on February 15, 2021: member

Tested ACK by reintroducing the bug, didn't review:

/tmp/cirrus-ci-build/test/functional/feature_taproot.py:521: unreachable code after 'return' (100% confidence)
^---- failure generated from test/lint/lint-python-dead-code.sh

MarcoFalke merged this on Feb 15, 2021

MarcoFalke closed this on Feb 15, 2021

sidhujag referenced this in commit 41bc01f866 on Feb 15, 2021

UdjinM6 referenced this in commit 68f62d4e93 on Sep 10, 2021

UdjinM6 referenced this in commit 31b4209318 on Sep 24, 2021

UdjinM6 referenced this in commit 5071a8070b on Oct 4, 2021

UdjinM6 referenced this in commit dbfa92990a on Oct 5, 2021

kittywhiskers referenced this in commit 8949ce83f8 on Oct 12, 2021

pravblockc referenced this in commit 6872c4a44d on Nov 18, 2021

DrahtBot locked this on Aug 16, 2022