Skip to content

Fix instance vs tuple subtyping edge case #18664

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

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

ilevkivskyi
Copy link
Member

Previously a code path was introduced that made fallback a subtype of its tuple type for non-generic tuples, while the intention was to cover tuple[Any, ...] and similar. I add a unit test + some refactoring to make this mistake much harder in future.

This may need to wait for #18663 to avoid "regressions" (the other fix needed to avoid "regressions" is already merged).

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Is it possible to write a test case for this? Even if the tests case was passing on master, could we have something to catch the original issue?

mypy/subtypes.py Outdated
"""Is this an instance where all args are Any types?"""
if not t.args:
return False
for arg in map(get_proper_type, t.args):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't thin mypyc can optimize this properly. Putting the get_proper_type call in the loop body would probably be significantly faster, since this could plausibly be used in perf critical code path.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, sure, I will change this now.

@ilevkivskyi
Copy link
Member Author

@JukkaL

Is it possible to write a test case for this? Even if the tests case was passing on master, could we have something to catch the original issue?

Do you mean like an "integration" test in test-data? Because I already added a unit test (that IMO should be hard to circumvent). If yes, I don't think it is possible, since normally we should never infer a plain instance instead of a tuple with fallback from a "user code".

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

It's good if the unit test can be trusted.

Copy link
Contributor

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

@ilevkivskyi ilevkivskyi merged commit 1ec3f44 into python:master Feb 13, 2025
18 checks passed
@ilevkivskyi ilevkivskyi deleted the fix-inst-tuple-subtype branch February 13, 2025 18:22
x612skm pushed a commit to x612skm/mypy-dev that referenced this pull request Feb 24, 2025
Previously a code path was introduced that made fallback a subtype of
its tuple type for non-generic tuples, while the intention was to cover
`tuple[Any, ...]` and similar. I add a unit test + some refactoring to
make this mistake much harder in future.

This may need to wait for python#18663 to
avoid "regressions" (the other fix needed to avoid "regressions" is
already merged).
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.

2 participants