Skip to content

Conversation

@benjimin
Copy link
Contributor

@benjimin benjimin commented Jul 26, 2019

builtins.__import__ was missing a check to flag that if no dot was found in the __name__ of a module that isn't explicitly a submodule of a package then a relative import is incorrect.

https://bugs.python.org/issue37409

benjimin added 2 commits July 26, 2019 18:06
…ere no package exists

Relative imports use resolve_name to get the absolute target name,
which first seeks the current module's absolute package name from the globals:
If __package__ (and __spec__.parent) are missing then
as a hail mary it uses __name__, truncating the last segment if
the module is any submodule rather than a package __init__.py
(which it guesses from whether __path__ is defined).

The hail mary should fail if there is no parent package (top level modules),
if __name__ is uninformatively '__main__' (-m entry points), or both (scripts).
That is, if both __name__ has no subcomponents and the module does not seem
to be a package __init__ module.

(Bug silently used module as if package, aliasing unexpected objects.
Note importlib contained an unaffected alternative __import__ implementation.)
@benjimin
Copy link
Contributor Author

This is a conservative (minimal) change, intended to also be appropriate for back-porting.

The bug was that (sibling-level) relative import statements would in some cases (when unable to identify any enclosing package) instead retrieve objects from the current module rather than raising the expected ImportError. (Only builtins.__import__ was affected, not importlib.__import__)

Specifically, the problem concerned circumstances where __package__ and __spec__.parent are uninformative and the current module is not itself a package (i.e. __path__ is undefined, contrary to common __init__.py). In these circumstances either the package can be identified from __name__ (after truncating the last .component), or no package can be found (e.g. top-level modules and, more commonly, scripts/modules run as "__main__"). This last case was failing to trigger (instead treating the module itself as the package).

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

I have a suggestion on how to make the fix while taking a more explicit/simpler solution.

And please add a news entry for this.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

benjimin and others added 7 commits August 5, 2019 23:19
Co-Authored-By: Brett Cannon <54418+brettcannon@users.noreply.github.com>
Co-Authored-By: Brett Cannon <54418+brettcannon@users.noreply.github.com>
Co-Authored-By: Brett Cannon <54418+brettcannon@users.noreply.github.com>
Co-Authored-By: Brett Cannon <54418+brettcannon@users.noreply.github.com>
@benjimin
Copy link
Contributor Author

benjimin commented Aug 7, 2019

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

@brettcannon brettcannon self-requested a review August 9, 2019 21:32
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Some very minor tweaks and then this will be ready to be merged!

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Co-Authored-By: Brett Cannon <54418+brettcannon@users.noreply.github.com>
@benjimin
Copy link
Contributor Author

@brettcannon is this also appropriate for backporting to 3.7?

I hope this fix may help people encounter more informative error messages when they are first learning/experimenting with explicit relative imports (say, migrating code from py2).

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

This looks great!
@brettcannon, do you still want to re-review?

@brettcannon brettcannon merged commit 92420b3 into python:master Sep 11, 2019
@miss-islington
Copy link
Contributor

Thanks @benjimin for the PR, and @brettcannon for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

@brettcannon: Please replace # with GH- in the commit message next time. Thanks!

@miss-islington
Copy link
Contributor

Sorry, @benjimin and @brettcannon, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 92420b3e679959a7d0ce875875601a4cee45231e 3.8

@miss-islington
Copy link
Contributor

Sorry @benjimin and @brettcannon, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 92420b3e679959a7d0ce875875601a4cee45231e 3.7

@bedevere-bot
Copy link

GH-15913 is a backport of this pull request to the 3.8 branch.

brettcannon added a commit that referenced this pull request Sep 11, 2019
)

Relative imports use resolve_name to get the absolute target name,
which first seeks the current module's absolute package name from the globals:
If __package__ (and __spec__.parent) are missing then
import uses __name__, truncating the last segment if
the module is a submodule rather than a package __init__.py
(which it guesses from whether __path__ is defined).

The __name__ attempt should fail if there is no parent package (top level modules),
if __name__ is '__main__' (-m entry points), or both (scripts).
That is, if both __name__ has no subcomponents and the module does not seem
to be a package __init__ module then import should fail..
(cherry picked from commit 92420b3)

Co-authored-by: Ben Lewis <benjimin@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 11, 2019
…ythonGH-15913)

Relative imports use resolve_name to get the absolute target name,
which first seeks the current module's absolute package name from the globals:
If __package__ (and __spec__.parent) are missing then
import uses __name__, truncating the last segment if
the module is a submodule rather than a package __init__.py
(which it guesses from whether __path__ is defined).

The __name__ attempt should fail if there is no parent package (top level modules),
if __name__ is '__main__' (-m entry points), or both (scripts).
That is, if both __name__ has no subcomponents and the module does not seem
to be a package __init__ module then import should fail..
(cherry picked from commit 92420b3)

Co-authored-by: Ben Lewis <benjimin@users.noreply.github.com>
(cherry picked from commit 0a6693a)

Co-authored-by: Brett Cannon <54418+brettcannon@users.noreply.github.com>
@brettcannon
Copy link
Member

#15925 is the 3.7 backport

miss-islington added a commit that referenced this pull request Sep 11, 2019
)

Relative imports use resolve_name to get the absolute target name,
which first seeks the current module's absolute package name from the globals:
If __package__ (and __spec__.parent) are missing then
import uses __name__, truncating the last segment if
the module is a submodule rather than a package __init__.py
(which it guesses from whether __path__ is defined).

The __name__ attempt should fail if there is no parent package (top level modules),
if __name__ is '__main__' (-m entry points), or both (scripts).
That is, if both __name__ has no subcomponents and the module does not seem
to be a package __init__ module then import should fail..
(cherry picked from commit 92420b3)

Co-authored-by: Ben Lewis <benjimin@users.noreply.github.com>
(cherry picked from commit 0a6693a)

Co-authored-by: Brett Cannon <54418+brettcannon@users.noreply.github.com>
@brettcannon
Copy link
Member

And all the backports are done! Thanks, @benjimin .

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
Relative imports use resolve_name to get the absolute target name,
which first seeks the current module's absolute package name from the globals:
If __package__ (and __spec__.parent) are missing then
import uses __name__, truncating the last segment if
the module is a submodule rather than a package __init__.py
(which it guesses from whether __path__ is defined).

The __name__ attempt should fail if there is no parent package (top level modules),
if __name__ is '__main__' (-m entry points), or both (scripts).
That is, if both __name__ has no subcomponents and the module does not seem
to be a package __init__ module then import should fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants