[ty] Avoid unnecessarily widening generic specializations #20875
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ibraheemdev
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Contributor
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-10-16 19:14:50.885037218 +0000
+++ new-output.txt 2025-10-16 19:14:54.231077442 +0000
@@ -266,8 +266,8 @@
dataclasses_transform_class.py:119:18: error[unknown-argument] Argument `id` does not match any known parameter of bound method `__init__`
dataclasses_transform_class.py:119:24: error[unknown-argument] Argument `name` does not match any known parameter of bound method `__init__`
dataclasses_transform_converter.py:25:6: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `T@model_field`
-dataclasses_transform_converter.py:48:31: error[invalid-argument-type] Argument to function `model_field` is incorrect: Expected `(Unknown, /) -> int`, found `def bad_converter1() -> int`
-dataclasses_transform_converter.py:49:31: error[invalid-argument-type] Argument to function `model_field` is incorrect: Expected `(Unknown, /) -> int`, found `def bad_converter2(*, x: int) -> int`
+dataclasses_transform_converter.py:48:31: error[invalid-argument-type] Argument to function `model_field` is incorrect: Expected `(Unknown, /) -> Unknown`, found `def bad_converter1() -> int`
+dataclasses_transform_converter.py:49:31: error[invalid-argument-type] Argument to function `model_field` is incorrect: Expected `(Unknown, /) -> Unknown`, found `def bad_converter2(*, x: int) -> int`
dataclasses_transform_converter.py:107:5: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 6
dataclasses_transform_converter.py:108:5: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 6
dataclasses_transform_converter.py:109:5: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 6
@@ -277,7 +277,7 @@
dataclasses_transform_converter.py:116:1: error[invalid-assignment] Object of type `Literal[b"f6"]` is not assignable to attribute `field3` of type `ConverterClass`
dataclasses_transform_converter.py:119:1: error[invalid-assignment] Object of type `Literal[1]` is not assignable to attribute `field3` of type `ConverterClass`
dataclasses_transform_converter.py:121:11: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 7
-dataclasses_transform_converter.py:130:31: error[invalid-argument-type] Argument to function `model_field` is incorrect: Expected `(Literal[1], /) -> int`, found `def converter_simple(s: str) -> int`
+dataclasses_transform_converter.py:130:31: error[invalid-argument-type] Argument to function `model_field` is incorrect: Expected `(Literal[1], /) -> Unknown`, found `def converter_simple(s: str) -> int`
dataclasses_transform_field.py:49:43: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `(...) -> @Todo(unsupported type[X] special form)`
dataclasses_transform_field.py:64:16: error[unknown-argument] Argument `id` does not match any known parameter
dataclasses_transform_field.py:75:1: error[missing-argument] No argument provided for required parameter `name`
@@ -301,7 +301,6 @@
dataclasses_usage.py:51:28: error[invalid-argument-type] Argument is incorrect: Expected `int | float`, found `Literal["price"]`
dataclasses_usage.py:52:36: error[too-many-positional-arguments] Too many positional arguments: expected 3, got 4
dataclasses_usage.py:83:13: error[too-many-positional-arguments] Too many positional arguments: expected 1, got 2
-dataclasses_usage.py:88:14: error[no-matching-overload] No overload of function `field` matches arguments
dataclasses_usage.py:127:8: error[too-many-positional-arguments] Too many positional arguments: expected 1, got 2
dataclasses_usage.py:130:1: error[missing-argument] No argument provided for required parameter `y` of bound method `__init__`
dataclasses_usage.py:179:6: error[too-many-positional-arguments] Too many positional arguments to bound method `__init__`: expected 1, got 2
@@ -901,5 +900,5 @@
typeddicts_usage.py:28:17: error[missing-typed-dict-key] Missing required key 'name' in TypedDict `Movie` constructor
typeddicts_usage.py:28:18: error[invalid-key] Invalid key access on TypedDict `Movie`: Unknown key "title"
typeddicts_usage.py:40:24: error[invalid-type-form] The special form `typing.TypedDict` is not allowed in type expressions. Did you mean to use a concrete TypedDict or `collections.abc.Mapping[str, object]` instead?
-Found 903 diagnostics
+Found 902 diagnostics
WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details. |
Contributor
|
ibraheemdev
commented
Oct 15, 2025
|
|
||
| d: list[int | tuple[int, int]] = f((1, 2)) | ||
| reveal_type(d) # revealed: list[int | tuple[int, int]] | ||
| reveal_type(d) # revealed: list[tuple[int, int] | int] |
Member
Author
There was a problem hiding this comment.
The churn here could be avoided if we performed a third specialization, inferring the call expression annotation first before the argument types, but I'm not sure it's worth it.
Member
There was a problem hiding this comment.
There is a TODO below indicating where we'd make the change if we wanted to fix this, so I'm 👍 to this. Though maybe add a TODO comment here, too, calling out that we realize that we reordered the union, and that we know how we'd fix it?
Contributor
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
7 | 7 | 0 |
no-matching-overload |
0 | 14 | 0 |
invalid-return-type |
0 | 4 | 7 |
redundant-cast |
2 | 3 | 0 |
invalid-key |
3 | 0 | 0 |
unused-ignore-comment |
0 | 3 | 0 |
unresolved-attribute |
0 | 2 | 0 |
index-out-of-bounds |
1 | 0 | 0 |
| Total | 13 | 33 | 7 |
Member
Author
|
The ecosystem results look good. There are some regressions around |
dcreager
reviewed
Oct 15, 2025
Comment on lines
2560
to
2563
| match call_expression_tcx { | ||
| // A type variable is not a useful type-context for expression inference, and applying it | ||
| // to the return type can lead to confusing unions in nested generic calls. | ||
| Type::TypeVar(_) => {} |
Member
There was a problem hiding this comment.
You can use Type::is_type_var here. Or alternatively, do we want to update this to skip the type context if it contains a typevar anywhere inside of it? In that case you'd use Type::has_typevar
Member
Author
There was a problem hiding this comment.
I'll use is_type_var for now. We still want to use the type context as much as possible, we just want to avoid ever adding a type-mapping to a typevar, e.g. dict[TypedDict(..), T] is still useful. I'll address this in a follow up PR.
Member
Author
There was a problem hiding this comment.
Actually, I'm not even sure we need this check anymore, given we'll prefer the first inference over the one unioned with a typevar. I suppose it might make for better diagnostics for invalid assignments?
| .generic_context | ||
| .map(|gc| builder.build(gc, *self.call_expression_tcx)); | ||
| // Build the specialization once without type context. | ||
| let isolated_specialization = builder.build(gc, *self.call_expression_tcx); |
Member
There was a problem hiding this comment.
Should this use TypeContext::default? The comment says "without the type context"
Member
Author
There was a problem hiding this comment.
I updated the comment, this is specifically talking about inferring the return type using the type context. We still need the type context to avoid eager literal promotion (regardless of whether or not we choose the inference with or without the type context).
Comment on lines
-10864
to
+11203
| filter_fn: impl FnMut(&&Type<'db>) -> bool, | ||
| mut f: impl FnMut(&Type<'db>) -> bool, | ||
| ) -> Type<'db> { | ||
| Self::from_elements(db, self.elements(db).iter().filter(filter_fn)) | ||
| Self::from_elements(db, self.elements(db).iter().filter(|ty| f(ty))) |
Member
There was a problem hiding this comment.
Can we make the callback take in Type<'db> instead of &Type<'db>? I find that much more ergonomic, and I think it just requires a .copied() before the filter. (Ditto above in Type::filter)
Member
There was a problem hiding this comment.
Ibraheem's current signature matches the signature for UnionType::map(). I don't have a strong objection to changing that signature, but I think it's nice to keep the signature of map() consistent with the signature of filter(). (And IIRC, I made the callback passed to UnionType::map() take &Type<'db> because it seemed generally to make most callsites more ergonomic than if it took Type<'db>. Due to the fact that some_union.elements(db).iter() calls return Iterator<Item = &Type<'db>>s rather than Iterator<Item = Type<'db>>s)
Member
There was a problem hiding this comment.
👍 I agree with keeping them consistent, but feel strongly enough about avoiding &Type when we can that I might tackle that as a separate follow on. You know, in my copious spare time.
Member
Author
There was a problem hiding this comment.
There are some Type methods that take &self, so we should probably address them all at once. Only changing filter would mean .filter(Type::is_typed_dict) no longer works, for example.
Member
There was a problem hiding this comment.
I think the last time we discussed it, we decided that it was pretty borderline but that it was probably slightly more efficient for methods like that to take &self rather than self, despite the fact that Type is Copy, given how big Type is
ibraheemdev
force-pushed
the
ibraheem/generic-call-inference
branch
from
981fd9c to
5bd7788
Compare
AlexWaygood
reviewed
Oct 15, 2025
ibraheemdev
force-pushed
the
ibraheem/generic-call-inference
branch
3 times, most recently
from
af6dad0 to
5c817d8
Compare
Member
Author
|
I updated the heuristic to ignore the type context if the specialization is already assignable to the type context. This seems more consistent than the previous subtyping check, and also means we don't have to perform a second specialization if the first one is valid. This is somewhat related to astral-sh/ty#136, but I think that is a decision we have to make separately and more related to preferring the declared type itself, not the inference that uses the declared type. For example, in #20796, we want re-assignments to be narrowed while still accounting for the type context. This seems to remove all the false positives that showed up in the ecosystem report as #20796, and removes a number of existing false positives as well. The regression with |
dcreager
approved these changes
Oct 16, 2025
|
|
||
| d: list[int | tuple[int, int]] = f((1, 2)) | ||
| reveal_type(d) # revealed: list[int | tuple[int, int]] | ||
| reveal_type(d) # revealed: list[tuple[int, int] | int] |
Member
There was a problem hiding this comment.
There is a TODO below indicating where we'd make the change if we wanted to fix this, so I'm 👍 to this. Though maybe add a TODO comment here, too, calling out that we realize that we reordered the union, and that we know how we'd fix it?
ibraheemdev
force-pushed
the
ibraheem/generic-call-inference
branch
from
e9e278f to
d806594
Compare
dcreager
added a commit
that referenced
this pull request
Oct 16, 2025
…rable * origin/main: [ty] Avoid unnecessarily widening generic specializations (#20875)
dcreager
added a commit
that referenced
this pull request
Oct 17, 2025
* main: [ty] Prefer declared type for invariant collection literals (#20927) [ty] Don't track inferability via different `Type` variants (#20677) [ty] Use declared variable types as bidirectional type context (#20796) [ty] Avoid unnecessarily widening generic specializations (#20875) [ty] Support dataclass-transform `field_specifiers` (#20888) Bump 0.14.1 (#20925) Standardize syntax error construction (#20903) [`pydoclint`] Implement `docstring-extraneous-parameter` (`DOC102`) (#20376) [ty] Fix panic 'missing root' when handling completion request (#20917) [ty] Run file watching tests serial when using nextest (#20918) [ty] Add version hint for failed stdlib attribute accesses (#20909) More CI improvements (#20920) [ty] Check typeshed VERSIONS for parent modules when reporting failed stdlib imports (#20908)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Ignore the type context when specializing a generic call if it leads to an unnecessarily wide return type. For example, the example mentioned here works as expected after this change:
I also added extended our usage of
filter_disjoint_elementsto tuple and typed-dict inference, which resolves astral-sh/ty#1266.