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

Jedi Autocomplete failing to populate properties for @property wrapped functon that returns a class object #1299

Closed
cdknorow opened this issue Mar 15, 2019 · 27 comments

Comments

@cdknorow
Copy link

cdknorow commented Mar 15, 2019

Autocomplete is failing to return values for the @Property when it returns a class.

Here is example code that reproduces the issue.

class Project():
    def __init__(self, name):
        self.name = name
    def foo(self):
        return
    def bar(self):
        return

class Client():
    @property
    def project(self):
        return self._project

    @project.setter
    def project(self, name):
        self._project = Project(name)

c = Client()
c.project = ''
c.project.<tab>

expected behavior is to autocomplete with the properties of project. Instead it doesn't find any properties.

dir(c.project) however shows the expected results.

when trying to do autocomplete, with jedi disabled I am able to see the properties of Project with tab complete. Without it it just shows none.

@davidhalter
Copy link
Owner

With Jedi enabled, I'm seeing the properties of Project(). I cannot reproduce.

@lodagro
Copy link

lodagro commented Mar 19, 2019

This relates to jupyter/notebook#2435.
I can reproduce this.

@davidhalter
Copy link
Owner

So are you talking both about the Interpreter case?

@lodagro
Copy link

lodagro commented Mar 21, 2019

You mean using IPython? Yes.

@davidhalter
Copy link
Owner

davidhalter commented Mar 21, 2019

jedi.Interpreter is what IPython internally uses. I didn't understand that @cdknorow was talking about IPython completions. This is a big different, because if you use jedi.Script, it works without any problem.

The problem with this is basically that you can either have safe or correct completions. Jedi does quite a lot to keep you safe in this case. Consider the example:



class Client():
    @property
    def project(self):
        os.system('rm *')

This can go horribly wrong. Jedi tries to avoid executing code at all costs. I know that this example is something that most people are never going to encounter, but here's another one that led us to change it (I'm pretty sure because IPython people asked for it to not do what you guys want):

>>> class Client():
...     @property
...     def project(self):
...         print('x')
...         return str
... 
>>> c=Client()
>>> c.prox
ject(
>>> c.project.x

...capitalize(    ...format_map(    ...isprintable(   ...mro(           ...split(
...casefold(      ...index(         ...isspace(       ...partition(     ...splitlines(
...center(        ...isalnum(       ...istitle(       ...replace(       ...startswith(
...count(         ...isalpha(       ...isupper(       ...rfind(         ...strip(
...encode(        ...isdecimal(     ...join(          ...rindex(        ...swapcase(
...endswith(      ...isdigit(       ...ljust(         ...rjust(         ...title(
...expandtabs(    ...isidentifier(  ...lower(         ...rpartition(    ...translate(
...find(          ...islower(       ...lstrip(        ...rsplit(        ...upper(
...format(        ...isnumeric(     ...maketrans(     ...rstrip(        ...zfill(

Can you spot the x twice? This is not what you want in a safe system.

@lodagro
Copy link

lodagro commented Mar 21, 2019

Ok, this is clear.
Is there a way to control this through config?

@zpincus
Copy link

zpincus commented Mar 21, 2019

Does Jedi actually need to fetch the value of the property in order to know that there's a property of that name that could be autocompleted?

That is, in the example above:

>>> class Client():
...     @property
...     def project(self):
...         print('x')
...         return str
... 
>>> c=Client()
>>> c.prox
ject(
>>> c.project.x

why does pressing TAB after c.pro result in the property-function being executed (and x\n being printed)? Certainly the function has to be run to be able to complete on the contents of c.project (the second x that shows up). But naively it seems like it would be possible to complete the name of an existing property without trouble.

For that matter, it might (at least in IPython-like cases) be useful if typing c.project.[TAB] would emit a warning to the effect of "members of dynamic properties will not be completed unless jedi.complete_properties=True" (or whatever).

@davidhalter
Copy link
Owner

I feel like the unsafe variant is just bad no matter what. IMO we should strive for decompiling Python code and then parsing it in Jedi:

import uncompyle6
from jedi.evaluate.compiled.getattr_static import getattr_static

class Client():
    @property
    def project(self):
        print('x')
        return str(1, 2)


c = Client()
attr, is_get_descriptor = getattr_static(c, 'project')
if is_get_descriptor:
    print(attr)
    code = attr.fget.__code__
    uncompyle6.code_deparse(code)
    print()

And the output:

$ python3 disas.py 
<property object at 0x7f868f121a98>
print('x')
return str(1, 2)

Of course this is not finished yet, and it would take a bit to get it working, but it's not too hard. The only thing that I don't like is the additional dependency (it's quite big and it also has a GPL 3 license).

@lodagro
Copy link

lodagro commented Mar 22, 2019

For my use case uncomply6 does not offer a solution. I do want code to be executed, and i`m often using multiple levels deep in properties in the interpreter. See it as interactively walking a tree, where tab completion is an important element.

@davidhalter
Copy link
Owner

Hmmm. I'm not sure you actually want that in all cases...

However we could add a setting. It's just not a priority on my side at all, because it's kind of a weird setting (and I haven't heard that request a lot).

@lodagro
Copy link

lodagro commented Mar 22, 2019

A setting/behaviour like what @zpincus proposed would work for me, and likely also for others - discussing the jupyter/notebook#2435 issue.

@zpincus
Copy link

zpincus commented Mar 22, 2019

Just to chime in further, my use-case is very like Wouter's -- we have a deeply-nested tree (dynamically generated to mirror a given configuration of some hardware that the code is designed to control) that we use IPython to navigate and manipulate. So we also do need to descend into properties. Recognizing that this does run code, it makes sense for it to be a configuration option. But the only other choice we have for our use-case is to just disable Jedi in IPython.

I know that this probably seems to be getting toward XKCD 1172 to you, but autocompleting on property contents is longstanding IPython behavior and switching to Jedi does cause that behavior to silently change.

@ngoldbaum
Copy link

Perhaps it can be possible for IPython's wrapper around jedi to say it's ok to do this unsafely? That way IPython can opt-in and retain backward compatibility and you won't have a bunch of people turning off jedi support just to get back something that used to work for them.

@ngoldbaum
Copy link

Another solution - could we perhaps signal to jedi/IPython somehow (e.g. by assigning a magic attribute) that our properties are "safe" to complete, allowing us to explicitly opt-in to allowing completions?

@davidhalter
Copy link
Owner

The solution is probably configuration from IPython, i.e. Interpreter(safe=False).

@flying-sheep
Copy link
Contributor

flying-sheep commented Jun 3, 2019

For me, it doesn’t even work when I add a type hint, how come?

    @property
    def project(self) -> Project:
        return self._project

Also I think jedi should support simple cases like this, where only one possible type gets returned and there’s no branching. (Or if there is branching, all other branches raise Exceptions or return None)

@davidhalter
Copy link
Owner

davidhalter commented Jun 5, 2019

@flying-sheep The problem is that Jedi doesn't see that code. Jedi sees a property called project and can inspect the function behind the property with project.__code__, but that's already very complicated.

If Jedi had in fact have access to that code, we could easily infer the type of self._project with static analysis.

@flying-sheep
Copy link
Contributor

What would be the best way to make Jedi as powerful as possible in IPython? Can we e.g. make it see all code cells? Jedi currectly only gets the code of the current cell I think.

Will Jedi use e.g. inspect.getsource? Could we make that function work for classes defined in the notebook?

@flying-sheep
Copy link
Contributor

I figured something out. If IPython did this internally, getsource would start to work. Would that make jedi work?

import sys
import linecache
from pathlib import Path
from nbformat import read, NO_CONVERT

with (Path() / notebook_name).open() as fp:
    notebook = read(fp, NO_CONVERT)
code_cells = [
    c['source']
    for c in notebook['cells']
    if c['cell_type'] == 'code' and not c['source'].startswith('%%')
]

code = '\n'.join(code_cells)

linecache.cache['__main__'] = len(code), None, code.splitlines(keepends=True), '__main__'
sys.modules['__main__'].__file__ = '__main__'

@ngoldbaum
Copy link

It would be really weird for tab completion to behave differently in a notebook and repl, I don’t think any solution for this issue would be notebook-specific.

@flying-sheep
Copy link
Contributor

flying-sheep commented Jun 6, 2019

I disagree. A notebook is a static file containing code, like a module, so it makes sense that things defined in code cells have an influence on autocompletion.

@davidhalter
Copy link
Owner

I'm open to suggestions. I'm also open about changing Interpreter so you guys have better completions. The problem is really getting the information reliably.

One way I just figured out for readline backed REPLs could be this:
https://stackoverflow.com/questions/6558765/how-do-you-see-the-entire-command-history-in-interactive-python
However I fail to see how I do this for IPython / Notebooks.

@ngoldbaum I also think REPL completion should be used if possible, but since we're already combining approaches anyway any extra information - like the lines given, could be really valuable and improve the general experience for people.

davidhalter added a commit that referenced this issue Aug 11, 2019
This time we catch all exceptions and try to avoid issues for the user.

This is only happening when working with an Interpreter. I don't feel this is
necessary otherwise.

See #1299
@davidhalter
Copy link
Owner

Since nobody really cared about the safety aspect of this feature in Interpreter, I'm removing it for Interpreter. I don't really mind personally. It's ok either way. If somebody wants this feature, I'm happy to reintroduce it with an option. For now this should be ok.

I just released this in 0.15.0, so you can just install/upgrade Jedi and everything should be fine.

@zymergen-luke
Copy link

Thank you @davidhalter!

@calvin-sykes
Copy link

Is this a done deal now or is there any scope for allowing users to choose the behaviour they want? E.g. setting an attribute on the property object to opt out from executing it?
The use case I have in mind is a property-like decorator that allows a large dataset to be lazily loaded from disk and then cached the first time it's accessed. Once loaded the data is immutable so having to always access it via function call feels unnecessary.

@davidhalter
Copy link
Owner

It's still a job for a function. I personally avoid properties as much as possible in general. They are usually a bad idea for APIs, because there's no way to alter functionality with arguments.

Jedi can actually do both of these things, it's just not public, just set Interpreter._allow_descriptor_getattr_default = False.

@flying-sheep
Copy link
Contributor

Properties are amazing for APIs, depending on what you want to do. As soon as behavior needs to be customized, they are of course not the tool to use, but they’re ideal for when you want an attribute that is type-coercing or validating itself.

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

No branches or pull requests

8 participants