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

Fix for #2044 #2045

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Fix for #2044 #2045

wants to merge 6 commits into from

Conversation

maxalbert
Copy link

@maxalbert maxalbert commented Jan 13, 2025

At the moment this branch only contains a regression test for the issue described in #2044. Strangely, the test passes even though the (seemingly) same code executed from within IPython results in the crash as outlined in #2044.

To reproduce the crash, save the following code as bug.py:

import os, sys, jedi
from test.helpers import get_example_dir

sys.path = [
    str(get_example_dir('implicit_namespace_package_with_subpackages', 'ns1')),
    str(get_example_dir('implicit_namespace_package_with_subpackages', 'ns2')),
]

import pkg

Then run this script and drop into an IPython session:

$ ipython -i --Completer.debug=True bug.py

Pressing <tab> in the following line produces the crash:

In [1]: pkg.<tab>
   ...: Oops Jedi has crashed, please report a bug with the following:
   ...: """
   ...: argument should be a str or an os.PathLike object where __fspath__ returns a str, not 'NoneType'
   ...: s"""

By contrast, running the test in this branch does not produce an error:

pytest -v -k test_implicit_namespace_package_with_subpackages
======================== test session starts ========================
platform darwin -- Python 3.13.1, pytest-8.3.4, pluggy-1.5.0 -- /Users/maxalbert/work/code/3rd_party/jedi/.venv/bin/python
cachedir: .pytest_cache
rootdir: /Users/maxalbert/work/code/3rd_party/jedi
configfile: pytest.ini
testpaths: jedi, test
collected 3894 items / 3893 deselected / 1 selected

test/test_api/test_interpreter.py::test_implicit_namespace_package_with_subpackages PASSED                                                                                 [100%]

================= 1 passed, 3893 deselected in 0.74s ===================

get_example_dir('implicit_namespace_package_with_subpackages', 'ns2'),
]
project = Project('.', sys_path=sys_path)
interpreter = jedi.Interpreter("import pkg; pkg.", namespaces=[], project=project)
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm it feels like the problem is that you did not actually import pkg here and pass it to namespaces like namespaces=[locals()].

You would typically do what you've done in your reproduction case: import pkg after modifying the sys.path.

So it would be:

    sys.path = [...]
    import pkg
    interpreter = jedi.Interpreter("pkg.", namespaces=[locals()], project=project)

Once you get it running, it should probably not be named pkg, but something like implicit_namespace_package_test or something. Because pkg might be used for other things/tests/whatever and by importing it you are polluting sys.modules, which is fine with a non-common name. But that shouldn't be your first priority.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I tried your suggestion but unfortunately this results in the following error from within the test:

    def test_implicit_namespace_package_with_subpackages():
        sys.path = [
            get_example_dir('implicit_namespace_package_with_subpackages', 'ns1'),
            get_example_dir('implicit_namespace_package_with_subpackages', 'ns2'),
        ]
>       import pkg
E       ModuleNotFoundError: No module named 'pkg'

test/test_api/test_interpreter.py:601: ModuleNotFoundError

It's not clear to me why it can't find the module. My immediate guess is that pytest applies some path wrangling magic which makes it behave differently to a regular script, but I'll have to dig deeper before I can say what's really going on (any ideas or other suggestions are appreciated!).

Out of curiosity, why is it necessary to modify sys.path directly within the test, instead of passing it as an argument to Project? What is the purpose of the Project instance that is passed to the Interpreter?

Copy link
Author

Choose a reason for hiding this comment

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

Turns out that using monkeypatch.syspath_prepend() fixes the problem. I have now tweaked the test according to your suggestion and it exhibits the bug described in #2044. I'll see if I can come up with a solution now (but again, any suggestions are always welcome).

Copy link
Owner

Choose a reason for hiding this comment

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

Out of curiosity, why is it necessary to modify sys.path directly within the test, instead of passing it as an argument to Project? What is the purpose of the Project instance that is passed to the Interpreter?

You are actually right, the project instance isn't necessary at all anymore. The important part is that it gets passed as an actual object to the Interpreter. This was a wrong idea on my end.

I'm pretty sure we'll find something when looking at the traceback. This looks like a typical "somewhere in the traceback there's an if is not None missing".

Copy link
Author

Choose a reason for hiding this comment

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

Below is the traceback. I tried inserting a check for is not None in jedi/inference/compiled/access.py:186 (the bottom-most frame of the traceback) as well as near jedi/inference/gradual/typeshed.py:194 (a bit higher up the stack). However, neither makes the test pass - while the crash is gone, completion does not result in the expected modules pkgA, pkgB.

Can you tell from the traceback if there are any other sensible places for this check? Or do we need a more sophisticated fix?

=================================== FAILURES ===================================
_______________ test_implicit_namespace_package_with_subpackages _______________

self = MixedName(<CompiledName: (<CompiledValueName: string_name=NamespaceObject>).pkg_implicit_namespace_package_test>)
args = (), kwargs = {}
cache_dict = {<function MixedName.infer at 0x102a25260>: {}}, dct = {}
key = ((), frozenset())

    @wraps(method)
    def wrapper(self, *args, **kwargs):
        cache_dict = self.__dict__.setdefault('_memoize_method_dct', {})
        dct = cache_dict.setdefault(method, {})
        key = (args, frozenset(kwargs.items()))
        try:
>           return dct[key]
E           KeyError: ((), frozenset())

jedi/cache.py:110: KeyError

During handling of the above exception, another exception occurred:

self = <AccessHandle of DirectObjectAccess(<module 'pkg_implicit_namespace_package_test' (nam..)>
args = ('py__file__',), kwargs = {}
cache_dict = {<function AccessHandle._cached_results at 0x1029f3a60>: {(('get_qualified_names',), frozenset()): (), (('is_instance',), frozenset()): False, (('py__name__',), frozenset()): 'pkg_implicit_namespace_package_test'}}
dct = {(('get_qualified_names',), frozenset()): (), (('is_instance',), frozenset()): False, (('py__name__',), frozenset()): 'pkg_implicit_namespace_package_test'}
key = (('py__file__',), frozenset())

    @wraps(method)
    def wrapper(self, *args, **kwargs):
        cache_dict = self.__dict__.setdefault('_memoize_method_dct', {})
        dct = cache_dict.setdefault(method, {})
        key = (args, frozenset(kwargs.items()))
        try:
>           return dct[key]
E           KeyError: (('py__file__',), frozenset())

jedi/cache.py:110: KeyError

During handling of the above exception, another exception occurred:

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x10362d810>

    def test_implicit_namespace_package_with_subpackages(monkeypatch):
        sys_path_dir1 = get_example_dir('implicit_namespace_package_with_subpackages', 'ns1')
        sys_path_dir2 = get_example_dir('implicit_namespace_package_with_subpackages', 'ns2')
        monkeypatch.syspath_prepend(sys_path_dir1)
        monkeypatch.syspath_prepend(sys_path_dir2)
    
        import pkg_implicit_namespace_package_test
        interpreter = jedi.Interpreter(
            "pkg_implicit_namespace_package_test.",
            namespaces=[locals()],
            project=Project('.')
        )
>       comps = interpreter.complete()

test/test_api/test_interpreter.py:608: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
jedi/api/helpers.py:487: in wrapper
    return func(self, line, column, *args, **kwargs)
jedi/api/__init__.py:215: in complete
    return completion.complete()
jedi/api/completion.py:178: in complete
    cached_name, completion_names = self._complete_python(leaf)
jedi/api/completion.py:297: in _complete_python
    cached_name, n = self._complete_trailer(dot.get_previous_leaf())
jedi/api/completion.py:398: in _complete_trailer
    values = infer_call_of_leaf(inferred_context, previous_leaf)
jedi/inference/helpers.py:79: in infer_call_of_leaf
    return context.infer_node(leaf)
jedi/inference/context.py:224: in infer_node
    return infer_node(self, node)
jedi/inference/syntax_tree.py:157: in infer_node
    return _infer_node_if_inferred(context, element)
jedi/inference/syntax_tree.py:170: in _infer_node_if_inferred
    return _infer_node_cached(context, element)
jedi/inference/cache.py:44: in wrapper
    rv = function(obj, *args, **kwargs)
jedi/inference/syntax_tree.py:175: in _infer_node_cached
    return _infer_node(context, element)
jedi/debug.py:81: in wrapper
    return func(*args, **kwargs)
jedi/inference/syntax_tree.py:83: in wrapper
    return func(context, *args, **kwargs)
jedi/inference/syntax_tree.py:185: in _infer_node
    return infer_atom(context, element)
jedi/inference/syntax_tree.py:305: in infer_atom
    return context.py__getattribute__(atom, position=position)
jedi/inference/context.py:77: in py__getattribute__
    values = ValueSet.from_sets(name.infer() for name in names)
jedi/inference/base_value.py:430: in from_sets
    for set_ in sets:
jedi/inference/context.py:77: in <genexpr>
    values = ValueSet.from_sets(name.infer() for name in names)
jedi/cache.py:112: in wrapper
    result = method(self, *args, **kwargs)
jedi/inference/compiled/mixed.py:137: in infer
    return _create(self._inference_state, compiled_value, module_context)
jedi/inference/cache.py:44: in wrapper
    rv = function(obj, *args, **kwargs)
jedi/inference/compiled/mixed.py:278: in _create
    tree_values = to_stub(compiled_value)
jedi/inference/gradual/conversion.py:184: in to_stub
    stub_module = _load_stub_module(value.get_root_context().get_value())
jedi/inference/gradual/conversion.py:100: in _load_stub_module
    return try_to_load_stub_cached(
jedi/inference/gradual/typeshed.py:146: in try_to_load_stub_cached
    _try_to_load_stub(inference_state, import_names, *args, **kwargs)
jedi/inference/gradual/typeshed.py:194: in _try_to_load_stub
    file_path = method()
jedi/inference/compiled/value.py:312: in py__file__
    return self.access_handle.py__file__()  # type: ignore[no-any-return]
jedi/inference/compiled/subprocess/__init__.py:508: in _workaround
    return self._cached_results(name, *args, **kwargs)
jedi/cache.py:112: in wrapper
    result = method(self, *args, **kwargs)
jedi/inference/compiled/subprocess/__init__.py:512: in _cached_results
    return self._subprocess.get_compiled_method_return(self.id, name, *args, **kwargs)
jedi/inference/compiled/subprocess/functions.py:26: in get_compiled_method_return
    return getattr(handle.access, attribute)(*args, **kwargs)
jedi/inference/compiled/access.py:186: in py__file__
    return Path(self._obj.__file__)
.devbox/nix/profile/default/lib/python3.13/pathlib/_local.py:503: in __init__
    super().__init__(*args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <[AttributeError("'pathlib._local.PosixPath' object has no attribute '_raw_paths'") raised in repr()] PosixPath object at 0x104819070>
args = (None,), paths = [], arg = None, path = None

    def __init__(self, *args):
        paths = []
        for arg in args:
            if isinstance(arg, PurePath):
                if arg.parser is not self.parser:
                    # GH-103631: Convert separators for backwards compatibility.
                    paths.append(arg.as_posix())
                else:
                    paths.extend(arg._raw_paths)
            else:
                try:
                    path = os.fspath(arg)
                except TypeError:
                    path = arg
                if not isinstance(path, str):
>                   raise TypeError(
                        "argument should be a str or an os.PathLike "
                        "object where __fspath__ returns a str, "
                        f"not {type(path).__name__!r}")
E                   TypeError: argument should be a str or an os.PathLike object where __fspath__ returns a str, not 'NoneType'

.devbox/nix/profile/default/lib/python3.13/pathlib/_local.py:132: TypeError
=========================== short test summary info ============================
FAILED test/test_api/test_interpreter.py::test_implicit_namespace_package_with_subpackages - TypeError: argument should be a str or an os.PathLike object where __fspath__ returns a str, not 'NoneType'
====================== 1 failed, 3893 deselected in 1.03s ======================

Copy link
Owner

@davidhalter davidhalter Jan 14, 2025

Choose a reason for hiding this comment

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

Kind of hard to say. I'm not surprised at all that this is not working. I personally would consider there not being any completions (but no crash) a "fix".

But if you want the name, you probably would have to import it first. This is not static analysis, this is looking at the actual Python objects and trying to use __dir__ and other methods to try and lookup objects.

I'm not sure if there's other issue, but I'm pretty sure that in your case pkg.pkgA would raise an AttributeError if actually executed in a Python shell.

Copy link
Author

@maxalbert maxalbert Jan 15, 2025

Choose a reason for hiding this comment

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

Ah! Excellent points, thanks for the clarification. Having thought about it I think you are right that no crash (but also no completions) could be regarded as a "fix".

I'm happy to get the PR ready with this solution. However, jedi already supports auto-expansion of submodules (even if they haven't been imported yet). For example, the following will complete with pkgA, pkgB.

In [1]: import pkg.<tab>

I don't know how easy it would be to support this also in the case where pkg was imported previously. Could jedi inspect the pkg object and if it represents a Python module check for submodules? It feels like tweaking the logic somewhere around L282-298 in jedi/api/completion.py might do the trick? (Those two elif branches are the ones that are executed for import pkg.<tab> and import pkg; pkg.<tab>, respectively.)

Copy link
Owner

Choose a reason for hiding this comment

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

Well, import pkg; pkg. is something very different than import pkg.. In the former case it depends whether something previously imported other modules on pkg (if not there are no objects defined on the namespace). In the latter case all files/dirs in pkg/ should be listed, because they are all valid!

In the test that you wrote, pkg.pkgA won't actually work when you execute it, so why complete it? It would just be wrong. If that is something you want to support, you would need to add an additional test case that actually imports all of it like

import pkg.pkgA
pkg.  # Now it complete pkgA, but not pkgB!

But please also keep the current test, the behavior you are testing makes a lot of sense!

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Thanks for your continued patience and explanations @davidhalter, this makes perfect sense.

For context, I stumbled across the bug in #2044 while familiarising myself with the python-polylith project, which provides Python tooling for the Polylith Architecture. python-polylith leverages namespace packages to enable composability and reusability of individual "bricks" (= building blocks) in the architecture, where each brick corresponds to a portion in the namespace package. A key benefit of the Polylith development setup is that all individual bricks are readily accessible from within an interactive Python session and can be reused and combined for prototyping and development.

Thus the reason I was keen to be able to complete import pkg; pkg. was to enable discoverability of those individual bricks within the namespace package. In other words, I really wanted the namespace package to behave as if it was a regular Python package whose toplevel __init__.py file looked like this:

from . import pkgA
from . import pkgB
...

That way, all bricks (here: pkgA, pkgB, ...) would be immediately discoverable using tab completion after importing the toplevel namespace package.

That said, thanks to your comments I now understand that this is not how namespace packages work. And I agree that it would be misleading to complete the subpackages if they are not actually available as part of the namespace. (Out of interest, do you see any conceptual reason why namespace packages couldn't behave this way if the Python folks decided to support this behaviour in the future?)

Thank you again for this discussion, I've learned a lot from it. I'll aim to get a version of the PR ready for review with the behaviour and tests you suggested.

Copy link
Owner

Choose a reason for hiding this comment

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

(Out of interest, do you see any conceptual reason why namespace packages couldn't behave this way if the Python folks decided to support this behaviour in the future?)

They could certainly. But this is generally not how Python behaves. It behaves the same as normal packages where a import foo also doesn't give you access to the module foo.bar. So there it could be enabled as well. But what about dir(foo)? Should that do essentially an ls on foo/? It just gets weird and very dynamic at that point, which is generally not what you want. At that point you could essentially just remove the necessity for imports altogether (which to be fair is an interesting idea).

I don't think this is the worst idea (at least not worse than a lot of other dynamic stuff in Python), but the dynamic nature of Python is IMO also its biggest weakness and it's not the reason why Python grew in the first place. I understand that there should be some dynamic tools (something like metaclasses, which could probably be better solved with macros (like rust)) or some runtime "reflection", but in general some of these capabilities made Python incredibly hard to optimize for. Additionally a mostly sound type system became impossible and a lot of people are just abusing the cool dynamic tools of Python. It would have been so nice to have a Python 4 or 5 without a lot of these tools, but they added more in Python 3, so this is not going to happen anymore now that Python is so big. It's a bit unfortunate, because all the really good and powerful (web/scientific/tools) libraries could have existed without the craziness that Python also is, but that's long gone. Sorry for the rant :)

BTW: You also have to factor in that the import machinery in Python is way more complicated than what you think. There are multiple ways of hooking into the import machinery and changing how to load code, to the point where some big companies created dynamic loading for Python code over a "cloud" (which is incredibly insane, but I guess some engineer had fun doing it).

Thus the reason I was keen to be able to complete import pkg; pkg. was to enable discoverability of those individual bricks within the namespace package.

If you want discoverability, why don't you use import pkg.? That should actually work and would also make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants