Skip to content

Fix crash on invalid starred assignment target - #21851

Open
TheMuffinMan1320 wants to merge 1 commit into
python:masterfrom
TheMuffinMan1320:fix-20796-star-assign-crash
Open

Fix crash on invalid starred assignment target#21851
TheMuffinMan1320 wants to merge 1 commit into
python:masterfrom
TheMuffinMan1320:fix-20796-star-assign-crash

Conversation

@TheMuffinMan1320

Copy link
Copy Markdown

Fixes #20796

Root cause

*a = b is invalid Python (a bare starred assignment target must be
wrapped in a list or tuple, e.g. *a, = b). Semantic analysis
already detects this and reports:

error: Starred assignment target must be in a list or tuple

However, in analyze_lvalue() (mypy/semanal.py), the branch
handling a non-nested StarExpr lvalue only called self.fail(...)
and returned, without analyzing the inner name expression
(lval.expr).

If the right-hand side of the assignment referenced an
as-yet-undefined name, semantic analysis has to defer the statement
for a later pass. As part of that deferral, mark_incomplete()
proactively registers the starred target's name (a) in the symbol
table as a PlaceholderNode, expecting it to later be replaced with
a real definition once the lvalue is fully analyzed.

Because the non-nested-StarExpr branch never analyzed lval.expr,
that placeholder was never resolved into a real Var. Later, when
mypy tried to write the incremental cache, serialization walked the
module's symbol table and hit the leftover PlaceholderNode,
crashing with:

NotImplementedError: Cannot serialize PlaceholderNode instance

Fix

mypy/semanal.py: still analyze the inner lvalue (lval.expr) after
reporting the "Starred assignment target must be in a list or tuple"
error, matching what already happens for a valid nested star target.
This lets any pending placeholder for the name get resolved normally,
while the invalid-target error is still reported.

Test

Added testInvalidBareStarAssignmentTargetNoCrash in
test-data/unit/check-statements.test, using --debug-serialize (the
existing mechanism used by a few other crash-regression tests, e.g.
testNamedTupleDefaultValueDefer) to exercise serialization even
without writing an on-disk cache. Verified this test crashes with the
reported NotImplementedError when the fix is reverted, and passes
with the fix.

Verification

$ cat repro.py
*a = b
$ python -m mypy repro.py
repro.py:1: error: Starred assignment target must be in a list or tuple  [misc]
repro.py:1: error: Name "b" is not defined  [name-defined]
Found 2 errors in 1 file (checked 1 source file)

No more crash, and the two errors are reported as expected.

  • python -m pytest -q mypy/test/testcheck.py -k check-statements — 172 passed
  • python -m pytest -q mypy/test/testsemanal.py — 577 passed
  • python -m mypy --config-file mypy_self_check.ini mypy/semanal.py — success

`*a = b` (a bare starred assignment target, not wrapped in a list or
tuple) is invalid, and semantic analysis already reports "Starred
assignment target must be in a list or tuple" for it. However,
analyze_lvalue() returned without recursing into the inner name of
the StarExpr, so if that name had earlier been registered as a
PlaceholderNode (e.g. because the right-hand side referenced an
as-yet-undefined name and analysis had to be deferred), the
placeholder was never replaced with a real definition. This caused a
later crash when writing the incremental cache: `NotImplementedError:
Cannot serialize PlaceholderNode instance`.

Fix by still analyzing the inner lvalue after reporting the error, so
any placeholder gets resolved like it would for a valid nested star
target.

Fixes python#20796
@github-actions

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

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.

NotImplementedError: Cannot serialize PlaceholderNode instance

1 participant