Go To Definition of decorated functions that get wrapped goes to the wrapping function

[PeterJCLaw]

This appears to be regression of https://github.com/DonJayamanne/pythonVSCode/issues/742

Environment data

• VS Code version: 1.22.2 and 1.23.0
• Extension version: 2018.4.0 (edit: confirmed not an issue under 2018.3.1)
• OS and version: Ubuntu 16.04.4 LTS (Xenial)
• Python version: 2.7 and 3.5 tested
• Type of virtual environment used: None (for Python 2.7), virtualenvwrapper (for Python 3.5)
• Relevant/affected Python packages and their versions: —

Actual behavior

Navigating to definition of a decorated function shows the definition of the decorator.

Note: this only reproduces in a package and not in a single file.

In one sense I can see why this is happening -- the decorator is actually what the symbol in question really is. However, that is not a useful thing for an editor to do and I would much prefer to be shown the definition of the decorated function instead.

I believe this is also a regression as I've only noticed this behaviour recently (I think it started failing some time after the new year, but I'm not sure).

Expected behavior

(copied from https://github.com/DonJayamanne/pythonVSCode/issues/742)
Navigating to definition of a decorated function should show the function in question.

Steps to reproduce:

(copied from https://github.com/DonJayamanne/pythonVSCode/issues/742)

• Create a package with a decorated function in one file and a usage of that function in another. I've put an example at https://gist.github.com/PeterJCLaw/38b39e6ee7bfa0cd69e00ce984aebde3.
• Open VSCode to the directory containing the above files
• Open the usages file
• Navigate to the definition of the my_context_manager function from either the import or the usage (I use Ctrl+Click, though F12 and the context menu also reproduce the issue)
• Will navigate to the wrong place

This issue shows up for both class- and function-based decorators and for both local and standard-library decorators.

Logs

None from either the Output pane or the developer tools

[PeterJCLaw]

It seems plausible that this is related to the environment changes alluded to in #1033, though that may be unrelated.

While I appreciate that the underlying functionality here is part of Jedi, the repeated reappearance of this bug is a little frustrating. Does this project have any automated testing around this sort of thing? If not, would you be open to some?
I'm imagining that these integration tests would take the form of a small number of chosen test cases which exercise common workflows, probably at the level of the Python <-> TypeScript interop boundary.

[suheti]

yes I'm seeing the same issue I'm using python plugin 2018.4.0, vscode 1.23.0

[brettcannon]

@PeterJCLaw more tests are always a good thing, so yes, we would accept them.

[PeterJCLaw]

@brettcannon I've just had a look at this and spotted that there are already some higher level integration tests (specifically src/test/definitions seems related) which I'd not spotted the last time I had a look :)

It seems more useful to add a test to those, rather than trying to create an entirely new infrastructure, so I'll look at doing that for this test case instead.

[PeterJCLaw]

Hrm, from what I can tell of looking at the results for a bunch of test cases, this issue appears to be a result of the change to using goto_definitions primarily (see footnote) and goto_assignments without follow_imports. I think I agree with #1033 (comment) that goto_assignments is what we should be using, however the missing piece here is that we also need to pass follow_imports=True (it defaults to False) so that the imports are followed.

As I've got a branch with a bunch of test cases for this, I'll add the fix to that and open a PR.

As a related point, I suspect that this means that the 'tooltip' lookups should also change (to using goto_assignments(follow_imports=True)) as they currently use goto_definitions, however that needs further investigation and probably deserves its own issue/PR.

Note on the current implementation: the first call in _process_request (goto_definition(follow_imports=True)) will always emit a TypeError. I'm not entirely sure why that is, however I suspect that may be a result of a misunderstanding of #1033 (comment) (cfee109 changes the first call from being goto_assignments to goto_definitions) which happens to appear to fix the previous issue by making that call always error, so priority ends up going to goto_definitions (when called without follow_imports=True).

Edited: a previous version was a little confused about which methods were used when.

[brettcannon]

To be very clear about this, it occurs only if the decorator returns a wrapping object, e.g. this does not cause any issues:

def identity(func):
  return func

@identity
def spam():
  pass

[PeterJCLaw]

@brettcannon to check my understanding of your clarification:

• do you get the failing case for a wrapped function in the same file as the usage? (I didn't think I'd seen that)
• do you get the failing case for a non-wrapped function in a different file as the usage? (I don't recall testing that, but could believe that this behaves correctly)

[brettcannon]

I tested across files like you did, I'm just saying that if the decorator doesn't modify the returned object there's no issue.