fix: jump to definition on dotted definition

[MaartenStaa]

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

[stagnation]

Nice 👍 . Verified on top of pull/82.
It now goes to the definition of the base member, if available I guess.

def aspect_impl(target, ctx):
    name = ctx.rule.attr.name
    out = ctx.actions.declare_file(name + ".aspect.tree")

Any segment of ctx goes to ctx the function argument.
I guess that makes sense for most variables.
But ctx is a little god object, so clicking go-to-def its member is hard.
The perfect solution would be to show the docs for functions,
and go to the real attribute defintions for {attr,files,...}.<member>.
But that requires a lot of special case logic.

 traverse = aspect(
   implementation = aspect_impl,
   attr_aspects = ["*"],
   attrs = {
       "_multiplicative": attr.string(default = "1"),
    }
 )

Clicking string here does nothing, which is good, there is no attr symbol to find.

[MaartenStaa]

Yeah, this PR is really just fixing a broken case. Ideally you'd have special handling for ctx parameters, but as you said, that requires quite some special logic. This crate is mainly concerned with Starlark as described in the spec, which doesn't do rules/aspects like in Bazel and Buck. You'd have to:

• Detect that the parameter is named ctx
• Find the name of the function you're in
• Find a call to rule or aspect where the parameter impl points to the function
• Find the definition for the attribute in the attrs parameter to the rule/aspect` call

Again, not sure if that fits in the project, but also not really something that (at the moment) is easy to add when you're implementing the LspContext externally. WDYT @ndmitchell?

There's also more room for improvement. The jump to definition on dotted definitions foo.bar.baz looks for foo = struct(bar = ...), but only in the top level scope. A follow-up PR could also recurse into the correct scopes to find a closer definition.

[facebook-github-bot]

@ndmitchell has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ndmitchell]

Thanks for the fix - this is clearly an improvement!

At some point we're going to need some good unit tests for the LSP. We (at Meta) didn't write them, so I'm not going to insist on them. But I do worry that without those tests someone might break these features inadvertently.

As for how to implement ctx, I have no real idea. I imagine it has to be on LspContext because it is going to be application specific. But no ideas what a good interface would look like or how to expose the right information.

[facebook-github-bot]

@ndmitchell merged this pull request in 335e74d.