Fix UnusedVariables false positive on result-referenced variables

[feiiiiii5]

Problem

Fixes #226.

fickling --check-safety reports false-positive UnusedVariables hits on variables that are used — specifically, variables referenced only inside the result = ... expression (the final assignment produced by STOP). The scanpy pickle in the issue shows _var16 flagged as unused even though it is referenced via BINGET inside the result tuple.

Root cause

Two related bugs, same effect:

1. Interpreter.unused_assignments() skips the result value. Upon encountering the result = ... assignment it breaks out of its loop without walking statement.value for ast.Name references. So any variable only referenced via the result expression is invisible to the heuristic.

2. Four opcode implementations construct ast.Tuple/ast.Dict with non-list field values. ast.walk only recurses into list-typed fields, not tuples or iterators. So even after fixing (1), variables nested inside these AST nodes would still be missed:

   • TupleOne / TupleTwo / TupleThree — elts was a tuple
   • Tuple (the variable-arity TUPLE opcode, StackSliceOpcode) — elts was tuple(stack_slice)
   • Dict (the DICT opcode) — keys/values were reversed(...) iterators

Fix

• unused_assignments() now walks statement.value with ast.walk and adds every ast.Name id to used before the break.
• All four opcode implementations now pass list instead of tuple/reversed().

Scope

This is a false-positive fix, not a bypass fix. It sharpens the UnusedVariables heuristic's precision (fewer false alarms on legitimate pickles) without weakening any security guarantee: genuinely malicious eval/import calls are still flagged with the same severity. Per SECURITY.md, UnusedVariables bypasses remain out of scope; reducing false positives on used variables is a quality-of-output improvement and is in scope.

Testing

• New regression tests in test/test_benign_edge_cases.py (7 tests):

  • test_var_used_in_result_not_flagged — result = _var0 (simplest case, exercises fix Add NewObj_EX opcode #1)
  • test_var_used_multiple_times_in_result_via_memo — result = (_var0, _var0) via BINGET + TupleTwo (mirrors the scanpy structure from False positive for --check-safety (variable is actually used afterwards) #226; exercises fix Add TUPLE1, TUPLE2 and TUPLE3 #2)
  • test_multiple_vars_used_in_result_not_flagged — result = (_var0, _var1) (guards against over-correction)
  • test_var_used_in_tuple_stack_slice_not_flagged — result = (_var0, _var0, _var0, _var0) via TUPLE opcode (exercises fix Add example showing the creation of a hello world Trojan on PyTorch e… #3)
  • test_var_used_in_dict_keys_not_flagged — result = {_var0: "v1", _var0: "v2"} via DICT opcode (exercises fix Carson/more poc #4)
  • test_genuinely_unused_var_still_flagged — _var1 is genuinely unused; ensures the fix does not over-correct and hide real unused variables
  • test_insert_python_eval_replacing_result_no_false_positive — end-to-end smoke test via check_safety(); asserts the eval is still OVERTLY_MALICIOUS and UnusedVariables no longer appears in detailed results

• Updated test/test_pickle.py::test_unused_variables — the old test used use_output_as_unpickle_result=True, which makes result = _var0 (so _var0 IS used) yet asserted _var0 was unused. That assertion codified the bug. Switched to run_first=True, use_output_as_unpickle_result=False, where _var0 is genuinely unused (result is the original [1,2,3,4]). Severity remains OVERTLY_MALICIOUS.

Full suite: 118 passed (was 116; +2 new tests for the StackSlice TUPLE and DICT cases). ruff format clean. ruff check reports only one pre-existing B905 in test_pickle.py:44 (unrelated to this change).

AI Disclosure

This contribution was developed with assistance from an AI coding agent (Claude / TRAE IDE). Specifically:

• What AI did: helped diagnose the root cause (the ast.walk non-list-field limitation), wrote the opcode-level regression tests using fickling's op.* API, and ran a skeptical self-review pass.
• What was human-verified: the bug hypothesis was confirmed against a PoC reproducing the scanpy structure from False positive for --check-safety (variable is actually used afterwards) #226; every fix point was manually traced against CPython's ast.walk semantics; the regression tests were empirically validated to fail without the fix and pass with it; the scope framing (false-positive fix vs. bypass) was manually checked against SECURITY.md.
• Tool / model: TRAE IDE with Claude (Anthropic).

I have reviewed every line of the diff and the tests, and I stand behind the correctness of this change.

[CLAassistant]

All committers have signed the CLA.