-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (CPython builtin functions actually completely skip |
||
let arg_typ = arg.typ(); | ||
let expected_type_name = $vm.to_pystr(&expected_type)?; | ||
let actual_type = $vm.to_pystr(&arg_typ)?; | ||
|
There was a problem hiding this comment.
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 callissubclass()
directly.With this contrived code:
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 :/
There was a problem hiding this comment.
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.