Closed MaartenStaa closed 10 months ago
Nice :+1: . 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.
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:
ctx
rule
or aspect
where the parameter impl
points to the functionattrs parameter to the
rule/
aspect` callAgain, 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.
@ndmitchell has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
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.
@ndmitchell merged this pull request in facebookexperimental/starlark-rust@335e74da4197f9607e2dd8ab5608549fba6e6e95.
In cases where you try to go-to-definition on a dotted identifier, e.g.
foo.bar.baz
, the LSP will try to find wherefoo
is defined, and check if it's astruct
definition with a property namedbar
, and jump to that. However, in cases where this location cannot be resolved, anunwrap_or_default
would result in jumping to to the top of the file. Instead, in such cases, jump to the definition of the variablefoo
itself, which is better than jumping to the top of the file.Fixes #90