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

3 participants