FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-95754: Better error when script shadows a standard library or third party module by hauntsaninja · Pull Request #113769 · python/cpython · GitHub

Repository navigation

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

Merged
hauntsaninja merged 51 commits into
python:mainfrom
hauntsaninja:stdlib-error
Apr 23, 2024
Merged

hauntsaninja merged 51 commits into
python:mainfrom
hauntsaninja:stdlib-error

Conversation

hauntsaninja commented Jan 6, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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/

hauntsaninja force-pushed the stdlib-error branch 2 times, most recently from 9279982 to 6f14b0d Compare January 6, 2024 10:34
hauntsaninja marked this pull request as draft January 6, 2024 10:37
hauntsaninja force-pushed the stdlib-error branch 3 times, most recently from f69e0b5 to d4111e5 Compare January 6, 2024 11:26
hauntsaninja added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 6, 2024

Copy link
Copy Markdown

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

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

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 6, 2024

Copy link
Copy Markdown
Contributor Author

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.

hauntsaninja marked this pull request as ready for review January 7, 2024 03:39
hauntsaninja force-pushed the stdlib-error branch 2 times, most recently from 4c09d8e to 9cb7291 Compare January 7, 2024 03:56

hauntsaninja commented Jan 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Should I disable this if sys.flags.safe_path? Also this won't currently work for python nested_folder/script.py where nested_folder contains the shadowing module... I could base this around Py_GetPath()[0] instead.

Oh hmm, Py_GetPath is deprecated, so I guess I have to get sys.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 before 7f24b99 , curious if people have suggestions that are more bulletproof than module name checks.

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

Comment thread Objects/moduleobject.c Outdated
Comment thread Objects/moduleobject.c
Comment thread Objects/moduleobject.c

ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A couple more minor comments.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor Author

Thank you so much for the detailed review!

brettcannon removed their request for review March 13, 2024 19:21

Copy link
Copy Markdown
Contributor Author

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)

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

AlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 😄

JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Doc/whatsnew/3.13.rst Outdated
Comment thread Lib/test/test_import/__init__.py Outdated
Comment thread Objects/moduleobject.c Outdated

Copy link
Copy Markdown
Contributor Author

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!

erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Objects/moduleobject.c Outdated
Comment thread Objects/moduleobject.c Outdated
Comment thread Objects/moduleobject.c Outdated
Comment thread Objects/moduleobject.c Outdated
Comment thread Objects/moduleobject.c Outdated
Comment thread Objects/moduleobject.c

Copy link
Copy Markdown
Contributor Author

Thanks Erlend!

hauntsaninja merged commit 8e86579 into python:main Apr 23, 2024
hauntsaninja deleted the stdlib-error branch April 23, 2024 01:24

Copy link
Copy Markdown
Contributor

This is great, but I'm greedy now: what about the other form? This works for AttributeError import x; x.y but not for the from 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 Path
ImportError: cannot import name 'Path' from 'pathlib' (/Users/henryschreiner/git/presentations/python313/pathlib.py)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants


Back | FazBrowse Home | New Git URL