Skip to content
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

py/objtype: Add type.__bases__ attribute. #5139

Closed
wants to merge 1 commit into from

Conversation

nevercast
Copy link
Contributor

@nevercast nevercast commented Sep 24, 2019

Proposal: Add __bases__ attribute to types so that the inheritance heirachy can be inspected. Related to #5106 and #4368

I have not implemented .__base__ nor have I made any changes to anything MRO related. This is simply a read-only attribute that supports single and multiple inheritance.

For me, the intent of this addition is so I can type check objects in my application to ensure they inherit from a common base class.

It should be noted that I would like to add documentation and tests for this, once we finalize the draft. I'm not proposing we ignore housekeeping.

Example:

>>> class A:
...     pass
>>> class B(object):
...     pass
>>> class C(B):
...     pass
>>> class D(C, A):
...     pass
>>> A.__bases__
(<class 'object'>,)
>>> B.__bases__
(<class 'object'>,)
>>> C.__bases__
(<class 'B'>,)
>>> D.__bases__
(<class 'C'>, <class 'A'>)

@nevercast
Copy link
Contributor Author

Sorry about the force push there, fixed a bad pointer NULL comparison (which should fix the preexisting tests). Didn't wish to introduce another commit for that, though it might have been cleaner and more obvious.

Copy link
Member

@jimmo jimmo left a comment

Choose a reason for hiding this comment

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

I don't have strong feelings about whether MICROPY_CPYTHON_COMPAT is the right flag to gate this feature. It's on by default, and only disabled in the really minimal ports. The whole feature costs 84 (or maybe 72, see comment) bytes on STM32 (e.g. PYBV11).

Based on your comment, is it a feature you'd only want for unit testing? In which case perhaps only the Unix port needs it (or specifically your board config).

py/objtype.c Outdated Show resolved Hide resolved
py/objtype.c Outdated Show resolved Hide resolved
@nevercast
Copy link
Contributor Author

Based on your comment, is it a feature you'd only want for unit testing? In which case perhaps only the Unix port needs it (or specifically your board config).

This is actually a feature I'm using on the microcontroller since I don't have different code generation between tests and application at this stage.

@nevercast
Copy link
Contributor Author

Code size changes:

Port Before After Bytes
Unix 496856 496944 +88
PYBV11 396832 396900 +68
ESP32 1202048 1202112 +64

I tried MP_OBJ_FROM_PTR((self->parent ? self->parent : &mp_type_object)); and it was no smaller.

@nevercast
Copy link
Contributor Author

Squish squash.

@nevercast nevercast marked this pull request as ready for review September 24, 2019 08:14
@nevercast
Copy link
Contributor Author

Squash tests.

@dpgeorge
Copy link
Member

It looks like object needs special handling in that its __bases__ is the empty tuple. I think it's important to get this correct because it can be used to signal the end of a search when recursively looking up __bases__ of a type.

One way to fix this would be to modify all built-in types (eg str, bytes, dict, etc) so that they explicitly have .parent = &mp_type_object. Currently the default is .parent = NULL which implies that the parent is actually object. Making it explicit might even simplify some of the inheritance code elsewhere.

@nevercast
Copy link
Contributor Author

You are correct, I didn't consider this case. I'll make some tests for it and go about fixing it. Can't say I'll get it done this weekend but I will look in to it.

@dpgeorge dpgeorge mentioned this pull request Oct 16, 2019
@nevercast
Copy link
Contributor Author

One way to fix this would be to modify all built-in types (eg str, bytes, dict, etc) so that they explicitly have .parent = &mp_type_object. Currently the default is .parent = NULL which implies that the parent is actually object.

Another way would be to special case object to have an empty tuple

Making it explicit might even simplify some of the inheritance code elsewhere.

This is a good motivation to go with the longer route but do we want to include that kind of change in this patch?

@nevercast
Copy link
Contributor Author

Alright - I've branched this pull request in to two streams, this is the simplist approach, treat object special. If that's the only edge case then this is the easiest way to cover it.

I'm now exploring the other route, redefining what it meants for .parent=NULL.

@nevercast
Copy link
Contributor Author

@nevercast
Copy link
Contributor Author

Pushed fix for bad compare between pointers.

@nevercast
Copy link
Contributor Author

Just while I'm doing this, I notice it starts to get to be a pretty bloated change.

extern const mp_obj_type_t mp_type_int;
extern const mp_obj_type_t mp_type_str;
extern const mp_obj_type_t mp_type_bytes;
extern const mp_obj_type_t mp_type_bytearray;
extern const mp_obj_type_t mp_type_memoryview;
extern const mp_obj_type_t mp_type_float;
extern const mp_obj_type_t mp_type_complex;
extern const mp_obj_type_t mp_type_tuple;
extern const mp_obj_type_t mp_type_list;
extern const mp_obj_type_t mp_type_map; // map (the python builtin, not the dict implementation detail)
extern const mp_obj_type_t mp_type_enumerate;
extern const mp_obj_type_t mp_type_filter;
extern const mp_obj_type_t mp_type_deque;
extern const mp_obj_type_t mp_type_dict;
extern const mp_obj_type_t mp_type_ordereddict;
extern const mp_obj_type_t mp_type_range;
extern const mp_obj_type_t mp_type_set;
extern const mp_obj_type_t mp_type_frozenset;
extern const mp_obj_type_t mp_type_slice;
extern const mp_obj_type_t mp_type_zip;
extern const mp_obj_type_t mp_type_array;
extern const mp_obj_type_t mp_type_super;

A lot of types to add default .parent= to.
Also, interestingly, bool has a parent on int

Should I go and make all the built-in types like CPython ? I've started writing tests checking that all the builtins .bases match, but I'm wondering if we should default to .parent = mp_type_object and just have an edge case for instantiated classes and <type 'object'>

@dpgeorge
Copy link
Member

Another way would be to special case object to have an empty tuple

I do like that idea, it would eliminate the special test for object in the PR here as it stands. But I don't think it'll work because the code assumes that, if MICROPY_MULTIPLE_INHERITANCE is disabled, .parent is either NULL or a type, and the empty tuple is neither of those.

Just while I'm doing this, I notice it starts to get to be a pretty bloated change.

Yes indeed it does...

but I'm wondering if we should default to .parent = mp_type_object and just have an edge case for instantiated classes and <type 'object'>

Yes I think that's the best way forward at this point, to just make small improvements.

So, as it currently stands I think this PR is good to go in. Later on we can improve/optimise things by explicitly populating the .parent entry for all types.

@nevercast
Copy link
Contributor Author

So, as it currently stands I think this PR is good to go in. Later on we can improve/optimise things by explicitly populating the .parent entry for all types.

I'm happy for this PR to land in master as it is now, and I can continue exploring better solutions and raise incremental changes in the future.

@nevercast
Copy link
Contributor Author

One last squash to drop the extra commit message.

@@ -1014,6 +1014,21 @@ STATIC void type_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
dest[0] = MP_OBJ_NEW_QSTR(self->name);
return;
}
if (attr == MP_QSTR___bases__) {
mp_obj_t parent_obj = self->parent ? MP_OBJ_FROM_PTR(self->parent) : MP_OBJ_FROM_PTR(&mp_type_object);
if (self == &mp_type_object) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two blocks can be swapped. I'll do that now.

py/objtype.c Outdated Show resolved Hide resolved
@dpgeorge
Copy link
Member

Thanks, merged in 8f9e2e3 with a minor change to the test to skip if the feature is not enabled.

@dpgeorge dpgeorge closed this Oct 18, 2019
@nevercast
Copy link
Contributor Author

skip if the feature is not enabled.

Whoops. Thank you!

@dpgeorge
Copy link
Member

skip if the feature is not enabled.

Whoops. Thank you!

To help with this in the future see #5227

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Aug 13, 2021
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Aug 24, 2021
This addendum to micropython#5139 allows actually turning off onewireio. (Not
currently used by any board.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants