Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLR1701 fix is not safe (especially together with SIM114) #11616

Closed
simonpercivall opened this issue May 30, 2024 · 2 comments · Fixed by #11622
Closed

PLR1701 fix is not safe (especially together with SIM114) #11616

simonpercivall opened this issue May 30, 2024 · 2 comments · Fixed by #11622
Assignees

Comments

@simonpercivall
Copy link

PLR1701 is mentioned in passing in #10245.

PLR1701 will merge repeated isinstance calls and also rewrite literal tuples into unions for modern Python versions. But an isinstance check with a variable bound tuple will also be merged into the union, and that will cause a TypeError at runtime.

This especially interacts badly with SIM114 where you could have defined two if arms, one with a variable bound tuple, and one with a literal tuple, and it'll get merged and wrongly rewritten as a union. Example below.

Ruff 0.45

pyproject.toml:

[tool.ruff]
target-version = "py311"

ruff check --select SIM114,PLR1701 --fix

TYPES = (dict, list)

if isinstance(None, TYPES):
    pass
elif isinstance(None, set | float):
    pass

actual

TYPES = (dict, list)

if isinstance(None, TYPES | set | float):
    pass

# Traceback (most recent call last):
#   File "PLR1701.py", line 3, in <module>
#     if isinstance(None, TYPES | set | float):
#                         ~~~~~~^~~~~
# TypeError: unsupported operand type(s) for |: 'tuple' and 'type'
@charliermarsh
Copy link
Member

Thanks - we should mark it as unsafe. We can improve the rule in the future when we support type inference.

@charliermarsh charliermarsh self-assigned this May 30, 2024
@charliermarsh
Copy link
Member

Actually, isinstance(None, (TYPES, set | float)) is safe... so in theory we could always rewrite to the tuple variant if we can't be certain that | will work?

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 a pull request may close this issue.

2 participants