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-91058: Add error suggestions to 'import from' import errors by pablogsal · Pull Request #98305 · python/cpython

Conversation

@pablogsal

@pablogsal pablogsal commented Oct 15, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Typo hint message for from-imports? #91058

Sorry, something went wrong.

Uh oh!

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

AlexWaygood, arhadthedev, and isidentical reacted with heart emoji erlend-aasland and isidentical reacted with rocket emoji
All reactions
  • ❤️ 3 reactions
  • 🚀 2 reactions
@pablogsal
pablogsal requested review from iritkatriel and markshannon as code owners October 15, 2022 21:24
@bedevere-bot bedevere-bot added the awaiting core review label Oct 15, 2022
@pablogsal pablogsal mentioned this pull request Oct 15, 2022
Typo hint message for from-imports? #91058
Closed
@pablogsal
pablogsal force-pushed the gh-91058 branch from e6c9bd4 to f8b529d Compare October 15, 2022 23:44
kumaraditya303
kumaraditya303 reviewed Oct 16, 2022
View reviewed changes
Comment thread Lib/test/test_traceback.py Resolved

Uh oh!

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

kumaraditya303
kumaraditya303 reviewed Oct 16, 2022
View reviewed changes
Comment thread Lib/test/test_traceback.py Resolved

Uh oh!

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

pablogsal and others added 4 commits October 17, 2022 10:23
@pablogsal
pythongh-91058: Add error suggestions to 'import from' import errors
a35c5ac
@pablogsal
Add NEWS entry
bdbb37d
@pablogsal @kumaraditya303
Update Lib/test/test_traceback.py
04e1e03
Co-authored-by: Kumar Aditya <[email protected]>
@pablogsal @kumaraditya303
Update Lib/test/test_traceback.py
77f9de1
Co-authored-by: Kumar Aditya <[email protected]>
@pablogsal
pablogsal force-pushed the gh-91058 branch from 73a1f23 to 77f9de1 Compare October 17, 2022 14:24
@pablogsal
pablogsal requested review from JelleZijlstra and isidentical October 17, 2022 14:25
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 17, 2022
@bedevere-bot

bedevere-bot commented Oct 17, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 77f9de1 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-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-buildbots Test PR w/ buildbots; report in status section label Oct 17, 2022
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 17, 2022
@bedevere-bot

bedevere-bot commented Oct 17, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit cd528b6 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-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-buildbots Test PR w/ buildbots; report in status section label Oct 17, 2022
@pablogsal
Update with latest code
dee788b
Signed-off-by: Pablo Galindo <[email protected]>
@pablogsal
pablogsal force-pushed the gh-91058 branch from cd528b6 to dee788b Compare October 17, 2022 18:07
isidentical
isidentical approved these changes Oct 17, 2022
View reviewed changes

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

Looks amazing (and a very useful error) @pablogsal. Just a quick question

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/suggestions.c Outdated
return NULL;
}

PyObject* mod = PyImport_Import(mod_name);

@isidentical isidentical Oct 17, 2022

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.

I was wondering whether we could avoid "realy-importing" a module if it isn't already (e.g. whether it is currently cached or not)? (IIRC from x import y should put the x to the sys.modules, but might be missing something too).

Sorry, something went wrong.

Uh oh!

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

All reactions

@pablogsal pablogsal Oct 18, 2022

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

PyImport_Import goes through the cache if for whatever reason the module is already imported, no? (And indeed, the module is imported as part of raising the exception).

Sorry, something went wrong.

Uh oh!

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

All reactions

@isidentical isidentical Oct 18, 2022

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.

PyImport_Import goes through the cache if for whatever reason the module is already imported, no? (And indeed, the module is imported as part of raising the exception).

Exactly, but consider the scenario where it isn't already imported and the module itself has some side effects (like it prints something at the top level). Importing it would then might produce unexpected side effects that only appear when the traceback is printed, so I thought whether we could limit this hint when the module is already cached.

if module not in sys.modules: return None # give up if it isn't already in the cache
module = PyImport_Import(mod_name)
...

Sorry, something went wrong.

Uh oh!

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

All reactions

@pablogsal pablogsal Oct 18, 2022

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

but consider the scenario where it isn't already imported and the module itself has some side effects

Hummmm. But isn't this a pre-condition for this kind of import error? If from foo import x fails to import foo then there is no lookup failure and we will never arrive here. And to be able to look x in foo then foo needs to be in sys.modules. Maybe I am missing something ... :(

Sorry, something went wrong.

Uh oh!

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

All reactions

@JelleZijlstra JelleZijlstra Oct 19, 2022

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.

I think it's at least theoretically possible for the module to be removed by now, because some Python code may run between the time the error is thrown and the time this code runs. For example, in pythonrun.c we call print_exception_message() before print_exception_suggestions(), and that does a bunch of allocations and DECREFs that could trigger GC.

You could call https://docs.python.org/3/c-api/import.html#c.PyImport_GetModule instead to return the module only if it's already imported.

Sorry, something went wrong.

Uh oh!

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

isidentical reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Oct 17, 2022
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 18, 2022
@bedevere-bot

bedevere-bot commented Oct 18, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit dee788b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-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-buildbots Test PR w/ buildbots; report in status section label Oct 18, 2022
JelleZijlstra
JelleZijlstra reviewed Oct 19, 2022
View reviewed changes

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

Thanks, a few suggestions.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_traceback.py Resolved

Uh oh!

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

Comment thread Lib/traceback.py Resolved

Uh oh!

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

Comment thread Lib/traceback.py Resolved

Uh oh!

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

Comment thread Lib/traceback.py Resolved

Uh oh!

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

Comment thread Python/suggestions.c Outdated
return NULL;
}

PyObject* mod = PyImport_Import(mod_name);

@JelleZijlstra JelleZijlstra Oct 19, 2022

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.

I think it's at least theoretically possible for the module to be removed by now, because some Python code may run between the time the error is thrown and the time this code runs. For example, in pythonrun.c we call print_exception_message() before print_exception_suggestions(), and that does a bunch of allocations and DECREFs that could trigger GC.

You could call https://docs.python.org/3/c-api/import.html#c.PyImport_GetModule instead to return the module only if it's already imported.

Sorry, something went wrong.

Uh oh!

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

isidentical reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@pablogsal
fixup! Update with latest code
c07068f
pablogsal and others added 2 commits October 20, 2022 15:04
@pablogsal @JelleZijlstra
Apply suggestions from code review
d6908e5
Co-authored-by: Jelle Zijlstra <[email protected]>
@pablogsal @JelleZijlstra
Update Lib/test/test_traceback.py
de553f6
Co-authored-by: Jelle Zijlstra <[email protected]>
@pablogsal
pablogsal merged commit 7cfbb49 into python:main Oct 25, 2022
@pablogsal
pablogsal deleted the gh-91058 branch October 25, 2022 22:57
@bedevere-bot bedevere-bot removed the awaiting merge label Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@isidentical isidentical isidentical approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

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.

5 participants

@pablogsal @bedevere-bot @JelleZijlstra @isidentical @kumaraditya303
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.