search

bazelbuild / vscode-bazel

Bazel support for Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=BazelBuild.vscode-bazel
Apache License 2.0
231 stars 76 forks source link

feat: Cmd-click to jump to a source file from a build rule #380

Closed kchodorow closed 2 months ago

kchodorow commented 2 months ago

Part of issue #353.

vogelsgesang commented 2 months ago

Thanks for your contribution, @kchodorow! Much appreciated 🙂

I am admittedly not sure which future direction we want to take regarding "Starlark / Bazel language features"... Should those be provided by the language server (which has a deeper logical understanding of the StarLark code) or by the TypeScript part of this extension (as proposed in this PR)?

On a related note, I am wondering if / how we would want to support glob expressions. I guess a hover information "glob expression matched 16 files" and a list of all the matched files would be useful? Not sure...

@cameron-martin @withered-magic What is your opinion on the long-term direction in which we should go here? Should this functionality be covered by the language servers or not?

Does one of your language servers already provide comparable functionality? If none of the language servers currently provides this functionality already, I would say we should move ahead with the PR as currently proposed (to unlock immediate user benefit) and then migrate this functionality into the language servers later on...

withered-magic commented 2 months ago

There was actually a similar request for starpls here, but going in the opposite direction (from source file to corresponding BUILD.bazel files): https://github.com/withered-magic/starpls/issues/100. Haven't gotten a chance to look into too much but it actually should be pretty straightforward to implement!

To me it seems reasonable for these sorts of functionalities to be covered by the language server and to have the language server be the only handler for fielding goto-def/find reference requests, which this feature sounds like

cameron-martin commented 2 months ago

bazel-lsp does have go-to-definition from BUILD files to source files. I'm generally for trying to push as much functionality into the language servers, but I'm also not against giving a several-line fix to overlapping functionality in this extension, particularly considering no language server is enabled by default.

vogelsgesang commented 2 months ago

Works smoothly 🙂

https://github.com/bazelbuild/vscode-bazel/assets/6820896/9ba609fb-0bb4-4252-8bae-7abd32b55700

One small issue which I noticed: The individual path components are linked individually (i.e. the py extension is linked, and not the complete file name). I think we can fix this by using a LocationLink instead of a Location and by setting the originSelectionRange. I think the necessary range should already be available from the variable range defined in line 43

Not introduced by you, but a patch to fix this would still be welcome :)

vogelsgesang commented 2 months ago

There was actually a similar request for starpls here, but going in the opposite direction (from source file to corresponding BUILD.bazel files): https://github.com/withered-magic/starpls/issues/100. Haven't gotten a chance to look into too much but it actually should be pretty straightforward to implement!

To me it seems reasonable for these sorts of functionalities to be covered by the language server and to have the language server be the only handler for fielding goto-def/find reference requests, which this feature sounds like

Interesting idea... I didn't realize that we could also implement "go from source file to BUILD file" in the language server. So far, I was only considering the "go from BUILD file to source file" (i.e. this pull request here) in mind for the language server. I like the idea to implement both directions in the LS.

bazel-lsp does have go-to-definition from BUILD files to source files.

I just tried this out. Worked without any issues for me.

I'm generally for trying to push as much functionality into the language servers, but I'm also not against giving a several-line fix to overlapping functionality in this extension, particularly considering no language server is enabled by default.

Then let's move ahead with this PR. I am still waiting if we can fix the originSelectionRange, and will then merge this PR.

withered-magic commented 2 months ago

Just landed this functionality on main for starpls! Will include it in the next release

Interesting idea... I didn't realize that we could also implement "go from source file to BUILD file" in the language server. So far, I was only considering the "go from BUILD file to source file" (i.e. this pull request here) in mind for the language server. I like the idea to implement both directions in the LS.

One thing that I realized for the opposite direction (i.e. "go to BUILD file from source file") was that the best approach for this in terms of UX wasn't super clear. To me this sounds like it'd be best fulfilled by a "Find references" request, but it's unclear how the user should issue that request (e.g. where to actually right click and select "Find references" from). As a starter I was going to just make it so that issuing a "Find references" from text that would be otherwise meaningless (e.g. blank space) would default to this behavior. But maybe it would be better to align on this here or in another thread before starting on any implementations?

kchodorow commented 2 months ago

I was thinking it would just be a command palette option, since as you say there's no particular code we're "finding references" from.

@cameron-martin had the neat idea of having the Bazel Build Targets pane in the sidebar keep pace with whatever you currently are editing. I'm going to take a look at the API and see if that's feasible.

vogelsgesang commented 2 months ago

I was thinking it would just be a command palette option, since as you say there's no particular code we're "finding references" from.

Yes, that would also be my user interface of choice... Afaik, language servers can also contribute commands, but I never tried this before.

@kchodorow regarding this pull request at hand here: Are you still planning to improve originSelectionRange (see previous messages) or do you want me to merge this PR as is?

withered-magic commented 2 months ago

Ah ok, I think my only reservation with making It a command palette option would be that other editors besides VSCode that are also using the LSP would also have to implement some custom way to use this functionality (if they have the equivalent of command palette). Language servers can definitely support custom commands though! There's a couple custom ones defined in starpls for debugging purposes.

kchodorow commented 2 months ago

Oh, sorry, I missed the originSelectionRange comment! I'm a little confused about how to do what you're asking for... it doesn't seem like mouseover highlight is handled by this code. provideDefinition is triggered after the cmd/ctrl key is hit, AFAICT.

vogelsgesang commented 2 months ago

ok, I will merge this PR and then see if can link the correct range in a follow-up commit

vogelsgesang commented 2 months ago

see #382 for what I had in mind regarding originSelectionRange