GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

@dr-carlos dr-carlos commented Nov 28, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • 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.
  • Issue: Assertion failures from calling create_builtin with invalid object #142029

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
dr-carlos added 3 commits November 29, 2025 07:24
@dr-carlos
Don't assume that all non-found modules in create_builtin() are `sy…
e85b222
…s`/`builtin`
@dr-carlos
Add tests for internal and nonexistent modules passed to `create_buil…
68e2b7e
…tin()`
@dr-carlos
Add NEWS entry
603d98b
@dr-carlos
dr-carlos requested review from brettcannon, ericsnowcurrently, kumaraditya303, ncoghlan and warsaw as code owners November 28, 2025 22:31
@bedevere-app bedevere-app Bot added the awaiting review label Nov 28, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Nov 28, 2025
Assertion failures from calling create_builtin with invalid object #142029
Closed
@dr-carlos

dr-carlos commented Nov 28, 2025

Copy link
Copy Markdown
Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-carlos
Properly handle inittab entries with no initfunc
d45c2d9
@dr-carlos

dr-carlos commented Dec 1, 2025

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-carlos

dr-carlos commented Dec 1, 2025

Copy link
Copy Markdown
Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Dec 2, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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
All reactions
  • 👍 1 reaction
@dr-carlos

dr-carlos commented Dec 2, 2025

Copy link
Copy Markdown
Contributor Author

I prefer raising an exception as done in #142033 instead of returning None.

Sounds good. Would a ModuleNotFoundError be most appropriate here?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dr-carlos added 2 commits December 3, 2025 09:09
@dr-carlos
Raise ModuleNotFoundError instead of returning None
d07443c
@dr-carlos
Update NEWS entry
5c67d47
@dr-carlos 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
@dr-carlos
Merge branch 'main' into importlib-nonexistent-builtin
b639e70
@vstinner

vstinner commented Dec 10, 2025

Copy link
Copy Markdown
Member

#142033 has been merged instead.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Dec 10, 2025
@dr-carlos

dr-carlos commented Dec 11, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@itamaro

itamaro commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

@vstinner I think this PR is still needed, regardless of gh-142033.
it's fixing a real bug I introduced in gh-139196

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@itamaro itamaro reopened this Dec 11, 2025
vstinner
vstinner approved these changes Dec 11, 2025
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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
All reactions
  • 👍 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Dec 11, 2025
@vstinner

vstinner commented Dec 11, 2025

Copy link
Copy Markdown
Member

Oh sorry, I didn't see that the issue had two sub-issues.

itamaro reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-carlos
Merge branch 'main' into importlib-nonexistent-builtin
42974ac
@dr-carlos

dr-carlos commented Dec 11, 2025

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Dec 12, 2025
View reviewed changes
Comment thread Lib/test/test_import/__init__.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

brettcannon and others added 3 commits December 12, 2025 12:02
@brettcannon
Merge branch 'main' into importlib-nonexistent-builtin
225a143
@dr-carlos @vstinner
Fix typo in test_import
40a9c84
Co-authored-by: Victor Stinner <[email protected]>
@brettcannon
Merge branch 'main' into importlib-nonexistent-builtin
0739d71
@vstinner

vstinner commented Dec 12, 2025

Copy link
Copy Markdown
Member

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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhsmith

mhsmith commented Dec 13, 2025

Copy link
Copy Markdown
Member

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
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-carlos

dr-carlos commented Dec 13, 2025

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dr-carlos added 2 commits December 13, 2025 11:38
@dr-carlos
Temporarily remove sys/builtin create_builtin tests
565836e
@dr-carlos
Re-add create_builtin(sys) test
11ceded
@dr-carlos

dr-carlos commented Dec 14, 2025

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
vstinner merged commit cd2ca74 into python:main Dec 15, 2025
46 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the awaiting merge label Dec 15, 2025
@vstinner

vstinner commented Dec 15, 2025

Copy link
Copy Markdown
Member

Merged, thanks for the fix.

dr-carlos reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Dec 15, 2025

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM Raspbian 3.x (tier-3) has failed when building commit cd2ca74.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. 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.
  4. Check if the failure is related to this commit (cd2ca74) or if it is a false positive.
  5. 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:

  • test_annotationlib

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>)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-carlos

dr-carlos commented Dec 15, 2025

Copy link
Copy Markdown
Contributor Author

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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dr-carlos
dr-carlos deleted the importlib-nonexistent-builtin branch December 15, 2025 22:16
fatelei pushed a commit to fatelei/cpython that referenced this pull request Dec 16, 2025
@dr-carlos @brettcannon
@vstinner @fatelei
pythongh-142029: Raise ModuleNotFoundError instead of crashing on n…
7dfa26c
…onexsistent module name given to `create_builtin()` (python#142054)

Co-authored-by: Brett Cannon <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
@vstinner

vstinner commented Dec 16, 2025

Copy link
Copy Markdown
Member

Unfortunately I'm unsure why this buildbot is failing

The failure seems to be unrelated.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes
@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

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.

6 participants

@dr-carlos @vstinner @itamaro @mhsmith @bedevere-bot @brettcannon
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.