gh-123930: Better error for "from imports" when script shadows module

[hauntsaninja]

This is a follow up to #113769. Thanks to @hugovk and @henryiii for mentioning this.

With this PR, we get e.g. the following error message for shadowing stdlib:

λ cat turtle.py 
from turtle import forward
λ ../python.exe turtle.py
Traceback (most recent call last):
  File "/Users/shantanu/dev/cpython/tmp/turtle.py", line 1, in <module>
    from turtle import forward
  File "/Users/shantanu/dev/cpython/tmp/turtle.py", line 1, in <module>
    from turtle import forward
ImportError: cannot import name 'forward' from 'turtle' (consider renaming '/Users/shantanu/dev/cpython/tmp/turtle.py' since it has the same name as the standard library module named 'turtle' and the import system gives it precedence)

and the following error message for shadowing third party:

λ cat numpy.py
from numpy import array
λ ../python.exe numpy.py
Traceback (most recent call last):
  File "/Users/shantanu/dev/cpython/tmp/numpy.py", line 1, in <module>
    from numpy import array
  File "/Users/shantanu/dev/cpython/tmp/numpy.py", line 1, in <module>
    from numpy import array
ImportError: cannot import name 'array' from 'numpy' (consider renaming '/Users/shantanu/dev/cpython/tmp/numpy.py' if it has the same name as a third-party module you intended to import)

This PR can mostly be reviewed commit by commit [*].

I try to make the code path in ceval.c:_PyEval_ImportFrom closely match the one in moduleobject.c:_Py_module_getattro_impl, so as to avoid consistency issues between the two and to make comparing of the logic easier.

Note the main semantic difference as a result of the consistency changes is that we now use __spec__.origin instead of __file__ in the _PyEval_ImportFrom code path.

[*] there's one decref I missed in an intermediate commit

• Issue: Better error message for from imports when a script shadows a module #123930

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @hauntsaninja for commit 53e6931 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[blhsing]

I think this is a good time to add test to sys.stdlib_module_names for developers who happen to name their test scripts as test.py but actually want to run test suites in the stdlib, based on a true story.

[pablogsal]

I am not sure I completely follow the one about "third party". It seems that you want to trigger this when a module it's using its own name and is not part of the standard library module names, right? But this message can be confusing (the part mentioning "third party") because this can be module foo importing by mistake itself where there is no "third party" intent involved.

I would suggest to point out what's happening (the module its importing from itself) more than doing an educated guess of the original intent.

[hauntsaninja]

Yeah, similar to the PR that's in 3.13 #113769, the error is triggered when a) module is still initialising itself, b) attribute access or from import fails because name doesn't exist, c) the module is on PyConfig->sys_path_0 (and not safe_path etc)

I'm little worried, users who run into this are typically really new to Python, may barely know what a module is, so it's nice for something to say "consider renaming". (This confusion is common with third party packages not just stdlib: here's a report from a library I help maintain , here's a discuss thread that was active when I made original PR , many more examples)

I'm happy to do whatever, but maybe you can advise me a little bit…

Option 1:

I would suggest to point out what's happening (the module its importing from itself) more than doing an educated guess of the original intent.

The existing message already says "most likely due to a circular import", which is similar to your suggestion. So if I understand correctly, we basically just revert this to existing logic. One question I have is about the exact same logic that was landed in #113769 — I can revert that as well, and if so should it be backported to 3.13(.1) and removed from What's New?

Option 2:

Maybe there are ways we can reduce false positives. The place I think false positives are most likely are from this suggestion I took: #113769 (comment) / test_package_shadowing_stdlib_module /

cpython/Objects/moduleobject.c

Lines 891 to 898 in aa2e667

	// If it's a package then we need to look one directory further up
	if (wcscmp(sep + 1, L"__init__.py") == 0) {
	*sep = L'\0';
	sep = wcsrchr(root, SEP);
	if (sep == NULL) {
	return 0;
	}
	}

. This will trigger the new error if a package imports a name from itself that doesn't exist, which is a probably thing that happens every now and then. People are less likely to make package with accidentally conflicting name than a script.

If we remove that, I think there are fewer false positives. At the time of the original PR, I tried to find single file scripts that import themselves and it's quite rare. The two examples I was able to find in codebases I contribute to was: 1) some trick to ensure cloudpickle pickled things in main module via reference not value, 2) some awful thing someone was doing in a setup.py. Both cases felt like users knew a lot about what they were doing and if they did a typo getting "consider renaming if you meant to import third party module" wouldn't scare them too much. I'm sure there are other use cases, but if they're niche or advanced I'm quite happy trading in favour of new users who make torch.py / flask.py / requests.py

Option 3:

Pablo magic happens and you have some clever trick in mind :-)
I had some versions of the logic where I tried to interact with the import system more, e.g. to see if there was actually some module on the rest of path, but it was quite scary, risked bad recursion happening and I couldn't see a way to do it safely.

I think this is a good time to add test to sys.stdlib_module_names

Thanks for suggestion, maybe open a new issue for this?

[pablogsal]

I'm little worried, users who run into this are typically really new to Python, may barely know what a module is, so it's nice for something to say "consider renaming".

Right, but precisely for those users the concept of "first party" and "third party" will be equally (or potentially more) confusing.

The existing message already says "most likely due to a circular import", which is similar to your suggestion.

It depends I think. "A circular import" means that there is a cycle in the import mechanism. Obviously the cycle can be of 1 element (which is the case) but thinking of the user group you mentioned previously that's going to sound more confusing that saying "the module it's importing itself" (if we are targetting exactly that case).

In any case my suggestion is to remove or rephrase the concept of "third party" becase (1) we don't really know if is third party and (2) a good set of users that can benefit from this error message may find that more confusing that if we speak just in terms of general modules or packages.

Maybe there are ways we can reduce false positives. The place I think false positives are most likely are from this suggestion I took: #113769 (comment) / test_package_shadowing_stdlib_module /

I agree with you that importing a name from itself on pourpose it's rare enough that when someone it's doing it on pourpose the slighly confusing error message with a suggestion is not going to scare them away. We should focus as you suggest in the vast group of users that are not doing this knowingly :)

Pablo magic happens and you have some clever trick in mind :-)

I have new clever tricks in mind for next time we hang out together but unfortunately not for solving this particular problem without introducing crazy potential problems 😅

---

In any case, this was just a suggestion and I don't feel very strongly against the current error message. If you want to keep it as is I will support it, so don't worry. I just want us to consider this particular challenge and making sure it did not pass without consideration :)

[hauntsaninja]

Sorry, this got a little lost. I've changed the wording.

I also plan on backporting this PR to Python 3.13. This is a little questionable, so let me know if you think it's a terrible idea.

My reasoning is: the missing helpful message in "from imports" is basically a bug in the new 3.13 feature, Pablo's suggested change in wording could similarly be considered a bug, unifying the two code paths makes it easier to maintain

[ncoghlan]

The actual change looks good to me (and backporting sounds reasonable), but I picked up another bit of dubious wording from the original feature implementation.

[hauntsaninja]

Thanks for the review! That wording went through a few iterations and this is the best one of them yet :-)

[AlexWaygood]

Some driveby nitpicks :-)

[miss-islington-app]

Thanks @hauntsaninja for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @hauntsaninja, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 500f5338a8fe13719478589333fcd296e8e8eb02 3.13

[bedevere-app]

GH-125937 is a backport of this pull request to the 3.13 branch.

[freakboy3742]

@hauntsaninja Looks like this PR has broken the iOS buildbot; investigating now.

[freakboy3742]

@hauntsaninja I think this may be the source of the problem. On iOS, module.__file__ isn't the same as __spec__.origin - module.__file__ is the original source of the binary module (in lib-dynload), in the sense of where Python thinks the library is installed; __spec__.origin is the actual iOS framework location (in the Frameworks folder).

[hauntsaninja]

Ah interesting, I see the special importlib Loader. Thanks for fixing the test!