Adjust is_namespace() to check ModuleSpec.loader

[perrinjerome]

Only consider modules with submodule_search_locations namespaces.

Type of Changes

	Type
✓	🐛 Bug fix

Description

Make astroid.interpreter._import.util.is_namespace only consider modules using a loader set to NamespaceLoader or None as namespaces.
This fixes a problem that six.moves brain was not effective if six.moves was already imported.

Closes #2409

[DanielNoord]

@jacobtylerwalls what do you think of this PR? It seems like a correct approach right?

[perrinjerome]

I pushed force to change the commit message of this "option 1" and also the "option 2" from #2411 this was referencing a wrong issue.

[jacobtylerwalls]

Yes, this looks promising, thank you!

[perrinjerome]

This is a draft pull request because I was suggesting this and #2411 at the same time, to discuss two possible approaches. From the feedback, it seems that this option would be better, so I will close the other pull request and mark this one as ready for review.

[DanielNoord]

@jacobtylerwalls do you want to review this as well?

[jacobtylerwalls]

Yes, on second thought, I would like to take a moment to consider this change in more detail. Reading PEP 451, I'm concerned that is not None is the demonstrated way to detect namespace packages, so I wouldn't mind the opportunity to look into it a little further.

[perrinjerome]

Thanks, it's good idea to check in more details, I am myself not convinced that this is the best way of detecting namespace packages, the example code you linked and also https://peps.python.org/pep-0451/#namespace-packages describe namespace packages as "a spec with loader set to None".

In the case of six.moves, the loader is not None:

(Pdb) import six.moves
(Pdb) six.moves.__spec__.loader
<six._SixMetaPathImporter object at 0x7f66230c99d0>

From this and from the PEP, it seems the condition should include loader is not None instead, something like this:

    return (
        found_spec is not None
        and found_spec.submodule_search_locations is not None
        and found_spec.loader is None
        and found_spec.origin is None
    )

but after trying a bit, in practice for namespace packages the loader is not None, it is importlib.machinery.NamespaceLoader ( and importlib._bootstrap_external._NamespaceLoader for python < 3.11 ). I did not research why this is different from the PEP.

Would it make sense to adjust the patch with something like this ? ( to be applied on top of this patch ):

diff --git a/astroid/interpreter/_import/util.py b/astroid/interpreter/_import/util.py
index cb947800..939f97d5 100644
--- a/astroid/interpreter/_import/util.py
+++ b/astroid/interpreter/_import/util.py
@@ -8,6 +8,11 @@ import pathlib
 import sys
 from functools import lru_cache
 from importlib._bootstrap_external import _NamespacePath
+
+if sys.version_info >= (3, 11):
+    from importlib.machinery import NamespaceLoader
+else:
+    from importlib._bootstrap_external import _NamespaceLoader as NamespaceLoader
 from importlib.util import _find_spec_from_path  # type: ignore[attr-defined]
 
 from astroid.const import IS_PYPY
@@ -99,6 +104,8 @@ def is_namespace(modname: str) -> bool:
 
     return (
         found_spec is not None
-        and found_spec.submodule_search_locations
-        and found_spec.origin is None
+        and found_spec.submodule_search_locations is not None
+        and (
+            found_spec.loader is None or isinstance(found_spec.loader, NamespaceLoader)
+        )
     )

I have only tested this patch on python3.9 so far.

EDIT: also in the patch I have removed origin is None, there is no good reason for that, thinking twice, it was probably a mistake.

[perrinjerome]

I did not research why this is different from the PEP.

I found https://bugs.python.org/issue35673 which mentions a "documentation bug".

[jacobtylerwalls]

EDIT: also in the patch I have removed origin is None, there is no good reason for that, thinking twice, it was probably a mistake.

I was following the example here, which gives:

if spec.origin is None and spec.submodule_search_locations is not None:
    # namespace package

[jacobtylerwalls]

@perrinjerome Alternatively, does this diff work for you? This looks like a mistake.

diff --git a/astroid/interpreter/_import/util.py b/astroid/interpreter/_import/util.py
index a8af9ec6..afead29f 100644
--- a/astroid/interpreter/_import/util.py
+++ b/astroid/interpreter/_import/util.py
@@ -32,7 +32,7 @@ def is_namespace(modname: str) -> bool:
     # not, but requires instead that each single parent ('astroid', 'nodes', etc.)
     # be specced from left to right.
     processed_components = []
-    last_submodule_search_locations: _NamespacePath | None = None
+    last_submodule_search_locations = []
     for component in modname.split("."):
         processed_components.append(component)
         working_modname = ".".join(processed_components)

[jacobtylerwalls]

Never mind, sorry, I ran your test and it doesn't pass with my suggestion.

Your latest suggested patch looks good to me--thanks for doing the research.

[perrinjerome]

Thank you @jacobtylerwalls , I have updated the pull request with the patch we discussed.

[jacobtylerwalls]

Thanks!

[jacobtylerwalls]

Ah, do you have any guidance on the failing pylint run? Did that expose a logic problem?

[perrinjerome]

I think I have fixed the failing pylint test now, by reorganizing imports

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.76%. Comparing base (de942f3) to head (404e6c4).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2410      +/-   ##
==========================================
+ Coverage   92.75%   92.76%   +0.01%     
==========================================
  Files          94       94              
  Lines       11088    11090       +2     
==========================================
+ Hits        10285    10288       +3     
+ Misses        803      802       -1     

Flag	Coverage Δ
linux	92.57% <100.00%> (+0.01%)	⬆️
pypy	92.76% <100.00%> (+2.00%)	⬆️
windows	92.66% <100.00%> (+0.31%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
astroid/interpreter/_import/util.py	91.48% <100.00%> (+0.58%)	⬆️

... and 1 file with indirect coverage changes

[DanielNoord]

@jacobtylerwalls this is good to go right?

[jacobtylerwalls]

Thanks, looks great!

[perrinjerome]

Thank you !