[ty] Infer better specializations of unions with None (etc)
#20749
+74
−19
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
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
possibly-missing-attribute |
0 | 182 | 0 |
invalid-argument-type |
7 | 67 | 8 |
invalid-return-type |
0 | 17 | 1 |
non-subscriptable |
0 | 15 | 0 |
unsupported-operator |
0 | 13 | 0 |
no-matching-overload |
0 | 10 | 0 |
call-non-callable |
0 | 5 | 0 |
invalid-assignment |
0 | 4 | 0 |
not-iterable |
0 | 2 | 0 |
unused-ignore-comment |
1 | 0 | 0 |
| Total | 8 | 315 | 9 |
|
Lots of false positives removed in the ecosystem report. The only additions involve splatting a |
AlexWaygood
approved these changes
Oct 7, 2025
Comment on lines
1173
to
1182
| let actual_non_subtypes = (actual_union.elements(self.db).iter().copied()) | ||
| .filter(|ty| !ty.is_subtype_of(self.db, formal)) | ||
| .fold(UnionBuilder::new(self.db), |builder, element| { | ||
| builder.add(element) | ||
| }); | ||
| let bound_typevar = bound_typevars.next(); | ||
| let additional_bound_typevars = bound_typevars.next(); | ||
| if let (Some(bound_typevar), None) = (bound_typevar, additional_bound_typevars) { | ||
| let Some(remaining_actual) = actual_non_subtypes.try_build() else { | ||
| return Ok(()); | ||
| }; | ||
| self.add_type_mapping(formal_bound_typevar, remaining_actual); | ||
| } |
There was a problem hiding this comment.
slightly simpler might be to do this? it would also mean you wouldn't need the UnionBuilder import up at the top
diff --git a/crates/ty_python_semantic/src/types/generics.rs b/crates/ty_python_semantic/src/types/generics.rs
index 14388f0518..e53bd41333 100644
--- a/crates/ty_python_semantic/src/types/generics.rs
+++ b/crates/ty_python_semantic/src/types/generics.rs
@@ -1170,14 +1170,11 @@ impl<'db> SpecializationBuilder<'db> {
if (actual_union.elements(self.db).iter()).any(|ty| ty.is_type_var()) {
return Ok(());
}
- let actual_non_subtypes = (actual_union.elements(self.db).iter().copied())
- .filter(|ty| !ty.is_subtype_of(self.db, formal))
- .fold(UnionBuilder::new(self.db), |builder, element| {
- builder.add(element)
- });
- let Some(remaining_actual) = actual_non_subtypes.try_build() else {
+ let remaining_actual =
+ actual_union.filter(self.db, |ty| !ty.is_subtype_of(self.db, formal));
+ if remaining_actual.is_never() {
return Ok(());
- };
+ }
self.add_type_mapping(formal_bound_typevar, remaining_actual);
}There was a problem hiding this comment.
Yeah I had thought about that, but was feeling mildly icky about using Never in that way. I guess for unions it's okay, since that really is equivalent to "no elements in the union".
There was a problem hiding this comment.
I don't have a strong opinion FWIW, but your current spelling did make me do a "Huh, don't we have a UnionType::filter() method yet... oh! We do" 😄
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.
This PR adds a specialization inference special case that lets us handle the following examples better:
We already have a special case for when the formal is a union where one element is a typevar, but it maps the entire actual type to the typevar (as you can see in the "previously" results above).
The new special case kicks in when the actual is also a union. Now, we filter out any actual union elements that are already subtypes of the formal, and only bind whatever types remain to the typevar. (The
| Nonepattern appears quite often in the ecosystem results, but it's more general and works with any number of non-typevar union elements.)The new constraint solver should handle this case as well, but it's worth adding this heuristic now with the old solver because it eliminates some false positives from the ecosystem report, and makes the ecosystem report less noisy on the other constraint solver PRs.