gh-142029: Raise ModuleNotFoundError instead of crashing on nonexsistent module name given to create_builtin()

[dr-carlos]

• 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

[dr-carlos]

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?

[dr-carlos]

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.

[dr-carlos]

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?

[vstinner]

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

[dr-carlos]

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

Sounds good. Would a ModuleNotFoundError be most appropriate here?

[vstinner]

#142033 has been merged instead.

[dr-carlos]

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

[itamaro]

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

[vstinner]

LGTM. But there are now conflicts in Lib/test/test_import/init.py: can you try to solve them? (git merge main)

[vstinner]

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

[dr-carlos]

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.

[vstinner]

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
 

[mhsmith]

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]

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.

[dr-carlos]

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.

[vstinner]

Merged, thanks for the fix.

[bedevere-bot]

⚠️⚠️⚠️ 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>)

[dr-carlos]

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

[vstinner]

Unfortunately I'm unsure why this buildbot is failing

The failure seems to be unrelated.