Fix setuptools>=64 import hooks

[DanielNoord]

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

This needs a test. Refs: pylint-dev/pylint#7306 and pypa/setuptools#3518.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

[DanielNoord]

@jacobtylerwalls This fixes the test that started failing after #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.

[coveralls]

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

[jacobtylerwalls]

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.

[DanielNoord]

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.

[DanielNoord]

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

[Pierre-Sassoulas]

Sure but I think this need a rebase the changelog is not in 2.14.0

[DanielNoord]

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!

[Pierre-Sassoulas]

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 ?

[DanielNoord]

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]

Codecov Report

Merging #1752 (6ea1a7d) into main (ac41d4e) will increase coverage by 0.00%.
The diff coverage is 95.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%)	⬆️
pypy	88.45% <72.72%> (-0.04%)	⬇️
windows	92.35% <95.45%> (+<0.01%)	⬆️

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%)	⬇️

[Pierre-Sassoulas]

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).

[jacobtylerwalls]

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.)

[DanielNoord]

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.

[jacobtylerwalls]

Thanks for the description, that really helped focus my review. This is looking really good. One requested change on guarding against a missing find_spec and then some questions.

[jacobtylerwalls]

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

[DanielNoord]

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?

[jacobtylerwalls]

Interesting that the failure is coming from pytest itself.

[DanielNoord]

Interesting that the failure is coming from pytest itself.

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.

[jacobtylerwalls]

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'>]

[DanielNoord]

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

[DanielNoord]

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.

[jacobtylerwalls]

Looks good, thanks! (A test would be nice.)

[DanielNoord]

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.

[Pierre-Sassoulas]

👍

[jacobtylerwalls]

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.)

[DanielNoord]

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!