LSP: builtin completions and docs

[cormacrelf]

Fixes #952.

Previously we had no docs or completion for builtins or prelude symbols. Now we do. And it's really, really good. You don't need to search through https://buck2.build nearly as often.

This builds on #1132 which is a big PR, but only really for the BuckLspContext::import_path_from_url function that determines whether the current file is a build file, so 1132 can be split up without much trouble if need be. For now this PR is just two commits at the end. Split out from that work.

It also fixes #1124. And an apparent bug where the docs for prelude were (I think) not ever being cached.

See? It works!

You get completions/docs for global builtins everywhere, but for prelude symbols you get them only in build files. Like below:

Caveat -- rust rules

Please note that the rust_* rules in the prelude have no docs due to being wrapped in macros (rust_common_macro_wrapper). That's out of scope for this PR. I don't know what was done to make this work for https://buck2.build where the rust rules clearly do get docs, maybe it can be replicated.

[meta-codesync]

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D87061519. (Because this pull request was imported automatically, there will not be any future comments.)

[cormacrelf]

If you look closely, you'll see we never wrote to self.valid_at after initializing it in the constructor.

[cormacrelf]

I think this diff could have been simpler if we just wrote to self.valid_at, but I only care if you do.

[cormacrelf]

Technically this diff makes it easier to special-case docs cache creation for starlark modules inside the prelude, as alluded to in the TODO you can see in the main diff.

[cormacrelf]

I see @Nero5023 has been adding various docs for starlark APIs this week. Would you have a look at this? It puts those docs you've been writing only a keystroke or hover away.

[JakobDegen]

Alright, sorry it took me so long to get to this. I'm attempting to review all of this and am far enough along now to be able to start leaving some comments, but this code has always felt pretty awful and my attempting to understand it now is not making me feel any better about it.

Before any of the details, let me get two things out of the way:

1. Thank you so much for doing this, this is a great contribution and I'm super excited about it
2. Thank you for leaving the code better than you found it, that seriously makes my life easier.

I'll leave comments one commit at a time

Extract import_path_from_url in lsp.rs

Wait I don't get it, wtf is going on in this function? What in the world is starlark_import_path doing? It constructs a "real" filesystem path from the starlark: uri, but the only thing the starlark: uri is used for is fake paths for native.whatever, isn't this just completely making up cell//native/whatever.bzl bs?

Obviously not on you.

Maybe I missed something, but if this sounds right, replace this and hardcode it with root//fake.bzl or something like that?

Determine kind of starlark file in LSP

I found the import_path naming confusing here, took me a while to realize that this isn't a path that shows up in a load but rather the path of the file we're providing LSP services for. Maybe rename this to starlark_path or something like that?

Implement LspContext::get_environment, to list & document builtin glo…

So unfortunately, this implementation is not quite correct for reasons that you can't really be expected to know: We support a thing called buildfile.includes which implicitly imports additional things into a buildfile besides the standard imports, implemented here:

buck2/app/buck2_interpreter_for_build/src/interpreter/interpreter_for_dir.rs

Lines 381 to 390 in abae83b

	if let Some(root_import) = self.root_import() {
	let root_env = loaded_modules
	.map
	.get(&StarlarkModulePath::LoadFile(&root_import))
	.with_internal_error(|| {
	format!("Should've had an env for the root import `{root_import}`",)
	})?
	.env();
	env.import_public_symbols(root_env);
	}

Now, we could go and add extra code to that, but that seems super brittle and all this code is way too complicated already so I don't really want to do that.

Here's my alternative suggestion: First, push the code I linked above into the create_env two lines up, making sure to add a check for the filetype. Then, just implement get_environment like this:

let calculator = dice_ctx
                .get_interpreter_calculator(import_path.clone())
                .await?;
let (module, _) = calculator.prepare_eval(import_path).await?;
let docs1 = module.get_private_docs();
let docs = calculator.globals().documentation();
// merge

Where get_private_docs is something you'd have to implement but shouldn't be hard.

That's all robust and easy to implement. If you're worried about the perf, see below.

Actually save an equality token so we do not churn the DocsCache

I have no idea wtf is going on with this dice cache or why this was done the way it was. We generally don't do explicit cache checks in buck2, is there a reason that this code couldn't just use a dice key to compute derivatives of dice data like everything else does? If so that'd be a lot better than this, it would mean that you can't get this wrong and also it would provide an obvious path towards fixing whatever perf concerns there might be. If you're worried about the perf of my previous suggestion, you could do the same there.

Besides that, I also have one question for you. We currently implement this thing where go-to-def on builtins opens up a new file with a stub implementation looking thing. I've... never seen that before in any other LSP and it honestly doesn't make a lot of sense to me. Do you find it valuable/see the value in it? With this implemented, could we just delete that behavior and rely on hovers for surfacing docs for builtins?

Thanks again

[JakobDegen]

Review automatically exported from Phabricator review in Meta.

[JakobDegen]

I don't know what was done to make this work for

buck2/website/gen_docs.py

Line 78 in abae83b

+ " prelude//docs:rules.bzl",

Don't copy this, we should improve the docs stuff so that we don't need it instead of propagating it further, we'll have to live with the disadvantages in the meantime

[cormacrelf]

You're welcome, thanks for getting round to it. This is pretty old code indeed, much of it blames back to "initial commit" so we might never know why some of it was done this way.

the only thing the starlark: uri is used for is fake paths for native.whatever, isn't this just completely making up cell//native/whatever.bzl bs?

I think the intention was for the LSP to be active on starlark: editor buffers, for these to contain valid starlark, and for go-to-def work so you could dig into docs for builtins referred to by other builtins, ad infinitum. But in Buck's usage, those starlark: files are markdown and not valid starlark, so this does not work in practice. This concept is kinda baked into the starlark_lsp crate but I don't think it generates any such links on its own, only if you explicitly make starlark: uris, which we do in DocsCache.

I think you're right that everything would still work if you hardcoded one importpath for all of them, but it doesn't really bother me and helps with debugging if the path reflects the one in the editor. We can delete the handling of LspUrl::Starlark when we stop sending users there. So the real question is your later one:

[...] We currently implement this thing where go-to-def on builtins opens up a new file with a stub implementation looking thing. I've... never seen that... With this implemented, could we just delete that behavior and rely on hovers for surfacing docs for builtins?

1. I have discovered that it only currently works in VSCode with the starlark extension (it uses a custom LSP method starlark/fileContents). I wasn't even aware the feature worked since I use neovim, turns out it does technically work.
2. If you goto def on regex(), it is equivalent to opening https://buck2.build/docs/api/build/regex/ but with wrong syntax highlighting (should be markdown). So you can always just open the page on the website. The one thing this feature really has is it exactly matches your buck2 version. I'm usually 6 months behind, so it's good to know exactly what APIs are available. However the same is true of the hover-docs.
   • (It's plausible we could literally open that URL in the browser when users gotodef, but that would also require a custom editor extension and I don't care enough to build it.)
3. There is some utility, I guess: we currently bundle all docs for a type and its methods onto a single symbol, which makes it quite hard to navigate a big type like AnalysisActions in a temporary hover popover. That much should be visible in the demo images above. I think if we could hover-doc methods individually and also dot-complete them, there would be no point in keeping this, but for now it's kinda nice to be able to scroll through.

I found the import_path naming confusing here ... rename to starlark_path?

Yes. Strikes me as obvious in retrospect, now that I'm familiar with all the different path types.

We support a thing called buildfile.includes which implicitly imports additional things into a buildfile besides the standard imports,
[...] couldn't [we] just use a dice key to compute derivatives of dice data like everything else does?

As for putting it in buck2_interpreter_for_build, sounds fine to me, and this is what I landed on for doing typechecking. Seems like buildfile.includes is set on a per-cell basis, so if you want to move this into DICE I think we can use a key like

enum DocEnvironmentKey {
    /// Each cell can configure buildfile.includes differently, so partition on cell name
    Buildfile(CellName),
    /// If inside prelude, we skip generating `native` symbol that holds the prelude
    OtherStarlark { inside_prelude: bool }
}

and compute/store very close to the bare minimum of these. We would parse an empty string with a fake path probably, which would conveniently avoid issues like "no docs for builtins when file on disk has parse error". I can give that a go.