Skip to content

Commit 66a1138

Browse files
youknowoneyouknowone
authored
Fix isinstance (#5869)
* lookup_special * Fix abstract_issubclass * Fix real_is_instance
1 parent b6ff541 commit 66a1138

File tree

2 files changed

+86
-46
lines changed

2 files changed

+86
-46
lines changed

‎vm/src/builtins/type.rs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,35 @@ impl PyType {
383383
self.attributes.read().get(attr_name).cloned()
384384
}
385385

386+
/// Equivalent to CPython's find_name_in_mro
387+
/// Look in tp_dict of types in MRO - bypasses descriptors and other attribute access machinery
388+
fn find_name_in_mro(&self, name: &'static PyStrInterned) -> Option<PyObjectRef> {
389+
// First check in our own dict
390+
if let Some(value) = self.attributes.read().get(name) {
391+
return Some(value.clone());
392+
}
393+
394+
// Then check in MRO
395+
for base in self.mro.read().iter() {
396+
if let Some(value) = base.attributes.read().get(name) {
397+
return Some(value.clone());
398+
}
399+
}
400+
401+
None
402+
}
403+
404+
/// Equivalent to CPython's _PyType_LookupRef
405+
/// Looks up a name through the MRO without setting an exception
406+
pub fn lookup_ref(&self, name: &Py<PyStr>, vm: &VirtualMachine) -> Option<PyObjectRef> {
407+
// Get interned name for efficient lookup
408+
let interned_name = vm.ctx.interned_str(name)?;
409+
410+
// Use find_name_in_mro which matches CPython's behavior
411+
// This bypasses descriptors and other attribute access machinery
412+
self.find_name_in_mro(interned_name)
413+
}
414+
386415
pub fn get_super_attr(&self, attr_name: &'static PyStrInterned) -> Option<PyObjectRef> {
387416
self.mro
388417
.read()

‎vm/src/protocol/object.rs

Lines changed: 57 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -410,43 +410,39 @@ impl PyObject {
410410
}
411411

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

422-
let Some(bases) = derived.abstract_get_bases(vm)? else {
423-
return Ok(false);
424-
};
425-
let n = bases.len();
426-
match n {
427-
0 => return Ok(false),
428-
1 => {
429-
// Avoid recursion in the single inheritance case
430-
// # safety
431-
// Intention:
432-
// ```
433-
// derived = bases.as_slice()[0].as_object();
434-
// ```
435-
// Though type-system cannot guarantee, derived does live long enough in the loop.
436-
derived = &*(bases.as_slice()[0].as_object() as *const _);
437-
continue;
438-
}
439-
_ => {
440-
// Multiple inheritance - break out to handle recursively
441-
break bases;
442-
}
417+
// First loop: handle single inheritance without recursion
418+
let bases = loop {
419+
if derived.is(cls) {
420+
return Ok(true);
421+
}
422+
423+
let Some(derived_bases) = derived.abstract_get_bases(vm)? else {
424+
return Ok(false);
425+
};
426+
427+
let n = derived_bases.len();
428+
match n {
429+
0 => return Ok(false),
430+
1 => {
431+
// Avoid recursion in the single inheritance case
432+
// Get the next derived class and continue the loop
433+
bases = derived_bases;
434+
derived = &bases.as_slice()[0];
435+
continue;
436+
}
437+
_ => {
438+
// Multiple inheritance - handle recursively
439+
break derived_bases;
443440
}
444441
}
445442
};
446443

447-
// Second loop: handle multiple inheritance with recursion
448-
// At this point we know n >= 2
449444
let n = bases.len();
445+
// At this point we know n >= 2
450446
debug_assert!(n >= 2);
451447

452448
for i in 0..n {
@@ -528,10 +524,10 @@ impl PyObject {
528524
return Ok(false);
529525
}
530526

531-
// Check for __subclasscheck__ method
532-
if let Some(checker) = vm.get_special_method(cls, identifier!(vm, __subclasscheck__))? {
527+
// Check for __subclasscheck__ method using lookup_special (matches CPython)
528+
if let Some(checker) = cls.lookup_special(identifier!(vm, __subclasscheck__), vm) {
533529
let res = vm.with_recursion("in __subclasscheck__", || {
534-
checker.invoke((derived.to_owned(),), vm)
530+
checker.call((derived.to_owned(),), vm)
535531
})?;
536532
return res.try_to_bool(vm);
537533
}
@@ -542,18 +538,17 @@ impl PyObject {
542538
/// Real isinstance check without going through __instancecheck__
543539
/// This is equivalent to CPython's _PyObject_RealIsInstance/object_isinstance
544540
pub fn real_is_instance(&self, cls: &PyObject, vm: &VirtualMachine) -> PyResult<bool> {
545-
if let Ok(typ) = cls.try_to_ref::<PyType>(vm) {
541+
if let Ok(cls) = cls.try_to_ref::<PyType>(vm) {
546542
// PyType_Check(cls) - cls is a type object
547-
let mut retval = self.fast_isinstance(typ);
548-
543+
let mut retval = self.class().is_subtype(cls);
549544
if !retval {
550545
// Check __class__ attribute, only masking AttributeError
551546
if let Some(i_cls) =
552547
vm.get_attribute_opt(self.to_owned(), identifier!(vm, __class__))?
553548
{
554549
if let Ok(i_cls_type) = PyTypeRef::try_from_object(vm, i_cls) {
555550
if !i_cls_type.is(self.class()) {
556-
retval = i_cls_type.fast_issubclass(typ);
551+
retval = i_cls_type.is_subtype(cls);
557552
}
558553
}
559554
}
@@ -572,11 +567,7 @@ impl PyObject {
572567
if let Some(i_cls) =
573568
vm.get_attribute_opt(self.to_owned(), identifier!(vm, __class__))?
574569
{
575-
if vm.is_none(&i_cls) {
576-
Ok(false)
577-
} else {
578-
i_cls.abstract_issubclass(cls, vm)
579-
}
570+
i_cls.abstract_issubclass(cls, vm)
580571
} else {
581572
Ok(false)
582573
}
@@ -624,10 +615,10 @@ impl PyObject {
624615
return Ok(false);
625616
}
626617

627-
// Check for __instancecheck__ method
628-
if let Some(checker) = vm.get_special_method(cls, identifier!(vm, __instancecheck__))? {
618+
// Check for __instancecheck__ method using lookup_special (matches CPython)
619+
if let Some(checker) = cls.lookup_special(identifier!(vm, __instancecheck__), vm) {
629620
let res = vm.with_recursion("in __instancecheck__", || {
630-
checker.invoke((self.to_owned(),), vm)
621+
checker.call((self.to_owned(),), vm)
631622
})?;
632623
return res.try_to_bool(vm);
633624
}
@@ -754,4 +745,24 @@ impl PyObject {
754745

755746
Err(vm.new_type_error(format!("'{}' does not support item deletion", self.class())))
756747
}
748+
749+
/// Equivalent to CPython's _PyObject_LookupSpecial
750+
/// Looks up a special method in the type's MRO without checking instance dict.
751+
/// Returns None if not found (masking AttributeError like CPython).
752+
pub fn lookup_special(&self, attr: &Py<PyStr>, vm: &VirtualMachine) -> Option<PyObjectRef> {
753+
let obj_cls = self.class();
754+
755+
// Use PyType::lookup_ref (equivalent to CPython's _PyType_LookupRef)
756+
let res = obj_cls.lookup_ref(attr, vm)?;
757+
758+
// If it's a descriptor, call its __get__ method
759+
let descr_get = res.class().mro_find_map(|cls| cls.slots.descr_get.load());
760+
if let Some(descr_get) = descr_get {
761+
let obj_cls = obj_cls.to_owned().into();
762+
// CPython ignores exceptions in _PyObject_LookupSpecial and returns NULL
763+
descr_get(res, Some(self.to_owned()), Some(obj_cls), vm).ok()
764+
} else {
765+
Some(res)
766+
}
767+
}
757768
}

0 commit comments

Comments
 (0)