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

feat(grit): parse Grit literal snippets #3051

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Jun 3, 2024

Summary

Grit patterns can contain snippet literals like such:

`console.log("foo");`

The part inside the backticks is a structural pattern that matches any function call node where the callee matches console.log and which has a single string argument with the content foo.

This PR updates the Grit crates and implements the parsing for snippet literals, so that our own Grit bindings invoke the Biome JS parser when a snippet like the above is encountered.

Test Plan

Added a quick test that helps with testing Grit patterns, although it still fails in the execution part. But at least it parses the query with a literal snippet now.

Copy link

codspeed-hq bot commented Jun 3, 2024

CodSpeed Performance Report

Merging #3051 will not alter performance

Comparing arendjr:grit-literal-parser (47e2cd8) with arendjr:grit-literal-parser (eac93bf)

Summary

✅ 92 untouched benchmarks

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I left some suggestions that might help to improve the code

Comment on lines 18 to 43
(Some(range), Some(source)) => source.text[..range.start().into()]
.lines()
.enumerate()
.last()
.map(|(i, line)| {
let start = Position {
line: (i + 1) as u32,
column: line.len() as u32,
};
let end = source.text[range].lines().enumerate().last().map_or(
start,
|(j, line)| Position {
line: start.line + j as u32,
column: if j == 0 {
start.column + line.len() as u32
} else {
line.len() as u32
},
},
);
Range {
start,
end,
start_byte: range.start().into(),
end_byte: range.end().into(),
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about the intention of this code, but we have a utility to extract columns and rows from the source code, I suggest to look here and see how we use it:

let source = SourceFile::new(source_code);
let start = source.location(span.start())?;
let end = source.location(span.end())?;

Severity::Error => 4,
Severity::Fatal => 5,
}),
message: PrintDescription(self).to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

We have an utility called markup_to_string that you might want to look at:

let text = markup_to_string(markup! {
{PrintDiagnostic::verbose(&error)}
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately that one is only available in the biome_cli crate. I'll see how far we get with the description and range. If necessary, I'll try to move the util.

Copy link
Member

Choose a reason for hiding this comment

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

It's not exactly that one, but we have it

pub fn print_diagnostic_to_string(diagnostic: &Error) -> String {
let mut buffer = termcolor::Buffer::no_color();
Formatter::new(&mut Termcolor(&mut buffer))
.write_markup(markup! {
{PrintDiagnostic::verbose(diagnostic)}
})
.expect("failed to emit diagnostic");
let mut content = String::new();
writeln!(
content,
"{}",
std::str::from_utf8(buffer.as_slice()).expect("non utf8 in error buffer")
)
.unwrap();
content
}

let token = node.value_token()?;
let text = token.text_trimmed();
let range = node.syntax().text_trimmed_range().to_byte_range();
parse_snippet_content(&text[1..text.len() - 1], range, context, is_rhs)
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth leaving a comment explaining what we are trimming here, e.g. why there is a 1 and a -1. For example, I don't know, even though I can guess it by looking at the PR

.map_or(None, |s| Some(DynamicPattern::Snippet(s)));
Ok(Pattern::CodeSnippet(GritCodeSnippet {
dynamic_snippet,
source: source.to_owned(),
Copy link
Member

Choose a reason for hiding this comment

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

I noticed a lot of to_owned in this PR. Internally, we prefer to use to_string whenever possible unless it's a type that needs to use this function.

@github-actions github-actions bot added the A-Diagnostic Area: diagnostocis label Jun 4, 2024
@arendjr arendjr merged commit cdd11b5 into biomejs:main Jun 4, 2024
11 checks passed
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants