Skip to content

Fix isinstance #5869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 3 commits into from
Jun 30, 2025
Merged

Fix isinstance #5869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f1acca3
lookup_special
youknowone Jun 29, 2025
c3857ee
Fix abstract_issubclass
youknowone Jun 30, 2025
899abbb
Fix real_is_instance
youknowone Jun 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions vm/src/builtins/type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,35 @@ impl PyType {
self.attributes.read().get(attr_name).cloned()
}

/// Equivalent to CPython's find_name_in_mro
/// Look in tp_dict of types in MRO - bypasses descriptors and other attribute access machinery
fn find_name_in_mro(&self, name: &'static PyStrInterned) -> Option<PyObjectRef> {
// First check in our own dict
if let Some(value) = self.attributes.read().get(name) {
return Some(value.clone());
}

// Then check in MRO
for base in self.mro.read().iter() {
if let Some(value) = base.attributes.read().get(name) {
return Some(value.clone());
}
}

None
}

/// Equivalent to CPython's _PyType_LookupRef
/// Looks up a name through the MRO without setting an exception
pub fn lookup_ref(&self, name: &Py<PyStr>, vm: &VirtualMachine) -> Option<PyObjectRef> {
// Get interned name for efficient lookup
let interned_name = vm.ctx.interned_str(name)?;

// Use find_name_in_mro which matches CPython's behavior
// This bypasses descriptors and other attribute access machinery
self.find_name_in_mro(interned_name)
}

pub fn get_super_attr(&self, attr_name: &'static PyStrInterned) -> Option<PyObjectRef> {
self.mro
.read()
Expand Down
103 changes: 57 additions & 46 deletions vm/src/protocol/object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,43 +410,39 @@ impl PyObject {
}

fn abstract_issubclass(&self, cls: &PyObject, vm: &VirtualMachine) -> PyResult<bool> {
// # Safety: The lifetime of `derived` is forced to be ignored
let bases = unsafe {
let mut derived = self;
// First loop: handle single inheritance without recursion
loop {
if derived.is(cls) {
return Ok(true);
}
// Store the current derived class to check
let mut bases: PyTupleRef;
let mut derived = self;

let Some(bases) = derived.abstract_get_bases(vm)? else {
return Ok(false);
};
let n = bases.len();
match n {
0 => return Ok(false),
1 => {
// Avoid recursion in the single inheritance case
// # safety
// Intention:
// ```
// derived = bases.as_slice()[0].as_object();
// ```
// Though type-system cannot guarantee, derived does live long enough in the loop.
derived = &*(bases.as_slice()[0].as_object() as *const _);
continue;
}
_ => {
// Multiple inheritance - break out to handle recursively
break bases;
}
// First loop: handle single inheritance without recursion
let bases = loop {
if derived.is(cls) {
return Ok(true);
}

let Some(derived_bases) = derived.abstract_get_bases(vm)? else {
return Ok(false);
};

let n = derived_bases.len();
match n {
0 => return Ok(false),
1 => {
// Avoid recursion in the single inheritance case
// Get the next derived class and continue the loop
bases = derived_bases;
derived = &bases.as_slice()[0];
continue;
}
_ => {
// Multiple inheritance - handle recursively
break derived_bases;
}
}
};

// Second loop: handle multiple inheritance with recursion
// At this point we know n >= 2
let n = bases.len();
// At this point we know n >= 2
debug_assert!(n >= 2);

for i in 0..n {
Expand Down Expand Up @@ -528,10 +524,10 @@ impl PyObject {
return Ok(false);
}

// Check for __subclasscheck__ method
if let Some(checker) = vm.get_special_method(cls, identifier!(vm, __subclasscheck__))? {
// Check for __subclasscheck__ method using lookup_special (matches CPython)
if let Some(checker) = cls.lookup_special(identifier!(vm, __subclasscheck__), vm) {
let res = vm.with_recursion("in __subclasscheck__", || {
checker.invoke((derived.to_owned(),), vm)
checker.call((derived.to_owned(),), vm)
})?;
return res.try_to_bool(vm);
}
Expand All @@ -542,18 +538,17 @@ impl PyObject {
/// Real isinstance check without going through __instancecheck__
/// This is equivalent to CPython's _PyObject_RealIsInstance/object_isinstance
pub fn real_is_instance(&self, cls: &PyObject, vm: &VirtualMachine) -> PyResult<bool> {
if let Ok(typ) = cls.try_to_ref::<PyType>(vm) {
if let Ok(cls) = cls.try_to_ref::<PyType>(vm) {
// PyType_Check(cls) - cls is a type object
let mut retval = self.fast_isinstance(typ);

let mut retval = self.class().is_subtype(cls);
if !retval {
// Check __class__ attribute, only masking AttributeError
if let Some(i_cls) =
vm.get_attribute_opt(self.to_owned(), identifier!(vm, __class__))?
{
if let Ok(i_cls_type) = PyTypeRef::try_from_object(vm, i_cls) {
if !i_cls_type.is(self.class()) {
retval = i_cls_type.fast_issubclass(typ);
retval = i_cls_type.is_subtype(cls);
}
}
}
Expand All @@ -572,11 +567,7 @@ impl PyObject {
if let Some(i_cls) =
vm.get_attribute_opt(self.to_owned(), identifier!(vm, __class__))?
{
if vm.is_none(&i_cls) {
Ok(false)
} else {
i_cls.abstract_issubclass(cls, vm)
}
i_cls.abstract_issubclass(cls, vm)
} else {
Ok(false)
}
Expand Down Expand Up @@ -624,10 +615,10 @@ impl PyObject {
return Ok(false);
}

// Check for __instancecheck__ method
if let Some(checker) = vm.get_special_method(cls, identifier!(vm, __instancecheck__))? {
// Check for __instancecheck__ method using lookup_special (matches CPython)
if let Some(checker) = cls.lookup_special(identifier!(vm, __instancecheck__), vm) {
let res = vm.with_recursion("in __instancecheck__", || {
checker.invoke((self.to_owned(),), vm)
checker.call((self.to_owned(),), vm)
})?;
return res.try_to_bool(vm);
}
Expand Down Expand Up @@ -754,4 +745,24 @@ impl PyObject {

Err(vm.new_type_error(format!("'{}' does not support item deletion", self.class())))
}

/// Equivalent to CPython's _PyObject_LookupSpecial
/// Looks up a special method in the type's MRO without checking instance dict.
/// Returns None if not found (masking AttributeError like CPython).
pub fn lookup_special(&self, attr: &Py<PyStr>, vm: &VirtualMachine) -> Option<PyObjectRef> {
let obj_cls = self.class();

// Use PyType::lookup_ref (equivalent to CPython's _PyType_LookupRef)
let res = obj_cls.lookup_ref(attr, vm)?;

// If it's a descriptor, call its __get__ method
let descr_get = res.class().mro_find_map(|cls| cls.slots.descr_get.load());
if let Some(descr_get) = descr_get {
let obj_cls = obj_cls.to_owned().into();
// CPython ignores exceptions in _PyObject_LookupSpecial and returns NULL
descr_get(res, Some(self.to_owned()), Some(obj_cls), vm).ok()
} else {
Some(res)
}
}
}
Loading