chore(grit): add auto-wrap + integrate with `search` command #3288

arendjr · 2024-06-25T20:25:05Z

Summary

This implements the first iteration of the biome search command:

Changes in this PR:

Completed more Grit bindings
Implemented auto-wrap
Added SearchCapabilities to our file features system.

Note that diagnostics are still rough in some places, and plenty of features are still unsupported.

Test Plan

Tested manually.

codspeed-hq · 2024-07-02T21:25:19Z

CodSpeed Performance Report

Merging #3288 will degrade performances by 7.88%

_{Comparing arendjr:small-steps (c5cdc08) with arendjr:small-steps (dcfb43d)}

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 106 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`arendjr:small-steps`	`arendjr:small-steps`	Change
⚡	`dojo_11880045762646467684.js[cached]`	8.6 ms	8 ms	+6.83%
❌	`db_2930068967297060348.json[cached]`	12.5 ms	13.6 ms	-7.88%

ematipico

Great! We are almost here :)

I believe there's some work to be done around diagnostics and the usage of anyhow. Not sure when they should be addressed, but really should at some point. Other than that, I just left few nitpicks.

ematipico · 2024-07-03T08:49:20Z

crates/biome_grit_patterns/src/grit_context.rs

+
+        for f in new_files {
+            let Some(file) = f.get_file() else {
+                bail!("Expected a list of files")


Internally, we don't use anyhow, so I wonder if you plan to change this later.

I'd like to, but it would require updating the Grit libraries too. I agree it's less than ideal now.

I believe this is a fix that we should follow up upstream. anyhow isn't meant for libraries but for applications, so I suppose we can open an issue.

ematipico · 2024-07-03T08:50:19Z

crates/biome_grit_patterns/src/grit_context.rs

+            let name: PathBuf = file
+                .name(&state.files)
+                .text(&state.files, &self.lang)
+                .unwrap()
+                .as_ref()
+                .into();
+            let body = file
+                .body(&state.files)
+                .text(&state.files, &self.lang)
+                .unwrap();
+            let owned_file =
+                new_file_owner(name.clone(), &body, &self.lang, logs)?.ok_or_else(|| {
+                    anyhow!(
+                        "failed to construct new file for file {}",
+                        name.to_string_lossy()
+                    )
+                })?;
+            self.files().push(owned_file);
+            let _ = state.files.push_new_file(self.files().last().unwrap());
+        }
+


Can you replace the unwrap with a safer alternative or provide a // SAFETY comment?

Addressed in 240455b.

ematipico · 2024-07-03T08:58:38Z

crates/biome_grit_patterns/src/errors.rs

+                match error.diagnostics.first() {
+                    Some(diag) => {
+                        fmt.write_str(": ")?;
+                        diag.message(fmt)
+                    }


This approach may not be correct. We are basically truncating diagnostics. I think we should revisit this part.

Also, diagnostics are marked as vector of SerializableDiagnostic, but they should not, theoretically. We want to use serialisable diagnostics only in particular cases; for example, we want to cross thread boundaries inside the workspace.

This means, the transformation must happen at the workspace level, like in this case:

biome/crates/biome_service/src/workspace/server.rs

Lines 633 to 635 in f663e6b

.map(|diag| {

let diag = diag.with_file_path(params.path.as_path().display().to_string());

SerdeDiagnostic::new(diag)

Addressed the part about SerializableDiagnostic in dcfb43d. I'll try to revisit the diagnostics further indeed, though I hope to push that to a follow-up PR. There's some even worse feedback issues still too 😅

crates/biome_grit_patterns/src/errors.rs

ematipico · 2024-07-03T09:05:58Z

crates/biome_service/src/diagnostics.rs

@@ -341,6 +343,30 @@ pub enum SearchError {
 )]
 pub struct InvalidPattern;

+#[derive(Debug, Serialize, Deserialize)]
+pub struct QueryError(pub String);


nit: QueryDiagnostic

Addressed in 240455b.

ematipico · 2024-07-03T09:07:41Z

crates/biome_service/src/diagnostics.rs

+    fn verbose_advices(&self, visitor: &mut dyn Visit) -> std::io::Result<()> {
+        visitor.record_log(
+            LogCategory::Info,
+            &markup! { "For reference, please consult: https://docs.grit.io/language/syntax" },


You can use <Hyperlink href="">please consult the official grit syntax page.</Hyperlink> to create a clickable link.

Addressed in 240455b.

ematipico · 2024-07-03T09:08:37Z

crates/biome_service/src/diagnostics.rs

+    fn message(&self, fmt: &mut biome_console::fmt::Formatter<'_>) -> std::io::Result<()> {
+        fmt.write_markup(markup! { "Error executing the Grit query: "{{&self.0}} })
+    }


Since this diagnostic is inside the service, it will likely be used by other clients, too, e.g., LSP. Hence, we should implement description, too.

Addressed in 240455b.

ematipico · 2024-07-03T09:10:19Z

crates/biome_service/src/file_handlers/mod.rs

+    &BiomePath,
+    &DocumentFileSource,
+    AnyParse,
+    &GritQuery,
+    WorkspaceSettingsHandle,


nit: Considering the great number of arguments, we could evaluate a struct instead

ematipico · 2024-07-03T09:13:12Z

crates/biome_service/src/file_handlers/mod.rs

@@ -584,6 +601,34 @@ pub(crate) fn parse_lang_from_script_opening_tag(script_opening_tag: &str) -> La
    .map_or(Language::JavaScript, |lang| lang)
 }

+pub(crate) fn search_file(


nit: I would call it search, without the use of file. While it feels correct now, it won't be once this feature will be wired to additional clients.

It will be inline with the format capabilities. I suppose we will have search_range too in the future.

Addressed in 240455b.

ematipico · 2024-07-03T09:14:18Z

crates/biome_service/src/workspace/server.rs

@@ -851,3 +847,9 @@ impl Workspace for WorkspaceServer {
 fn is_dir(path: &Path) -> bool {
    path.is_dir() || (path.is_symlink() && fs::read_link(path).is_ok_and(|path| path.is_dir()))
 }
+
+fn make_search_pattern_id() -> String {


What's this for?

For searching, the workspace protocol is split into three functions: parse_pattern(), search_pattern(), and drop_pattern(). The reason for this is that we only need to parse once, and all workers can search using the pattern by referencing a handle instead of parsing again. This function generates the IDs for these "handles".

Add auto-wrap with Step + integrate with search command

0713457

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Core Area: core A-Parser Area: parser A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages labels Jun 25, 2024

Register search capabilities per language

2bdb9fd

arendjr force-pushed the small-steps branch from 46682ec to 2bdb9fd Compare July 2, 2024 19:41

(Somewhat) improved diagnostics

163c7da

github-actions bot added the A-Diagnostic Area: diagnostocis label Jul 2, 2024

arendjr marked this pull request as ready for review July 2, 2024 20:48

arendjr requested a review from a team July 2, 2024 20:48

arendjr changed the title ~~Add auto-wrap with Step + integrate with search command~~ chore(grit): add auto-wrap + integrate with search command Jul 2, 2024

Merge branch 'main' into small-steps

cf202de

ematipico reviewed Jul 3, 2024

View reviewed changes

arendjr added 3 commits July 8, 2024 19:58

PR feedback

240455b

Use ParseDiagnostic instead of SerdeDiagnostic in CompileError

dcfb43d

Merge branch 'main' into small-steps

c5cdc08

arendjr force-pushed the small-steps branch from bf6d37a to c5cdc08 Compare July 8, 2024 19:21

arendjr merged commit a285781 into biomejs:main Jul 8, 2024
12 checks passed

Conaclos added the A-Changelog Area: changelog label Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(grit): add auto-wrap + integrate with `search` command #3288

chore(grit): add auto-wrap + integrate with `search` command #3288

arendjr commented Jun 25, 2024 •

edited

Loading

codspeed-hq bot commented Jul 2, 2024 •

edited

Loading

ematipico left a comment

ematipico Jul 3, 2024

arendjr Jul 8, 2024

ematipico Jul 9, 2024

ematipico Jul 3, 2024

arendjr Jul 8, 2024

ematipico Jul 3, 2024

arendjr Jul 8, 2024

ematipico Jul 3, 2024

arendjr Jul 8, 2024

ematipico Jul 3, 2024

arendjr Jul 8, 2024

ematipico Jul 3, 2024

arendjr Jul 8, 2024

ematipico Jul 3, 2024

ematipico Jul 3, 2024

arendjr Jul 8, 2024

ematipico Jul 3, 2024

arendjr Jul 8, 2024

	.map(\|diag\| {
	let diag = diag.with_file_path(params.path.as_path().display().to_string());
	SerdeDiagnostic::new(diag)

chore(grit): add auto-wrap + integrate with search command #3288

chore(grit): add auto-wrap + integrate with search command #3288

Conversation

arendjr commented Jun 25, 2024 • edited Loading

Summary

Test Plan

codspeed-hq bot commented Jul 2, 2024 • edited Loading

Merging #3288 will degrade performances by 7.88%

Summary

Benchmarks breakdown

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore(grit): add auto-wrap + integrate with `search` command #3288

chore(grit): add auto-wrap + integrate with `search` command #3288

arendjr commented Jun 25, 2024 •

edited

Loading

codspeed-hq bot commented Jul 2, 2024 •

edited

Loading