-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix #16935 fix type of tuple[X,Y] expression #17235
Conversation
... so e.g. reveal_type(tuple[int, int]) gives expected def (p0: builtins.int, p1: builtins.int) -> tuple[builtins.int, builtins.int] ... rather than def [_T_co] (typing.Iterable[_T_co`1] =) -> builtins.tuple[_T_co`1, ...]
This comment has been minimized.
This comment has been minimized.
test-data/unit/fixtures/tuple.pyi
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
import _typeshed | |||
from typing import Iterable, Iterator, TypeVar, Generic, Sequence, Optional, overload, Tuple, Type | |||
from abc import ABCMeta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this from an earlier iteration of the PR?
It seems to be unused in the current code, and we probably don't want to add abc
to all the tuple tests if we can help it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the first iteration of the PR...
I'd happily remove it, just not sure how (and I'm no expert, so don't take that as "can't"!):
checkexpr.py line 4849 (that I added) needs it... without adding the tuple.pyi import I get unit test failures like:
data: /home/xju/mypy/test-data/unit/check-type-aliases.test:1016:
...
File "/home/xju/mypy/mypy/checkexpr.py", line 4849, in apply_type_arguments_to_callable
self.chk.named_type("abc.ABCMeta"),
...
KeyError: 'abc'
... but maybe my checkexpr.py line 4849 is wrong or could be expressed differently? I chose that by looking at the result of CheckerPluginInterface.get_expression_type() for the expression "list[int]":
(Pdb) p expr_type
Overload(def () -> builtins.list[builtins.int], def (typing.Iterable[builtins.int]) -> builtins.list[builtins.int])
(Pdb) p expr_type.items[0]
def () -> builtins.list[builtins.int]
(Pdb) p expr_type.items[0].fallback
abc.ABCMeta
(Pdb) p type(expr_type.items[0].fallback)
<class 'mypy.types.Instance'>
... and somewhere I noticed that chk.named_type("abc.ABCMeta") appeared to produce exactly that:
(Pdb) self.chk.named_type("abc.ABCMeta")
abc.ABCMeta
(Pdb) type(self.chk.named_type("abc.ABCMeta"))
<class 'mypy.types.Instance'>
... thoughts/suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it's likely that I was mistaken here and it's actually needed, I'm also new to MyPy.
I suppose the import resolver determines which modules MyPy will parse, and that's why you hit an error without this line.
I didn't entirely understand why we need ABCMeta
as a fallback either, but this is also related to me being new to MyPy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm, I don't think ABCMeta is correct:
- in the list[int] case we want a function that returns a list[int], i.e. like def g() -> list[int]
- that is a specific instance of a function, so its fallback should be "function"
... I wonder what the fallback is for a plain function? Let's try:
def ff(): pass
... for that pdb mypy shows that CheckerPluginInterface.get_expression_type() returns:
(Pdb) p expr_type
def () -> Any
(Pdb) p type(expr_type)
<class 'mypy.types.CallableType'>
(Pdb) p expr_type.fallback
builtins.function
... that makes more sense, yes?
I will try that out... might be a day or two...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... ok after a bit more digging I believe ABCMeta is correct:
- ff's Callable above has fallback builtins.function, and that is correct for ff, which is a just a function
- but ff is not a mypy "type object" Callable, and the mypy type of expression "tuple" must be a type object Callable; mypy uses Callable's fallback to distinguish (see mypy.types.Callable.is_type_obj() implementation)
- "int" gets fallback "builtins.type", I believe because its definition in builtins.pyi has no explicit base class
- "list" gets fallback "abc.ABCMeta", I believe because its definition in builtins.pyi has an abstract base class
- ... so "tuple" should get fallback "abc.ABCMeta" too because its definition in builtins.pyi has an abstract base class
... but I then realized that I can get the fallback from "tp" rather than looking it up, and doing that avoids needing to add abc.ABCMeta to test-data/unit/fixtures/tuple.pyi... so that is what I've done in commit [5bd1c4c]; I also removed the "p1" as param name, using None instead because that seems to be the convention for type object Callables
…back i.e. the mypy type of the expression union[str, int] is a CallableType with fallback builtins.type this matches the mypy type of the expression int which is a CallableType with fallback builtins.type (suggested by PR review discussion)
This comment has been minimized.
This comment has been minimized.
…back ... the original fallback ABCMeta is "correct" in that it aligns with the (abstract) base class of builtins.tuple. But rather than explicitly specify abc.ABCMeta as fallback, use the same fallback as the type we're applying arguments to (and that is abc.ABCMeta in our "tuple" case)
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After getting rid of the explicit ABCMeta, this looks right to me, tests look good and the code change seems reasonable.
I'm not a maintainer, but approving because it may mean someone else can do a quick review sooner.
implement the mypy/checkexpr.py TODO: Specialize the callable for the type arguments
... so e.g. reveal_type(tuple[int, int]) gives expected
def (p0: tuple[builtins.int, builtins.int]) -> tuple[builtins.int, builtins.int]
... rather than
def [_T_co] (typing.Iterable[_T_co
1] =) -> builtins.tuple[_T_co
1, ...]Fixes #16935