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

MaartenStaa · 2023-03-07T14:45:29Z

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 added bazel workspace lsp support #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 added bazel workspace lsp support #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!

…ther file this only works for symbols that aren't loaded yet, for the ones that are we'd need to do path resolution of the loaded file to check the type of the loaded symbol

ndmitchell · 2023-03-21T07:58:53Z

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?

ndmitchell

Overall, this looks really good. We need to unpick the bazel file path dependency, so suggest you split into two diffs - one doing the collect_symbols and exported changes (those are good to land before we figure out the rest), then we iterate on #51, and then can land this.

Note that https://sapling-scm.com/ lets you do stacked pull requests, which might be useful.

starlark/src/lsp/server.rs

starlark/src/codemap.rs

starlark/src/analysis/exported.rs

starlark/src/lsp/server.rs

MaartenStaa · 2023-03-21T08:36:26Z

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 · 2023-03-21T08:41:54Z

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 · 2023-03-21T08:46:25Z

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 · 2023-03-21T08:52:35Z

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>

…ify statements

…unit tests

…ly contains', not 'overlaps'

facebook-github-bot · 2023-05-02T10:42:31Z

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

ndmitchell · 2023-05-02T19:11:32Z

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 · 2023-05-03T08:14:43Z

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 · 2023-05-04T11:39:17Z

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 · 2023-05-05T16:00:25Z

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.

# Conflicts: # starlark/src/analysis/exported.rs # starlark/src/codemap.rs

MaartenStaa · 2023-05-11T11:11:46Z

@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 :)

also refactor `completion_options` a bit and extract text edit construction to its own function

… literal for target names NOTE: Unfortunately this has the side effect of having to return a `Span` (unresolved) from the `LspContext`, the hover needs this to extract the string contents from the codemap, to show in the hover popup.

Use the new auto complete type detection to offer different completion options in different scenarios: - Default: Just offer all known local symbols, global symbols, and those that can be loaded from other files. - Parameter: Same, but also offer known parameter names for the current function call. - LoadPath: Offer file system based completions, including files and folders, and if a build system was detected, known repository names. - LoadSymbol: Offer names of symbols exported from the file loaded in the current load statement. - String: Like LoadPath, but also offer files that can't be loaded according to the detected build system, as well as target names, again as detected by the detected build system. - Type: Offer type name completion (very basic at the moment). - None: Don't offer any completions.

By making the args optional, the parser is able to parse an incomplete load statement like this: `load("")` Without this, the LSP is unable to update the AST while this is typed, and as such we can't detect that the cursor is in a load statement, and offer auto complete for the load path.

MaartenStaa · 2023-06-30T09:40:02Z

@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 · 2023-07-02T21:25:11Z

@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.

ndmitchell · 2023-07-03T09:13:37Z

starlark/src/syntax/grammar.lalrpop

@@ -186,7 +186,7 @@ ExprStmt_: Stmt = <Test> => Stmt::Expression(<>);
 LoadStmt: AstStmt = ASTS<LoadStmt_>;
 LoadStmt_: Stmt = LoadStmtInner => Stmt::Load(<>);

-LoadStmtInner: Load = "load" "(" <module:string> <args:("," <LoadStmtSyms>)+> ","? ")" => {
+LoadStmtInner: Load = "load" "(" <module:string> <args:("," <LoadStmtSyms>)*> ","? ")" => {


Why is this changing?

Check this commit: 1d745b9

In short, it's because the parser is too strict/not error correcting. If the input doesn't fully parse, then when an autocomplete is triggered, the parsed AST is the latest snapshot that did parse. As a result, if the input line is:

load("|")

Where | is the cursor, there's no way of knowing that the cursor is inside a load statement—as it's not syntactically correct.

ndmitchell · 2023-07-03T09:17:16Z

starlark/src/build_system/mod.rs

+
+/// A hint for which build system to resolve.
+#[derive(ValueEnum, Debug, Clone, PartialEq, Eq)]
+pub enum BuildSystemHint {


I would have expected this to go outside this repo - it seems that encoding the possible build systems stops others from using this,

ndmitchell · 2023-07-03T09:18:44Z

starlark/src/docs/mod.rs

@@ -786,6 +800,30 @@ pub enum DocItem {
    Object(DocObject),
    Function(DocFunction),
    Property(DocProperty),
+    Param(DocParam),


This feels like the wrong place to put DocParam.

ndmitchell · 2023-07-03T09:19:47Z

starlark/src/analysis/find_call_name.rs

@@ -30,7 +29,7 @@ impl AstModule {
    ///
    /// NOTE: If the AST is exposed in the future, this function may be removed and implemented
    ///       by specific programs instead.
-    pub fn find_function_call_with_name(&self, name: &str) -> Option<ResolvedSpan> {
+    pub fn find_function_call_with_name(&self, name: &str) -> Option<Span> {


Why is this a useful change?

fdaa22b
It's useful because it allows to extract the source code from the codemap, which you can't do with a ResolvedSpan. So the hover can use the Span to return the code in which a target is defined.

facebook-github-bot · 2023-07-03T10:20:41Z

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

ndmitchell · 2023-07-03T20:39:07Z

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).

Summary: This is code from facebook#68, with cleanups. Differential Revision: D47208501 fbshipit-source-id: 4baf8714123b08de3e111c8002669ca4d673383a

ndmitchell · 2023-07-04T16:12:35Z

All the dependencies are now landed. #79 is the code which is basically a version of this.

Summary: Pull Request resolved: facebook#79 This is code from facebook#68, with cleanups. Differential Revision: D47208501 fbshipit-source-id: bd744e9625f932d5bd6bc26909f360e6441ae0af

Summary: Pull Request resolved: facebook#79 This is code from facebook#68, with cleanups. Differential Revision: D47208501 fbshipit-source-id: 7f06543338246494054e739c6fc2066bbe31941f

Summary: X-link: facebook/starlark-rust#79 This is code from facebook/starlark-rust#68, with cleanups. Reviewed By: stepancheg Differential Revision: D47208501 fbshipit-source-id: 44f6704dc06fb9150be5c2e7326de0df9f13060d

Summary: Pull Request resolved: #79 This is code from #68, with cleanups. Reviewed By: stepancheg Differential Revision: D47208501 fbshipit-source-id: 44f6704dc06fb9150be5c2e7326de0df9f13060d

ndmitchell · 2023-07-06T09:20:08Z

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 · 2023-07-06T09:31:01Z

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 :)

MaartenStaa added 7 commits March 7, 2023 09:32

feat: first implementation of auto-complete

490c847

feat: update existing load when inserting load from same module

32dfb2d

chore: tidy up

43ed7b1

feat: de-duplicate already loaded symbols in auto-complete

f248479

feat: don't autocomplete variables from other methods

16872c9

fix: handle windows file paths for inserted load statements

fdbdd11

feat: show correct symbol kind when suggesting exported symbol from o…

b28e1e5

…ther file this only works for symbols that aren't loaded yet, for the ones that are we'd need to do path resolution of the loaded file to check the type of the loaded symbol

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2023

ndmitchell reviewed Mar 21, 2023

View reviewed changes

ndmitchell mentioned this pull request Mar 21, 2023

added bazel workspace lsp support #51

Open

MaartenStaa added 4 commits May 1, 2023 14:08

fix: correctly set exported symbol kind in both assign and assign mod…

df738bd

…ify statements

feat: refactor find_symbols_at_location to its own module, including …

372bdaa

…unit tests

docs: add comment to show that ResolvedSpan::contains means 'entire…

60d1491

…ly contains', not 'overlaps'

chore: remove commented out code

41b66ab

stepancheg assigned ndmitchell May 2, 2023

MaartenStaa added 2 commits May 11, 2023 11:47

feat: autocomplete keywords

086c0ba

Merge remote-tracking branch 'origin/main' into add-autocomplete

2370723

# Conflicts: # starlark/src/analysis/exported.rs # starlark/src/codemap.rs

MaartenStaa added 2 commits May 30, 2023 16:15

feat: add function to get symbols loaded in a module

369325f

feat: add autocomplete options from symbols loaded in open files

e3db1d6

also refactor `completion_options` a bit and extract text edit construction to its own function

MaartenStaa added 12 commits June 27, 2023 11:35

Merge remote-tracking branch 'origin/main' into add-autocomplete

7a1138b

feat: jump to definition for labels in build files

14cb731

chore: move doc item extraction from AST into its own file

bacf4d0

feat: show documentation for completion options exported from open files

2a61284

feat: implement rudimentary hover support

88824f2

test: make tests pass again

78cd4a6

feat: provide argument names for exported function symbols

1415d9e

feat: add module that inspects AST and decides type of autocomplete

8497455

feat: provide more information from build systems

e598992

MaartenStaa changed the title ~~Add autocomplete functionality to the LSP~~ LSP: Add autocomplete & hover functionality, and jump to target name Jun 30, 2023

ndmitchell reviewed Jul 3, 2023

View reviewed changes

Merge branch 'main' into add-autocomplete

1ec491a

ndmitchell mentioned this pull request Jul 4, 2023

Add completions and hover #79

Closed

ndmitchell added a commit to ndmitchell/starlark-rust that referenced this pull request Jul 4, 2023

Add completions and hover

a15f87e

Summary: This is code from facebook#68, with cleanups. Differential Revision: D47208501 fbshipit-source-id: 4baf8714123b08de3e111c8002669ca4d673383a

fix: target for load insertion was incorrect

384b9c1

ndmitchell closed this Jul 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MaartenStaa commented Mar 7, 2023

ndmitchell commented Mar 21, 2023

ndmitchell left a comment

MaartenStaa commented Mar 21, 2023

ndmitchell commented Mar 21, 2023

ndmitchell commented Mar 21, 2023

MaartenStaa commented Mar 21, 2023

facebook-github-bot commented May 2, 2023

ndmitchell commented May 2, 2023

MaartenStaa commented May 3, 2023

ndmitchell commented May 4, 2023

ndmitchell commented May 5, 2023

MaartenStaa commented May 11, 2023

MaartenStaa commented Jun 30, 2023

ndmitchell commented Jul 2, 2023

ndmitchell Jul 3, 2023

MaartenStaa Jul 3, 2023

ndmitchell Jul 3, 2023

ndmitchell Jul 3, 2023

ndmitchell Jul 3, 2023

MaartenStaa Jul 3, 2023

facebook-github-bot commented Jul 3, 2023

ndmitchell commented Jul 3, 2023

ndmitchell commented Jul 4, 2023

ndmitchell commented Jul 6, 2023

stagnation commented Jul 6, 2023

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

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

Conversation

MaartenStaa commented Mar 7, 2023

ndmitchell commented Mar 21, 2023

ndmitchell left a comment

Choose a reason for hiding this comment

MaartenStaa commented Mar 21, 2023

ndmitchell commented Mar 21, 2023

ndmitchell commented Mar 21, 2023

MaartenStaa commented Mar 21, 2023

facebook-github-bot commented May 2, 2023

ndmitchell commented May 2, 2023

MaartenStaa commented May 3, 2023

ndmitchell commented May 4, 2023

ndmitchell commented May 5, 2023

MaartenStaa commented May 11, 2023

MaartenStaa commented Jun 30, 2023

ndmitchell commented Jul 2, 2023

ndmitchell Jul 3, 2023

Choose a reason for hiding this comment

MaartenStaa Jul 3, 2023

Choose a reason for hiding this comment

ndmitchell Jul 3, 2023

Choose a reason for hiding this comment

ndmitchell Jul 3, 2023

Choose a reason for hiding this comment

ndmitchell Jul 3, 2023

Choose a reason for hiding this comment

MaartenStaa Jul 3, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Jul 3, 2023

ndmitchell commented Jul 3, 2023

ndmitchell commented Jul 4, 2023

ndmitchell commented Jul 6, 2023

stagnation commented Jul 6, 2023