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

lsp jump to definition bad behavior #90

Closed
cjhopman opened this issue Aug 29, 2023 · 3 comments
Closed

lsp jump to definition bad behavior #90

cjhopman opened this issue Aug 29, 2023 · 3 comments

Comments

@cjhopman
Copy link
Contributor

often when doing jump-to-definition on a field, it just jumps me to the top of the current file. this is worse than doing nothing.

@cjhopman cjhopman changed the title starlark lsp jump to definition bad behavior lsp jump to definition bad behavior Aug 29, 2023
@MaartenStaa
Copy link
Contributor

@cjhopman this one sounds odd to me, it's not really behaviour I've seen. Can you provide some more context? What kind of field are you triggering the jump-to-definition on? And does it just jump to the start of the file (0:0), or to the argument for that symbol in a load statement?

I've seen the latter behaviour mainly before I implemented correct resolve_load and render_as_load for Bazel, so if that's what you're seeing, I'm wonder if the internal version of this project fully implements the things from LspContext.

@stagnation
Copy link

I see this too with the bazel mode:

def aspect_impl(target, ctx):
     name = ctx.rule.attr.name
     ...

Jumping from the name member of ctx.rule.attr goes to the top for me,
and some builtin methods/functions. <list variable>.append(<x>) jumps to the top from append.
Also ctx.actions.write from write.

@MaartenStaa
Copy link
Contributor

MaartenStaa commented Aug 30, 2023

@stagnation Ah, interesting, thanks for the insight! In general I would say anything that uses dotted definitions (foo.bar) have very poor support at the moment in the LSP. Most functionality in such cases defers to the first segment (i.e. foo in this example), but maybe that leads to some weird edge cases with go to definition.

MaartenStaa added a commit to MaartenStaa/starlark-rust that referenced this issue Aug 30, 2023
In cases where you try to go-to-definition on a dotted identifier, e.g.
`foo.bar.baz`, the LSP will try to find where `foo` is defined, and
check if it's a `struct` definition with a property named `bar`, and
jump to that. However, in cases where this location cannot be resolved,
an `unwrap_or_default` would result in jumping to to the top of the
file. Instead, in such cases, jump to the definition of the variable
`foo` itself, which is better than jumping to the top of the file.

Fixes facebook#90
facebook-github-bot pushed a commit that referenced this issue Aug 31, 2023
Summary:
In cases where you try to go-to-definition on a dotted identifier, e.g. `foo.bar.baz`, the LSP will try to find where `foo` is defined, and check if it's a `struct` definition with a property named `bar`, and jump to that. However, in cases where this location cannot be resolved, an `unwrap_or_default` would result in jumping to to the top of the file. Instead, in such cases, jump to the definition of the variable `foo` itself, which is better than jumping to the top of the file.

Fixes #90

Pull Request resolved: #92

Reviewed By: JakobDegen

Differential Revision: D48863997

Pulled By: ndmitchell

fbshipit-source-id: 07015bd7484d0c2d5e74544a9a91fcfbbd76c046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants