[ty] Respect intersections in iterations#21965
Merged
charliermarsh merged 6 commits intomainfrom Dec 19, 2025
Merged
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
charliermarsh
changed the title
Member
Author
|
The ecosystem changes in Materialize seems incorrect, hmm... |
charliermarsh
force-pushed
the
charlie/int
branch
from
1e2ac20 to
7bdce29
Compare
Contributor
|
Those diagnostics on materialize are correct, though arguably pedantic. They are us recognizing that it is possible for there to be a common subtype of The fix for this code would be to check We could debate whether ty's pedantic correctness is useful in this kind of scenario (astral-sh/ty#1578 tracks this), but in any case, it's not a problem for this PR. |
carljm
reviewed
Dec 13, 2025
| let first_element_spec = elements_iter.next()?.try_iterate_with_mode(db, EvaluationMode::Sync).ok()?; | ||
| let mut builder = TupleSpecBuilder::from(&*first_element_spec); | ||
| for element in elements_iter { | ||
| builder = builder.intersect(db, &*element.try_iterate_with_mode(db, EvaluationMode::Sync).ok()?); |
Contributor
There was a problem hiding this comment.
I don't think short-circuiting here (or above, for the first element) is correct. The error-handling logic here needs to be "collect all successful results, ignoring failed ones, and intersect the successful ones". We should only fail iteration if all intersection elements fail to iterate.
We should add some tests where some intersection elements fail to iterate and others succeed. And some where all intersection elements fail.
charliermarsh
requested review from
AlexWaygood,
dcreager and
sharkdp
as code owners
AlexWaygood
reviewed
Dec 13, 2025
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-argument-type |
7 | 13 | 6 |
possibly-missing-attribute |
3 | 1 | 12 |
invalid-assignment |
1 | 0 | 6 |
invalid-return-type |
0 | 2 | 4 |
unsupported-operator |
0 | 6 | 0 |
unresolved-attribute |
0 | 2 | 1 |
redundant-cast |
0 | 1 | 0 |
unused-ignore-comment |
0 | 1 | 0 |
| Total | 11 | 26 | 29 |
charliermarsh
force-pushed
the
charlie/int
branch
from
d1ceb75 to
f0b54ab
Compare
charliermarsh
force-pushed
the
charlie/int
branch
from
f0b54ab to
1672c79
Compare
AlexWaygood
approved these changes
Dec 19, 2025
| /// | ||
| /// For other cases (e.g., different fixed lengths, or complex variable-length tuples with | ||
| /// prefixes or suffixes), we fall back to a homogeneous variable-length tuple. | ||
| pub(crate) fn intersect(mut self, db: &'db dyn Db, other: &TupleSpec<'db>) -> Self { |
Member
There was a problem hiding this comment.
My concern here is that if you have two tuples X and Y, the tuple spec of X & Y should always be narrower than either the tuple spec for X or the tuple spec for Y. The last branch of this method violates that, I think: you could end up with a spec from the intersected tuples that's broader than one of the two tuples being thrown into the intersection, which is going to lead to some seriously weird behaviour at some point 😄
Here's a version of this method that's still not totally free of TODOs, but I think is much more generalised than what you have currently:
/// Return a new tuple-spec builder that reflects the intersection of this tuple and another tuple.
///
/// For example, if `self` is a tuple-spec builder for `tuple[int, str]` and `other` is a
/// tuple-spec for `tuple[object, object]`, the result will be a tuple-spec builder for
/// `tuple[int, str]` (since `int & object` simplifies to `int`, and `str & object` to `str`).
pub(crate) fn intersect(mut self, db: &'db dyn Db, other: &TupleSpec<'db>) -> Self {
match (&mut self, other) {
// Both fixed-length with the same length: element-wise intersection.
(TupleSpecBuilder::Fixed(our_elements), TupleSpec::Fixed(new_elements))
if our_elements.len() == new_elements.len() =>
{
for (existing, new) in our_elements.iter_mut().zip(new_elements.elements()) {
*existing = IntersectionType::from_elements(db, [*existing, *new]);
}
return self;
}
(TupleSpecBuilder::Fixed(our_elements), TupleSpec::Variable(var)) => {
if let Ok(tuple) = var.resize(db, TupleLength::Fixed(our_elements.len())) {
return self.intersect(db, &tuple);
}
}
(TupleSpecBuilder::Variable { .. }, TupleSpec::Fixed(fixed)) => {
if let Ok(tuple) = self
.clone()
.build()
.resize(db, TupleLength::Fixed(fixed.len()))
{
return TupleSpecBuilder::from(&tuple).intersect(db, other);
}
}
(
TupleSpecBuilder::Variable {
prefix,
variable,
suffix,
},
TupleSpec::Variable(var),
) => {
if prefix.len() == var.prefix.len() && suffix.len() == var.suffix.len() {
for (existing, new) in prefix.iter_mut().zip(var.prefix_elements()) {
*existing = IntersectionType::from_elements(db, [*existing, *new]);
}
*variable = IntersectionType::from_elements(db, [*variable, var.variable]);
for (existing, new) in suffix.iter_mut().zip(var.suffix_elements()) {
*existing = IntersectionType::from_elements(db, [*existing, *new]);
}
return self;
}
let self_built = self.clone().build();
let self_len = self_built.len();
if let Ok(resized) = var.resize(db, self_len) {
return self.intersect(db, &resized);
} else if let Ok(resized) = self_built.resize(db, var.len()) {
return TupleSpecBuilder::from(&resized).intersect(db, other);
}
}
_ => {}
}
// TODO: probably incorrect? `tuple[int, str] & tuple[int, str, bytes]` should resolve to `Never`.
// So maybe this function should be fallible (return an `Option`)?
let intersected =
IntersectionType::from_elements(db, self.all_elements().chain(other.all_elements()));
TupleSpecBuilder::Variable {
prefix: vec![],
variable: intersected,
suffix: vec![],
}
}
charliermarsh
force-pushed
the
charlie/int
branch
from
1672c79 to
8d6b3c8
Compare
Member
Author
|
Tyvm! |
charliermarsh
added a commit
that referenced
this pull request
Jan 6, 2026
## Summary This PR attempts to address a TODO in #21965 (comment).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
This PR implements the strategy described in astral-sh/ty#1871: we iterate over the positive types, resolve them, then intersect the results.