Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.2k
py/runtime.c: Add support for using __all__ in star import.#17331
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
88a3d7b tofa6f6c0Comparecodecovbot commentedMay 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## master #17331 +/- ##======================================= Coverage 98.54% 98.54% ======================================= Files 169 169 Lines 21898 21916 +18 =======================================+ Hits 21579 21597 +18 Misses 319 319 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actionsbot commentedMay 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Code size report: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On the other hand, limiting imports to a subset of symbols rather than importing most of them in the parent globals will be reduce RAM usage
If the module containing__all__ has to be loaded in RAM (e.g. it is a .py file), then wouldn't RAM usage actually increase because of the extra list (unless there are significantly more "private" members not starting with "_" than public)?
(Not saying we shouldn't accept this, just want to make sure the details are clear.)
py/mpconfig.h Outdated
| @@ -603,6 +603,11 @@ | |||
| #define MICROPY_ENABLE_EXTERNAL_IMPORT (MICROPY_CONFIG_ROM_LEVEL_AT_LEAST_CORE_FEATURES) | |||
| #endif | |||
| // Whether to enable parsing __all__ when importing all public symbols from modules | |||
| #ifndef MICROPY_ENABLE_MODULE_ALL | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| #ifndefMICROPY_ENABLE_MODULE_ALL | |
| #ifndefMICROPY_ENABLE_MODULE_IMPORT_ALL |
perhaps a more informative name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed, I have changed it accordingly
py/runtime.c Outdated
| @@ -1247,6 +1247,19 @@ void mp_load_method(mp_obj_t base, qstr attr, mp_obj_t *dest) { | |||
| mp_raise_msg_varg(&mp_type_AttributeError, | |||
| MP_ERROR_TEXT("type object '%q' has no attribute '%q'"), | |||
| ((mp_obj_type_t *)MP_OBJ_TO_PTR(base))->name, attr); | |||
| #if MICROPY_ERROR_REPORTING >= MICROPY_ERROR_REPORTING_DETAILED | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems like it should be in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have included that error handling code in the same pull request because it is clearly specific to the handling of__all__ : invalid imports unrelated to__all__ trigger a separate ImportError exception.
To make this more explicit - and to avoid including this reporting code in caseMICROPY_ENABLE_MODULE_IMPORT_ALL is not enabled, I have added tham symbol in the#if test.
py/runtime.c Outdated
| #if MICROPY_ENABLE_MODULE_ALL | ||
| mp_map_elem_t *elem = mp_map_lookup(map, MP_OBJ_NEW_QSTR(MP_QSTR___all__), MP_MAP_LOOKUP); | ||
| if (elem != NULL && mp_obj_is_type(elem->value, &mp_type_list)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would it be OK to just let it throw an exception later if the type is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
mp_obj_is_type() was necessary as I was usingmp_obj_list_get() afterwards, which does not perform any type checking. But as I have now switched tomp_obj_get_array() as per your next comment, this check can indeed be removed,
py/runtime.c Outdated
| // symbols, possibly invoking the module __getattr__ function | ||
| size_t len; | ||
| mp_obj_t *items; | ||
| mp_obj_list_get(elem->value, &len, &items); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| mp_obj_list_get(elem->value,&len,&items); | |
| mp_obj_get_array(elem->value,&len,&items); |
Could be a bit more flexible to allow tuple as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are absolutely right. This is actually CPython behaviour, and tuple have the big advantage of being treated as constants in bytecode and beeing fully frozen in ROM asmp_rom_obj_tuple_t for frozen modules, requiring no RAM storage.
I have force-pushed the new version with suggested changes.
fa6f6c0 to8ddf09cCompare
It will of course depend on the cases. You are correct in saying that for simple modules, default import is likely to work as well. However for more complex modules that use imports, those imported symbols would typically be unnecessarily re-exported in the parent global dictionary by a star import. |
8ddf09c to30ceed9CompareWhen the symbol __all__ is defined in a module, mp_import_allshould import all listed symbols into the global environment,rather than relying on the underscore-is-private default.This is the standard in CPython.Each item is loaded in the same way as if it wouldbe an explicit import statement, and will invokethe module's __getattr__ function if needed. Thisprovides a straightforward solution for fixing starimport of modules using a dynamic loader, such asextmod/asyncio (see issuemicropython#7266).This improvement has been enabled at BASIC_FEATURES level,to avoid impacting devices with limited ressources, forwhich star import is of little use anyway.Additionally, detailled reporting of errors during __all__import has been implemented to match CPython, but this isonly enabled when ERROR_REPORTING is set toMICROPY_ERROR_REPORTING_DETAILED.Signed-off-by: Yoctopuce dev <dev@yoctopuce.com>
30ceed9 toc3527deCompare
Summary
If a module defines the symbol
__all__, that definition should be used when doing a wildcard import: all listed symbols and no others should be added to the parent global environment. This is the standard in CPython.This pull requests fixes a longtimeFIXME in
mp_parse_allto implement this behaviour.Each item is loaded in the same way as if it would be an explicit import statement, including invoking the the module's
__getattr__function if needed. This provides a straightforward solution for fixing star import of modules using a dynamic loader, such asextmod/asyncio(see issue#7266, dated 2022).Testing
The corresponding test cases have been added to
tests/import, including the default behaviour when__all__is not defined.As star imports are not expected to be useful on systems with very limited ressources, this improvement is only enabled for ports at BASIC_FEATURES level. In order to skip the new test cases on builds at lower feature level, the test file checks for the availability of another feature enabled at the same level (next's 2nd argument).
Trade-offs and Alternatives
This improvement will slightly increase the code size, for the sake of CPython compatibility.
On the other hand, limiting imports to a subset of symbols rather than importing most of them in the parent globals will be reduce RAM usage. The feature also fixes a longstanding issue with dynamic loading of modules, which is another method used to reduce RAM usage.