test: Remove python dead code linter #16961

1. 
   laanwj commented at 9:17 am on September 25, 2019: member

   Primarily I’d like to remove this because it is very imprecise, due to Python’s dynamic nature, giving it a large list of false positives that need to be listed as exceptions. See for example #16906.

   It’s also a frequent source of complaints. I’m doubtful of the usefulness of checking for dead code in a linter in the first place. Having some dead code in the test framework for a while is not a disaster.

2. test: Remove python dead code linter

   Primarily I'd like to remove this because it is very imprecise, due to
   Python's dynamic nature, giving it a large list of false positives that
   need to be listed as exceptions. See for example #16906.
   
   It's also a frequent source of complaints. I'm doubtful of the
   usefulness of checking for dead code in a linter in the first place.
   Having some dead code in the test framework for a while is not a
   disaster.

   f4beb4996d
3. laanwj added the label Tests on Sep 25, 2019
4. 
   practicalswift commented at 10:41 am on September 25, 2019: contributor

   Concept ACK on making something about the current state of this linter: the cost seems to exceed the benefit for this specific linter as currently configured.

   Perhaps an alternative would be to simply switch from exit 1 to exit 0 on vulture warnings? To make it behave like codespell (i.e. non-fatal).

5. 
   DrahtBot commented at 1:31 pm on September 25, 2019: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #16936 (Bugfix: QA: Fix service flag comparison check in rpc_net test by luke-jr)

   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.

6. 
   laanwj commented at 1:39 pm on September 25, 2019: member

   The thing is, I don’t particularly want the list of exceptions to need to be maintained as part of the repository anymore.
7. 
   sdaftuar commented at 2:15 pm on September 25, 2019: member

   utACK f4beb4996d27f2cdaf4f0a63e7dc044bf17decce
8. 
   practicalswift commented at 2:28 pm on September 25, 2019: contributor

   ACK f4beb4996d27f2cdaf4f0a63e7dc044bf17decce – diff looks correct
9. 
   jamesob commented at 2:33 pm on September 25, 2019: member

   ACK https://github.com/bitcoin/bitcoin/pull/16961/commits/f4beb4996d27f2cdaf4f0a63e7dc044bf17decce
10. 
    luke-jr commented at 3:25 pm on September 25, 2019: member

    Concept ACK
11. 
    promag commented at 3:44 pm on September 25, 2019: member

    Bye ACK.
12. 
    MarcoFalke commented at 5:13 pm on September 25, 2019: member

    Going to merge this, as there is overwhelming support for it and it is probably not worth to spend more time discussion on it.
13. MarcoFalke referenced this in commit 4c4ff4911a on Sep 25, 2019
14. MarcoFalke merged this on Sep 25, 2019
15. MarcoFalke closed this on Sep 25, 2019

    ---

16. 
    laanwj commented at 11:30 am on September 26, 2019: member

    Going to merge this, as there is overwhelming support for it and it is probably not worth to spend more time discussion on it.

    Thanks. Maybe in the future at some point this tool will be mature enough to use in a setting like this, and we can re-evaluate, but for now it’s too much bother.

17. sidhujag referenced this in commit 5ce21ba032 on Sep 26, 2019
18. 
    flack commented at 4:00 pm on February 5, 2021: contributor

    I wonder if it would have been better to change min-confidence to 100 (or something) instead. At least that would have probably caught #21081. Having dead code in the test framework may not be a disaster, but having it in the test themselves seems slightly more worrying
19. 
    MarcoFalke commented at 4:03 pm on February 5, 2021: member

    @flack Pull requests are welcome if it can be show that the previously observed issues, false positives, and annoyances are fixed
20. 
    flack commented at 4:08 pm on February 5, 2021: contributor

    @MarcoFalke thanks, I literally have no Python skills whatsoever, but who knows, maybe I’ll give it a try. ofc, if someone more qualified wants to give it a try, have at it!
21. 
    laanwj commented at 6:16 pm on February 5, 2021: member

    The underlying problem here was that determining reachability statically an undecidable issue for python. This caused issue after issue. I don’t think that changed.
22. 
    flack commented at 6:52 pm on February 5, 2021: contributor

    Sure, but even not knowing Python I cannot imagine there’s a way for code after a return statement to ever be run. So I still think --min-confidence 100 might be worth a try, i.e. tell the linter to skip the fancy guesswork and just report stuff it’s 100% sure about.
23. UdjinM6 referenced this in commit 29caaf3ee7 on Sep 10, 2021
24. UdjinM6 referenced this in commit 7b8e019ceb on Sep 24, 2021
25. UdjinM6 referenced this in commit 849e9d055b on Oct 4, 2021
26. UdjinM6 referenced this in commit 313d79e319 on Oct 5, 2021
27. kittywhiskers referenced this in commit cbd5510338 on Oct 12, 2021
28. pravblockc referenced this in commit 7cdb038a2b on Nov 18, 2021
29. kittywhiskers referenced this in commit f87d4fde0d on Dec 12, 2021
30. DrahtBot locked this on Aug 16, 2022

Contributors
laanwj practicalswift DrahtBot sdaftuar jamesob luke-jr promag MarcoFalke flack

Labels
Tests