Skip to content

Fix false positive in UnusedVariables for dict key subscripts#248

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

Fix false positive in UnusedVariables for dict key subscripts#248
frankentini wants to merge 1 commit intotrailofbits:masterfrom
frankentini:fix-unused-variable-false-positive

Conversation

@frankentini
Copy link

Summary

Fixes #226.

Variables used as dictionary keys in subscript assignments (e.g. _var_dict[_var16] = value, generated by SETITEM opcodes) were not being recognized as "used" by unused_assignments(), causing --check-safety to incorrectly flag them as suspicious.

Root Cause

unused_assignments() iterates over statements and, for ast.Assign nodes, only records top-level ast.Name targets as definitions, then walks statement.value for used names. However, when SETITEM creates an assignment like:

_var_dict[_var16] = (_var21, 0)

The target is an ast.Subscript, not an ast.Name. The variable _var16 used as the subscript key lives inside the target and was never walked for uses -- so it appeared in defined - used despite being genuinely referenced.

Fix

When an assignment target is not a plain ast.Name (e.g. ast.Subscript, ast.Starred), walk the entire target subtree and add any ast.Name nodes found to the used set.

Test

Added regression test test_unused_variables_no_false_positive_for_dict_keys that constructs a pickle with the SETITEM dict-key pattern and verifies no false positives are reported.

…fbits#226)

Variables used as dictionary keys in subscript assignments (e.g.
_var_dict[_var16] = value, generated by SETITEM opcodes) were not
being recognized as 'used', causing them to be incorrectly flagged
as suspicious unused variables by --check-safety.

The root cause: unused_assignments() only walked ast.Assign.value for
Name references, but subscript targets like _var_dict[_var16] contain
Name nodes in the target's slice that were never visited.

Fix: when an assignment target is not a plain ast.Name (e.g. it's an
ast.Subscript), walk the entire target subtree and add any Name nodes
found to the 'used' set.

Adds regression test using a pickle with SETITEM dict-key pattern.
@frankentini frankentini requested a review from ESultanik as a code owner March 18, 2026 08:17
@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.

@thomas-chauchefoin-tob
Copy link
Collaborator

@frankentini Can you please sign the CLA before we review both of your PRs? Thanks!

@thomas-chauchefoin-tob thomas-chauchefoin-tob added the missing cla Contributor has to sign the CLA before review of the PR label Mar 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

missing cla Contributor has to sign the CLA before review of the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

3 participants