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

Prevent crash on generic NamedTuple with unresolved typevar bound #18585

Merged

Conversation

sterliakov
Copy link
Collaborator

Fixes #18582. Fixes #17396. Supersedes #18351.

cc @hauntsaninja - see my comments on your original PR, maybe this change should be lifted to BoolTypeQuery and TypeQuery instead? (opening a separate PR to see CI results)

This comment has been minimized.

@sterliakov sterliakov marked this pull request as ready for review February 2, 2025 04:06
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look, this makes sense. Let's merge this and try and get it into the release, since it's been reported multiple times.

I'm not sure about TypeQuery / BoolTypeQuery, probably makes sense to try? Would also be good to try and get a regression test working.

@hauntsaninja hauntsaninja mentioned this pull request Feb 2, 2025
5 tasks
@ilevkivskyi
Copy link
Member

Nice catch! Yes, I think fixing this in TypeQuery/BoolTypeQuery is a more principled thing to do, it is also a bit more risky, but I think we should try it.

For more context, partial_fallback is an important part of the tuple types, not just some internal detail, this is essentially how we support named tuples, by constructing effectively an intersection of proper tuple type and an instance type. I guess it was an unintentional omission in the initial implementation of query visitors, as many other visitors actually do properly handle it.

This comment has been minimized.

@sterliakov
Copy link
Collaborator Author

Both primer hits appear to reveal a different problem, reproducible as follows:

[case testTemp]
foo = [
    (1, ("a", "b")),
    (2, []),
]
[builtins fixtures/tuple.pyi]

The inferred type is:

builtins.list[tuple[builtins.int, typing.Sequence[builtins.str]]]

nothing suspicious, but...

(Pdb)  init_type.args[0].partial_fallback
builtins.tuple[Union[builtins.int, tuple[builtins.str, builtins.str], builtins.list[Never]], ...]

So when inferring list expr type as a constructor call, we synthetize a fully defined tuple with a partially unknown fallback (union of all initial values).

I think it's fine to keep for now and investigate separately?

And I'm still unable to write a testcase for the original problem, any suggestions are welcome!

@sterliakov
Copy link
Collaborator Author

Alternatively, we can exclude fallback in InvalidInferredTypes visitor if that join behavior is correct and we just don't want to ask for explicit type for it. (or do we really want that and new behaviour is best?)

@ilevkivskyi
Copy link
Member

Alternatively, we can exclude fallback in InvalidInferredTypes visitor if that join behavior is correct and we just don't want to ask for explicit type for it.

This looks like a good short-term solution. Long term we may actually want to completely erase the fallback argument in TupleType constructor if fallback is plain tuple (everyone should use typeops.tuple_fallback() anyway). I think however it may be better done in a separate PR.

Could you please skip fallback for now, and add a TODO there? (And maybe make a follow up PR if you have time)

This comment has been minimized.

This comment has been minimized.

@ilevkivskyi
Copy link
Member

OK, so here is the repro for the crash (should be added to any of the incremental test files). The caveat is that repro requires #18592, as I didn't find any existing test fixture with a Self type in tuple MRO (as in real typeshed).

[case testSerializeDeferredGenericNamedTuple]
import pkg
[file pkg/__init__.py]
from .lib import NT
[file pkg/lib.py]
from typing import Generic, NamedTuple, TypeVar
from pkg import does_not_exist  # type: ignore
from pkg.missing import also_missing  # type: ignore

T = TypeVar("T", bound=does_not_exist)
class NT(NamedTuple, Generic[T]):
    values: also_missing[T]
[file pkg/__init__.py.2]
# touch
from .lib import NT
[builtins fixtures/tuple.pyi]
[out]
[out2]

JukkaL pushed a commit that referenced this pull request Feb 3, 2025
This makes it more similar to the real typeshed. It is needed to
reproduce tricky failures in tests, e.g.
#18585. If this causes slower tests,
some tests may be switched to `tuple-simple.pyi`.
@wesleywright
Copy link
Collaborator

Since this is a prominent crashing bug, I'll try to cherry pick it for the 1.15 release if it lands in the next couple of days. If it's not ready by that point, I can prioritize cutting a quick 1.15.1 release to pick it up instead.

@sterliakov
Copy link
Collaborator Author

Thank you @ilevkivskyi! I missed the Self-type relation. I verified that this testcase fails on current master and passes here.

Copy link
Contributor

github-actions bot commented Feb 3, 2025

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

@ilevkivskyi ilevkivskyi merged commit 237933a into python:master Feb 3, 2025
18 checks passed
wesleywright pushed a commit that referenced this pull request Feb 4, 2025
This makes it more similar to the real typeshed. It is needed to
reproduce tricky failures in tests, e.g.
#18585. If this causes slower tests,
some tests may be switched to `tuple-simple.pyi`.
wesleywright pushed a commit that referenced this pull request Feb 4, 2025
…8585)

Fixes #18582. Fixes #17396. Supersedes #18351.

---------

Co-authored-by: hauntsaninja <hauntsaninja@gmail.com>
wesleywright pushed a commit that referenced this pull request Feb 4, 2025
…8585)

Fixes #18582. Fixes #17396. Supersedes #18351.

---------

Co-authored-by: hauntsaninja <hauntsaninja@gmail.com>
wesleywright pushed a commit that referenced this pull request Feb 4, 2025
This makes it more similar to the real typeshed. It is needed to
reproduce tricky failures in tests, e.g.
#18585. If this causes slower tests,
some tests may be switched to `tuple-simple.pyi`.
ilevkivskyi added a commit that referenced this pull request Feb 10, 2025
Removes todo added in #18585 - such normalization is not technically
correct when a tuple is used as a base class.

---------

Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants