From 3173fa1aedfc6fd809c2e045f6de6aa52ef1cc16 Mon Sep 17 00:00:00 2001 From: Anson Mansfield Date: Fri, 19 Jul 2024 13:01:09 -0400 Subject: [PATCH 1/5] tests/basics/class_descriptor: Test for __set_name__. Signed-off-by: Anson Mansfield --- tests/basics/class_descriptor.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/basics/class_descriptor.py b/tests/basics/class_descriptor.py index 83d31674301d5..feaed2fbb2a43 100644 --- a/tests/basics/class_descriptor.py +++ b/tests/basics/class_descriptor.py @@ -1,22 +1,28 @@ class Descriptor: def __get__(self, obj, cls): - print('get') + print("get") print(type(obj) is Main) print(cls is Main) - return 'result' + return "result" def __set__(self, obj, val): - print('set') + print("set") print(type(obj) is Main) print(val) def __delete__(self, obj): - print('delete') + print("delete") print(type(obj) is Main) + def __set_name__(self, owner, name): + print("set_name", name) + print(owner.__name__ == "Main") + + class Main: Forward = Descriptor() + m = Main() try: m.__class__ @@ -26,15 +32,15 @@ class Main: raise SystemExit r = m.Forward -if 'Descriptor' in repr(r.__class__): +if "Descriptor" in repr(r.__class__): # Target doesn't support descriptors. - print('SKIP') + print("SKIP") raise SystemExit # Test assignment and deletion. print(r) -m.Forward = 'a' +m.Forward = "a" del m.Forward # Test that lookup of descriptors like __get__ are not passed into __getattr__. From 5da9c94d857a4a59fbe2e8c42fec5e5f50566106 Mon Sep 17 00:00:00 2001 From: Anson Mansfield Date: Fri, 19 Jul 2024 18:23:45 -0400 Subject: [PATCH 2/5] py/objtype: Implement __set_name__. Signed-off-by: Anson Mansfield --- py/objtype.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/py/objtype.c b/py/objtype.c index b9af1008995ef..876bd75ad0fd2 100644 --- a/py/objtype.c +++ b/py/objtype.c @@ -661,8 +661,8 @@ static void mp_obj_instance_load_attr(mp_obj_t self_in, qstr attr, mp_obj_t *des // try __getattr__ if (attr != MP_QSTR___getattr__) { #if MICROPY_PY_DESCRIPTORS - // With descriptors enabled, don't delegate lookups of __get__/__set__/__delete__. - if (attr == MP_QSTR___get__ || attr == MP_QSTR___set__ || attr == MP_QSTR___delete__) { + // With descriptors enabled, don't delegate lookups of __get__/__set__/__delete__/__set_name__. + if (attr == MP_QSTR___get__ || attr == MP_QSTR___set__ || attr == MP_QSTR___delete__ || attr == MP_QSTR___set_name__) { return; } #endif @@ -960,7 +960,7 @@ static bool check_for_special_accessors(mp_obj_t key, mp_obj_t value) { #endif #if MICROPY_PY_DESCRIPTORS static const uint8_t to_check[] = { - MP_QSTR___get__, MP_QSTR___set__, MP_QSTR___delete__, + MP_QSTR___get__, MP_QSTR___set__, MP_QSTR___delete__, // not needed for MP_QSTR___set_name__ tho }; for (size_t i = 0; i < MP_ARRAY_SIZE(to_check); ++i) { mp_obj_t dest_temp[2]; @@ -1241,6 +1241,23 @@ mp_obj_t mp_obj_new_type(qstr name, mp_obj_t bases_tuple, mp_obj_t locals_dict) } } + #if MICROPY_PY_DESCRIPTORS + // call __set_name__ on all entries (especially descriptors) + for (size_t i = 0; i < locals_map->alloc; i++) { + if (mp_map_slot_is_filled(locals_map, i)) { + elem = &(locals_map->table[i]); + + mp_obj_t set_name_method[4]; + mp_load_method_maybe(elem->value, MP_QSTR___set_name__, set_name_method); + if (set_name_method[1] != MP_OBJ_NULL) { + set_name_method[2] = MP_OBJ_FROM_PTR(o); + set_name_method[3] = elem->key; + mp_call_method_n_kw(2, 0, set_name_method); + } + } + } + #endif + return MP_OBJ_FROM_PTR(o); } From 76718bab99eaa4b5a79d4c359ebf4bf09557b596 Mon Sep 17 00:00:00 2001 From: Anson Mansfield Date: Sat, 20 Jul 2024 07:52:49 -0400 Subject: [PATCH 3/5] py/mpconfig: Add __set_name__ to feature flag description. Signed-off-by: Anson Mansfield --- py/mpconfig.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py/mpconfig.h b/py/mpconfig.h index 4c1276275964d..e84a1f9c050b3 100644 --- a/py/mpconfig.h +++ b/py/mpconfig.h @@ -1124,7 +1124,7 @@ typedef time_t mp_timestamp_t; #define MICROPY_PY_FUNCTION_ATTRS_CODE (MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_FULL_FEATURES) #endif -// Whether to support the descriptors __get__, __set__, __delete__ +// Whether to support the descriptors __get__, __set__, __delete__, __set_name__ // This costs some code size and makes load/store/delete of instance // attributes slower for the classes that use this feature #ifndef MICROPY_PY_DESCRIPTORS From 3ae416ccc74cda8fbfb3d242da8c41692d0c4843 Mon Sep 17 00:00:00 2001 From: Anson Mansfield Date: Sat, 22 Feb 2025 15:54:22 -0500 Subject: [PATCH 4/5] tests/basics/class_setname_hazard: Test for __set_name__ hazard. Signed-off-by: Anson Mansfield --- tests/basics/class_setname_hazard.py | 179 ++++++++++++++++++++++ tests/basics/class_setname_hazard_rand.py | 130 ++++++++++++++++ 2 files changed, 309 insertions(+) create mode 100644 tests/basics/class_setname_hazard.py create mode 100644 tests/basics/class_setname_hazard_rand.py diff --git a/tests/basics/class_setname_hazard.py b/tests/basics/class_setname_hazard.py new file mode 100644 index 0000000000000..dad4089a66610 --- /dev/null +++ b/tests/basics/class_setname_hazard.py @@ -0,0 +1,179 @@ +def skip_if_no_descriptors(): + class Descriptor: + def __get__(self, obj, cls): + return + + class TestClass: + Forward = Descriptor() + + a = TestClass() + try: + a.__class__ + except AttributeError: + # Target doesn't support __class__. + print("SKIP") + raise SystemExit + + b = a.Forward + if "Descriptor" in repr(b.__class__): + # Target doesn't support descriptors. + print("SKIP") + raise SystemExit + + +skip_if_no_descriptors() + + +# Test basic hazard-free mutations of the enclosing class. + + +class GetSibling: + def __set_name__(self, owner, name): + print(getattr(owner, name + "_sib")) + + +class GetSiblingTest: + desc = GetSibling() + desc_sib = 111 + + +t110 = GetSiblingTest() + + +class SetSibling: + def __set_name__(self, owner, name): + setattr(owner, name + "_sib", 121) + + +class SetSiblingTest: + desc = SetSibling() + + +t120 = SetSiblingTest() + +print(t120.desc_sib) + + +class DelSibling: + def __set_name__(self, owner, name): + delattr(owner, name + "_sib") + + +class DelSiblingTest: + desc = DelSibling() + desc_sib = 131 + + +t130 = DelSiblingTest() + +try: + print(t130.desc_sib) +except AttributeError: + print("AttributeError") + + +class GetSelf: + x = 211 + + def __set_name__(self, owner, name): + print(getattr(owner, name).x) + + +class GetSelfTest: + desc = GetSelf() + + +t210 = GetSelfTest() + + +class SetSelf: + def __set_name__(self, owner, name): + setattr(owner, name, 221) + + +class SetSelfTest: + desc = SetSelf() + + +t220 = SetSelfTest() + +print(t220.desc) + + +class DelSelf: + def __set_name__(self, owner, name): + delattr(owner, name) + + +class DelSelfTest: + desc = DelSelf() + + +t230 = DelSelfTest() + +try: + print(t230.desc) +except AttributeError: + print("AttributeError") + + +# Test exception behavior + + +class Raise: + def __set_name__(self, owner, name): + raise Exception() + + +try: + + class RaiseTest: + desc = Raise() +except Exception as e: + print("Exception") + + +# Test simple hazards: whether other class attributes still get __set_name__ even if removed before being run + + +class SetSpecific: + def __init__(self, sib_name, sib_replace): + self.sib_name = sib_name + self.sib_replace = sib_replace + + def __set_name__(self, owner, name): + setattr(owner, self.sib_name, self.sib_replace) + + +class SetReplaceTest: + a = SetSpecific("b", 312) # one of these is changed first + b = SetSpecific("a", 311) + + +t310 = SetReplaceTest() +print(t310.a) +print(t310.b) + + +class DelSpecific: + def __init__(self, sib_name): + self.sib_name = sib_name + + def __set_name__(self, owner, name): + delattr(owner, self.sib_name) + + +class DelReplaceTest: + a = DelSpecific("b") # one of these is removed first + b = DelSpecific("a") + + +t320 = DelReplaceTest() +try: + print(t320.a) +except AttributeError: + print("AttributeError") +try: + print(t320.b) +except AttributeError: + print("AttributeError") diff --git a/tests/basics/class_setname_hazard_rand.py b/tests/basics/class_setname_hazard_rand.py new file mode 100644 index 0000000000000..10270285dc5a4 --- /dev/null +++ b/tests/basics/class_setname_hazard_rand.py @@ -0,0 +1,130 @@ +# Test to make sure there's no sequence hazard even when a __set_name__ implementation +# mutates and reorders the class namespace. +# VERY hard bug to prove out except via a stochastic test. + + +def skip_if_no_descriptors(): + class Descriptor: + def __get__(self, obj, cls): + return + + class TestClass: + Forward = Descriptor() + + a = TestClass() + try: + a.__class__ + except AttributeError: + # Target doesn't support __class__. + print("SKIP") + raise SystemExit + + b = a.Forward + if "Descriptor" in repr(b.__class__): + # Target doesn't support descriptors. + print("SKIP") + raise SystemExit + + +def skip_if_no_libs(): + try: + import random + except ImportError: + # Target doesn't have a random library + print("SKIP") + raise SystemExit + + try: + random.choice + except AttributeError: + # Target doesn't have an ACTUAL random library + print("SKIP") + raise SystemExit + + try: + import re + except ImportError: + # Target doesn't have a regex library + print("SKIP") + raise SystemExit + + +skip_if_no_descriptors() +skip_if_no_libs() + + +import random +import re + +letters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + +junk_re = re.compile(r"[A-Z][A-Z][A-Z][A-Z][A-Z]") + + +def junk_fill(obj, n=10): # Add randomly-generated attributes to an object. + for i in range(n): + name = "".join(random.choice(letters) for j in range(5)) + setattr(obj, name, object()) + + +def junk_clear(obj): # Remove attributes added by junk_fill. + to_del = [name for name in dir(obj) if junk_re.match(name)] + for name in to_del: + delattr(obj, name) + + +def junk_sequencer(): + global runs + try: + while True: + owner, name = yield + runs[name] = runs.get(name, 0) + 1 + junk_fill(owner) + finally: + junk_clear(owner) + + +class JunkMaker: + def __set_name__(self, owner, name): + global seq + seq.send((owner, name)) + + +runs = {} +seq = junk_sequencer() +next(seq) + + +class Main: + a = JunkMaker() + b = JunkMaker() + c = JunkMaker() + d = JunkMaker() + e = JunkMaker() + f = JunkMaker() + g = JunkMaker() + h = JunkMaker() + i = JunkMaker() + j = JunkMaker() + k = JunkMaker() + l = JunkMaker() + m = JunkMaker() + n = JunkMaker() + o = JunkMaker() + p = JunkMaker() + q = JunkMaker() + r = JunkMaker() + s = JunkMaker() + t = JunkMaker() + u = JunkMaker() + v = JunkMaker() + w = JunkMaker() + x = JunkMaker() + y = JunkMaker() + z = JunkMaker() + + +seq.close() + +for k in letters.lower(): + print(k, runs.get(k, 0)) From 87cb9a08809449e587038dde1b2d9babc190e32b Mon Sep 17 00:00:00 2001 From: Anson Mansfield Date: Mon, 24 Feb 2025 16:05:44 -0500 Subject: [PATCH 5/5] py/objtype: Eliminate __set_name__ hazard. Signed-off-by: Anson Mansfield --- py/objtype.c | 57 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/py/objtype.c b/py/objtype.c index 876bd75ad0fd2..c86aa53222ed1 100644 --- a/py/objtype.c +++ b/py/objtype.c @@ -974,6 +974,48 @@ static bool check_for_special_accessors(mp_obj_t key, mp_obj_t value) { } #endif +#if MICROPY_PY_DESCRIPTORS +static void run_set_name_hooks(mp_map_t *locals_map_orig, mp_obj_t owner) { + // copy the dict so we can iterate safely even while __set_name__ potentially modifies the original + mp_map_t locals_map = *locals_map_orig; + locals_map.table = mp_local_alloc(locals_map.alloc * sizeof(mp_map_elem_t)); + memcpy(locals_map.table, locals_map_orig->table, locals_map.alloc * sizeof(mp_map_elem_t)); + + #if MICROPY_ENABLE_PYSTACK + nlr_buf_t nlr; + if (nlr_push(&nlr) == 0) + // Note: on !MICROPY_ENABLE_PYSTACK ports, `mp_local_alloc` is just `alloca` and `mp_local_free` is a no-op. + // Therefore we don't need to set an exception trap; the exception handler implicitly frees as it unwinds the stack. + #endif + { + // use the copy to call __set_name__ on each + for (size_t i = 0; i < locals_map.alloc; i++) { + if (mp_map_slot_is_filled(&locals_map, i)) { + mp_map_elem_t *elem = &(locals_map.table[i]); + mp_obj_t set_name_method[4]; + mp_load_method_maybe(elem->value, MP_QSTR___set_name__, set_name_method); + if (set_name_method[1] != MP_OBJ_NULL) { + set_name_method[2] = owner; + set_name_method[3] = elem->key; + mp_call_method_n_kw(2, 0, set_name_method); + } + } + } + + #if MICROPY_ENABLE_PYSTACK + nlr_pop(); + #endif + mp_local_free(locals_map.table); + } + #if MICROPY_ENABLE_PYSTACK + else { + mp_local_free(locals_map.table); + nlr_raise(nlr.ret_val); // TODO cpython raises a RuntimeError in this situation + } + #endif +} +#endif + static void type_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) { (void)kind; mp_obj_type_t *self = MP_OBJ_TO_PTR(self_in); @@ -1242,20 +1284,7 @@ mp_obj_t mp_obj_new_type(qstr name, mp_obj_t bases_tuple, mp_obj_t locals_dict) } #if MICROPY_PY_DESCRIPTORS - // call __set_name__ on all entries (especially descriptors) - for (size_t i = 0; i < locals_map->alloc; i++) { - if (mp_map_slot_is_filled(locals_map, i)) { - elem = &(locals_map->table[i]); - - mp_obj_t set_name_method[4]; - mp_load_method_maybe(elem->value, MP_QSTR___set_name__, set_name_method); - if (set_name_method[1] != MP_OBJ_NULL) { - set_name_method[2] = MP_OBJ_FROM_PTR(o); - set_name_method[3] = elem->key; - mp_call_method_n_kw(2, 0, set_name_method); - } - } - } + run_set_name_hooks(locals_map, MP_OBJ_FROM_PTR(o)); #endif return MP_OBJ_FROM_PTR(o);