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.

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

Don't rely on the module cache when "importing self" #1747

Merged
merged 8 commits into from
Aug 23, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Aug 22, 2022

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

Relevant historic commits are:
6d59ad0
9684d0f

Basically, by not relying on the cache we trigger a new import system and check and rightfully end up with the stdlib math module instead of the current module. I tested with pylint and test suite there still passes.

Refs: pylint-dev/pylint#7289 and pylint-dev/pylint#5151, but mostly the latter.

Type of Changes

Type
🐛 Bug fix

@DanielNoord DanielNoord added Bug 🪳 pylint-tested PRs that don't cause major regressions with pylint labels Aug 22, 2022
@DanielNoord DanielNoord added this to the 2.13.0 milestone Aug 22, 2022

# Infer the 'import math' statement
stdlib_math = next(module.body[1].value.args[0].infer())
assert self.manager.astroid_cache["math"] != stdlib_math
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that the old math stays in place in the cache. This is how we handle all cache clashes so I think this is also good in this case?

@coveralls
Copy link

coveralls commented Aug 22, 2022

Pull Request Test Coverage Report for Build 2911196102

  • 7 of 7 (100.0%) changed or added relevant lines in 3 files are covered.
  • 21 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.09%) to 92.345%

Files with Coverage Reduction New Missed Lines %
astroid/interpreter/objectmodel.py 6 92.63%
astroid/brain/brain_dataclasses.py 15 91.63%
Totals Coverage Status
Change from base Build 2891370842: 0.09%
Covered Lines: 9711
Relevant Lines: 10516

💛 - Coveralls

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Had some time to do a quick first pass.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Nice MR, I like the typing update too :)

@@ -20,6 +20,10 @@ def find(name: str) -> str:
return os.path.normpath(os.path.join(os.path.dirname(__file__), DATA_DIR, name))


def find_in_testdata(name: str) -> Path:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def find_in_testdata(name: str) -> Path:
def construct_absolute_path_in_testdata(name: str) -> Path:

For me the verb "find" imply searching for it by walking the directory tree. I would expect the give "foo.py" and get back the file in testdata/bar/ called foo.py

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it to use get_testdata_path. The other one is a bit long.

Comment on lines 333 to 334
data_dir = resources.find_in_testdata("import_conflicting_names")
math_file = data_dir / "math.py"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data_dir = resources.find_in_testdata("import_conflicting_names")
math_file = data_dir / "math.py"
math_file = resources.construct_absolute_path_in_testdata("import_conflicting_names/math.py")

I think this would work ?

@DanielNoord
Copy link
Collaborator Author

Ready for review again! @Pierre-Sassoulas @jacobtylerwalls 😄

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

The test passes for me on main. Would you have the opportunity to create a test that fails before the fix?

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Aug 23, 2022

The test passes for me on main. Would you have the opportunity to create a test that fails before the fix?

Are you sure?

diff --git a/tests/unittest_manager.py b/tests/unittest_manager.py
index 9cf02aff8..fe1f22642 100644
--- a/tests/unittest_manager.py
+++ b/tests/unittest_manager.py
@@ -325,6 +325,22 @@ class AstroidManagerTest(
             with self.assertRaises(AstroidBuildingError):
                 self.manager.ast_from_module_name("foo.bar.baz")
 
+    def test_same_name_import_module(self) -> None:
+        """Test inference of an import statement with the same name as the module.
+        See https://github.com/PyCQA/pylint/issues/5151.
+        """
+        math_file = resources.find("data/import_conflicting_names/math.py")
+        module = self.manager.ast_from_file(math_file)
+
+        # Change the cache key and module name to mimic importing the test file
+        # from the root/top level. This creates a clash between math.py and stdlib math.
+        self.manager.astroid_cache["math"] = self.manager.astroid_cache.pop(module.name)
+        module.name = "math"
+
+        # Infer the 'import math' statement
+        stdlib_math = next(module.body[1].value.args[0].infer())
+        assert self.manager.astroid_cache["math"] != stdlib_math
+
 
 class BorgAstroidManagerTC(unittest.TestCase):
     def test_borg(self) -> None:

together with the math.py file

Fails for me with pytest -k test_same_name_import_module on:

>       assert self.manager.astroid_cache["math"] != stdlib_math
E       assert <Module.math l.0 at 0x110670210> != <Module.math l.0 at 0x110670210>

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Aug 23, 2022

Oh never mind. That doesn't actually fail. You just can't Module != Module. I'll have a look today.

Edit: @jacobtylerwalls I should have taken my coffee. That assertion fails correctly. On main self.manager.astroid_cache["math"] == stdlib_math for me, which shows that we are still using the cache as stdlib_math is the math.py file Module object.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Yeah, I did something horribly wrong when testing it, I guess. Sorry!

@DanielNoord DanielNoord merged commit ddfba9c into pylint-dev:main Aug 23, 2022
@DanielNoord DanielNoord deleted the import-self branch August 23, 2022 12:04
@emcd
Copy link

emcd commented Aug 27, 2022

EDIT: This PR may cause breakage with the use of importlib.import_module on PyPy 3.8. Please see #1755 for reference. The breakage actually comes from earlier - this commit is not the most recent release. Apologies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪳 pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants