astroid
astroid copied to clipboard
pylint-dev
Fix setuptools>=64 import hooks
Steps
- [x] For new features or bug fixes, add a ChangeLog entry describing what your PR does.
- [x] Write a good description on what the PR does.
Description
This needs a test. Refs: https://github.com/PyCQA/pylint/issues/7306 and https://github.com/pypa/setuptools/issues/3518.
Type of Changes
| Type | |
|---|---|
| ✓ | :bug: Bug fix |
Related Issue
@jacobtylerwalls This fixes the test that started failing after https://github.com/PyCQA/astroid/pull/1714
Would you mind taking a look at the PR and the linked issues/discussions? I'm hesitant to go forward, but this actually seems like it fixes some issues.
Pull Request Test Coverage Report for Build 3280118627
- 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.007%) to 92.279%
| Totals | |
|---|---|
| Change from base Build 3280113293: | 0.007% |
| Covered Lines: | 9872 |
| Relevant Lines: | 10698 |
💛 - Coveralls
I'd like to provide input, but the fall is going to be very busy for me. I'm not sure when I'll have the time to take a look. I'm very hesitant on doing anything additional touching the import system for 2.15.
I'd like to provide input, but the fall is going to be very busy for me. I'm not sure when I'll have the time to take a look. I'm very hesitant on doing anything additional touching the import system for 2.15.
Let's definitely not put this in 2.15. However, since this is going to affect myself as well when I update my local setuptools installation at some point I'd like to get this on astroid main at least. I'll try and see if I can get some more context in the meantime.
@Pierre-Sassoulas Would you be okay with putting this in the next release? This will be necessary for pylint not to be incompatible with setuptools>=64 and I'd like to get feedback on it from users before everybody is on >=64.
Sure but I think this need a rebase the changelog is not in 2.14.0
Yeah I will prepare the PR for final review then. Thanks!
On closer inspection (seeing Jacob's comment) this feel like something a little risky if we want to release 2.16.0 soon and not do another beta release. Maybe we should do an rc release ?
On closer inspection (seeing Jacob's comment) this feel like something a little risky if we want to release 2.16.0 soon and not do another beta release. Maybe we should do an rc release ?
Yeah I don't know. Based on the tests I think this should be fine, but we have been wrong before. If anything we can just quickly revert the change and release a patch astroid release? There is no code that builds upon this (both in astroid or pylint) so I don't foresee many merge issues when back porting.
Codecov Report
Merging #1752 (6ea1a7d) into main (ac41d4e) will increase coverage by
0.00%. The diff coverage is95.45%.
Additional details and impacted files
@@ Coverage Diff @@
## main #1752 +/- ##
=======================================
Coverage 92.74% 92.74%
=======================================
Files 94 94
Lines 10943 10962 +19
=======================================
+ Hits 10149 10167 +18
- Misses 794 795 +1
| Flag | Coverage Δ | |
|---|---|---|
| linux | 92.51% <95.45%> (+<0.01%) |
:arrow_up: |
| pypy | 88.45% <72.72%> (-0.04%) |
:arrow_down: |
| windows | 92.35% <95.45%> (+<0.01%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| astroid/interpreter/_import/spec.py | 97.35% <95.45%> (-0.30%) |
:arrow_down: |
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
I'm afraid 😄 Let's wait for a review by Jacob, forcing a namespace change last minute can ruin the effort we made with two beta releases and make our lives difficult (when we lived without this for months).
Sorry for not prioritizing this yet. @DanielNoord would you be able to write a (short) summary of why you believe this is the correct approach? That would help me get into this. (There has been more discussion on the linked PyPA issue.)
The idea is that we actually have a quite an advanced import system compared to other tools and are able to follow imports (somewhat successfully).
However, when installing editable installs with pip a new type of _SPEC_FINDERS is returned. We don't have support for those but we can quite easily add them by telling our import system that we should handle them as normal (namespace) packages.
I chose to use the __name__ attribute of the finders as we then don't build in dependencies on pip and setuptools etc. but can just infer the type of package from the finder's name.
The test case that now succeeds is notable as it shows that with this PR we actually are able to infer more imports than we previously could.
Also, I noticed two failures in pylint with this change, including one crash and one astroid-error:
pytest -k private-import
Exception on node <ImportFrom l.33 at 0x1144f4790> in file '/Users/.../pylint/tests/functional/ext/private_import/private_import.py'
Traceback (most recent call last):
File "/Users/.../pylint/pylint/utils/ast_walker.py", line 91, in walk
callback(astroid)
File "/Users/.../pylint/pylint/checkers/imports.py", line 562, in visit_importfrom
self._add_imported_module(node, f"{imported_module.name}.{name}")
File "/Users/.../pylint/pylint/checkers/imports.py", line 894, in _add_imported_module
elif not astroid.modutils.is_standard_module(importedmodname):
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/.../astroid/astroid/modutils.py", line 528, in is_standard_module
filename = file_from_modpath([modname])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/.../astroid/astroid/modutils.py", line 334, in file_from_modpath
return file_info_from_modpath(modpath, path, context_file).location
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/.../astroid/astroid/modutils.py", line 385, in file_info_from_modpath
return _spec_from_modpath(modpath, path, context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/.../astroid/astroid/modutils.py", line 595, in _spec_from_modpath
found_spec = spec.find_spec(modpath, path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/.../astroid/astroid/interpreter/_import/spec.py", line 423, in find_spec
finder, spec = _find_spec_with_path(
^^^^^^^^^^^^^^^^^^^^^
File "/Users/.../astroid/astroid/interpreter/_import/spec.py", line 372, in _find_spec_with_path
spec = meta_finder.find_spec(modname, submodule_path)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/_pytest/assertion/rewrite.py", line 92, in find_spec
if self._early_rewrite_bailout(name, state):
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/_pytest/assertion/rewrite.py", line 193, in _early_rewrite_bailout
path = PurePath(*parts).with_suffix(".py")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/pathlib.py", line 694, in with_suffix
raise ValueError("%r has an empty name" % (self,))
ValueError: PurePosixPath('.') has an empty name
pytest -k non_init_parent_called
E AssertionError: Wrong results for file "non_init_parent_called":
E
E Expected in testdata:
E 6: import-error
E 14: non-parent-init-called
E 22: no-member
E 27: no-member
E 50: no-member
E
E Unexpected in testdata:
E 1: astroid-error
PurePosixPath
Feels like we should expand "." to the current working directory? 😓 That sounds like more issues..
We can switch that logic around? Only use the meta_finders in sys.meta_path that we know we support?
Interesting that the failure is coming from
pytestitself.
They probably have their own MetaPathFinder to handle all the fixture magic which we then try to use to locate an import.
That's why I'm debating on moving the check for known MetaPathFinders to above line 372, in _find_spec_with_path. That should resolve this I think.
That's why I'm debating on moving the check for known MetaPathFinders to above line 372, in _find_spec_with_path. That should resolve this I think.
I'd be curious to see the pylint test results: I worry about giving new priority to the builtin importers, if that's what you're suggesting:
>>> sys.meta_path
[<_distutils_hack.DistutilsMetaFinder object at 0x104452070>, <class '_frozen_importlib.BuiltinImporter'>, <class '_frozen_importlib.FrozenImporter'>, <class '_frozen_importlib_external.PathFinder'>]
@jacobtylerwalls See my last change, this is what I meant with changing the order of checks. We still rely on our own 4 Finders initially, but if that doesn't work we start trying MetaPathFinders for which we now stuff works.
This means that we would need to add new support by hand, but it is much less error prone.
pylint test suite passed with https://github.com/PyCQA/astroid/pull/1752/commits/f889b5d6bb49278d9736b799172f19b3f09631df.
If we agree that this is the way to go I'll add support for SixMetaPathImporter so the test case that I "unfailed" in this PR will pass again.
Looks good, thanks! (A test would be nice.)
I think the six test serves as a good test for the general logic. We can now import and find that module where we previously couldn't as we support the _SixMetaPathImporter now.
As for setuptools I don't think there is a good way to test it without introducing a dependency on it, which I don't really want.
I have tested this with https://github.com/jooola/pylint-import-error-bug to confirm that this branch does indeed fix the issue mentioned in the linked issue.
Great!
I think the six test serves as a good test for the general logic. We can now import and find that module where we previously couldn't as we support the _SixMetaPathImporter now.
Indeed. (Sorry, I thought you had said you were about to revert the changes to that test.)
Yeah, that was unclear. My bad!
Thanks for the reviews all. I'm skipping the coverage check on this one!