Update our calls to goto_assignments() for Jedi

[EvgeniyMakhmudov]

Environment data

• VS Code version: 1.21.0
• Extension version (available under the Extensions sidebar): 2018.2.1
• OS and version: Linux Mint 18.3
• Python version: 3.5
• Type of virtual environment used (if applicable): venv
• Relevant/affected Python packages and their versions:

Actual behavior

Go to definition of some used class not open file with it definition

Expected behavior

Go to definition of some used class open file with it definition

Steps to reproduce:

1. File a1.py have class Foo: pass
2. File b1.py have

from a1 import Foo
class Bar(Foo):pass

3. Click "go to definition" in file b1 on Foo, must open a1 file with definition of Foo, but moving cursor to first line with import of Foo
4. Downgrade to 2018.1.0 solve this problem

Logs

Output for Python in the Output panel (View→Output, change the drop-down the upper-right of the Output panel to Python)

##########Linting Output - flake8##########
##########Linting Output - flake8##########

Output from Console under the Developer Tools panel (toggle Developer Tools on under Help)

[/home/john_16/.vscode/extensions/donjayamanne.githistory-0.4.0]: Команда "git.viewFileHistory" встречается несколько раз в разделе commands.
/usr/share/code/resources/app/out/vs/workbench/workbench.main.js:257 [Extension Host] TypeDefinition null
/usr/share/code/resources/app/out/vs/workbench/workbench.main.js:257 [Extension Host] Extension "currenttodos" activated!
/usr/share/code/resources/app/out/vs/workbench/workbench.main.js:257 [Extension Host] vscode-icons is active!
/usr/share/code/resources/app/out/vs/workbench/workbench.main.js:257 [Extension Host] (node:23354) DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
t.log @ /usr/share/code/resources/app/out/vs/workbench/workbench.main.js:257

[PeterJCLaw]

This reproduces for me under Python 2, 3.5 and 3.6 (all virtualenvs on Ubuntu).

Notably on importing a1 as a whole module and referencing Foo as an attribute of that, going to the definition of Foo goes directly to the class definition in a1.py:

import a1
class Bar2(a1.Foo): pass

Separately, this bug feels reminiscent of https://github.com/DonJayamanne/pythonVSCode/issues/781 and https://github.com/DonJayamanne/pythonVSCode/issues/742; perhaps a fix might be found in a similar location?

[Varkal]

This reproduce for me on macOS python3.6.

[ocavue]

I found something that may help from here

This seems to be because the extension uses goto_definitions instead of goto_assignments when calling Jedi. Calling goto_assignments instead fixes this, but changes the behavior when going to an imported function.

i.e. with the current code clicking on bar in this code

from foo import bar

bar()

will jump to bar in the foo module, but if goto_assignments is used this will jump to the from foo import bar line.

I guess, vscode-python switch from goto_definitions to goto_assignments.

I think giving a setting option to choose between goto_definitions and goto_assignments is a better solution. (I like goto_definitions)

[brettcannon]

@ocavue we actually use both as necessary.

This appears to be a semantic change between Jedi 0.10.x and 0.11.x which we upgraded to in the 2018.2.0 release.

[ocavue]

@brettcannon you are right. I clone v0.9.0 Jedi and everything is fine. Thanks to python.jediPath setting.

[PeterJCLaw]

@brettcannon does that mean that there's now an open issue upstream for this? (I can't immediately see one, but would be happy to create one)

The jedi changelog doesn't note any changes around the goto* api methods in 0.11.x, it would be good to understand where you think the difference is?

[brettcannon]

@PeterJCLaw I did not open an issue upstream, no. Are you up for opening one?

[PeterJCLaw]

@brettcannon apologies for the delay on this. I've just had a look into this with the aim of understanding it better to report it upstream.

My understanding of things is as follows:

• definition requests make it into completion.py, specifically the section in _process_request() for lookup == 'definitions'
• while that code hasn't changed, the behaviour of goto_assignments(follow_imports=False) has done
• side-note: I can't tell why goto_assignments is called twice, once with the explicit follow_imports=False and once without. The latter appears redundant to me since the value of follow_imports is the same in both cases. (Is there some side-effect to the intervening goto_definitions call that we're making use of?)
• I strongly suspect that the change in goto_assignments is a bug-fix. The previous implementation was returning a value which had followed an import, despite being asked not to do so. I can't find a direct bug-report for that in jedi, though the commits which make the changes (davidhalter/jedi@6940900, davidhalter/jedi@619acbd, davidhalter/jedi@4a544c2, davidhalter/jedi@5ff7e3d) claim to be fixing a number of issues, including When in __init__, a module in current directory isn't found davidhalter/jedi#956 which appears related to import following

Based on all that (plus a hunch based on the name of the argument) I tried changing the value of follow_imports in the first call (such that it's passed named as True rather than named as False). Doing so fixes this bug for me.
If you (@brettcannon) agree I'll put up a PR in this repo with that fix (rather than opening a bug against jedi). Are there tests for this area which I could add a test case to?

I am slightly puzzled why we previously had follow_imports=False though as that seems undesirable, so I'm worried that there are other cases which I'm missing and that that breaks. Are there any similar cases I should check still work?

Edit: I've validated that the potentially related https://github.com/DonJayamanne/pythonVSCode/issues/781 and https://github.com/DonJayamanne/pythonVSCode/issues/742 still behave correctly; area there others?

[brettcannon]

@davidhalter might have more info on the change, but if we were relying on a buggy feature then changing on our end makes sense. Feel free to send a PR and we can have a look (knowing how this will affect IntelliSense all-up is just plain hard to track).

[davidhalter]

I owe you an answer here and in another place. I'll try to follow up ASAP. Hopefully tomorrow. Been quite busy. Sorry.

[davidhalter]

IMO you can just use goto_assignments and nothing else. Using it three times just makes it slower. goto_assignments should always return a result.

The only thing that might make sense is if you try goto_definitions and then goto_assignments, since in that direction you might have additional results. AFAIK in jedi 0.11.1 there weren't any serious bugs regarding goto_assignments that would make something like the code you wrote necessary. That was probably necessary in 0.9.0 or something else that is a few years old (like 2000 commits ago).

You can basically simplify your logic as much as possible after every release and just spam my issue tracker if there's bugs. Currently there's like 5 open bugs that affect jedi users and they are not really a problem, so I'm happy if people report new ones :)

[brettcannon]

Thanks for the info @davidhalter ! (And no need to apologize; it's open source and we all have only so much time to contribute.)

[EvgeniyMakhmudov]

Good news!
P.S. I am still on version 2018.1.0 and wait fix

[PeterJCLaw]

@brettcannon how large a fix would you like to make here?
I can see a couple of options: just the small fix which resolves the original report, or a larger change which simplifies the definition lookup code in _process_request to a single goto_assignments(follow_imports=True) call. I'm interested in helping fix this either way, though might need a bit more guidance on testing of the larger change (if that's the approach you want).

[brettcannon]

How large is large? I'm personally fine with a small fix, but if e.g. less than 20 lines or so for the bigger one and there is larger win for users then we will happily help you with the bigger fix.

[PeterJCLaw]

Great to see this fixed :)
Apologies for not replying here, I had meant to come back to this but haven't had a lot of time recently.

[brettcannon]

@PeterJCLaw it's open source; you were trying to do a kindness and you simply couldn't get to it. No worries. 😄