Skip to content

Call __instancecheck__ and __subclasscheck__ #554

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.

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
merged 3 commits into from
Feb 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 54 additions & 0 deletions tests/snippets/isinstance.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@

class Regular:
pass


assert isinstance(Regular(), Regular)


class MCNotInstanceOf(type):
def __instancecheck__(self, instance):
return False


class NotInstanceOf(metaclass=MCNotInstanceOf):
pass


class InheritedNotInstanceOf(NotInstanceOf):
pass


assert not isinstance(Regular(), NotInstanceOf)
assert not isinstance(1, NotInstanceOf)

# weird cpython behaviour if exact match then isinstance return true
assert isinstance(NotInstanceOf(), NotInstanceOf)
assert not NotInstanceOf.__instancecheck__(NotInstanceOf())
assert not isinstance(InheritedNotInstanceOf(), NotInstanceOf)


class MCAlwaysInstanceOf(type):
def __instancecheck__(self, instance):
return True


class AlwaysInstanceOf(metaclass=MCAlwaysInstanceOf):
pass


assert isinstance(AlwaysInstanceOf(), AlwaysInstanceOf)
assert isinstance(Regular(), AlwaysInstanceOf)
assert isinstance(1, AlwaysInstanceOf)


class MCReturnInt(type):
def __instancecheck__(self, instance):
return 3


class ReturnInt(metaclass=MCReturnInt):
pass


assert isinstance("a", ReturnInt) is True
63 changes: 63 additions & 0 deletions tests/snippets/issubclass.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@

class A:
pass


class B(A):
pass


assert issubclass(A, A)
assert issubclass(B, A)
assert not issubclass(A, B)


class MCNotSubClass(type):
def __subclasscheck__(self, subclass):
return False


class NotSubClass(metaclass=MCNotSubClass):
pass


class InheritedNotSubClass(NotSubClass):
pass


assert not issubclass(A, NotSubClass)
assert not issubclass(NotSubClass, NotSubClass)
assert not issubclass(InheritedNotSubClass, NotSubClass)
assert not issubclass(NotSubClass, InheritedNotSubClass)


class MCAlwaysSubClass(type):
def __subclasscheck__(self, subclass):
return True


class AlwaysSubClass(metaclass=MCAlwaysSubClass):
pass


class InheritedAlwaysSubClass(AlwaysSubClass):
pass


assert issubclass(A, AlwaysSubClass)
assert issubclass(AlwaysSubClass, AlwaysSubClass)
assert issubclass(InheritedAlwaysSubClass, AlwaysSubClass)
assert issubclass(AlwaysSubClass, InheritedAlwaysSubClass)


class MCAVirtualSubClass(type):
def __subclasscheck__(self, subclass):
return subclass is A


class AVirtualSubClass(metaclass=MCAVirtualSubClass):
pass


assert issubclass(A, AVirtualSubClass)
assert not isinstance(B, AVirtualSubClass)
26 changes: 15 additions & 11 deletions vm/src/builtins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,21 +349,25 @@ fn builtin_id(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
// builtin_input

fn builtin_isinstance(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
arg_check!(vm, args, required = [(obj, None), (typ, None)]);
arg_check!(
vm,
args,
required = [(obj, None), (typ, Some(vm.get_type()))]
);

let isinstance = objtype::isinstance(obj, typ);
Ok(vm.context().new_bool(isinstance))
let isinstance = objtype::real_isinstance(vm, obj, typ)?;
Ok(vm.new_bool(isinstance))
}

fn builtin_issubclass(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
if args.args.len() != 2 {
panic!("issubclass expects exactly two parameters");
}

let cls1 = &args.args[0];
let cls2 = &args.args[1];
arg_check!(
vm,
args,
required = [(subclass, Some(vm.get_type())), (cls, Some(vm.get_type()))]
);

Ok(vm.context().new_bool(objtype::issubclass(cls1, cls2)))
let issubclass = objtype::real_issubclass(vm, subclass, cls)?;
Ok(vm.context().new_bool(issubclass))
}

fn builtin_iter(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
Expand Down Expand Up @@ -818,7 +822,7 @@ pub fn builtin_build_class_(vm: &mut VirtualMachine, mut args: PyFuncArgs) -> Py
let mut metaclass = args.get_kwarg("metaclass", vm.get_type());

for base in bases.clone() {
if objtype::issubclass(&base.typ(), &metaclass) {
if objtype::real_issubclass(vm, &base.typ(), &metaclass)? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, pretty sure we shouldn't use the overloaded __subclasscheck__ and just call issubclass() directly.

With this contrived code:


class AlwaysGood(type):
	def __subclasscheck__(self, x):
		return True

class M(type): pass
class A(metaclass=M): pass

class M2(type, metaclass=AlwaysGood): pass
class C(A, metaclass=M2): pass

RustPython after this patch accepts this code, but CPython seems to have ignored overloaded method and still reports the metaclass conflict - which IMO is the correct behavior.

In general, looks like it'll be tricky to manage which calls should use builtin method, and which accept the overloaded method. Probably the best bet will be to hope that probably-imported-in-the-future CPython test suite will catch the inconsistencies :/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct this shouldn't use the real version, I will create a PR soon to fix this.

metaclass = base.typ();
} else if !objtype::issubclass(&metaclass, &base.typ()) {
return Err(vm.new_type_error("metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases".to_string()));
Expand Down
3 changes: 2 additions & 1 deletion vm/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ macro_rules! type_check {
// None indicates that we have no type requirement (i.e. we accept any type)
if let Some(expected_type) = $arg_type {
let arg = &$args.args[$arg_count];
if !$crate::obj::objtype::isinstance(arg, &expected_type) {

if !$crate::obj::objtype::real_isinstance($vm, arg, &expected_type)? {
Copy link
Contributor

@adrian17 adrian17 Feb 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I'm late, but... do we need it here? type checking is already one of the slowest part of code, and I'd expect most, if not all, of types used for builtin functions' typechecks to not overload _instancecheck_.

(CPython builtin functions actually completely skip isinstance and directly check types with eg PyLong_CheckExact and PyType_FastSubclass macros)

let arg_typ = arg.typ();
let expected_type_name = $vm.to_pystr(&expected_type)?;
let actual_type = $vm.to_pystr(&arg_typ)?;
Expand Down
4 changes: 3 additions & 1 deletion vm/src/obj/objsuper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ fn super_init(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
};

// Check obj type:
if !(objtype::isinstance(&py_obj, &py_type) || objtype::issubclass(&py_obj, &py_type)) {
if !(objtype::real_isinstance(vm, &py_obj, &py_type)?
|| objtype::real_issubclass(vm, &py_obj, &py_type)?)
{
return Err(vm.new_type_error(
"super(type, obj): obj must be an instance or subtype of type".to_string(),
));
Expand Down
70 changes: 68 additions & 2 deletions vm/src/obj/objtype.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::objbool;
use super::objdict;
use super::objstr;
use super::objtype; // Required for arg_check! to use isinstance
Expand All @@ -8,6 +9,7 @@ use crate::pyobject::{
use crate::vm::VirtualMachine;
use std::cell::RefCell;
use std::collections::HashMap;
use std::rc::Rc;

/*
* The magical type type
Expand Down Expand Up @@ -56,6 +58,16 @@ pub fn init(context: &PyContext) {
"__getattribute__",
context.new_rustfunc(type_getattribute),
);
context.set_attr(
&type_type,
"__instancecheck__",
context.new_rustfunc(type_instance_check),
);
context.set_attr(
&type_type,
"__subclasscheck__",
context.new_rustfunc(type_subclass_check),
);
context.set_attr(&type_type, "__doc__", context.new_str(type_doc.to_string()));
}

Expand Down Expand Up @@ -89,16 +101,70 @@ pub fn base_classes(obj: &PyObjectRef) -> Vec<PyObjectRef> {
_mro(obj.typ()).unwrap()
}

/// Determines if `obj` actually an instance of `cls`, this doesn't call __instancecheck__, so only
/// use this if `cls` is known to have not overridden the base __instancecheck__ magic method.
pub fn isinstance(obj: &PyObjectRef, cls: &PyObjectRef) -> bool {
let mro = _mro(obj.typ()).unwrap();
mro.into_iter().any(|c| c.is(&cls))
}

pub fn issubclass(typ: &PyObjectRef, cls: &PyObjectRef) -> bool {
let mro = _mro(typ.clone()).unwrap();
/// Determines if `obj` is an instance of `cls`, either directly, indirectly or virtually via the
/// __instancecheck__ magic method.
pub fn real_isinstance(
vm: &mut VirtualMachine,
obj: &PyObjectRef,
cls: &PyObjectRef,
) -> PyResult<bool> {
// cpython first does an exact check on the type, although documentation doesn't state that
// https://github.com/python/cpython/blob/a24107b04c1277e3c1105f98aff5bfa3a98b33a0/Objects/abstract.c#L2408
if Rc::ptr_eq(&obj.typ(), cls) {
Ok(true)
} else {
let ret = vm.call_method(cls, "__instancecheck__", vec![obj.clone()])?;
objbool::boolval(vm, ret)
}
}

fn type_instance_check(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
arg_check!(
vm,
args,
required = [(typ, Some(vm.ctx.type_type())), (obj, None)]
);
Ok(vm.new_bool(isinstance(obj, typ)))
}

/// Determines if `subclass` is actually a subclass of `cls`, this doesn't call __subclasscheck__,
/// so only use this if `cls` is known to have not overridden the base __subclasscheck__ magic
/// method.
pub fn issubclass(subclass: &PyObjectRef, cls: &PyObjectRef) -> bool {
let mro = _mro(subclass.clone()).unwrap();
mro.into_iter().any(|c| c.is(&cls))
}

/// Determines if `subclass` is a subclass of `cls`, either directly, indirectly or virtually via
/// the __subclasscheck__ magic method.
pub fn real_issubclass(
vm: &mut VirtualMachine,
subclass: &PyObjectRef,
cls: &PyObjectRef,
) -> PyResult<bool> {
let ret = vm.call_method(cls, "__subclasscheck__", vec![subclass.clone()])?;
objbool::boolval(vm, ret)
}

fn type_subclass_check(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
arg_check!(
vm,
args,
required = [
(cls, Some(vm.ctx.type_type())),
(subclass, Some(vm.ctx.type_type()))
]
);
Ok(vm.new_bool(issubclass(subclass, cls)))
}

pub fn get_type_name(typ: &PyObjectRef) -> String {
if let PyObjectPayload::Class { name, .. } = &typ.payload {
name.clone()
Expand Down
4 changes: 2 additions & 2 deletions vm/src/pyobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,7 @@ impl PyFuncArgs {
) -> Result<Option<PyObjectRef>, PyObjectRef> {
match self.get_optional_kwarg(key) {
Some(kwarg) => {
if objtype::isinstance(&kwarg, &ty) {
if objtype::real_isinstance(vm, &kwarg, &ty)? {
Ok(Some(kwarg))
} else {
let expected_ty_name = vm.to_pystr(&ty)?;
Expand Down Expand Up @@ -1148,7 +1148,7 @@ impl Signature {

for (pos, arg) in args.args.iter().enumerate() {
if let Some(expected_type) = self.arg_type(pos) {
if !objtype::isinstance(arg, expected_type) {
if !objtype::real_isinstance(vm, arg, expected_type)? {
let arg_typ = arg.typ();
let expected_type_name = vm.to_pystr(&expected_type)?;
let actual_type = vm.to_pystr(&arg_typ)?;
Expand Down