Skip to content

fix: walk result expression value to prevent false positive in UnusedVariables#247

Open
frankentini wants to merge 1 commit intotrailofbits:masterfrom
frankentini:fix/unused-variables-false-positive
Open

fix: walk result expression value to prevent false positive in UnusedVariables#247
frankentini wants to merge 1 commit intotrailofbits:masterfrom
frankentini:fix/unused-variables-false-positive

Conversation

@frankentini
Copy link

Summary

Fixes #226.

unused_assignments() breaks out of its loop upon encountering the result = ... assignment. However, it did so before walking the assignment's value for variable references. This caused any variable only referenced within the final result expression (e.g. via BINGET/memo-based sharing, or as part of a complex result tuple/dict) to be incorrectly flagged as unused.

Root Cause

In Interpreter.unused_assignments() (fickle.py), the loop processed statements sequentially:

if statement.targets[0].id == "result":
    break  # <-- exits WITHOUT walking statement.value

Variables referenced exclusively in the result expression were never added to the used set, so they appeared in defined - used and were falsely reported as suspicious.

Fix

Before breaking, walk the result assignment's value to record all variable references:

if statement.targets[0].id == "result":
    for node in ast.walk(statement.value):
        if isinstance(node, ast.Name):
            used.add(node.id)
    break

Test

Added test_unused_variables_no_false_positive_for_result_refs which crafts a minimal pickle where a REDUCE-created variable is referenced only in the result tuple via BINGET. Without the fix, this variable is falsely flagged; with the fix, it correctly reports zero unused variables.

This directly reproduces the pattern seen in the scanpy pickle from #226, where variables created by numpy.core.multiarray.scalar REDUCE calls end up referenced only within result-level structures.

…Variables

The unused_assignments() method broke out of its loop when encountering
the 'result = ...' assignment without first walking its value for
variable references. This caused variables that were only referenced
in the final result expression (e.g. via BINGET/memo sharing) to be
incorrectly flagged as unused.

Now the result assignment's value is walked before the break, properly
recording any variables it references as used.

Fixes trailofbits#226
@frankentini frankentini requested a review from ESultanik as a code owner March 16, 2026 08:21
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False positive for --check-safety (variable is actually used afterwards)

2 participants