Skip to content

py/modbuiltins: Keep class locals ordered as per their definition. #6130

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.

Sign up for GitHub

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

Jump to bottom
Open
andrewleech wants to merge 2 commits into
base: master
Choose a base branch
from
Open

py/modbuiltins: Keep class locals ordered as per their definition. #6130

andrewleech wants to merge 2 commits into from
+34 −1

Conversation

@andrewleech
Copy link
Contributor

@andrewleech andrewleech commented Jun 9, 2020

When MICROPY_PY_CLASS_ORDERED_LOCALS build/board define is set this will keep all class attributes ordered as per their python definition.

As discussed in #5322 (comment)

@andrewleech
Copy link
Contributor Author

andrewleech commented Jun 9, 2020

I actually don't know how to test this without #5324 so perhaps once it's cleaned up this will be ready for some tests

@andrewleech andrewleech force-pushed the ordered_class_locals branch from 3dd07c6 to 888129c Compare June 10, 2020 03:18
@andrewleech
Copy link
Contributor Author

andrewleech commented Jun 10, 2020

I've just added a second commit that allows controlling the attribute order when creating a new class dynamically with type()

@andrewleech andrewleech force-pushed the ordered_class_locals branch from 888129c to 67e98e3 Compare June 10, 2020 20:58
@andrewleech
Copy link
Contributor Author

andrewleech commented Jun 10, 2020

Thanks for accepting my class.__dict__ PR @dpgeorge, I've now been able to add a unit test for this feature using that.

I've enabled the included feature in the coverage build to run the test, let me know if this isn't the best / most appropriate way to add a coverage-only unit test.

@andrewleech andrewleech force-pushed the ordered_class_locals branch from 67e98e3 to aaa3746 Compare June 14, 2020 07:52
@dpgeorge
Copy link
Member

dpgeorge commented Jun 17, 2020

I've just added a second commit that allows controlling the attribute order when creating a new class dynamically with type()

This is good, it allows to selectively create classes with ordered dicts for the locals. I would prefer to just merge that and not the commit that adds the MICROPY_PY_CLASS_ORDERED_LOCALS option, because doing it dynamically with type() is then available to everyone without making a custom build.

Would it work for your use-case to just use type()?

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Jun 17, 2020
dpgeorge
dpgeorge reviewed Jun 17, 2020
View reviewed changes
py/objtype.c Outdated
mp_obj_dict_t *locals = MP_OBJ_TO_PTR(locals_dict);
locals->base.type = &mp_type_dict;
}
#endif
Copy link
Member

@dpgeorge dpgeorge Jun 17, 2020

Choose a reason for hiding this comment

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

Instead of making a copy of the dict, I think it'd be much simpler and more efficient to just replace all checks in this file for locals_dict being an mp_type_dict to test for either mp_type_dict or mp_type_ordereddict. It looks like there are only 3 such checks, 2 of which are just assertions. Probably just add a macro in this file like IS_LOCALS_COMPATIBLE_DICT(locals_dict).

Copy link
Contributor Author

@andrewleech andrewleech Jun 17, 2020

Choose a reason for hiding this comment

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

I initially thought of this, however at a quick search I thought it looked like other parts of the codebase also expect locals to be a mp_type_dict. I'm not sure where I was looking now though, I guess I could try it out and test if it seems to work.

Copy link
Member

@dpgeorge dpgeorge Jun 17, 2020

Choose a reason for hiding this comment

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

I think it'll work... one way to test is to change all dicts that come in here to mp_type_ordereddict and then run the test suite.

Copy link
Member

@dpgeorge dpgeorge Jun 18, 2020

Choose a reason for hiding this comment

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

I tested this out, by forcing locals_dict->base.type = &mp_type_ordereddict in the code here, and all tests pass on unix coverage build. So I'd say go for this approach (I can make a PR for this part, just let me know either way).

Copy link
Member

@dpgeorge dpgeorge Jun 21, 2020

Choose a reason for hiding this comment

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

See #6176

@andrewleech
Copy link
Contributor Author

andrewleech commented Jun 17, 2020

This is good, it allows to selectively create classes with ordered dicts for the locals. I would prefer to just merge that and not the commit that adds the MICROPY_PY_CLASS_ORDERED_LOCALS option, because doing it dynamically with type() is then available to everyone without making a custom build.

Would it work for your use-case to just use type()?

No this only helps in a very small use case of ours. The class attribute ordering is required by structured_config module (https://gitlab.com/alelec/micropython-structured-config#field-documentation) which is somewhat similar the the new dataclasses in cpython (https://docs.python.org/3/library/dataclasses.html) where structures are defined as classes and the order of the attributes is important, along with the syntax of declaring them.
One of the primary features of structured_config is that auto-complete works as they appear as a normal class to editors, this would be lost if I tried to dynamically create them with type(), as well as the clear usage syntax.

The dynamic class ordering is needed for serialise/deserialise feature of structured_config where structures can be sent down the wire as json and re-created in the same order at the other end.

@dpgeorge
Copy link
Member

dpgeorge commented Jun 17, 2020

Ok, thanks for the explanation. It could be that metaclasses would help here (to force the inherited class to have ordered members), but that's out of scope for now.

This was referenced Jun 18, 2020
Open
Merged
stinos
stinos reviewed Dec 4, 2020
View reviewed changes
py/modbuiltins.c
mp_obj_dict_t *old_locals = mp_locals_get();
mp_obj_t class_locals = mp_obj_new_dict(0);
#if MICROPY_PY_CLASS_ORDERED_LOCALS
// keep the locals ordered
Copy link
Contributor

@stinos stinos Dec 4, 2020

Choose a reason for hiding this comment

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

Nitpick, but this looks superfluous: it's in a block 'class ordered locals' and sets the is_ordered flag

@andrewleech andrewleech force-pushed the ordered_class_locals branch from 29b358b to 358bca6 Compare July 13, 2021 00:08
@andrewleech andrewleech force-pushed the ordered_class_locals branch 3 times, most recently from c49e937 to 92795eb Compare July 28, 2021 23:04
@andrewleech andrewleech force-pushed the ordered_class_locals branch from 92795eb to d9f84fd Compare May 12, 2025 05:57
@codecov
Copy link

codecov bot commented May 12, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (27b7bf3) to head (36ce2f4).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #6130   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22297    22299    +2     
=======================================
+ Hits        21936    21938    +2     
  Misses        361      361

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@andrewleech andrewleech force-pushed the ordered_class_locals branch 2 times, most recently from 3dc396c to 2ca633a Compare May 27, 2025 05:55
@andrewleech andrewleech force-pushed the ordered_class_locals branch 4 times, most recently from 425e292 to cab3a1f Compare September 24, 2025 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

+1 more reviewer

@stinos stinos stinos left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

py-core Relates to py/ directory in source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@andrewleech @dpgeorge @stinos @pi-anl