mirror of https://github.com/astral-sh/ruff
[ty] Remove unnecessary `.expect()` call from `types/instance.rs` (#21527)
## Summary The `.expect()` call here:5dd56264fb/crates/ty_python_semantic/src/types/instance.rs (L816-L827)is the direct cause of the panic in https://github.com/astral-sh/ty/issues/1587. This patch gets rid of the panic by refactoring our `Protocol` enum so that the `Protocol::FromClass` variant holds a `ProtocolClass` instance rather than a `ClassType` instance (all the `.expect()` call was doing was attempting to convert form a `ClassType` to a `ProtocolClass`). I hoped that this would provide a fix for https://github.com/astral-sh/ty/issues/1587, but we still panic on the provided reproducible examples in that issue even with this PR. Nonetheless, I think this PR is a worthwhile change to make because: - It's probably slightly more efficient this way (we no longer have to re-verify that the wrapped class in a `Protocol::FromClass()` variant is a protocol class every time we want to access its interface) - It's nice to get rid of `.expect()` calls where possible, and this one seems definitely unnecessary - The _new_ panic message on this PR branch makes it much clearer what the underlying cause of the bug in https://github.com/astral-sh/ty/issues/1587 is: <details> <summary>New panic message</summary> ``` error[panic]: Panicked at /Users/alexw/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/a885bb4/src/function/execute.rs:321:21 when checking `/Users/alexw/dev/ruff/foo.py`: `ClassLiteral < 'db >::explicit_bases_(Id(4c09)): execute: too many cycle iterations` info: This indicates a bug in ty. info: If you could open an issue at https://github.com/astral-sh/ty/issues/new?title=%5Bpanic%5D, we'd be very appreciative! info: Platform: macos aarch64 info: Version: ruff/0.14.5+60 (18a14bfaf2025-11-19) info: Args: ["target/debug/ty", "check", "foo.py", "--python-version=3.14"] info: run with `RUST_BACKTRACE=1` environment variable to show the full backtrace information info: query stacktrace: 0: cached_protocol_interface(Id(6805)) at crates/ty_python_semantic/src/types/protocol_class.rs:790 1: is_equivalent_to_object_inner(Id(8003)) at crates/ty_python_semantic/src/types/instance.rs:667 2: infer_deferred_types(Id(1409)) at crates/ty_python_semantic/src/types/infer.rs:141 cycle heads: infer_definition_types(Id(140b)) -> iteration = 200, TypeVarInstance < 'db >::lazy_bound_(Id(5803)) -> iteration = 200 3: TypeVarInstance < 'db >::lazy_bound_(Id(5802)) at crates/ty_python_semantic/src/types.rs:8734 4: infer_definition_types(Id(140c)) at crates/ty_python_semantic/src/types/infer.rs:94 5: infer_deferred_types(Id(140a)) at crates/ty_python_semantic/src/types/infer.rs:141 6: TypeVarInstance < 'db >::lazy_bound_(Id(5803)) at crates/ty_python_semantic/src/types.rs:8734 7: infer_definition_types(Id(140b)) at crates/ty_python_semantic/src/types/infer.rs:94 8: infer_scope_types(Id(1000)) at crates/ty_python_semantic/src/types/infer.rs:70 9: check_file_impl(Id(c00)) at crates/ty_project/src/lib.rs:535 Found 1 diagnostic WARN A fatal error occurred while checking some files. Not all project files were analyzed. See the diagnostics list above for details. ``` </details> ## Test Plan All existing tests pass.
This commit is contained in:
parent
97935518e9
commit
ce06094ada
|
|
@ -392,12 +392,14 @@ impl Display for DisplayRepresentation<'_> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Type::ProtocolInstance(protocol) => match protocol.inner {
|
Type::ProtocolInstance(protocol) => match protocol.inner {
|
||||||
Protocol::FromClass(ClassType::NonGeneric(class)) => {
|
Protocol::FromClass(class) => match *class {
|
||||||
class.display_with(self.db, self.settings.clone()).fmt(f)
|
ClassType::NonGeneric(class) => {
|
||||||
}
|
class.display_with(self.db, self.settings.clone()).fmt(f)
|
||||||
Protocol::FromClass(ClassType::Generic(alias)) => {
|
}
|
||||||
alias.display_with(self.db, self.settings.clone()).fmt(f)
|
ClassType::Generic(alias) => {
|
||||||
}
|
alias.display_with(self.db, self.settings.clone()).fmt(f)
|
||||||
|
}
|
||||||
|
},
|
||||||
Protocol::Synthesized(synthetic) => {
|
Protocol::Synthesized(synthetic) => {
|
||||||
f.write_str("<Protocol with members ")?;
|
f.write_str("<Protocol with members ")?;
|
||||||
let interface = synthetic.interface();
|
let interface = synthetic.interface();
|
||||||
|
|
|
||||||
|
|
@ -1570,9 +1570,9 @@ impl<'db> SpecializationBuilder<'db> {
|
||||||
// generic protocol, we will need to check the types of the protocol members to be
|
// generic protocol, we will need to check the types of the protocol members to be
|
||||||
// able to infer the specialization of the protocol that the class implements.
|
// able to infer the specialization of the protocol that the class implements.
|
||||||
Type::ProtocolInstance(ProtocolInstanceType {
|
Type::ProtocolInstance(ProtocolInstanceType {
|
||||||
inner: Protocol::FromClass(ClassType::Generic(alias)),
|
inner: Protocol::FromClass(class),
|
||||||
..
|
..
|
||||||
}) => Some(alias),
|
}) => class.into_generic_alias(),
|
||||||
_ => None,
|
_ => None,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -10,7 +10,7 @@ use crate::semantic_index::definition::Definition;
|
||||||
use crate::types::constraints::{ConstraintSet, IteratorConstraintsExtension};
|
use crate::types::constraints::{ConstraintSet, IteratorConstraintsExtension};
|
||||||
use crate::types::enums::is_single_member_enum;
|
use crate::types::enums::is_single_member_enum;
|
||||||
use crate::types::generics::{InferableTypeVars, walk_specialization};
|
use crate::types::generics::{InferableTypeVars, walk_specialization};
|
||||||
use crate::types::protocol_class::walk_protocol_interface;
|
use crate::types::protocol_class::{ProtocolClass, walk_protocol_interface};
|
||||||
use crate::types::tuple::{TupleSpec, TupleType};
|
use crate::types::tuple::{TupleSpec, TupleType};
|
||||||
use crate::types::{
|
use crate::types::{
|
||||||
ApplyTypeMappingVisitor, ClassBase, ClassLiteral, FindLegacyTypeVarsVisitor,
|
ApplyTypeMappingVisitor, ClassBase, ClassLiteral, FindLegacyTypeVarsVisitor,
|
||||||
|
|
@ -44,13 +44,17 @@ impl<'db> Type<'db> {
|
||||||
.as_ref(),
|
.as_ref(),
|
||||||
)),
|
)),
|
||||||
Some(KnownClass::Object) => Type::object(),
|
Some(KnownClass::Object) => Type::object(),
|
||||||
_ if class_literal.is_protocol(db) => {
|
_ => class_literal
|
||||||
Self::ProtocolInstance(ProtocolInstanceType::from_class(class))
|
.is_typed_dict(db)
|
||||||
}
|
.then(|| Type::typed_dict(class))
|
||||||
_ if class_literal.is_typed_dict(db) => Type::typed_dict(class),
|
.or_else(|| {
|
||||||
// We don't call non_tuple_instance here because we've already checked that the class
|
class.into_protocol_class(db).map(|protocol_class| {
|
||||||
// is not `object`
|
Self::ProtocolInstance(ProtocolInstanceType::from_class(protocol_class))
|
||||||
_ => Type::NominalInstance(NominalInstanceType(NominalInstanceInner::NonTuple(class))),
|
})
|
||||||
|
})
|
||||||
|
.unwrap_or(Type::NominalInstance(NominalInstanceType(
|
||||||
|
NominalInstanceInner::NonTuple(class),
|
||||||
|
))),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -601,7 +605,7 @@ pub(super) fn walk_protocol_instance_type<'db, V: super::visitor::TypeVisitor<'d
|
||||||
impl<'db> ProtocolInstanceType<'db> {
|
impl<'db> ProtocolInstanceType<'db> {
|
||||||
// Keep this method private, so that the only way of constructing `ProtocolInstanceType`
|
// Keep this method private, so that the only way of constructing `ProtocolInstanceType`
|
||||||
// instances is through the `Type::instance` constructor function.
|
// instances is through the `Type::instance` constructor function.
|
||||||
fn from_class(class: ClassType<'db>) -> Self {
|
fn from_class(class: ProtocolClass<'db>) -> Self {
|
||||||
Self {
|
Self {
|
||||||
inner: Protocol::FromClass(class),
|
inner: Protocol::FromClass(class),
|
||||||
_phantom: PhantomData,
|
_phantom: PhantomData,
|
||||||
|
|
@ -625,7 +629,7 @@ impl<'db> ProtocolInstanceType<'db> {
|
||||||
pub(super) fn as_nominal_type(self) -> Option<NominalInstanceType<'db>> {
|
pub(super) fn as_nominal_type(self) -> Option<NominalInstanceType<'db>> {
|
||||||
match self.inner {
|
match self.inner {
|
||||||
Protocol::FromClass(class) => {
|
Protocol::FromClass(class) => {
|
||||||
Some(NominalInstanceType(NominalInstanceInner::NonTuple(class)))
|
Some(NominalInstanceType(NominalInstanceInner::NonTuple(*class)))
|
||||||
}
|
}
|
||||||
Protocol::Synthesized(_) => None,
|
Protocol::Synthesized(_) => None,
|
||||||
}
|
}
|
||||||
|
|
@ -805,11 +809,17 @@ impl<'db> VarianceInferable<'db> for ProtocolInstanceType<'db> {
|
||||||
|
|
||||||
/// An enumeration of the two kinds of protocol types: those that originate from a class
|
/// An enumeration of the two kinds of protocol types: those that originate from a class
|
||||||
/// definition in source code, and those that are synthesized from a set of members.
|
/// definition in source code, and those that are synthesized from a set of members.
|
||||||
|
///
|
||||||
|
/// # Ordering
|
||||||
|
///
|
||||||
|
/// Ordering between variants is stable and should be the same between runs.
|
||||||
|
/// Ordering within variants is based on the wrapped data's salsa-assigned id and not on its values.
|
||||||
|
/// The id may change between runs, or when e.g. a `Protocol` was garbage-collected and recreated.
|
||||||
#[derive(
|
#[derive(
|
||||||
Copy, Clone, Debug, Eq, PartialEq, Hash, salsa::Update, PartialOrd, Ord, get_size2::GetSize,
|
Copy, Clone, Debug, Eq, PartialEq, Hash, salsa::Update, PartialOrd, Ord, get_size2::GetSize,
|
||||||
)]
|
)]
|
||||||
pub(super) enum Protocol<'db> {
|
pub(super) enum Protocol<'db> {
|
||||||
FromClass(ClassType<'db>),
|
FromClass(ProtocolClass<'db>),
|
||||||
Synthesized(SynthesizedProtocolType<'db>),
|
Synthesized(SynthesizedProtocolType<'db>),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -817,10 +827,7 @@ impl<'db> Protocol<'db> {
|
||||||
/// Return the members of this protocol type
|
/// Return the members of this protocol type
|
||||||
fn interface(self, db: &'db dyn Db) -> ProtocolInterface<'db> {
|
fn interface(self, db: &'db dyn Db) -> ProtocolInterface<'db> {
|
||||||
match self {
|
match self {
|
||||||
Self::FromClass(class) => class
|
Self::FromClass(class) => class.interface(db),
|
||||||
.into_protocol_class(db)
|
|
||||||
.expect("Class wrapped by `Protocol` should be a protocol class")
|
|
||||||
.interface(db),
|
|
||||||
Self::Synthesized(synthesized) => synthesized.interface(),
|
Self::Synthesized(synthesized) => synthesized.interface(),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -42,7 +42,14 @@ impl<'db> ClassType<'db> {
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Representation of a single `Protocol` class definition.
|
/// Representation of a single `Protocol` class definition.
|
||||||
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
|
///
|
||||||
|
/// # Ordering
|
||||||
|
///
|
||||||
|
/// Ordering is based on the wrapped data's salsa-assigned id and not on its values.
|
||||||
|
/// The id may change between runs, or when e.g. a `ProtocolClass` was garbage-collected and recreated.
|
||||||
|
#[derive(
|
||||||
|
Debug, Copy, Clone, PartialEq, Eq, Hash, salsa::Update, get_size2::GetSize, PartialOrd, Ord,
|
||||||
|
)]
|
||||||
pub(super) struct ProtocolClass<'db>(ClassType<'db>);
|
pub(super) struct ProtocolClass<'db>(ClassType<'db>);
|
||||||
|
|
||||||
impl<'db> ProtocolClass<'db> {
|
impl<'db> ProtocolClass<'db> {
|
||||||
|
|
@ -124,6 +131,19 @@ impl<'db> ProtocolClass<'db> {
|
||||||
report_undeclared_protocol_member(context, first_definition, self, class_place_table);
|
report_undeclared_protocol_member(context, first_definition, self, class_place_table);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub(super) fn apply_type_mapping_impl<'a>(
|
||||||
|
self,
|
||||||
|
db: &'db dyn Db,
|
||||||
|
type_mapping: &TypeMapping<'a, 'db>,
|
||||||
|
tcx: TypeContext<'db>,
|
||||||
|
visitor: &ApplyTypeMappingVisitor<'db>,
|
||||||
|
) -> Self {
|
||||||
|
Self(
|
||||||
|
self.0
|
||||||
|
.apply_type_mapping_impl(db, type_mapping, tcx, visitor),
|
||||||
|
)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
impl<'db> Deref for ProtocolClass<'db> {
|
impl<'db> Deref for ProtocolClass<'db> {
|
||||||
|
|
@ -134,6 +154,12 @@ impl<'db> Deref for ProtocolClass<'db> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl<'db> From<ProtocolClass<'db>> for Type<'db> {
|
||||||
|
fn from(value: ProtocolClass<'db>) -> Self {
|
||||||
|
Self::from(value.0)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// The interface of a protocol: the members of that protocol, and the types of those members.
|
/// The interface of a protocol: the members of that protocol, and the types of those members.
|
||||||
///
|
///
|
||||||
/// # Ordering
|
/// # Ordering
|
||||||
|
|
|
||||||
|
|
@ -2,6 +2,7 @@ use crate::place::PlaceAndQualifiers;
|
||||||
use crate::semantic_index::definition::Definition;
|
use crate::semantic_index::definition::Definition;
|
||||||
use crate::types::constraints::ConstraintSet;
|
use crate::types::constraints::ConstraintSet;
|
||||||
use crate::types::generics::InferableTypeVars;
|
use crate::types::generics::InferableTypeVars;
|
||||||
|
use crate::types::protocol_class::ProtocolClass;
|
||||||
use crate::types::variance::VarianceInferable;
|
use crate::types::variance::VarianceInferable;
|
||||||
use crate::types::{
|
use crate::types::{
|
||||||
ApplyTypeMappingVisitor, BoundTypeVarInstance, ClassType, DynamicType,
|
ApplyTypeMappingVisitor, BoundTypeVarInstance, ClassType, DynamicType,
|
||||||
|
|
@ -289,6 +290,12 @@ impl<'db> From<DynamicType<'db>> for SubclassOfInner<'db> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl<'db> From<ProtocolClass<'db>> for SubclassOfInner<'db> {
|
||||||
|
fn from(value: ProtocolClass<'db>) -> Self {
|
||||||
|
SubclassOfInner::Class(*value)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
impl<'db> From<SubclassOfInner<'db>> for Type<'db> {
|
impl<'db> From<SubclassOfInner<'db>> for Type<'db> {
|
||||||
fn from(value: SubclassOfInner<'db>) -> Self {
|
fn from(value: SubclassOfInner<'db>) -> Self {
|
||||||
match value {
|
match value {
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue