Call __instancecheck__ and __subclasscheck__ #554
Conversation
Codecov Report
@@ Coverage Diff @@
## master #554 +/- ##
==========================================
+ Coverage 42.02% 42.15% +0.13%
==========================================
Files 73 73
Lines 16425 16443 +18
Branches 4308 4316 +8
==========================================
+ Hits 6902 6931 +29
+ Misses 7561 7542 -19
- Partials 1962 1970 +8
Continue to review full report at Codecov.
|
|
There appear some files in conflict. Could you have a look at those? |
222d193 to
5779124
Compare
|
Swapped the names over and resolved the conflicts. |
| @@ -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.
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)
| @@ -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)? { | |||
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
You are correct this shouldn't use the real version, I will create a PR soon to fix this.
|
I'm also late to this one. I don't like the real_ naming (I do prefer the merged version to the original.) I think the convention we've gone with is that there should be a method on VirtualMachine for the full magic method powered version, and a simple function for the low level implementation. Ie. |
|
I will make another PR to move the real_ version to the vm. |
These magic methods are called as part of isinstance and issubclass to override their behavior. This means objtype::{isinstance, issubclass} require the vm, but in cases where the second argument is known to not override the associated magic method(i.e. most built in types), a version objtype::{real_isinstance, real_issubclass} can be used that doesn't require the vm.