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

[wit-component] Extend the WIT printer for use in syntax highlighting #1956

Merged
merged 18 commits into from
Dec 19, 2024

Conversation

tomasol
Copy link
Contributor

@tomasol tomasol commented Dec 17, 2024

Introduce Output trait as a visitor for collecting syntax tokens.
This trait enables syntax highlighting and supports advanced use cases, such as filtering tokens by package, interface, or function.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for this! Are you planning on integrating this externally into an IDE or similar perhaps?

As an optional addition to this PR, it'd be pretty neat if this could get integrated into wasm-tools component wit in the same way that wasm-tools print will use terminal colors by default. No need to do that here though if you're not interested in doing so.

use wit_parser::*;

// NB: keep in sync with `crates/wit-parser/src/ast/lex.rs`
const PRINT_F32_F64_DEFAULT: bool = true;

/// A utility for printing WebAssembly interface definitions to a string.
pub struct WitPrinter {
output: Output,
ext: WitPrinterExt<OutputToString>,
Copy link
Member

Choose a reason for hiding this comment

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

FWIW it's ok to have breaking changes to this crate, so it might make more sense to repurpose the existing WitPrinter as WitPrinter<O> now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

self.output.push_str(";");
/// Print the specified package without its content.
/// Does not print the semicolon nor starts the indentation.
pub fn print_package_outer(&mut self, pkg: &Package) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

How come some methods were made pub in this PR? It's totally ok to do so, but I just wanted to confirm this wasn't by accident and it was intentional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was intentional, answering here: #1956 (comment)

crates/wit-component/src/printing.rs Outdated Show resolved Hide resolved
Comment on lines 1120 to 1139
/// A newline is added.
fn newline(&mut self);
/// A keyword is added. Keywords are hardcoded strings from `[a-z]`, but can start with `@`
/// when printing a [Feature Gate](https://github.com/WebAssembly/component-model/blob/main/design/mvp/WIT.md#feature-gates)
fn keyword(&mut self, src: &str);
/// A type is added.
fn r#type(&mut self, src: &str, kind: TypeKind);
/// A parameter name of a function, record or a named return is added.
fn param(&mut self, src: &str);
/// A case belonging to a variant, enum or flags is added.
fn case(&mut self, src: &str);
/// Generic argument section starts. In WIT this represents the `<` character.
fn generic_args_start(&mut self);
/// Generic argument section ends. In WIT this represents the '>' character.
fn generic_args_end(&mut self);
/// Called when a single documentation line is added.
/// The `doc` parameter starts with `///` omitted, and can be an empty string.
fn doc(&mut self, doc: &str);
/// A semicolon is added.
fn semicolon(&mut self);
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it might make sense to forward these methods to str by default? That way the implementation for OutputString could be a bit simpler perhaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've extracted most of the functions into default trait methods, keeping only push_str and three others that are involved with indentation.

@tomasol
Copy link
Contributor Author

tomasol commented Dec 18, 2024

Thanks for this! Are you planning on integrating this externally into an IDE or similar perhaps?

Thanks for the quick review! I am building an HTML highlighter as part of a larger project - not just for displaying colors, but also for deeper integration, such as showing icons next to certain functions and filtering out parts of a WIT document.

This is also why some higher-level functions are marked as pub in the PR.

Here is how it looks now:
image

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks for this, and that looks pretty cool! One small thing here but otherwise should be good to go

Comment on lines 32 to 43
impl<O: Output> WitPrinter<O>
where
String: From<O>,
{
/// Convenience wrapper around [`print_all`](WitPrinter::print_all) that prints the specified `pkg` which is located in `resolve` to a string.
///
/// The `nested` list of packages are other packages to include at the end
/// of the output in `package ... { ... }` syntax.
pub fn print(self, resolve: &Resolve, pkg: PackageId, nested: &[PackageId]) -> Result<String> {
self.print_all(resolve, pkg, nested).map(String::from)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

With self.output being a pub field could this be removed and could print_all below remain as print with &mut self? It should be ok to change its return value to Result<()> and then what to do with all the output is up to the caller with respect to self.output

As per PR request, merge `print` with `print_all`.
@alexcrichton alexcrichton added this pull request to the merge queue Dec 19, 2024
Merged via the queue into bytecodealliance:main with commit 1e85a2a Dec 19, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants