Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Prevent first-party imports from being resolved to site-packages #1756

Merged
merged 8 commits into from
Aug 29, 2022
Merged

Prevent first-party imports from being resolved to site-packages #1756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9b99b64
Prevent first-party imports from being resolved to `site-packages`
jacobtylerwalls Aug 27, 2022
ce62502
fixup! Prevent first-party
jacobtylerwalls Aug 27, 2022
ffad944
Merge branch 'main' into first-party-import-ext-lib
Pierre-Sassoulas Aug 28, 2022
b89ef27
fixup test
jacobtylerwalls Aug 28, 2022
776218a
wip -- debug result on ci
jacobtylerwalls Aug 28, 2022
4c60c9d
another debug stmt
jacobtylerwalls Aug 28, 2022
22ce69e
fixup path comparison
jacobtylerwalls Aug 28, 2022
5b3b37c
fixup -- update comments
jacobtylerwalls Aug 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ What's New in astroid 2.12.5?
=============================
Release date: TBA


* Prevent first-party imports from being resolved to `site-packages`.

Refs PyCQA/pylint#7365

* Fix ``astroid.interpreter._import.util.is_namespace()`` incorrectly
returning ``True`` for frozen stdlib modules on PyPy.

Expand Down
16 changes: 15 additions & 1 deletion astroid/interpreter/_import/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@

@lru_cache(maxsize=4096)
def is_namespace(modname: str) -> bool:
from astroid.modutils import ( # pylint: disable=import-outside-toplevel
EXT_LIB_DIRS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move these to const to remove any import cycles? We could also move the union there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that, but there's too much logic in modutils that would have to be moved. My preference would be to move is_namespace into modutils, but I'd prefer to do that in a followup commit that wouldn't be backported. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to back port this. We never know how long it takes until we release a minor version and we might want to make more fixes here.

I'd be happy to make a back port PR 😄

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Aug 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I opened #1759 we just need to merge this and cherry pick it before releasing 2.12.5.

Copy link
Member Author

@jacobtylerwalls jacobtylerwalls Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think it would be good to back port this

Ah, we might be miscommunicating! I'm saying yes, we should definitely backport the crash fix, but also that I wouldn't want to block that on refactoring the cyclic import out. There's a lot of logic in modutils that would have to be moved, and it's all public, so I wouldn't want to break backwards compatibility on that in a patch release.

So I was hoping to merge and backport this in its current form, and leave the refactor for another day (and not backport the refactor, as usual). How does that sound?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's merge this as is, but let's also back port the refactor. Like I said: with astroid the time in between minor versions can be quite long and we might need to fix something and release a patch version after the refactor. To avoid future merge issues I would also merge any refactor to this particular piece of code.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @DanielNoord about cherry-picking the refactors. Case in point the 2.12 maintenance branch has been live for 2 months now. Cherry-picking things after a refactor without the refactor is a lot harder than just cherry-picking the refactor too. (We do need to make sure our refactor are just refactor, but we never had any problem with this so far).

STD_LIB_DIRS,
)

STD_AND_EXT_LIB_DIRS = STD_LIB_DIRS.union(EXT_LIB_DIRS)

if modname in sys.builtin_module_names:
return False

Expand Down Expand Up @@ -70,8 +77,15 @@ def is_namespace(modname: str) -> bool:
last_submodule_search_locations.append(str(assumed_location))
continue

# Update last_submodule_search_locations
# Update last_submodule_search_locations for next iteration
if found_spec and found_spec.submodule_search_locations:
# But immediately return False if we can detect we are in stdlib
# or external lib (e.g site-packages)
if any(
any(location.startswith(lib_dir) for lib_dir in STD_AND_EXT_LIB_DIRS)
for location in found_spec.submodule_search_locations
):
return False
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
last_submodule_search_locations = found_spec.submodule_search_locations

return (
Expand Down
5 changes: 4 additions & 1 deletion tests/unittest_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from astroid.const import IS_JYTHON, IS_PYPY
from astroid.exceptions import AstroidBuildingError, AstroidImportError
from astroid.interpreter._import import util
from astroid.modutils import is_standard_module
from astroid.modutils import EXT_LIB_DIRS, is_standard_module
from astroid.nodes import Const
from astroid.nodes.scoped_nodes import ClassDef

Expand Down Expand Up @@ -130,6 +130,9 @@ def test_submodule_homonym_with_non_module(self) -> None:
def test_module_is_not_namespace(self) -> None:
self.assertFalse(util.is_namespace("tests.testdata.python3.data.all"))
self.assertFalse(util.is_namespace("__main__"))
self.assertFalse(
util.is_namespace(list(EXT_LIB_DIRS)[0].rsplit("/", maxsplit=1)[-1]),
)
self.assertFalse(util.is_namespace("importlib._bootstrap"))

def test_module_unexpectedly_missing_spec(self) -> None:
Expand Down