Skip to content

Fix type narrowing for variable swaps after isinstance checks#21624

Open
BHUVANSH855 wants to merge 2 commits into
python:masterfrom
BHUVANSH855:fix-21586-variable-swap-narrowing
Open

Fix type narrowing for variable swaps after isinstance checks#21624
BHUVANSH855 wants to merge 2 commits into
python:masterfrom
BHUVANSH855:fix-21586-variable-swap-narrowing

Conversation

@BHUVANSH855

Copy link
Copy Markdown

Fixes #21586

Summary

This PR fixes incorrect type narrowing when variables are swapped after an isinstance() check.

Previously, in code such as:

class Base: ...
class Sub1(Base): ...
class Sub2(Base): ...

def f(a: Base, b: Base) -> None:
    if isinstance(b, Sub1) and isinstance(a, Sub2):
        a, b = b, a

        reveal_type(a)  # Sub1
        reveal_type(b)  # Sub1

mypy incorrectly revealed b as Sub1 instead of Sub2.

The issue occurred because narrowed binder information for RHS variables could be overwritten while processing a multi-assignment statement. This change captures narrowed RHS NameExpr types before assignments are applied, preserving simultaneous assignment semantics.

Changes

  • Capture narrowed RHS variable types before processing multi-assignment pairs.

  • Use the captured types when checking assignments to avoid interference from earlier assignments in the same statement.

  • Add regression tests covering:

    • Direct variable swaps after isinstance() narrowing.
    • Tuple-based swaps that preserve narrowed types.

Testing

Verified with:

  • python -m pytest mypy/test/testcheck.py -k testIsInstanceVariableSwapNarrowing -n0
  • python -m pytest mypy/test/testcheck.py -k testTupleSwapAfterIsinstance -n0
  • python -m pytest mypy/test/testcheck.py -k narrowing -n0
  • python -m pytest mypy/test/testcheck.py -k assignment -n0
  • python -m pytest mypy/test/testcheck.py -k tuple -n0
  • python -m pytest mypy/test/testcheck.py -k isinstance -n0
  • python -m pytest mypy/test/testcheck.py -n0
  • python -m pytest mypy/test/testcheck.py -n auto

All tests pass.

@github-actions

Copy link
Copy Markdown
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

discord.py (https://github.com/Rapptz/discord.py)
- discord/app_commands/commands.py:1583: error: Incompatible types in assignment (expression has type "str", variable has type "locale_str | None")  [assignment]
- discord/app_commands/commands.py:1595: error: Incompatible types in assignment (expression has type "str", variable has type "locale_str | None")  [assignment]

@BHUVANSH855

Copy link
Copy Markdown
Author

I investigated the mypy-primer report from discord.py.

The reported locations are tuple assignments of the form:

name, locale = validate_name(name.message), name
description, locale = description.message, description

which are directly affected by this change.

I tried reproducing the pattern locally with a minimal example and mypy reports no issues:

python -m mypy primer_test.py
Success: no issues found in 1 source file

All existing mypy tests also pass, including the full testcheck suite.

I'd appreciate maintainer feedback on whether the primer diffs represent genuine newly-detected issues or indicate a problem with the implementation.

@adhavan18 adhavan18 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean fix for a real simultaneous-assignment narrowing bug. a few thoughts:

the approach is correct — capturing narrowed RHS types before processing any pair is exactly the right way to implement true simultaneous-assignment semantics. the binder mutation during earlier pairs was the root cause and this sidesteps it neatly.

one edge case worth checking: self.binder.get(rv) returns None when the name has no narrowed type in the current binder frame. the fallthrough to captured_pairs.append((lv, rv)) handles that correctly. but if narrowed is a DeletedType (e.g., the variable was conditionally deleted), temp_node will produce a node with DeletedType — does check_assignment handle that gracefully, or should there be a not isinstance(narrowed, DeletedType) guard before capturing?

test coverage looks solid — the direct swap case (a, b = b, a) and the tuple intermediary case cover the two main patterns. might be worth adding a case where one variable is NOT narrowed to confirm only the narrowed side is captured (the current fallthrough path).

overall the change is minimal and well-targeted. the captured_pairs two-pass pattern is easy to follow.

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.

Incorrect type narrowing when swapping variables after isinstance check

2 participants