From 089f5152f64e98c8e7e683f6e4596ef6af4d5286 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Sat, 21 Jun 2025 13:09:23 -0700 Subject: [PATCH] [ty] Fix mixed tuple subtyping (#18852) ## Summary The code in the `Variable` branch of `VariableLengthTupleSpec::has_relation_to` made the incorrect assumption that if you zip two possibly-different-length iterators together and iterate over the resulting zip iterator, the original two iterators will only have their common elements consumed. But in fact, the zip iterator detects that it is done when it receives a `None` from one iterator and `Some()` element from the other iterator, which means that it consumes one additional element from the longer iterator. This meant that we failed to detect mismatched types on this extra consumed element, because we never compared it to the variable type of the other tuple. Use `zip_longest` from itertools as an alternative, which allows us to combine all the handling into just two `zip_longest`, one for prefixes and one for suffixes. Marking this PR internal since it fixes a bug in a commit that wasn't released yet. ## Test Plan Added mdtests that failed before this fix and pass after it. --- .../mdtest/type_properties/is_subtype_of.md | 12 +++ crates/ty_python_semantic/src/types/tuple.rs | 73 +++++++++---------- 2 files changed, 48 insertions(+), 37 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/type_properties/is_subtype_of.md b/crates/ty_python_semantic/resources/mdtest/type_properties/is_subtype_of.md index ba445907ff..1619e3a027 100644 --- a/crates/ty_python_semantic/resources/mdtest/type_properties/is_subtype_of.md +++ b/crates/ty_python_semantic/resources/mdtest/type_properties/is_subtype_of.md @@ -245,6 +245,18 @@ static_assert( ) ) +static_assert( + not is_subtype_of( + tuple[Literal["foo"], *tuple[int, ...]], + tuple[int, ...], + ) +) +static_assert( + not is_subtype_of( + tuple[*tuple[int, ...], Literal["foo"]], + tuple[int, ...], + ) +) static_assert( not is_subtype_of( tuple[Literal[1], Literal[2], *tuple[int, ...], Literal[10]], diff --git a/crates/ty_python_semantic/src/types/tuple.rs b/crates/ty_python_semantic/src/types/tuple.rs index 4092a53372..d189b05072 100644 --- a/crates/ty_python_semantic/src/types/tuple.rs +++ b/crates/ty_python_semantic/src/types/tuple.rs @@ -16,7 +16,7 @@ //! that adds that "collapse `Never`" behavior, whereas [`TupleSpec`] allows you to add any element //! types, including `Never`.) -use itertools::Either; +use itertools::{Either, EitherOrBoth, Itertools}; use crate::types::class::{ClassType, KnownClass}; use crate::types::{Type, TypeMapping, TypeRelation, TypeVarInstance, TypeVarVariance, UnionType}; @@ -544,45 +544,44 @@ impl<'db> VariableLengthTupleSpec<'db> { TupleSpec::Variable(other) => { // The overlapping parts of the prefixes and suffixes must satisfy the relation. - let mut self_prefix = self.prefix.iter(); - let mut other_prefix = other.prefix.iter(); - let prefixes_match = (&mut self_prefix) - .zip(&mut other_prefix) - .all(|(self_ty, other_ty)| self_ty.has_relation_to(db, *other_ty, relation)); - if !prefixes_match { + // Any remaining parts must satisfy the relation with the other tuple's + // variable-length part. + if !self + .prefix + .iter() + .zip_longest(&other.prefix) + .all(|pair| match pair { + EitherOrBoth::Both(self_ty, other_ty) => { + self_ty.has_relation_to(db, *other_ty, relation) + } + EitherOrBoth::Left(self_ty) => { + self_ty.has_relation_to(db, other.variable, relation) + } + EitherOrBoth::Right(other_ty) => { + self.variable.has_relation_to(db, *other_ty, relation) + } + }) + { return false; } - let mut self_suffix = self.suffix.iter().rev(); - let mut other_suffix = other.suffix.iter().rev(); - let suffixes_match = (&mut self_suffix) - .zip(&mut other_suffix) - .all(|(self_ty, other_ty)| self_ty.has_relation_to(db, *other_ty, relation)); - if !suffixes_match { - return false; - } - - // Any remaining parts of either prefix or suffix must satisfy the relation with - // the other tuple's variable-length portion. - let prefix_matches_variable = self_prefix - .all(|self_ty| self_ty.has_relation_to(db, other.variable, relation)); - if !prefix_matches_variable { - return false; - } - let prefix_matches_variable = other_prefix - .all(|other_ty| self.variable.has_relation_to(db, *other_ty, relation)); - if !prefix_matches_variable { - return false; - } - - let suffix_matches_variable = self_suffix - .all(|self_ty| self_ty.has_relation_to(db, other.variable, relation)); - if !suffix_matches_variable { - return false; - } - let suffix_matches_variable = other_suffix - .all(|other_ty| self.variable.has_relation_to(db, *other_ty, relation)); - if !suffix_matches_variable { + if !self + .suffix + .iter() + .rev() + .zip_longest(other.suffix.iter().rev()) + .all(|pair| match pair { + EitherOrBoth::Both(self_ty, other_ty) => { + self_ty.has_relation_to(db, *other_ty, relation) + } + EitherOrBoth::Left(self_ty) => { + self_ty.has_relation_to(db, other.variable, relation) + } + EitherOrBoth::Right(other_ty) => { + self.variable.has_relation_to(db, *other_ty, relation) + } + }) + { return false; }