Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.51%. Comparing base (d901e98) to head (d027332).

@@           Coverage Diff           @@
##           master   #18594   +/-   ##
=======================================
  Coverage   98.51%   98.51%           
=======================================
  Files         176      176           
  Lines       22903    22910    +7     
=======================================
+ Hits        22562    22569    +7     
  Misses        341      341           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Code size report:

Reference:  unix/README: Update the supported targets list. [d901e98]
Comparison: py/objtype: Fix attr delegation in subclasses of native types. [merge of d027332]
  mpy-cross:   +96 +0.025% 
   bare-arm:   +64 +0.114% 
minimal x86:  +115 +0.061% 
   unix x64:   +96 +0.011% standard
      stm32:   +56 +0.014% PYBV10
      esp32:   +64 +0.004% ESP32_GENERIC
     mimxrt:   +56 +0.014% TEENSY40
        rp2:   +64 +0.007% RPI_PICO_W
       samd:   +56 +0.020% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:   +64 +0.014% VIRT_RV32

It would be nice to to have additional tests without a .exp file to show that this also works like CPython.

It would be nice to to have additional tests without a .exp file to show that this also works like CPython.

Yes. but the problem is that i need a cpython built in with an attr handler for that. I could extend a test with something like this:

class Base:
    def __init__(self):
        self._value = 0
    
    @property
    def value(self):
        return self._value
    
    @value.setter
    def value(self, val):
        # This setter should be called even in subclasses
        if not isinstance(val, int):
            raise TypeError("value must be int")
        self._value = val

class Sub(Base):
    pass

Will then use this and more or less test hat the properties of the subclass work as expected. What do you think?

Hmm... this is a bit tricky. That isn't a valid test since the base class is not implement in C in MicroPython.

I looked at all of the builtin types in MicroPython that have an attrs slot that allows setting an attribute that also exist in CPython. The only one I came up with was Exception that has a special case to allows setting __traceback__ to None. Maybe there would be some way to make a test out of that?

Hmm... this is a bit tricky. That isn't a valid test since the base class is not implement in C in MicroPython.

I looked at all of the builtin types in MicroPython that have an attrs slot that allows setting an attribute that also exist in CPython. The only one I came up with was Exception that has a special case to allows setting __traceback__ to None. Maybe there would be some way to make a test out of that?

Yes, I thought custom classes will not be the right approach.
I tried to follow your suggestion, but sadly the __traceback__ path is solely a store path and I cannot read the attribute back or its consequences in a test (or at least I didn't found a way).

A possibility would be to extend the objexcept.c so that it raises a TypeError if we try to set the __traceback__ not to None (would be more c-python compatible). In this case I could test this path. I did this locally and the test passed and this PR and c-python produced the same output.

At the moment I just added some additional tests. They don't bring more coverage, and IMO could be deleted, but I left them here for discussion at the moment.

Hi everyone,

I wanted to politely follow up on this PR. It's been a few weeks since I submitted this fix for the native attribute handler issue in subclasses.

I'd be happy to address any feedback or make adjustments if needed. Could someone let me know if there are any concerns with the approach, or if additional tests/documentation would be helpful?

Thanks for taking the time to review this, and I appreciate your work on MicroPython!

Best regards,
Chris

Thanks @cnadler86 for the PR.

I think MicroPython already implements the correct behaviour here, namely that stores to an instance go to the top-level instance, and are not delegated to base classes.

Loads are similar in that the top-level instance is searched first. Only if that fails does the search continue in the base classes.

Since a store will always succeed at the top-level, there's no need to go to the base class for a store.

Your original bug report was about inheriting from uctypes.struct not working as expected. But, I don't think there's a good expectation of what should happen there. Probably the existing behaviour is correct (per CPython semantics) albeit unexpected.

So I'd be inclined to close this PR.

Hi @dpgeorge,
I probably should test this with the newest master, but here is an explanation of the problem:

I have on c-level the following property definition (only as example

// Property handler
static void camera_obj_property(mp_obj_t self_in, qstr attr, mp_obj_t *dest) {
    mp_camera_obj_t *self = MP_OBJ_TO_PTR(self_in);
    if (dest[0] == MP_OBJ_NULL) {
        // Load (reading)
        switch (attr) {
            case MP_QSTR_frame_size:
                dest[0] = MP_OBJ_NEW_SMALL_INT(mp_camera_hal_get_frame_size(self));
                break;
            default:
                // Delegate to locals_dict
                dest[1] = MP_OBJ_SENTINEL;
                return;
        }
    } else if (dest[0] == MP_OBJ_SENTINEL && dest[1] != MP_OBJ_NULL) {
        // Store (writing)
        switch (attr) {
            case MP_QSTR_frame_size:
                mp_camera_hal_set_frame_size(self, mp_obj_get_int(dest[1]));
                break;
            default:
                // Delegate to locals_dict for methods not handled here
                return;
        }
        // Success - indicate attribute was stored
        dest[0] = MP_OBJ_NULL;
    }

...

MP_DEFINE_CONST_OBJ_TYPE(
    camera_type,
    MP_QSTR_Camera,
    MP_TYPE_FLAG_NONE,
    make_new, mp_camera_make_new,
    print, mp_camera_hal_print,
    attr, camera_obj_property,
    locals_dict, &camera_camera_locals_dict
);

Then, in python I subclass from my C-Level class (e.g. in modul acamera.py):

from camera import Camera as _Camera
class Camera(_Camera):

Then, if I instantiate a Camera object from the acamera module, the properties inherited from _Camera are not settable. I can get get them, but not use the c-level setter. Instead, they are stored in the __dict__ of the upper level instance.

Of course, there is always a work around for this, but I found the behaviour as you mention unexpected. Not sure if c-python would handle such a situation the same way. I just used uctypes as an example since this built-in object had the needed low level setter.

Thanks for the code snippet. That definitely helps to explain the issue.

After some testing I did find a way to exhibit this behaviour in CPython. Here's the code:

import socket
class MySocket(socket.socket):
    pass
s = MySocket()
print(s.__dict__)
s.foo = 123
print(s.__dict__)
print(s.proto)
s.proto = 456

That gives AttributeError: readonly attribute (for storing to proto).

This CPython tests shows that CPython does indeed delegate stores first to the underlying native object. Only if there's no such attribute does it then create a new entry in the locals __dict__ of the Python subclass. If the attribute exists (eg proto) then it defers the store to it, even if it's read-only.

Using the same test but storing s.type = 456 leads to AttributeError: property 'type' of 'MySocket' object has no setter. So indeed it defers fully to the attribute.

Still... adding this to MicroPython will add code size, and more importantly give a performance hit to stores for all user classes. Need to consider if it's worth it.

This PR now needs #19345 to land in the master branch in order to have green tests.

I changed the test path to have no PR dependencies.