Update submodule_path after finding an editable install

[noah-weingarden]

Type of Changes

	Type
✓	🐛 Bug fix

Description

#1752 enabled astroid to find modules that use an editable install with import hooks, but it didn't enable finding submodules of a package that uses such an editable install. This PR updates submodule_path when necessary to fix that. All modules in the path after the first one should use ImportlibFinder as a result of this change. See this comment for a more detailed description and a minimal reproducible example.

I didn't update the changelog because the bug this is fixing is tied to a different bugfix that hasn't been released yet, but let me know if I should add one.

Refs pylint-dev/pylint#7306

[DanielNoord]

Thanks for this @noah-weingarden!

Do you think it will be possible to add a test for this?

[noah-weingarden]

Hmm, you mentioned in #1752 that you don't want to introduce a dependency on setuptools, so that definitely makes it harder. Could we test it by mocking sys.meta_path to have an _EditableFinder class? I'm just worried that a test like that would be brittle if setuptools changes how this works on their end.

To be honest though, I'm pretty confused in general about which aspects of the Python packaging world are stable. #1752 seems to depend on there being an _EditableFinder class in sys.meta_path which has a find_spec method, so if any of those things changes, astroid would probably need an update anyway, right? In that case, mocking sys.meta_path seems like it would make sense for now, but would fail to automatically reveal those hypothetical changes.

[DanielNoord]

Hmm, you mentioned in #1752 that you don't want to introduce a dependency on setuptools, so that definitely makes it harder. Could we test it by mocking sys.meta_path to have an _EditableFinder class? I'm just worried that a test like that would be brittle if setuptools changes how this works on their end.

To be honest though, I'm pretty confused in general about which aspects of the Python packaging world are stable. #1752 seems to depend on there being an _EditableFinder class in sys.meta_path which has a find_spec method, so if any of those things changes, astroid would probably need an update anyway, right? In that case, mocking sys.meta_path seems like it would make sense for now, but would fail to automatically reveal those hypothetical changes.

I have you are willing to put in the in the time to create the tests that mocks meta_path I think it would still be worthwhile. It is much easier to change such a test when setuptools changes their behaviour than being completely in the dark about what we expect setuptools to do.

[noah-weingarden]

Alrighty, I made a test! If you remove the change I made to astroid/interpreter/_import/spec.py, the test will pass the first assertion and fail the second one because the latter imports a subpackage.

(Side-note: To get this to work locally, I had to make a separate import for unittest.mock. For some reason, just importing unittest as the test module currently does results in AttributeError: module 'unittest' has no attribute 'mock'. I didn't have time to look into why, but it might be some pytest magic? Notably, running all the tests at once with just pytest is fine—nothing fails, unlike these CI runs—but running pytest tests/test_modutils.py results in the AttributeError unless I add import unittest.mock.)

[DanielNoord]

I'm wondering if we should extract this logic into a class, but for now I think this is fine!

[noah-weingarden]

I agree. Do you think the logic will apply to all of the keys in _MetaPathFinderModuleTypes? (I'm not familiar with six). If so, an improvement for now would be to use that.

[DanielNoord]

When I implemented this @jacobtylerwalls actually (rightly) stressed that we should probably be careful here: we rather want to have less support for installers we don't know than making assumptions that don't turn out to be true.

We can always add support for new ones if we encounter them.

The import system is apparently not that well defined..

[noah-weingarden]

Sounds good. I left the set alone, but moved it to be a top-level variable instead of treating it as a constant. Any new classes can be added to that set.

[DanielNoord]

@jacobtylerwalls Do you want to take a look?

Thanks for this @noah-weingarden

I'll create a PR myself to unblock the CI, that seems unassosciated.

[DanielNoord]

@noah-weingarden Could you rebase this?

[codecov]

Codecov Report

Merging #2033 (dca22c2) into main (27352c2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2033   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files          95       95           
  Lines       11007    11011    +4     
=======================================
+ Hits        10213    10217    +4     
  Misses        794      794           

Flag	Coverage Δ
linux	92.55% <100.00%> (+<0.01%)	⬆️
pypy	88.52% <100.00%> (+0.01%)	⬆️
windows	92.34% <100.00%> (+<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.40% <100.00%> (+0.05%)	⬆️

[jacobtylerwalls]

Looks good!