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

class __setattr__ #2375

Closed
dmazzella opened this issue Aug 30, 2016 · 26 comments
Closed

class __setattr__ #2375

dmazzella opened this issue Aug 30, 2016 · 26 comments

Comments

@dmazzella
Copy link
Contributor

Hi all,
i have this:

def test():

    class B(object):
        def __init__(self):
            print("__init__")
        def __getattr__(self, name):
            print("__getattr__")
            object.__getattr__(self, name)
        def __setattr__(self, name, value):
            print("__setattr__")
            object.__setattr__(self, name, value)
        def __delattr__(self, name):
            print("__delattr__")
            object.__delattr__(self, name, value)

    b = B()
    b.test = "test"
    print(b.test)

if __name__ == "__main__":
    test()

on cPy the execution result is:

__init__
__setattr__
test

on uPy:

__init__
test

How i can call setattr?

Thanks.

@dpgeorge
Copy link
Member

The __setattr__ special method is not implemented (nor is __delattr__, but __getattr__ is implemented).

@dmazzella
Copy link
Contributor Author

There are plans to implement this?

@dpgeorge
Copy link
Member

No, there are no plans, we've not had a request for it before. Implementing it would really give a performance hit because it requires doing a load (to check for __setattr__) before all stores (it can be optimised to cache the load but at the expense of RAM).

What's the use-case?

@dmazzella
Copy link
Contributor Author

I'm working on porting a project from cPy (asn1 generator/parser) in uPy.

This work in declarative mode for generator and i need to implement__setattr___ and __delattr__:

cert_bag = asn1.SEQUENCE[
        asn1.OBJECT_IDENTIFIER( encode_oid(oid_X509Certificate)),
        asn1.TAG0[ asn1.OCTET_STRING( cert ) ]
    ]

@dmazzella
Copy link
Contributor Author

any hint to implement it?

@dpgeorge
Copy link
Member

The following will implement set and del. But your test case won't fully work because the object type doesn't support these special methods.

diff --git a/py/objtype.c b/py/objtype.c
index 907308a..77cfd76 100644
--- a/py/objtype.c
+++ b/py/objtype.c
@@ -627,10 +627,37 @@ STATIC bool mp_obj_instance_store_attr(mp_obj_t self_in, qstr attr, mp_obj_t val

     if (value == MP_OBJ_NULL) {
         // delete attribute
+
+        // try __delattr__ first
+        if (attr != MP_QSTR___delattr__) {
+            mp_obj_t dest2[3];
+            mp_load_method_maybe(self_in, MP_QSTR___delattr__, dest2);
+            if (dest2[0] != MP_OBJ_NULL) {
+                // __delattr__ exists, call it and return its result
+                dest2[2] = MP_OBJ_NEW_QSTR(attr);
+                mp_call_method_n_kw(1, 0, dest2);
+                return true;
+            }
+        }
+
         mp_map_elem_t *elem = mp_map_lookup(&self->members, MP_OBJ_NEW_QSTR(attr), MP_MAP_LOOKUP_REMOVE_IF_FOUND);
         return elem != NULL;
     } else {
         // store attribute
+
+        // try __setattr__ first
+        if (attr != MP_QSTR___setattr__) {
+            mp_obj_t dest2[4];
+            mp_load_method_maybe(self_in, MP_QSTR___setattr__, dest2);
+            if (dest2[0] != MP_OBJ_NULL) {
+                // __setattr__ exists, call it and return its result
+                dest2[2] = MP_OBJ_NEW_QSTR(attr);
+                dest2[3] = value;
+                mp_call_method_n_kw(2, 0, dest2);
+                return true;
+            }
+        }
+
         mp_map_lookup(&self->members, MP_OBJ_NEW_QSTR(attr), MP_MAP_LOOKUP_ADD_IF_NOT_FOUND)->value = value;
         return true;
     }

@dmazzella
Copy link
Contributor Author

Thanks, the following (with your patch) will implement get, set and del support these special methods into object type for fully work in my test case.


diff C3 C:/Users/Damiano/OneDrive/micropython/micropython/py/objobject.c C:/msys64_micropython/home/dma/projects/micropython/py/objobject.c
*** C:/Users/Damiano/OneDrive/micropython/micropython/py/objobject.c    Sat Apr 23 16:46:21 2016
--- C:/msys64_micropython/home/dma/projects/micropython/py/objobject.c  Wed Aug 31 10:28:14 2016
***************
*** 60,71 ****
--- 60,108 ----
  STATIC MP_DEFINE_CONST_FUN_OBJ_1(object___new___fun_obj, object___new__);
  STATIC MP_DEFINE_CONST_STATICMETHOD_OBJ(object___new___obj, MP_ROM_PTR(&object___new___fun_obj));

+ STATIC mp_obj_t object___setattr__(mp_obj_t self_in, mp_obj_t attr_in, mp_obj_t value) {
+     qstr attr = mp_obj_str_get_qstr(attr_in);
+     mp_obj_type_t *type = mp_obj_get_type(self_in);
+     mp_map_lookup(&type->locals_dict->map, MP_OBJ_NEW_QSTR(attr), MP_MAP_LOOKUP_ADD_IF_NOT_FOUND)->value = value;
+     return mp_const_none;
+ }
+ 
+ STATIC MP_DEFINE_CONST_FUN_OBJ_3(object___setattr___fun_obj, object___setattr__);
+ STATIC MP_DEFINE_CONST_STATICMETHOD_OBJ(object___setattr___obj, MP_ROM_PTR(&object___setattr___fun_obj));
+ 
+ STATIC mp_obj_t object___getattr__(mp_obj_t self_in, mp_obj_t attr_in) {
+     qstr attr = mp_obj_str_get_qstr(attr_in);
+     mp_obj_type_t *type = mp_obj_get_type(self_in);
+     return mp_map_lookup(&type->locals_dict->map, MP_OBJ_NEW_QSTR(attr), MP_MAP_LOOKUP);
+ }
+ 
+ STATIC MP_DEFINE_CONST_FUN_OBJ_2(object___getattr___fun_obj, object___getattr__);
+ STATIC MP_DEFINE_CONST_STATICMETHOD_OBJ(object___getattr___obj, MP_ROM_PTR(&object___getattr___fun_obj));
+ 
+ STATIC mp_obj_t object___delattr__(mp_obj_t self_in, mp_obj_t attr_in) {
+     qstr attr = mp_obj_str_get_qstr(attr_in);
+     mp_obj_type_t *type = mp_obj_get_type(self_in);
+     mp_map_lookup(&type->locals_dict->map, MP_OBJ_NEW_QSTR(attr), MP_MAP_LOOKUP_REMOVE_IF_FOUND);
+     return mp_const_none;
+ }
+ 
+ STATIC MP_DEFINE_CONST_FUN_OBJ_2(object___delattr___fun_obj, object___delattr__);
+ STATIC MP_DEFINE_CONST_STATICMETHOD_OBJ(object___delattr___obj, MP_ROM_PTR(&object___delattr___fun_obj));
+ 
  STATIC const mp_rom_map_elem_t object_locals_dict_table[] = {
      #if MICROPY_CPYTHON_COMPAT
      { MP_ROM_QSTR(MP_QSTR___init__), MP_ROM_PTR(&object___init___obj) },
      #endif
      #if MICROPY_CPYTHON_COMPAT
      { MP_ROM_QSTR(MP_QSTR___new__), MP_ROM_PTR(&object___new___obj) },
+     #endif
+     #if MICROPY_CPYTHON_COMPAT
+     { MP_ROM_QSTR(MP_QSTR___delattr__), MP_ROM_PTR(&object___delattr___obj) },
+     #endif
+     #if MICROPY_CPYTHON_COMPAT
+     { MP_ROM_QSTR(MP_QSTR___getattr__), MP_ROM_PTR(&object___getattr___obj) },
+     #endif
+     #if MICROPY_CPYTHON_COMPAT
+     { MP_ROM_QSTR(MP_QSTR___setattr__), MP_ROM_PTR(&object___setattr___obj) },
      #endif
  };

now my test case:

def test():

    class B(object):
        def __init__(self):
            print("__init__")

        def __getattr__(self, name):
            print("__getattr__", name)
            return object.__getattr__(self, name)

        def __setattr__(self, name, value):
            print("__setattr__", name, value)
            object.__setattr__(self, name, value)

        def __delattr__(self, name):
            print("__delattr__", name)
            object.__delattr__(self, name)

    b = B()
    b.a = "c"
    print(b.a)
    del b.a

    try:
        print(b.a)
    except AttributeError:
        print("AttributeError")

if __name__ == "__main__":
    test()

produce the same result on cPy and uPy:

__init__
__setattr__ a c
c
__delattr__ a
__getattr__ a
AttributeError

Thanks for your support.
D.

@dmazzella
Copy link
Contributor Author

Attached .path file to enable getattr, delattr and setattr

obj_attrs.zip

test:

def test():

    class B(object):
        def __init__(self):
            print("__init__")

        def __getattr__(self, name):
            print("__getattr__", name)
            return super(B, self).__getattr__(name)

        def __setattr__(self, name, value):
            print("__setattr__", name, value)
            super(B, self).__setattr__(name, value)

        def __delattr__(self, name):
            print("__delattr__", name)
            super(B, self).__delattr__(name)

    b = B()
    b.a = "c"
    print(b.a)
    del b.a

    try:
        print(b.a)
    except AttributeError:
        print("AttributeError")

if __name__ == "__main__":
    test()

@stinos
Copy link
Contributor

stinos commented Dec 5, 2016

Thanks, tried this with https://github.com/mavier/jsobject/blob/master/jsobject/jsobject.py and that works ok

@dmazzella
Copy link
Contributor Author

@stinos thanks your for test it ;)

@dpgeorge any chance to see this merged? suggest for improve?

@dpgeorge
Copy link
Member

@dpgeorge any chance to see this merged? suggest for improve?

Let's first see if other people find they need/use it. As I said above it gives a big performance hit so should only be used if you really need such a feature.

@pfalcon
Copy link
Contributor

pfalcon commented Dec 15, 2016

... And so should be submitted as a proper pull request, likely will fail CI checks, and then can go (slowly) from there.

@stinos
Copy link
Contributor

stinos commented Dec 15, 2016

Shouldn't all changes be guarded by #if MICROPY_CPYTHON_COMPAT, instead of just the table?

@peterhinch
Copy link
Contributor

I suspect it's seldom used. I'd be loath to take a performance hit to stores which are heavily used.

@dmazzella
Copy link
Contributor Author

I can add a new macro into py/mpconfig.h like:

// Whether to support object (__getattr__, __delattr__ and __setattr__)
// This costs some code size and makes all load attrs and store attrs slow
#ifndef MICROPY_PY_ATTRS_METHODS
#define MICROPY_PY_ATTRS_METHODS (0)
#endif

enable it into specific ports if needed and guard like

#if MICROPY_PY_ATTRS_METHODS

@dmazzella
Copy link
Contributor Author

@pfalcon

... And so should be submitted as a proper pull request, likely will fail CI checks, and then can go (slowly) from there.

done

@rlcase
Copy link

rlcase commented Feb 23, 2019

The use case where this really helps is in an embedded system where we use uctypes to access hardware. With settings registers you can't just say
self.register.bitfield = number
and cause a serial bus access to push the change to the hardware. The line above always needs to be followed with an I2C writeto_mem call to write to the hardware. I would be willing to take the time hit and do a special build of uP for this as it really simplifies code for performing serial bus device register manipulation. Implementing the setattr allows us to have hooks for performance by implementing caching on top uctypes. My only alternative is to modify the uctypes module and add hooks so that I can register callback methods when a write is performed. This is not ideal as it requires tracking any changes to uctypes module. Thanks for reading.

@stinos
Copy link
Contributor

stinos commented Feb 23, 2019

Fair enough, but just using self.register.SetBitField(number) doesn't seem like that much of a deal-breaker?

@rlcase
Copy link

rlcase commented Feb 23, 2019

Agreed. So a wrapper class that implements getattr would intercept the field name, perform a lookup in the uctypes dictionary to see if the field name is legit, return another class that implements the SetBitfield method. This class would intercept the value write it to the uctypes field with the normal '=' operation (I need to test if this works) and then perform the serial bus write of the byte array that uctypes manages.

Having setattr would eliminate the need for the second class. Only one class that decorates the field access with a bus write needs to be used. The simple assignment operator is all that is needed.

This is all doable in C++ with a proxy class returned in the assignment operator that determines if the access is on the right or left hand side of the equal sign.

@imgcre
Copy link

imgcre commented Mar 5, 2019

Agreed. So a wrapper class that implements getattr would intercept the field name, perform a lookup in the uctypes dictionary to see if the field name is legit, return another class that implements the SetBitfield method. This class would intercept the value write it to the uctypes field with the normal '=' operation (I need to test if this works) and then perform the serial bus write of the byte array that uctypes manages.

Having setattr would eliminate the need for the second class. Only one class that decorates the field access with a bus write needs to be used. The simple assignment operator is all that is needed.

This is all doable in C++ with a proxy class returned in the assignment operator that determines if the access is on the right or left hand side of the equal sign.

I just came up with the same idea recently!

@pmp-p
Copy link
Contributor

pmp-p commented Jun 22, 2019

That should be kept open, since there's code available provided here untill someone proposes a properly formatted PR.

Though they should be kept optionnal for size/perf __setattr__ is vital for RPC systems and __delattr__ for cleaning up C/C++ objects properly when integrating libraries.

unless MICROPY_PY_DELATTR_SETATTR (1) is solving that issue.
edit/ seems it can.

@stinos
Copy link
Contributor

stinos commented Jun 23, 2019

__delattr__ is vital for cleaning up C/C++ objects properly when integrating libraries.

Can you explain why?

@dpgeorge
Copy link
Member

Note that __delattr__ and __setattr_ were implemented in PR #2753, commit 18e6569

@pmp-p
Copy link
Contributor

pmp-p commented Sep 11, 2019

Can you explain why?

@stinos , i can but i won't

and for further and complete documentation there's still a blocker for proper C/C++ ffi/rpc integration #1878

tannewt added a commit to tannewt/circuitpython that referenced this issue Dec 12, 2019
@rtwfroody
Copy link

rtwfroody commented Nov 6, 2022

I spent several hours discovering my code didn't work because circuitpython doesn't implement __setattr__. It would be nice if it displayed a warning when parsing python code that defines a function with that name.

@jimmo
Copy link
Member

jimmo commented Nov 7, 2022

I spent several hours discovering my code didn't work because circuitpython doesn't implement __setattr__. It would be nice if it displayed a warning when parsing python code that defines a function with that name.

@rtwfroody See #2375 (comment) above -- MicroPython has supported __setattr__ since 2019, and this is enabled in MicroPython firmware by default on most boards (except for ones that have limited flash space).

You're right though, CircuitPython does not appear to enable this option by default on any boards. But you'll need to raise an issue for CircuitPython at https://github.com/adafruit/circuitpython/issues/new/choose

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants