Skip to content

Allow Jedi "goto" to perform multiple hops for "go to definition" #443

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

Merged
merged 5 commits into from
Sep 30, 2023

Conversation

smacke
Copy link
Contributor

@smacke smacke commented Sep 15, 2023

The implementation of lsp definitions performs one hop of jedi "goto". This is inadequate for cases such as the following:

d = {}
d[0]
d

A "definitions" request on the final reference to d will run a jedi goto that finds the reference to d in d[0] = 0, which then gets filtered out by the language server because it's not considered a definition. To fix, when we run a goto that finds something that's not a definition, keep running gotos on the thing we found until we either do find one, or exhaust the chain.

Added a unit test to cover this case, as well as type definitions to the definitions plugin.

def _resolve_definition(
maybe_defn: Name, script: Script, settings: Dict[str, Any]
) -> Name:
while not maybe_defn.is_definition() and maybe_defn.module_path == script.path:
Copy link
Member

@krassowski krassowski Sep 16, 2023

Choose a reason for hiding this comment

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

Is there a guarantee that this will never become an infinite loop, for example if follow_imports is True and the two modules (wrongly) import the same symbol from each other? If not, is it worth adding a limit of how many iterations can be performed, setting it to something arbitrarily large and raising an informative exception if it is exceeded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now this doesn't follow hops across modules (that should be fixed as a followup potentially), but generally I agree it would be good to be robust to jedi or other weird user-introduced circular references. Added what feels like a reasonable limit to me (100 hops) but happy to adjust if there's another good reason to do so.

@ccordoba12 ccordoba12 added this to the v1.8.1 milestone Sep 16, 2023
@ccordoba12 ccordoba12 added the enhancement New feature or request label Sep 16, 2023
@ccordoba12 ccordoba12 changed the title allow jedi "goto" to perform multiple hops for "go to definition" Allow jedi "goto" to perform multiple hops for "go to definition" Sep 16, 2023
@smacke smacke requested a review from krassowski September 16, 2023 19:00
@ccordoba12
Copy link
Member

@krassowski, this looks good to me now. Are you ok with merging?

@krassowski
Copy link
Member

Yes. My only suggestion would be adding another test based on the code snippet in https://jedi.readthedocs.io/en/latest/docs/api.html#type-inference-goto to show that this still works as expected.

@krassowski
Copy link
Member

Also, was there ever a discussion upstream on why this is preferred behaviour in jedi?

@ccordoba12
Copy link
Member

Also, was there ever a discussion upstream on why this is preferred behaviour in jedi?

Not to my knowledge. @davidhalter, we'd appreciate your input here if you have some free time.

@davidhalter
Copy link

This should probably be changed upstream, yes. I feel like this implementation has almost no upsides and a lot of downsides when it comes to potential caching issues or recursive stuff (it's probably also considerably slower).

@smacke
Copy link
Contributor Author

smacke commented Sep 25, 2023

This should probably be changed upstream, yes.

Depending on the application, I can see the value of a goto that visits intermediate non-definition writes and doesn't go straight to the definition. If this were changed upstream, I think it would make most sense to add a new kwarg to jedi's goto called skip_to_definition or something like that and have it default to False to avoid breaking anything that relies on the existing behavior. Then in pylsp we would set skip_to_definition=True.

I unfortunately won't have cycles for a little while to implement anything on the jedi side if the consensus is to change this upstream. If other folks are similarly lacking cycles, maybe we could add something in pylsp that does the following:

  • if the signature of goto supports skip_to_definition or whatever, use that
  • otherwise, use the strategy in this PR that does multiple hops

That way, whenever somebody does have cycles to submit a PR to jedi implementing this suggestion (assuming it makes sense of course), it will get automatically picked up by pylsp.

^ What do folks think about that?

@ccordoba12
Copy link
Member

If other folks are similarly lacking cycles

Yeah, I think we don't have time for that.

maybe we could add something in pylsp that does the following:

  • if the signature of goto supports skip_to_definition or whatever, use that
  • otherwise, use the strategy in this PR that does multiple hops

Until this is fixed in Jedi, I think there's no need to implement the first part and only go with the second one (which is what you've already done).

@krassowski, what do you think?

@krassowski
Copy link
Member

Yes, we are version pinning jedi in python-lsp-server anyways so it makes sense to keep it as-is. Let's just add the test case I mentioned earlier, open an issue to track the future planned improvement (whether on jedi side or in here) and then I think we can merge this patch in.

@smacke
Copy link
Contributor Author

smacke commented Sep 29, 2023

Let's just add the test case I mentioned earlier

Done! (My assumption is that we do not want the multihop behavior in this case, since the 1-hop goto is still an actual definition)

open an issue to track the future planned improvement (whether on jedi side or in here)

I will do it here: #446

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

This looks good to me, just one comment to update.

@ccordoba12 ccordoba12 changed the title Allow jedi "goto" to perform multiple hops for "go to definition" Allow Jedi "goto" to perform multiple hops for "go to definition" Sep 30, 2023
@ccordoba12 ccordoba12 merged commit 4211502 into python-lsp:develop Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants