Fix type variable clash in nested positions and in attributes #14095
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Btw, I think we can improve performance by having a version of |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Bit surprised the overhead is as high as 2%, but I agree correctness trumps perf.
I have some questions about mypy semantics (see inline comments), but I do think recursively processing types is strictly more correct then what we're doing now, so :stamp: on those grounds.
| T = TypeVar("T") | ||
| R = TypeVar("R") | ||
| class C(Generic[R]): | ||
| x: Callable[[T], R] |
There was a problem hiding this comment.
Is having this something we actually want to support? (Allowing "T" to exist as a "free" typevar)
I was always under the impression that having free typevars like this was effectively undefined behavior and not something we really want to encourage users to do.
So, an alternative approach here might be to report an error here then replace 'T' with either 'object', 'never', or 'any' for the purposes of subsequent type analysis.
There was a problem hiding this comment.
Couple things here:
- In general, things like
Callable[[T], str]are fine (e.g. for a function with unused argument), the opposite likeCallable[[str], T]are wrong/undefined (and mypy actually gives an error for such types when defining a function). - The bug also happens for other situations as well, e.g. if I type
x: Callable[[T], Tuple[T, R]]and adjust the callsite, I just use the simplest case for a test. I will update the test to make it more clear.
| @@ -1654,7 +1654,7 @@ class C: | |||
| def bar(self) -> Self: ... | |||
| foo: Callable[[S, Self], Tuple[Self, S]] | |||
|
|
|||
| reveal_type(C().foo) # N: Revealed type is "def [S] (S`-1, __main__.C) -> Tuple[__main__.C, S`-1]" | |||
| reveal_type(C().foo) # N: Revealed type is "def [S] (S`1, __main__.C) -> Tuple[__main__.C, S`1]" | |||
There was a problem hiding this comment.
Not sure if I remember the exact rules, but shouldn't S continue to use a negative ID, since it's a method-scoped typevar instead of a class-scoped one?
There was a problem hiding this comment.
Yes, but we didn't follow this convention since when we started using freshen_function_type_vars() in checkmember ~5 years ago. This is because unification variables have positive ids. But yeah, at some point we should try to clean this up.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Only perform type variable freshening if it's needed, i.e. there is a nested generic callable, since it's fairly expensive. Make the check for generic callables fast by creating a specialized type query visitor base class for queries with bool results. The visitor tries hard to avoid memory allocation in typical cases, since allocation is slow. This addresses at least some of the performance regression in #14095. This improved self-check performance by about 3% when compiled with mypyc (-O2). The new visitor class can potentially help with other type queries as well. I'll explore it in follow-up PRs.
Addresses the non-crash part of #10244 (and similar situations).
The
freshen_function_type_vars()use incheckmember.pywas inconsistent:The downsides are ~2% performance regression, and people will see more large ids in
reveal_type()(since refreshing functions uses a global unique counter). But since this is a correctness issue that can cause really bizarre error messages, I think it is totally worth it.