Skip to content

Update our calls to goto_assignments() for Jedi #1033

Closed
#1418
Closed
Update our calls to goto_assignments() for Jedi#1033
#1418
Assignees
MikhailArkhipov
Labels
bugIssue identified by VS Code Team member as probable bug
Milestone
April 2018

Description

@EvgeniyMakhmudov
Evgeniy Makhmudov (EvgeniyMakhmudov)
opened on Mar 11, 2018

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
  1. 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
  2. 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

Activity

  1. Brett Cannon (brettcannon)
    added
    bugIssue identified by VS Code Team member as probable bug
    needs verification
    feature-code navigation
    on Mar 12, 2018

  2. PeterJCLaw commented on Mar 13, 2018

    @PeterJCLaw
    Peter Law (PeterJCLaw)
    on Mar 13, 2018

    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?

  3. Varkal commented on Mar 17, 2018

    @Varkal
    Romain Moreau (Varkal)
    on Mar 17, 2018

    This reproduce for me on macOS python3.6.

    2018-03-17 23 24 44

  4. ocavue commented on Mar 20, 2018

    @ocavue
    ocavue
    on Mar 20, 2018

    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)

  5. Brett Cannon (brettcannon)
    added
    needs PR
    upstream-jedi
    and removed
    needs verification
    on Mar 20, 2018

  6. brettcannon commented on Mar 20, 2018

    @brettcannon
    Brett Cannon (brettcannon)
    on Mar 20, 2018
    Member

    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.

  7. ocavue commented on Mar 21, 2018

    @ocavue
    ocavue
    on Mar 21, 2018

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

  8. Brett Cannon (brettcannon)
    closed this as completedon Mar 21, 2018
  9. Brett Cannon (brettcannon)
    added
    closed-upstream
    and removed
    needs PR
    on Mar 21, 2018

  10. PeterJCLaw commented on Mar 21, 2018

    @PeterJCLaw
    Peter Law (PeterJCLaw)
    on Mar 21, 2018

    Brett Cannon (@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?

  11. 12 remaining items

  12. EvgeniyMakhmudov commented on Apr 5, 2018

    @EvgeniyMakhmudov
    Evgeniy Makhmudov (EvgeniyMakhmudov)
    on Apr 5, 2018
    Author

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

  13. Brett Cannon (brettcannon)
    assigned
    Don Jayamanne (DonJayamanne)
    and unassigned
    Don Jayamanne (DonJayamanne)
    on Apr 5, 2018

  14. PeterJCLaw commented on Apr 6, 2018

    @PeterJCLaw
    Peter Law (PeterJCLaw)
    on Apr 6, 2018

    Brett Cannon (@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).

  15. brettcannon commented on Apr 9, 2018

    @brettcannon
    Brett Cannon (brettcannon)
    on Apr 9, 2018
    Member

    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.

  16. Mikhail Arkhipov (MikhailArkhipov)
    self-assigned this
    on Apr 16, 2018
  17. Mikhail Arkhipov (MikhailArkhipov)
    closed this as completedin #1418on Apr 17, 2018
  18. Brett Cannon (brettcannon)
    added
    validate fix
    on Apr 17, 2018

  19. PeterJCLaw commented on Apr 17, 2018

    @PeterJCLaw
    Peter Law (PeterJCLaw)
    on Apr 17, 2018

    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.

  20. brettcannon commented on Apr 18, 2018

    @brettcannon
    Brett Cannon (brettcannon)
    on Apr 18, 2018
    Member

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

  21. Brett Cannon (brettcannon)
    removed
    validate fix
    on Apr 30, 2018
  22. Peter Law (PeterJCLaw)
    added 2 commits that reference this issue on May 19, 2018

    Add tests for current cases

    d1e5eb9

    Make navigation to definitions follow imports

    baa6310
  23. Don Jayamanne (DonJayamanne)
    added a commit that references this issue on Jun 5, 2018

    Ensure navigation to definitons follows imports and is transparent to…

    3b2667e
  24. lock
    locked as resolved and limited conversation to collaborators on Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugIssue identified by VS Code Team member as probable bug

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions