-
Notifications
You must be signed in to change notification settings - Fork 308
LSP: builtin completions and docs #1142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
LSP: builtin completions and docs #1142
Conversation
|
@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.) |
This is generally quite useful, needed in next diff.
We look at the build file names and check against that, same with package file names. This gets us a full OwnedStarlarkPath. So we can change our LSP behaviour depending on on what file the user is looking at (e.g. the next diff, whether to load prelude).
As you can see, valid_at was written only once, at startup. So DocsCache was a cache in name only. As soon as you touched prelude, the DocsCache would never be is_reusable again.
We fall back to builtins only if prelude fails to document.
ff8888a to
efc1698
Compare
| match self.is_reusable(¤t_dice_ctx) { | ||
| true => (), | ||
| false => { | ||
| let new_docs_cache = Self::new_docs_cache(fs, &mut current_dice_ctx).await?; | ||
| *docs_cache = new_docs_cache | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look closely, you'll see we never wrote to self.valid_at after initializing it in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this diff could have been simpler if we just wrote to self.valid_at, but I only care if you do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
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. |
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 theSplit out from that work.BuckLspContext::import_path_from_urlfunction 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.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.