py/runtime: Return obj->attr for obj.attr if attr has offset type. - #4969

Updates:

• 8b2a4b2 fixes the original PR which had an error in the unix nanbox build.

• 796404f (latest) provides an alternative where the additions are guarded by MICROPY_PY_INSTANCE_ATTRS so the base code size does not change.

Thanks for the patch, it's quite interesting. My main reservation would be that it's adding a feature that won't be used much. Are there any places in the existing code base that could be converted to use this feature?

Alternatives to this are 1) use the type->attr function to handle attribute access explicitly, which probably leads to smaller code (because there's only one function to handle all attributes); 2) use properties (this is pretty heavy though).

OTOH, this feature might be able to be used to implement modifiable sys.stdin/stdout/stderr, by also extending it to allow setting of the attribute.

Thanks for sharing your thoughts on this, I appreciate it.

Are there any places in the existing code base that could be converted to use this feature?

Good point, I'll have to look at that.

Alternatives to this are 1) use the type->attr function to handle attribute access explicitly

I tried it via a custom attribute handler first, but it seems this led to more code and less generic code. But this may have been my use case. Some issued I encountered with that approach are:

• I was trying to use one generic handler for various classes to organize and save code size. So, this handler would treat all the various QSTRs and return self->something accordingly. But when using the same handler for multiple classes, I also had to hard-code type checks to make sure the class indeed has that attribute. And then for each one get some_special_type_obj_t *self of the right type before being able to access self->the_attribute. So essentially adding statement to an ever growing switch, each of which essentially doing the same but for a different type. This is when I started with the offsetof approach, because that lets you do the equivalent at compile-time in a very generic sense.
• To keep things generic, I tried treating only the special cases (a handful of QSTRs) and then call the default attr functions to apply default behavior to the rest (standard methods), because otherwise I would be duplicating nearly the same code. I encountered two issues here:
  • Most of the default attribute handlers are static functions, seemingly making re-use harder.
  • The default attribute handlers check whether a custom attr handler exists and then call it. When you reuse the default attr handlers like this, you end up getting a recursion.

Are there any places in the existing code base that could be converted to use this feature?

One example where this could be used in the existing code base as-is (even without adding a write equivalent) is in py/objslice, where the whole attr handler becomes a locals_dict.

#if MICROPY_PY_BUILTINS_SLICE_ATTRS
STATI const mp_rom_map_elem_t slice_locals_dict_table[] = {
    { MP_ROM_QSTR(MP_QSTR_start), MP_ROM_ATTRIBUTE_OFFSET(mp_obj_slice_t, start)},
    { MP_ROM_QSTR(MP_QSTR_stop),  MP_ROM_ATTRIBUTE_OFFSET(mp_obj_slice_t, stop) },
    { MP_ROM_QSTR(MP_QSTR_step),  MP_ROM_ATTRIBUTE_OFFSET(mp_obj_slice_t, step) },
};
STATIC MP_DEFINE_CONST_DICT(slice_locals_dict, slice_locals_dict_table);
#endif

const mp_obj_type_t mp_type_slice = {
    { &mp_type_type },
    .name = MP_QSTR_slice,
    .print = slice_print,
#if MICROPY_PY_BUILTINS_SLICE_ATTRS
    .locals_dict = (mp_obj_dict_t*)&slice_locals_dict,
#endif
};

But the real benefit is for types that have both regular methods and class attributes with the default attribute handler, alongside the instance attributes introduced here. There's not really a gain in this particular example.

And it isn't a size improvement here because of the mp_type_offset introduced to store the offset. This is a bit overkill because all it stores is one integer. The reason for having it is just so the runtime has a way to check what type it is so it can deal with it accordingly. If there's another way to detect that a key/value pair in the locals_dict just represents an integer, that would be more efficient.

Archiving some similar questions/requests:

• MicroPython Forum: Instance attributes via C-API
• MicroPython Forum: Implement a property in a builtin module

This is used heavily in the upcoming Pybricks MicroPython ports, so I'm going to try to make the case why this could be helpful for other ports as well.

Our main use case is to allow a certain Python API design style, but implement it entirely in C due to device constraints. It is useful for other applications too however, see below.

I won't go into the pros and cons of the API style here; this just sets out the thinking behind the implementation.

Our Python stubs API is here. Rendered docs are here.

I'm going to take the various LEGO hubs and sensors as an example, but this could also apply to boards like Pyboard variants.

The API is designed around the "fixed modularity" of such boards. They consist of distinct "modules/components" (LEDs, screens, buttons, batteries, ...) but they are fixed instances on the boards.

For example, a LEGO hub usually has a built-in status LED. (Likewise, a Pyboard has several LEDs soldered onto the board.)

Consider for example these two completely different programmable hubs in pseudo code:

from ._common import _Speaker, _Battery, _Light, _KeyPad

class EV3Brick:
    """LEGO® MINDSTORMS® EV3 Brick."""

    def __init__(self):
        self.speaker = _Speaker()
        self.battery = _Battery()
        self.light = _Light()
        self.buttons = _KeyPad()


class MoveHub:
    """LEGO® Powered Up Move Hub."""

    def __init__(self):
        self.battery = _Battery()
        self.light = _Light()

The end-user instantiates the device and is then able to interact with the pre-configured built-in devices like the light or battery:

hub = MoveHub()
hub.light.off()

To make this work, we use the solution in this PR and then do:

typedef struct _hubs_MoveHub_obj_t {
    mp_obj_base_t base;
    mp_obj_t light;
} hubs_MoveHub_obj_t;

Then inside hubs_MoveHub_make_new we have:

// Create an instance of the Light class, representing the hub light
self->light = common_Light_obj_make_new(internal_id, internal_capability_flags);

And then hubs_MoveHub_locals_dict_table just has:

{ MP_ROM_QSTR(MP_QSTR_light),       MP_ROM_ATTRIBUTE_OFFSET(hubs_MoveHub_obj_t, light) },

We use this implementation across various classes, each with different common devices as in the example above. This avoids having to make a custom attribute handler for each one.

In our case, the read-only aspect is by design, as it is meant to expose access but not override builtin physical devices.

Applications are not limited to this style alone, though. It applies to exposing any desired runtime "attribute" of the C-implemented MicroPython class. It could be runtime constants (like screen size), or runtime variables (like counters, status objects, etc). So this is not about properties or handlers that execute code, but strictly about exposing an attribute like it would be accessible in Python.

And arguably, setters might be useful if the goal is to generalize it, but that is perhaps better suited for fully fledged property setters because running some code is usually required.

In our case, the read-only aspect is by design, as it is meant to expose access but not override builtin physical devices.

Thanks for the detailed explanation. I agree this new feature is useful to expose C-level attributes to Python. But it does seem quite limiting in that it can only do read-only loads of fixed/known attributes. Eg it can't handle store, delete, or dynamic selection of whether the attribute exist or not. I would have thought the latter was needed for some cases, eg a Hub with pluggable components.

I agree this new feature is useful to expose C-level attributes to Python. But it does seem quite limiting in that it can only do read-only loads of fixed/known attributes. Eg it can't handle store, delete, or dynamic selection of whether the attribute exist or not.

True, this could certainly be generalized to be useful for other cases. Currently it does safely handle non-existing attributes by failing to return something if self->obj is MP_OBJ_NULL.

I would have thought the latter was needed for some cases, eg a Hub with pluggable components.

We don't auto-populate classes when things are plugged in. It's just that when a user instantiates, e.g. hub = MoveHub(), that class instance has an attribute hub.light to control the built-in light of that hub.

Are there any places in the existing code base that could be converted to use this feature?

I responded to this earlier but the new usyncio seems to have a close match too.

If storing is added, it looks like the following task_attr could almost entirely be done this way. The only exception looks to be MP_QSTR_waiting. I'm not entirely sure what it is intended to do (I have not used uasyncio yet), but if it is not supposed to return anything it could be done by letting self->waiting = MP_OBJ_NULL instead of mp_const_none as mentioned above. If more advanced logic is needed it can be treated as a special case using #6253 .

STATIC void task_attr(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
    mp_obj_task_t *self = MP_OBJ_TO_PTR(self_in);
    if (dest[0] == MP_OBJ_NULL) {
        // Load
        if (attr == MP_QSTR_coro) {
            dest[0] = self->coro;
        } else if (attr == MP_QSTR_data) {
            dest[0] = self->data;
        } else if (attr == MP_QSTR_waiting) {
            if (self->waiting != mp_const_none) {
                dest[0] = self->waiting;
            }
        } else if (attr == MP_QSTR_cancel) {
            dest[0] = MP_OBJ_FROM_PTR(&task_cancel_obj);
            dest[1] = self_in;
        } else if (attr == MP_QSTR_ph_key) {
            dest[0] = self->ph_key;
        }
    } else if (dest[1] != MP_OBJ_NULL) {
        // Store
        if (attr == MP_QSTR_coro) {
            self->coro = dest[1];
            dest[0] = MP_OBJ_NULL;
        } else if (attr == MP_QSTR_data) {
            self->data = dest[1];
            dest[0] = MP_OBJ_NULL;
        } else if (attr == MP_QSTR_waiting) {
            self->waiting = dest[1];
            dest[0] = MP_OBJ_NULL;
        }
    }
}

I think I'll have a look to see how store could work. Since there is the offset_obj_t to keep the offset value anyway, it could have a read/write flag also to indicate whether an attribute should be writable.

Since there is the offset_obj_t to keep the offset value anyway, it could have a read/write flag also to indicate whether an attribute should be writable.

Yes, that's exactly what I was thinking (and perhaps delete could work by storing MP_OBJ_NULL).

I think I'll have a look to see how store could work.

Below is one way to store (and adding write protection will be easy to add), but I'm not sure I'm familiar enough with the MicroPython codebase to decide what's the best place something like this should go, if anywhere. This version feels like a bit too much duplication with the complementary load step via mp_convert_member_lookup. Setting it directly in mp_convert_member_lookup if dest[0] == MP_OBJ_SENTINEL works too but that doesn't seem like the right thing to do.

It also seemed like objtype/mp_obj_instance_store_attr has rather similar goals for special accessors but those functions don't get called for these custom types.

Quoting from #6253 :

Another thing to keep in mind with this and #4969 is that if/when they get merged they become part of the public C API, which makes it harder to change/refactor things later on, eg to make lookups for native types more efficient, see #4360 .

So feel free to defer taking action on this one if you want to give those other areas some more thought first. We can keep our patch on our development fork for now. If you'd like to see this happen in a particular way (or just expand on this attempt) I'm happy to give it a try.

diff --git a/py/runtime.c b/py/runtime.c
index 57188c930..527e3029a 100644
--- a/py/runtime.c
+++ b/py/runtime.c
@@ -1157,6 +1157,22 @@ void mp_store_attr(mp_obj_t base, qstr attr, mp_obj_t value) {
             return;
         }
     }
+
+    #if MICROPY_PY_INSTANCE_ATTRS
+    if (type->locals_dict != NULL) {
+        // generic lookup in the object (ie not class or type)
+        assert(type->locals_dict->base.type == &mp_type_dict); // MicroPython restriction, for now
+        mp_map_t *locals_map = &type->locals_dict->map;
+        mp_map_elem_t *elem = mp_map_lookup(locals_map, MP_OBJ_NEW_QSTR(attr), MP_MAP_LOOKUP);
+        if (elem != NULL && MP_OBJ_IS_TYPE(elem->value, &mp_type_offset)) {
+            // Set instance attribute self->member to x using self.member = x
+            size_t offset = ((offset_obj_t*)MP_OBJ_TO_PTR(elem->value))->offset;
+            *(mp_obj_t*)((char*)MP_OBJ_TO_PTR(base) + offset) = value;
+            return;
+        }
+    }
+    #endif // MICROPY_PY_INSTANCE_ATTRS
+
     #if MICROPY_ERROR_REPORTING == MICROPY_ERROR_REPORTING_TERSE
     mp_raise_msg(&mp_type_AttributeError, MP_ERROR_TEXT("no such attribute"));
     #else

Some further thoughts: It may be possible to dispense with this one entirely and still achieve similar goals after cleaning up #6253.

With #6253, it should be possible to make a single generic attr handler that looks through the locals_dict for objects with offset type, and do a second generic lookup if it isn't found. Slightly less efficient, but it would avoid the problem of writing an attr for each class separately and it would not require any modification of the runtime.c.

I'll have a look and report back later this week.

Implemented in a different way in e2ca8ab