GeezerVison is showing github.com — gh-142029: Raise `ModuleNotFoundError` instead of crashing on nonexsistent module name given to `create_builtin()` by dr-carlos · Pull Request #142054 · python/cpython
Conversation
dr-carlos
commented
Nov 28, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
- Prior to 1e4e59b,
Py_NewRef(Py_None) was returned in this case; so, theoretically, this should be re-introducing behaviour that was previously broken by that commit.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
On further inspection, is_core_module() is only defined under an #ifndef NDEBUG statement (since it was only used in assert() calls), so it's not valid to use in non-debug builds. Would it be acceptable to move this out of the #if?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
On further inspection, is_core_module() is only defined under an #ifndef NDEBUG statement (since it was only used in assert() calls), so it's not valid to use in non-debug builds. Would it be acceptable to move this out of the #if?
No longer relevant for the improved implementation. Thanks to @itamaro.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Tests seem to be failing on Android/iOS because pickled __loader__ is not builtins.__loader__. This is probably because __loader__.create_module calls _imp.create_builtin(), which was modified in this PR.
Unfortunately it's not reproducible on my Linux computer (hence the Linux tests passing fine). Not sure if it's trying to compare the new __loader__ with the old __loader__ from a previous Python version?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 prefer raising an exception as done in #142033 instead of returning None.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dr-carlos reacted with thumbs up emoji
I prefer raising an exception as done in #142033 instead of returning None.
Sounds good. Would a ModuleNotFoundError be most appropriate here?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dr-carlos
changed the title
gh-142029: Return None instead of crashing on nonexsistent module name given to create_builtin()
gh-142029: Raise ModuleNotFoundError instead of crashing on nonexsistent module name given to create_builtin()
Dec 2, 2025
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dr-carlos
commented
Dec 11, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
@vstinner #142033 only fixes one of the two issues included in #142029.
The first issue mentioned in that bug report is still unfixed, i.e. the following will crash on main:
>>> import _imp
>>> class A: pass
...
>>> a = A()
>>> a.name = "123"
>>> _imp.create_builtin(a)
But this would raise a ModuleNotFoundError were this PR merged.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
LGTM. But there are now conflicts in Lib/test/test_import/init.py: can you try to solve them? (git merge main)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
dr-carlos reacted with thumbs up emoji
|
Oh sorry, I didn't see that the issue had two sub-issues.
|
itamaro reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
LGTM. But there are now conflicts in Lib/test/test_import/init.py: can you try to solve them? (git merge main)
Yep, just done so.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Strange, pickle tests failed on iOS and Android:
2 re-run tests:
test_pickle test_pickletools
Example of failure:
ERROR: test_builtin_types (test.test_pickletools.OptimizedPickleTests.test_builtin_types)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/data/user/0/org.python.testbed/files/python/lib/python3.15/test/pickletester.py", line 3217, in test_builtin_types
s = self.dumps(t, proto)
File "/data/user/0/org.python.testbed/files/python/lib/python3.15/test/test_pickletools.py", line 16, in dumps
return pickletools.optimize(pickle.dumps(arg, proto, **kwargs))
~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
_pickle.PicklingError: Can't pickle <class 'importlib._bootstrap.BuiltinImporter'>: it's not the same object as importlib._bootstrap.BuiltinImporter
|
dr-carlos reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This error came up in #103247, and I think @freakboy3742 dealt with a more recent instance, but I can't find it now.
|
dr-carlos reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This error came up in #103247, and I think @freakboy3742 dealt with a more recent instance, but I can't find it now.
Thanks!
I had a look at the initial fix, which was just an uncache - but we can't uncache sys or builtins, which are the only modules imported here. I've tried just removing the sys/builtin test liens for now to see what CI thinks and if that doesn't work, I'll have a more detailed look later.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Okay, manged to reproduce the error on Linux with ./python -m test test_import test_importlib test_pickletools. Turns out that you can uncache the builtins module, but then other tests in importlib break. I couldn't find a proper fix, so for now, I've re-added the test with sys, leaving the builtins one out.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Merged, thanks for the fix.
|
dr-carlos reacted with heart emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️
Hi! The buildbot ARM Raspbian 3.x (tier-3) has failed when building commit cd2ca74.
What do you need to do:
- Don't panic.
- Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
- Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/424/builds/12349) and take a look at the build logs.
- Check if the failure is related to this commit (cd2ca74) or if it is a false positive.
- If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.
You can take a look at the buildbot page here:
https://buildbot.python.org/#/builders/424/builds/12349
Failed tests:
Failed subtests:
- test_nonexistent_attribute - test.test_annotationlib.TestForwardRefFormat.test_nonexistent_attribute
Summary of the results of the build (if available):
==
Click to see traceback logs
Traceback (most recent call last):
File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_annotationlib.py", line 160, in test_nonexistent_attribute
self.assertEqual(epsilon_anno, support.EqualToForwardRef("some | {obj, module}", owner=f))
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: ForwardRef('some | {module, obj}', owner=[79 chars]3f0>) != EqualToForwardRef('some | {obj, module}',[86 chars]3f0>)
Traceback (most recent call last):
File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_annotationlib.py", line 160, in test_nonexistent_attribute
self.assertEqual(epsilon_anno, support.EqualToForwardRef("some | {obj, module}", owner=f))
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: ForwardRef('some | {module, obj}', owner=[79 chars]760>) != EqualToForwardRef('some | {obj, module}',[86 chars]760>)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Unfortunately I'm unsure why this buildbot is failing - it's only happening on ARM Raspbian. It's not having issues importing anything (which is what this PR changes). Plus, the failure is only on this one specific test which seems totally unrelated.
However, the test only seems to be failing after this commit, and it's happening now on other commits (27a2e49).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Unfortunately I'm unsure why this buildbot is failing
The failure seems to be unrelated.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|