LSP: Add autocomplete & hover functionality, and jump to target name

[MaartenStaa]

Fixes #52.

This PR adds the capability of sending auto-complete suggestions when in LSP mode. Currently it supports:

• Locally defined methods
• Imported symbols
• Local variables
• Function arguments
• Scoping said local variables and function arguments based on the position of the cursor (inside/outside a method)
• Automatically inserting/updating a load for exported symbols from another opened file

Not yet handled/open questions:

• The kind of previously loaded symbols is not always correct (i.e. it's always a CompletionItemKind::METHOD at the moment), because I'd need to resolve the file, which it doesn't do at the moment.
• Auto-complete for exported symbols when the cursor is in a load statement doesn't work yet, for the same reason of resolving the path.
• The path for generated load statements is based on Bazel, I don't know what this should look like for Buck and/or Starlark usage outside of these two build systems. This ties in with #51, which has been open for a while.
• The path for generated load statements doesn't handle external repository paths yet, this also ties in with #51.

I'm hoping with some feedback, and maybe a merge of the Bazel PR, we can get this to a mergeable state 🙂 looking forward to hearing your thoughts!

[ndmitchell]

Sorry for the delay in looking at this - I was travelling.

How much of a problem is the use of kind = Method? What is the impact of that on a client such as VS Code? Just an incorrect icon in the completion list?

[MaartenStaa]

Thanks for going through this @ndmitchell! I'll go through your comments some time this week and a) tidy up the code and b) split out the Bazel specific stuff to a separate PR. In that case the initial PR might have to omit completion of exported symbols from other files altogether, as completing those requires inserting the appropriate load statement.

Regarding the "method" type, yeah it's just an incorrect icon basically, or e.g. in Neovim it might have the type "method" as text instead of an icon. Not a huge deal obviously.

Thanks for the tip about SaplingSCM, I'll take a look at that as well. For the follow-up PR, I might create a new branch based on both this one and #51, thoughts?

[ndmitchell]

I'm just about to go through #51 (the person who was working from this on our side left Meta, so it accidentally got lost, oops). Let me see.

[ndmitchell]

For generating a load statement, I imagine you need a method on LspContext like render_as_load(&self, target: &LspUrl, current_file: &LspUrl) -> anyhow::Result<String> ? That seems quite reasonable to add in.

[MaartenStaa]

That sounds like a good solution, yeah. Probably you want to be able to do the mapping in two directions:

• LspContext::render_as_load(&self, target: &LspUrl, current_file: &LspUrl) -> anyhow::Result<String>
• LspContext::resolve_load(&self, target: &str, current_file: &LspUrl) -> anyhow::Result<LspUrl>

[facebook-github-bot]

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

[ndmitchell]

Thanks! I'm going through the code now - plan to split pieces off and land them separately (those obvious bits) and slowly work until there is nothing left. Expect to see pieces of your code trickle out over the next day or so.

[MaartenStaa]

Thanks! I'm going through the code now - plan to split pieces off and land them separately (those obvious bits) and slowly work until there is nothing left. Expect to see pieces of your code trickle out over the next day or so.

@ndmitchell Awesome, looking forward to it. Feel free to ping me if any questions come up.

[ndmitchell]

I've submitted all the changes apart from those to lsp/server.rs and a handful of lines to definition.rs. I'll update once they are all landed. For the changes to server:

• I think we need the LspContext so that it isn't Buck/Bazel specific, with the mapping from a URL to the load name and vice versa. Did anything happen to your LspContext suggestion above?
• As an extension, I note you aren't auto completing using names that are loaded by other open modules. E.g. if I have a separate file open, with load("filename", "foo") then offering to complete to foo makes sense, even though foo isn't exported by anything open.

Overall, looks good, and the automatic adding of load statements is super slick.

[ndmitchell]

Other notes:

• We should probably have a method on LspContext to get a list of the global symbols.
• Completing keywords is quite nice - a hardcoded list of those should be fairly easy to add.

[MaartenStaa]

@ndmitchell Nice! I merged in origin/main and resolved any conflicts.

I think we need the LspContext so that it isn't Buck/Bazel specific, with the mapping from a URL to the load name and vice versa. Did anything happen to your LspContext suggestion above?

I don't think so, no. Not sure if the idea is to tackle that via #51 or not? I can try to come up with an initial implementation. One thought I had later on: should that actually be on the LspContext? Just wondering if it makes more sense to have Bazel as a "dialect", and let the LSP be aware of the Starlark dialect. I should probably add that I'm not sure what, if any, the actual differences are between Bazel and Buck and how they render load paths. If they're the same, then having it on the LspContext seems fine.

As an extension, I note you aren't auto completing using names that are loaded by other open modules. E.g. if I have a separate file open, with load("filename", "foo") then offering to complete to foo makes sense, even though foo isn't exported by anything open.

That's a nice idea, I'll try to find some time to add that!

We should probably have a method on LspContext to get a list of the global symbols.

Makes sense. I guess that would be the things under starlark/src/stdlib?

Completing keywords is quite nice - a hardcoded list of those should be fairly easy to add.

Added!

Overall, looks good, and the automatic adding of load statements is super slick.

Thanks! I was pretty happy with that too :)

[MaartenStaa]

@ndmitchell I have now added the autocomplete options gathered from load statements in open files.

[MaartenStaa]

@ndmitchell Added a few more things:

• I realised the autocomplete options from load statements in other files needed to have their path resolved and re-rendered correctly from the point-of-view of the target file
• So I implemented render_as_load
• Also added some fixes to resolve_load
• Added the workspace root as a parameter to both
• Added tests to both
• Also added autocompletion for global symbols (including docs!)

I'd appreciate it if you or someone else can take a critical look at render_as_load and resolve_load—I based my logic on how Bazel works, but don't know enough about Buck(2) to know whether it uses the same format of:

• In same folder: :filename
• Absolute: //some/sub/folder:filename
• In root: //:filename

(also, it doesn't really handle stuff in other workspaces, e.g. @workspace//some:file)

[ndmitchell]

Thanks - hoping to take a look this weekend!

[MaartenStaa]

@ndmitchell OK, so I went off the rails a bit this week 😅 added a bunch of stuff. You can see it from the commits above, but summarizing:

• Autocomplete takes context into account now, e.g. load paths/exported symbols in load statements, parameter names in function calls, target names in string literals
• Hover info, reusing much of the same infrastructure as was used for go to definition
• Go to definition (and thus hover info) as well now also works for target names, when those are defines with a macro/rules invocation in a build file with a name attribute
• Checks the used build system (support for Buck2 and Bazel) to gather information about known repositories and target names in packages. This also means resolve_load and such now also works for paths containing a @repository//-like prefix.

I appreciate that this probably makes reviewing this more complicated, so I'm sorry about that. Is there some (semi-)public channel (Slack, Discord) you have where maybe we can work together a bit more closely to be able to land this? e.g. I know for the Relay team they have a private channel in the GraphQL Discord server.

[ndmitchell]

@MaartenStaa - might need a VC to ensure we move forward with this. Can you email me at ndmitchell -at- gmail.com and we can figure it out.

[facebook-github-bot]

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

[ndmitchell]

I've gone through this PR in consultation with @MaartenStaa:

• The build_system stuff and the string completion in load stuff should be in a separate binary, starlark-bazel. The suggestion is that is a temporary binary in starlark-rust repo while it is in active development, then fairly soon after, becomes a standalone project.
• The work to make the load statement less strict is valuable, but needs doing as a more general step to make the parser more tolerant for LSP. We can put that in later.
• I moved a lot of code that lived outside of starlark/lsp into starlark/lsp. The rough logic being that the code outside there should have strong, stable, thoughtful APIs we rely on. The stuff inside lsp is whatever LSP currently needs to get its job done.
• I tweaked some types in somewhat insignificant ways:
  • StringCompletion doesn't need to be passed the literal (if you care, capture it when giving back the function)
  • The environment completions might vary by URL, and DocModule is a better type for them.
  • The FixturesWith type didn't actually need the deferred CodeMap thing so I dropped it.

With that I have 6 diffs that are ready to land now (being reviewed and landed internally) then one diff on top which is the hover/completion stuff. Once the prerequisites land I'll publish that and share it back here (it is 95%+ the work of @MaartenStaa).

[ndmitchell]

All the dependencies are now landed. https://github.com/facebookexperimental/starlark-rust/pull/79 is the code which is basically a version of this.

[ndmitchell]

Given #79 is now landed, closing this diff as it is 90% landed. Please open follow up PR for the rest of the pieces. Thanks so much for your improvements!

[stagnation]

Hooray, thank you! I checked with neovim and a simple bazel program to hopefully see if the starlark-bazel parts were included. The linting functionality that we've always had works just like before, and there are now more LSP functions implemented, go-to-definition can be called where it makes sense, loads and target references. But the landing location is typically just a (new) file with the token name, which I assume is expected for a bazel project. I'm happy to test-run starlark-bazel when it comes :)