Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-95754: Better error when script shadows a standard library or third party module#113769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
hauntsaninja merged 51 commits intopython:mainfromhauntsaninja:stdlib-error
Apr 23, 2024

Conversation

hauntsaninja
Copy link
Contributor

@hauntsaninjahauntsaninja commentedJan 6, 2024
edited
Loading

This is a very common mistake for beginners. This PR tries to detect the most common case: where a file in the current directory ends up on sys.path and shadows a standard library module. (My previous PR#112577 was only marginally helpful and wouldn't help with many such errors)

The first commit in this PR is just refactoring.


📚 Documentation preview 📚:https://cpython-previews--113769.org.readthedocs.build/

Eclips4 and erlend-aasland reacted with thumbs up emojidanielhollas and hroncok reacted with heart emoji
@hauntsaninjahauntsaninjaforce-pushed thestdlib-error branch 2 times, most recently from9279982 to6f14b0dCompareJanuary 6, 2024 10:34
@hauntsaninjahauntsaninja marked this pull request as draftJanuary 6, 2024 10:37
@hauntsaninjahauntsaninjaforce-pushed thestdlib-error branch 3 times, most recently fromf69e0b5 tod4111e5CompareJanuary 6, 2024 11:26
@hauntsaninjahauntsaninja added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJan 6, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@hauntsaninja for commitd4111e5 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelJan 6, 2024
@hauntsaninja
Copy link
ContributorAuthor

Oh actually looks like I can use stdlib_module_names.h do get sys.stdlib_module_names in C. And I can of course do the path munging in C as well.

@hauntsaninjahauntsaninja marked this pull request as ready for reviewJanuary 7, 2024 03:39
@hauntsaninjahauntsaninjaforce-pushed thestdlib-error branch 2 times, most recently from4c09d8e to9cb7291CompareJanuary 7, 2024 03:56
@hauntsaninja
Copy link
ContributorAuthor

hauntsaninja commentedJan 7, 2024
edited
Loading

Should I disable this ifsys.flags.safe_path? Also this won't currently work forpython nested_folder/script.py wherenested_folder contains the shadowing module... I could base this aroundPy_GetPath()[0] instead.

Oh hmm,Py_GetPath is deprecated, so I guess I have to getsys.path[0] (and assume it hasn't changed). This means we have to call back into Python and risk the bad recursion again. See the code from before7f24b99 , curious if people have suggestions that are more bulletproof than module name checks.

(edit from later: I discoveredPySys_GetObject, so now I can safely do a bunch of stuff!)

Copy link
Member

@ericsnowcurrentlyericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A couple more minor comments.

@ericsnowcurrently
Copy link
Member

FYI, I think we're at the tail end of my review and I don't expect to have much more feedback to offer. Thanks for working on this.

You'll still want to double-check with@pablogsal and anyone else with a vested interest.

@hauntsaninja
Copy link
ContributorAuthor

Thank you so much for the detailed review!

@brettcannonbrettcannon removed their request for reviewMarch 13, 2024 19:21
@hauntsaninja
Copy link
ContributorAuthor

Does anyone have any more comments? If not, I'd love a green check mark to unblock the merge. (And of course I'll be happy to address any post-merge follow-ups)

@ericsnowcurrently
Copy link
Member

We need to be sure the other reviewers are on board, especially@pablogsal.

erlend-aasland reacted with thumbs up emoji

@ericsnowcurrently
Copy link
Member

Copy link
Member

@AlexWaygoodAlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I previously gave feedback on the error message, which looks great to me now! Not confident enough to give other kinds of feedback on this sort of PR 😄

Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No need to wait for me before merging, as other reviewers know a lot more about the import system. However, a few minor points.

@hauntsaninja
Copy link
ContributorAuthor

This has been quiet for a while and I'd love to get it in ahead of beta1, so I will probably go ahead and merge soon.

Please let me know if you have additional feedback and I'll follow up in additional PRs. Thanks to everyone who left reviews, in particular to ericsnowcurrently!

hugovk and erlend-aasland reacted with thumbs up emojiAlexWaygood and erlend-aasland reacted with heart emoji

Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good to me. I added some minor suggestions (C idioms and a PEP 7 nit); feel free to ignore them.

@hauntsaninja
Copy link
ContributorAuthor

Thanks Erlend!

erlend-aasland reacted with heart emoji

@hauntsaninjahauntsaninja merged commit8e86579 intopython:mainApr 23, 2024
@hauntsaninjahauntsaninja deleted the stdlib-error branchApril 23, 2024 01:24
@henryiii
Copy link
Contributor

This is great, but I'm greedy now: what about the other form? This works for AttributeErrorimport x; x.y but not for thefrom x import y ImportError form.

$touch pathlib.py$python3.13 -c"import pathlib; pathlib.Path"Traceback (most recent call last):  File "<string>", line 1, in <module>    import pathlib; pathlib.Path                    ^^^^^^^^^^^^AttributeError: module 'pathlib' has no attribute 'Path' (consider renaming '/Users/henryschreiner/git/presentations/python313/pathlib.py' since it has the same name as the standard library module named 'pathlib' and the import system gives it precedence)$python3.13 -c"from pathlib import Path"Traceback (most recent call last):  File "<string>", line 1, in <module>    from pathlib import PathImportError: cannot import name 'Path' from 'pathlib' (/Users/henryschreiner/git/presentations/python313/pathlib.py)
AlexWaygood reacted with eyes emoji

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

@JelleZijlstraJelleZijlstraJelleZijlstra left review comments

@ericsnowcurrentlyericsnowcurrentlyericsnowcurrently requested changes

@hugovkhugovkhugovk left review comments

@pablogsalpablogsalpablogsal left review comments

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@AlexWaygoodAlexWaygoodAlexWaygood left review comments

@ncoghlanncoghlanAwaiting requested review from ncoghlanncoghlan is a code owner

@warsawwarsawAwaiting requested review from warsawwarsaw is a code owner

Assignees

@pablogsalpablogsal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

9 participants
@hauntsaninja@bedevere-bot@pablogsal@erlend-aasland@ericsnowcurrently@henryiii@JelleZijlstra@hugovk@AlexWaygood

[8]ページ先頭

©2009-2025 Movatter.jp