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-123930: Better error for "from imports" when script shadows module by hauntsaninja · Pull Request #123929 · python/cpython

Conversation

@hauntsaninja

@hauntsaninja hauntsaninja commented Sep 11, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

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

hugovk, blhsing, nineteendo, and Zheaoli reacted with thumbs up emoji danielhollas reacted with heart emoji
All reactions
  • 👍 4 reactions
  • ❤️ 1 reaction
hauntsaninja added 4 commits September 10, 2024 16:33
@hauntsaninja
rename pkgname to mod_name
9b88d72
@hauntsaninja
rename pkgpath to origin (this isn't semantically true yet)
d0de86e
@hauntsaninja
switch logic to actually use origin, matching other code path
6ce3a67
@hauntsaninja
add error for shadowing
aa2e667
@hauntsaninja
hauntsaninja requested review from brettcannon, ericsnowcurrently, markshannon, ncoghlan and warsaw as code owners September 11, 2024 01:25
@bedevere-app bedevere-app Bot added the awaiting core review label Sep 11, 2024
@hauntsaninja hauntsaninja changed the title Better error for "from imports" when script shadows module gh-123930: Better error for "from imports" when script shadows module Sep 11, 2024
blurb-it Bot and others added 2 commits September 11, 2024 01:32
@blurb-it
📜🤖 Added by blurb_it.
cac393b
@hauntsaninja
check number of lines, now that we're looping
53e6931
@hauntsaninja
hauntsaninja requested a review from pablogsal September 11, 2024 01:38
@hauntsaninja hauntsaninja added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 11, 2024
@bedevere-bot

bedevere-bot commented Sep 11, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 11, 2024
hauntsaninja added 2 commits September 10, 2024 18:42
@hauntsaninja
update comment
bd038b3
@hauntsaninja
fix test on windows
49871c8
@blhsing

blhsing commented Sep 11, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.

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

@pablogsal

pablogsal commented Sep 11, 2024

Copy link
Copy Markdown
Member

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 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-app bedevere-app Bot mentioned this pull request Sep 11, 2024
Better error message for from imports when a script shadows a module #123930
Closed
@hauntsaninja

hauntsaninja commented Sep 12, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

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?

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

@pablogsal

pablogsal commented Sep 12, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@hauntsaninja
change wording based on pablo feedback
cd227cc
@hauntsaninja

hauntsaninja commented Oct 20, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

hauntsaninja added 2 commits October 19, 2024 18:59
@hauntsaninja
Merge branch 'main' into from-error
84a3f96
@hauntsaninja
Merge branch 'main' into from-error
79ab582
ncoghlan
ncoghlan reviewed Oct 23, 2024
View reviewed changes
Comment thread Lib/test/test_import/__init__.py Resolved

Uh oh!

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

Comment thread Python/ceval.c Resolved

Uh oh!

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

@ncoghlan

ncoghlan commented Oct 23, 2024

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@hauntsaninja
alyssa review
336a722
@hauntsaninja

hauntsaninja commented Oct 23, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@hauntsaninja hauntsaninja added the needs backport to 3.13 only security fixes label Oct 23, 2024
AlexWaygood
AlexWaygood reviewed Oct 23, 2024
View reviewed changes

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

Some driveby nitpicks :-)

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_import/__init__.py Resolved

Uh oh!

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

Comment thread Lib/test/test_import/__init__.py Resolved

Uh oh!

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

Comment thread Lib/test/test_import/__init__.py Resolved

Uh oh!

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

Comment thread Lib/test/test_import/__init__.py Resolved

Uh oh!

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

hauntsaninja and others added 2 commits October 23, 2024 15:53
@hauntsaninja
subtest from alex
0cea35b
@hauntsaninja
Merge branch 'main' into from-error
e7371f0
@hauntsaninja
hauntsaninja merged commit 500f533 into python:main Oct 24, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 24, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the awaiting core review label Oct 24, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 24, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

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

@hauntsaninja
hauntsaninja deleted the from-error branch October 24, 2024 19:11
@miss-islington-app miss-islington-app Bot assigned hauntsaninja Oct 24, 2024
@bedevere-app

bedevere-app Bot commented Oct 24, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

hauntsaninja added a commit to hauntsaninja/cpython that referenced this pull request Oct 24, 2024
@hauntsaninja
pythongh-123930: Better error for "from imports" when script shadows …
25d5236
…module (python#123929)

(cherry picked from commit 500f533)
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Oct 24, 2024
hauntsaninja added a commit that referenced this pull request Oct 24, 2024
@hauntsaninja
[3.13] gh-123930: Better error for "from imports" when script shadows…
3d8b6f0
… module (GH-123929) (#125937)

gh-123930: Better error for "from imports" when script shadows module (#123929)

(cherry picked from commit 500f533)
@freakboy3742

freakboy3742 commented Oct 25, 2024

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

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

freakboy3742
freakboy3742 reviewed Oct 25, 2024
View reviewed changes
Comment thread Objects/moduleobject.c
}
// If origin is not a location, or doesn't exist, or is not a str), we could consider falling
// If origin is not a location, or doesn't exist, or is not a str, we could consider falling
// back to module.__file__. But the cases in which module.__file__ is not __spec__.origin

@freakboy3742 freakboy3742 Oct 25, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

@hauntsaninja hauntsaninja Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions
@freakboy3742 freakboy3742 mentioned this pull request Oct 25, 2024
gh-123930: Correct test of attribute failure to account for iOS #125959
Merged
@hauntsaninja hauntsaninja mentioned this pull request Dec 9, 2024
gh-127651: Use __file__ in diagnostics if origin is missing #127660
Merged
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@hauntsaninja @ebonnal
pythongh-123930: Better error for "from imports" when script shadows …
741dbfb
…module (python#123929)
@hauntsaninja hauntsaninja mentioned this pull request Jun 29, 2025
Getting ImportError and suggestion to rename module for no valid reason #136094
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@freakboy3742 freakboy3742 freakboy3742 left review comments
@ncoghlan ncoghlan ncoghlan left review comments
@AlexWaygood AlexWaygood AlexWaygood left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@pablogsal pablogsal Awaiting requested review from pablogsal

Uh oh!

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

Assignees

@hauntsaninja hauntsaninja

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.

7 participants

@hauntsaninja @bedevere-bot @blhsing @pablogsal @ncoghlan @freakboy3742 @AlexWaygood
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 01402503 — 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.