Skip to content
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

Add bazel help builtin-symbols-as-proto #21936

Closed
wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Apr 9, 2024

Work towards bazelbuild/vscode-bazel#1

RELNOTES: The new bazel help builtin-symbols-as-proto subcommand emits a base64-encoded proto message describing the built-in Starlark types and global symbols. See src/main/protobuf/builtin.proto for the message definition.

RELNOTES: The new `bazel help builtin-symbols-as-proto` subcommand emits a base64-encoded proto message describing the built-in Starlark types and global symbols. See `src/main/protobuf/builtin.proto` for the message definition.
@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 9, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 9, 2024

@tetromino Would you be available to review this?

There are now two very promising Starlark language servers with Bazel-specific functionality (https://github.com/bazelbuild/vscode-bazel?tab=readme-ov-file#using-a-language-server-experimental). Having a standard way of obtaining the correct Starlark env for the current Bazel version would help them a lot and this seems to be the most direct way of achieving that goal. Happy to implement this differently though.

FYI @withered-magic

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 9, 2024

Note for review: This is related to #21929 in that depending on which PR is merged first, the accompanying test could be migrated over/updated.

@fmeum fmeum requested a review from tetromino April 9, 2024 11:10
@fmeum
Copy link
Collaborator Author

fmeum commented Apr 9, 2024

Tests are failing because the proto would also need to be included in the distfiles. I can set that up properly when the overall approach has been approved.

@withered-magic
Copy link

Awesome, thank you so much for your work on this!! Once this lands on master I'll add logic in starpls to try bazel help builtin-symbols-as-proto first before loading the bundled proto, that way it'll be ready for when the command is officially landed in the next release

@withered-magic
Copy link

@fmeum Would you also be able to incorporate the functionality from this PR into your current one? I think review on the original PR stalled :( #21135

Basically it modifies the APIExporter to dump out docs and type information (which are otherwise blank in the generated proto) - this is super useful for validating builtins argument types as well as providing hover docs. (Currently starpls actually a proto built off of this PR to provide the functionality I mentioned)

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 9, 2024

@withered-magic It looks fine as is and is on my radar, I'll try to get someone's attention.

@iancha1992 iancha1992 added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label Apr 9, 2024
Copy link
Contributor

@tetromino tetromino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, I don't think that builtin_symbols_proto is the right proto for this purpose. I'm biased of course, but I'd suggest Stardoc protos (compare how much you can tell about a provider from builtin_symbols vs. Stardoc proto)

And second, I don't think a bazel help subcommand is the right place to put this - the set of global symbols depends on flags / starlark semantics.

Instead of bazel help, I might suggest bazel info, which does allow flags (take a look at the existing bazel info build-language which is already used by IDEs).

However, I'm currently in the process of designing a way to export Stardoc protos from a .bzl file without needing a stardoc / starlark_doc_extract rule - specifically, whether bazel query can do it or if we need an entirely new command. If it turns out that a new command is needed and bazel query cannot be shoehorned-into, this yet-to-be-added command would be the right place to export builtin symbols too.

@fmeum
Copy link
Collaborator Author

fmeum commented Apr 11, 2024

First, I don't think that builtin_symbols_proto is the right proto for this purpose. I'm biased of course, but I'd suggest Stardoc protos (compare how much you can tell about a provider from builtin_symbols vs. Stardoc proto)

The problem with Stardoc protos, as far as I can tell, is that they lack type information for anything that isn't an attribute of a rule or module extension tag. But this information is what is most relevant to a Starlark language server. When you say that you would prefer Stardoc proto over builtin_symbols_proto, do you mean that we should extend the proto to cover this use case?

And second, I don't think a bazel help subcommand is the right place to put this - the set of global symbols depends on flags / starlark semantics.

Instead of bazel help, I might suggest bazel info, which does allow flags (take a look at the existing bazel info build-language which is already used by IDEs).

That seems reasonable, but the hard problem in the background would be to make ApiExporter something that can be part of Bazel rather than an external tool. Is this what you are imagining with the following?

However, I'm currently in the process of designing a way to export Stardoc protos from a .bzl file without needing a stardoc / starlark_doc_extract rule - specifically, whether bazel query can do it or if we need an entirely new command. If it turns out that a new command is needed and bazel query cannot be shoehorned-into, this yet-to-be-added command would be the right place to export builtin symbols too.

Do you have a rough estimate for when this would be available? The current situation for Bazel users is that we finally have two promising generic Starlark LSP implementations, but "just" lack a way to get structured type information from Bazel. With a few fixes (see #21929 and #21135), the builtin proto is good enough for these purposes. I would very much like to avoid a "perfect is the enemy of good" situation.

@tetromino
Copy link
Contributor

The problem with Stardoc protos, as far as I can tell, is that they lack type information for anything that isn't an attribute of a rule or module extension tag.

That's an excellent point. We should fix it. Filed #21979

Do you have a rough estimate for when this would be available? The current situation for Bazel users is that we finally have two promising generic Starlark LSP implementations, but "just" lack a way to get structured type information from Bazel. With a few fixes (see #21929 and #21135), the builtin proto is good enough for these purposes. I would very much like to avoid a "perfect is the enemy of good" situation.

I have it in my OKRs for this quarter to expose Stardoc protos via either bazel query or a new command. Re "perfect is the enemy of good" - I want to avoid incompatible representations for APIs depending on whether a particular symbol is implemented in Java, or in pure Starlark in @_builtins, or in pure Starlark in @bazel_tools.

@vogelsgesang
Copy link

However, I'm currently in the process of designing a way to export Stardoc protos from a .bzl file without needing a stardoc / starlark_doc_extract rule - specifically, whether bazel query can do it or if we need an entirely new command

Amazing!

In case you can already share a sneak peak into your current design thoughts, that would be very interesting 🙂

This PR here is aiming for a long-term solution, such that the language servers work out-of-the-box against any Bazel version. Currently, the language servers simply bundle the builtin.pb and are thereby tied to a particular Bazel version. But given that the builtins are changing rather slowly, having a version mismatch in the builtin.pb is acceptable. In my opinion, it's acceptable to keep bundling the builtin.pb until the stardoc-based solution becomes available.

In the short-term, #21135 and #21929 would bring more immediate benefits to the language servers and their users than this PR: With those, the LSPs could also work with bzlmod's MODULE files and type checking and function signature help could be improved across the board.

@tetromino do you agree that we can still move ahead with #21135 and #21929, even if the builtin.pb will be superseded by stardoc eventually? In contrast to this PR, the other two PRs don't introduce any new API surfaces, and will hence not add a long-term maintenance burden

@fmeum fmeum closed this Apr 23, 2024
@fmeum fmeum deleted the lsp-export-builtins-proto branch April 23, 2024 13:43
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants