Looks like recent versions of vulture are triggering false positives on while True loops with internal breaks.
Failure: https://github.com/bitcoin/bitcoin/actions/runs/22981056848/job/66720754466?pr=34628
Upstream issue: https://github.com/jendrikseipp/vulture/issues/412
I guess the triggering change was: https://github.com/jendrikseipp/vulture/pull/378
I wonder if this tool ever found a real true positive, given that we are running it at confidence=100%, which can only find very limited things? Sure, dead code in tests isn’t ideal and possibly scary, but if this only finds the plain obvious cases, that are unlikely to be written, and trivial to catch during manual review, maybe it could be removed?
No strong opinion, just wondering. In the meantime, I’ve submitted https://github.com/bitcoin/bitcoin/pull/34814
Yes I’ve been hoping astral would pick this up too for some time, but it seems like for now, all astral has on this front (via ty) is unused variable detection and a few other easy-to-spot types. But no “unused classes or functions”. AFAIU there seems to be some barriers on how to implement sanely for python, as there’s not an obious way to do it outside of parsing all files for all usage of all classes etc. as everything in python in public.
ty has a similar issue here: https://github.com/astral-sh/ty/issues/2829, and I might slightly expect it to be added there rather than ruff…
all astral has on this front (via
ty) is unused variable detection and a few other easy-to-spot types. But no “unused classes or functions”.
I think this is also what vulture does for us, no? Unused classes and functions need to be manually removed, see 5c005363a880c136cc44ff2456a402e398fcbf44
I guess it could help to have a set of MWE what vulture catches today for use, and what ty catches. If they are the same, it could make sense to use ty.
I guess it could help to have a set of MWE what vulture catches today for use, and what ty catches. If they are the same, it could make sense to use ty.
I could try and work something like that out. I do have #34547 open to switch to ty (amongst other things) already :)
I guess it could help to have a set of MWE what vulture catches today for use, and what ty catches. If they are the same, it could make sense to use ty.
Ok so ty does not have this capability at all yet, my bad. Ruff can detect some per-file items (unused imports/local variables/arguments), but nothing cross-file like vulture.
If we dropped vulture for ruff we’d lose:
We could enable unused argument rules:
0 "ARG001", # unused function argument
1 "ARG002", # unused method argument
2 "ARG003", # unused classmethod argument
3 "ARG004", # unused staticmethod argument
4 "ARG005", # unused lambda argument
which when enabled (on top of 34547) find 44 “infractions”:
0core/worktrees/modernise-linter on modernise-linter [$!?] via △ v4.1.2 via 🐍 v3.13.12 via ❄️ impure (nix-shell-env)
1❯ ruff check .
2ARG001 Unused function argument: `size`
3 --> contrib/tracing/log_raw_p2p_msgs.py:159:33
4 |
5158 | # BCC: perf buffer handle function for inbound_messages
6159 | def handle_inbound(_, data, size):
7 | ^^^^
8160 | """ Inbound message handler.
9 |
10
11ARG001 Unused function argument: `size`
12 --> contrib/tracing/log_raw_p2p_msgs.py:169:34
13 |
14167 | # BCC: perf buffer handle function for outbound_messages
15168 |
16169 | def handle_outbound(_, data, size):
17 | ^^^^
18170 | """ Outbound message handler.
19 |
20
21ARG001 Unused function argument: `size`
22 --> contrib/tracing/log_utxocache_flush.py:84:31
23 |
2482 | b = BPF(text=program, usdt_contexts=[bitcoind_with_usdts])
2583 |
2684 | def handle_flush(_, data, size):
27 | ^^^^
2885 | """ Coins Flush handler.
2986 | Called each time coin caches and indexes are flushed."""
30 |
31
32ARG001 Unused function argument: `size`
33 --> contrib/tracing/mempool_monitor.py:140:31
34 |
35138 | return datetime.now(timezone.utc)
36139 |
37140 | def handle_added(_, data, size):
38 | ^^^^
39141 | event = bpf["added_events"].event(data)
40142 | events.append((get_timestamp(), "added", event))
41 |
42
43ARG001 Unused function argument: `size`
44 --> contrib/tracing/mempool_monitor.py:144:33
45 |
46142 | events.append((get_timestamp(), "added", event))
47143 |
48144 | def handle_removed(_, data, size):
49 | ^^^^
50145 | event = bpf["removed_events"].event(data)
51146 | events.append((get_timestamp(), "removed", event))
52 |
53
54ARG001 Unused function argument: `size`
55 --> contrib/tracing/mempool_monitor.py:148:34
56 |
57146 | events.append((get_timestamp(), "removed", event))
58147 |
59148 | def handle_rejected(_, data, size):
60 | ^^^^
61149 | event = bpf["rejected_events"].event(data)
62150 | events.append((get_timestamp(), "rejected", event))
63 |
64
65ARG001 Unused function argument: `size`
66 --> contrib/tracing/mempool_monitor.py:152:34
67 |
68150 | events.append((get_timestamp(), "rejected", event))
69151 |
70152 | def handle_replaced(_, data, size):
71 | ^^^^
72153 | event = bpf["replaced_events"].event(data)
73154 | events.append((get_timestamp(), "replaced", event))
74 |
75
76ARG001 Unused function argument: `size`
77 --> contrib/tracing/p2p_monitor.py:140:33
78 |
79139 | # BCC: perf buffer handle function for inbound_messages
80140 | def handle_inbound(_, data, size):
81 | ^^^^
82141 | """ Inbound message handler.
83 |
84
85ARG001 Unused function argument: `size`
86 --> contrib/tracing/p2p_monitor.py:153:34
87 |
88152 | # BCC: perf buffer handle function for outbound_messages
89153 | def handle_outbound(_, data, size):
90 | ^^^^
91154 | """ Outbound message handler.
92 |
93
94ARG001 Unused function argument: `kwargs`
95 --> src/crc32c/.ycm_extra_conf.py:125:30
96 |
97125 | def FlagsForFile(filename, **kwargs):
98 | ^^^^^^
99126 | """Implements the YouCompleteMe API."""
100 |
101
102ARG002 Unused method argument: `success`
103 --> test/functional/feature_segwit.py:347:52
104 |
105345 | return [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh]
106346 |
107347 | def create_and_mine_tx_from_txids(self, txids, success=True):
108 | ^^^^^^^
109348 | tx = CTransaction()
110349 | for i in txids:
111 |
112
113ARG005 Unused lambda argument: `h`
114 --> test/functional/feature_taproot.py:877:16
115 |
116875 | lambda h: h,
117876 | # Combine with hash 0
118877 | lambda h: bytes([0 for _ in range(32)]),
119 | ^
120878 | # Combine with hash 2^256-1
121879 | lambda h: bytes([0xff for _ in range(32)]),
122 |
123
124ARG005 Unused lambda argument: `h`
125 --> test/functional/feature_taproot.py:879:16
126 |
127877 | lambda h: bytes([0 for _ in range(32)]),
128878 | # Combine with hash 2^256-1
129879 | lambda h: bytes([0xff for _ in range(32)]),
130 | ^
131880 | # Combine with itself-1 (BE)
132881 | lambda h: (int.from_bytes(h, 'big') - 1).to_bytes(32, 'big'),
133 |
134
135ARG005 Unused lambda argument: `h`
136 --> test/functional/feature_taproot.py:1217:22
137 |
1381215 | # == Test case for [#24765](/bitcoin-bitcoin/24765/) ==
1391216 |
1401217 | zero_fn = lambda h: bytes([0 for _ in range(32)])
141 | ^
1421218 | tap = taproot_construct(pubs[0], [("leaf", CScript([pubs[1], OP_CHECKSIG, pubs[1], OP_CHECKSIGADD, OP_2, OP_EQUAL])), zero_fn])
1431219 | add_spender(spenders, "case24765", tap=tap, leaf="leaf", inputs=[getter("sign"), getter("sign")], key=secs[1], no_fail=True)
144 |
145
146ARG005 Unused lambda argument: `idx`
147 --> test/functional/interface_rpc.py:148:40
148 |
149146 | def test_batch_requests(self):
150147 | self.log.info("Testing empty batch request...")
151148 | self.test_batch_request(lambda idx: None)
152 | ^^^
153149 |
154150 | self.log.info("Testing basic JSON-RPC 2.0 batch request...")
155 |
156
157ARG005 Unused lambda argument: `idx`
158 --> test/functional/interface_rpc.py:151:40
159 |
160150 | self.log.info("Testing basic JSON-RPC 2.0 batch request...")
161151 | self.test_batch_request(lambda idx: BatchOptions(version=2))
162 | ^^^
163152 |
164153 | self.log.info("Testing JSON-RPC 2.0 batch with notifications...")
165 |
166
167ARG005 Unused lambda argument: `idx`
168 --> test/functional/interface_rpc.py:157:40
169 |
170156 | self.log.info("Testing JSON-RPC 2.0 batch of ALL notifications...")
171157 | self.test_batch_request(lambda idx: BatchOptions(version=2, notification=True))
172 | ^^^
173158 |
174159 | # JSONRPC 1.1 does not support batch requests, but test them for backwards compatibility.
175 |
176
177ARG005 Unused lambda argument: `idx`
178 --> test/functional/interface_rpc.py:161:40
179 |
180159 | # JSONRPC 1.1 does not support batch requests, but test them for backwards compatibility.
181160 | self.log.info("Testing nonstandard JSON-RPC 1.1 batch request...")
182161 | self.test_batch_request(lambda idx: BatchOptions(version=1))
183 | ^^^
184162 |
185163 | self.log.info("Testing nonstandard mixed JSON-RPC 1.1/2.0 batch request...")
186 |
187
188ARG005 Unused lambda argument: `idx`
189 --> test/functional/interface_rpc.py:167:40
190 |
191166 | self.log.info("Testing nonstandard batch request without version numbers...")
192167 | self.test_batch_request(lambda idx: BatchOptions())
193 | ^^^
194168 |
195169 | self.log.info("Testing nonstandard batch request without version numbers or ids...")
196 |
197
198ARG005 Unused lambda argument: `idx`
199 --> test/functional/interface_rpc.py:170:40
200 |
201169 | self.log.info("Testing nonstandard batch request without version numbers or ids...")
202170 | self.test_batch_request(lambda idx: BatchOptions(notification=True))
203 | ^^^
204171 |
205172 | self.log.info("Testing nonstandard jsonrpc 1.0 version number is accepted...")
206 |
207
208ARG005 Unused lambda argument: `idx`
209 --> test/functional/interface_rpc.py:173:40
210 |
211172 | self.log.info("Testing nonstandard jsonrpc 1.0 version number is accepted...")
212173 | self.test_batch_request(lambda idx: BatchOptions(request_fields={"jsonrpc": "1.0"}))
213 | ^^^
214174 |
215175 | self.log.info("Testing unrecognized jsonrpc version number is rejected...")
216 |
217
218ARG005 Unused lambda argument: `idx`
219 --> test/functional/interface_rpc.py:176:40
220 |
221175 | self.log.info("Testing unrecognized jsonrpc version number is rejected...")
222176 | self.test_batch_request(lambda idx: BatchOptions(
223 | ^^^
224177 | request_fields={"jsonrpc": "2.1"},
225178 | response_fields={"result": None, "error": {"code": RPC_INVALID_REQUEST, "message": "JSON-RPC version not supported"}}))
226 |
227
228ARG001 Unused function argument: `args`
229 --> test/functional/mocks/invalid_signer.py:20:15
230 |
23118 | sys.exit(int(mock_result[0]))
23219 |
23320 | def enumerate(args):
234 | ^^^^
23521 | sys.stdout.write(json.dumps([{"fingerprint": "b3c19bfc", "type": "trezor", "model": "trezor_t"}]))
236 |
237
238ARG001 Unused function argument: `args`
239 --> test/functional/mocks/multi_signers.py:10:15
240 |
241 8 | import sys
242 9 |
24310 | def enumerate(args):
244 | ^^^^
24511 | sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"},
24612 | {"fingerprint": "00000002", "type": "trezor", "model": "trezor_one"}]))
247 |
248
249ARG001 Unused function argument: `args`
250 --> test/functional/mocks/no_signer.py:10:15
251 |
252 8 | import sys
253 9 |
25410 | def enumerate(args):
255 | ^^^^
25611 | sys.stdout.write(json.dumps([]))
257 |
258
259ARG001 Unused function argument: `args`
260 --> test/functional/mocks/signer.py:20:15
261 |
26218 | sys.exit(int(mock_result[0]))
26319 |
26420 | def enumerate(args):
265 | ^^^^
26621 | sys.stdout.write(json.dumps([{"fingerprint": "00000001", "type": "trezor", "model": "trezor_t"}]))
267 |
268
269ARG002 Unused method argument: `message`
270 --> test/functional/p2p_add_connections.py:23:26
271 |
27222 | class P2PFeelerReceiver(P2PInterface):
27323 | def on_version(self, message):
274 | ^^^^^^^
27524 | # The bitcoind node closes feeler connections as soon as a version
27625 | # message is received from the test framework. Don't send any responses
277 |
278
279ARG002 Unused method argument: `message`
280 --> test/functional/p2p_addr_relay.py:60:26
281 |
28258 | assert addr.ip.startswith('123.123.')
28359 |
28460 | def on_getaddr(self, message):
285 | ^^^^^^^
28661 | # When the node sends us a getaddr, it increments the addr relay tokens for the connection by 1000
28762 | self._tokens += 1000
288 |
289
290ARG002 Unused method argument: `message`
291 --> test/functional/p2p_addr_relay.py:77:26
292 |
29375 | return self.num_ipv4_received != 0
29476 |
29577 | def on_version(self, message):
296 | ^^^^^^^
29778 | self.send_version()
29879 | self.send_without_ping(msg_verack())
299 |
300
301ARG002 Unused method argument: `message`
302 --> test/functional/p2p_compactblocks.py:82:29
303 |
30480 | self.last_sendcmpct.append(message)
30581 |
30682 | def on_cmpctblock(self, message):
307 | ^^^^^^^
30883 | self.block_announced = True
30984 | self.announced_blockhashes.add(self.last_message["cmpctblock"].header_and_shortids.header.hash_int)
310 |
311
312ARG002 Unused method argument: `message`
313 --> test/functional/p2p_compactblocks.py:86:26
314 |
31584 | self.announced_blockhashes.add(self.last_message["cmpctblock"].header_and_shortids.header.hash_int)
31685 |
31786 | def on_headers(self, message):
318 | ^^^^^^^
31987 | self.block_announced = True
32088 | for x in self.last_message["headers"].headers:
321 |
322
323ARG002 Unused method argument: `message`
324 --> test/functional/p2p_compactblocks.py:91:22
325 |
32689 | self.announced_blockhashes.add(x.hash_int)
32790 |
32891 | def on_inv(self, message):
329 | ^^^^^^^
33092 | for x in self.last_message["inv"].inv:
33193 | if x.type == MSG_BLOCK:
332 |
333
334ARG002 Unused method argument: `message`
335 --> test/functional/p2p_feefilter.py:19:28
336 |
33717 | feefilter_received = False
33818 |
33919 | def on_feefilter(self, message):
340 | ^^^^^^^
34120 | self.feefilter_received = True
342 |
343
344ARG002 Unused method argument: `message`
345 --> test/functional/p2p_filter.py:67:30
346 |
34765 | self.send_without_ping(want)
34866 |
34967 | def on_merkleblock(self, message):
350 | ^^^^^^^
35168 | self._merkleblock_received = True
352 |
353
354ARG002 Unused method argument: `message`
355 --> test/functional/p2p_filter.py:70:21
356 |
35768 | self._merkleblock_received = True
35869 |
35970 | def on_tx(self, message):
360 | ^^^^^^^
36171 | self._tx_received = True
362 |
363
364ARG002 Unused method argument: `message`
365 --> test/functional/p2p_leak.py:71:29
366 |
36769 | def on_getblocktxn(self, message): self.bad_message(message)
36870 | def on_blocktxn(self, message): self.bad_message(message)
36971 | def on_wtxidrelay(self, message): self.got_wtxidrelay = True
370 | ^^^^^^^
37172 | def on_sendaddrv2(self, message): self.got_sendaddrv2 = True
372 |
373
374ARG002 Unused method argument: `message`
375 --> test/functional/p2p_leak.py:72:29
376 |
37770 | def on_blocktxn(self, message): self.bad_message(message)
37871 | def on_wtxidrelay(self, message): self.got_wtxidrelay = True
37972 | def on_sendaddrv2(self, message): self.got_sendaddrv2 = True
380 | ^^^^^^^
381 |
382
383ARG002 Unused method argument: `message`
384 --> test/functional/p2p_leak.py:85:26
385 |
38683 | # node will give us a message that it shouldn't. This is not an exhaustive
38784 | # list!
38885 | def on_version(self, message):
389 | ^^^^^^^
39086 | self.version_received = True
39187 | self.send_without_ping(msg_ping())
392 |
393
394ARG002 Unused method argument: `message`
395 --> test/functional/p2p_sendtxrcncl.py:49:26
396 |
39748 | class P2PFeelerReceiver(SendTxrcnclReceiver):
39849 | def on_version(self, message):
399 | ^^^^^^^
40050 | # feeler connections can not send any message other than their own version
40151 | self.send_version()
402 |
403
404ARG002 Unused method argument: `message`
405 --> test/functional/p2p_tx_privacy.py:41:26
406 |
40739 | self.all_invs = []
40840 |
40941 | def on_version(self, message):
410 | ^^^^^^^
41142 | self.send_without_ping(msg_wtxidrelay())
412 |
413
414ARG002 Unused method argument: `wallet`
415 --> test/functional/rpc_getdescriptoractivity.py:133:45
416 |
417131 | node.getdescriptoractivity, [invalid_blockhash], [f"addr({addr_1})"], True)
418132 |
419133 | def test_invalid_descriptor(self, node, wallet):
420 | ^^^^^^
421134 | self.log.info("Test that an invalid descriptor raises the correct RPC error")
422135 | blockhash = self.generate(node, 1)[0]
423 |
424
425ARG002 Unused method argument: `wallet`
426 --> test/functional/rpc_getdescriptoractivity.py:207:37
427 |
428205 | [blockhash_1, blockhash_2, blockhash_2], [wallet.get_descriptor()], True))
429206 |
430207 | def test_no_address(self, node, wallet):
431 | ^^^^^^
432208 | self.log.info("Test that activity is still reported for scripts without an associated address")
433209 | raw_wallet = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK)
434 |
435
436ARG002 Unused method argument: `node`
437 --> test/functional/wallet_listdescriptors.py:29:30
438 |
43928 | # do not create any wallet by default
44029 | def init_wallet(self, *, node):
441 | ^^^^
44230 | return
443 |
444
445ARG002 Unused method argument: `split`
446 --> test/functional/wallet_simulaterawtx.py:25:29
447 |
44823 | self.skip_if_no_wallet()
44924 |
45025 | def setup_network(self, split=False):
451 | ^^^^^
45226 | self.setup_nodes()
453 |
454
455Found 44 errors.
However I think all but 1 are false-positives due to callback interfaces and method overrides though, so not too useful for us…
I wonder if this tool ever found a real true positive, given that we are running it at confidence=100%, which can only find very limited things?
Ah, that explains why it cannot find unused functions like https://github.com/bitcoin/bitcoin/blob/136132e075fa79f1b74b48447c2d76734f57754b/test/functional/p2p_v2_encrypted.py#L49-L52 then I guess… I wondered about that for a while. (edit: ty foudn this one somehow in this branch, but I don’t recall how now)
So as per the vulture readme at 100% we only find
But we don’t find unused functions, classes, methods, variables, or imports, as those are rated 60-90% confidence.
So at –min-confidence 100 we lose the cross-file dead code detection, which is the main thing vulture offers over ruff…
We could enable unused argument rules:
I wouldn’t mind enabling them, if the error message provides a clear solution like: Remove it or prefix it with an underscore (haven’t tried if this works). Otherwise, it could be confusing for devs, not knowing what possible options exist.
So as per the vulture readme at 100% we only find
1. Unreachable code after return/break/continue/raise 2. Unused function arguments
I guess “unused function arguments” aren’t at 100% either, because ruff ARG* finds some?
And trivially unreachable code like
0return 1
1return 2
Seems unlikely to be written/reviewed while passing tests. So if they still happen, it should mostly be harmless text, which is annoying, but not severe? So we may consider removing vulture?
So we may consider removing vulture?
I believe so
Ok suggested that in #34816
Enabling https://docs.astral.sh/ruff/rules/#flake8-unused-arguments-arg for ruff, or ty stuff can be done in a separate pull.
Labels
Tests