feat: implement `grit format` command #595

Arian8j2 · 2025-01-01T16:24:56Z

trying to resolve #574
this is just a proof of concept right now and needs more work and time to get complete
/claim #574

algora-pbc · 2025-01-01T16:25:05Z

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe.

morgante · 2025-01-03T03:01:17Z

Thanks @Arian8j2 for working on this, just wanted to chime in that it's definitely on the right track for what I had in mind. Once you're ready, feel free to request a review.

crates/cli/Cargo.toml

Arian8j2 · 2025-01-04T07:28:53Z

crates/cli/src/commands/format.rs

+    resolved.sort();
+
+    let file_path_to_resolved = group_resolved_patterns_by_group(resolved);
+    for (file_path, resovled_patterns) in file_path_to_resolved {


This can be easily executed in parallel, however the tests that verify stdout will fail due to inconsistencies in the output

Instead of changing the input to make tests happy, another option is to sort the test output by line so it is stabilized.

Arian8j2 · 2025-01-04T07:29:32Z

crates/cli/src/commands/format.rs

+    Ok(())
+}
+
+fn format_yaml_file(file_content: &str) -> Result<String> {


@morgante Is it ok to also format the whole yaml file?

What's the reason?

because we need to parse the whole yaml and edit the body and then serialize it back to yaml format, in this way the yaml code also get's formatted. i couldn't find a way to just change body field, also because it's usually multi line string it's also hard to just find the bytes and replace it, because we need additional space and tabs to make it valid yaml code.

We should not not serialize/deserialize the whole body. Round-tripping is lossy. We'd also lose any comments in the yaml.

I understand finding and replacing just the bytes is harder but it's very much possible.

the problem is in finding the code, for example consider this yaml file:

version: 0.0.1 patterns: - name: aspect_ratio_yaml description: Yaml version of aspect_ratio.md body: | language css `a { $props }` where { $props <: contains `aspect-ratio: $x` } - file: ./others/test_move_import.md

i can easily get the grit code:

language css `a { $props }` where { $props <: contains `aspect-ratio: $x` }

and format it no problem, but then changing the body with new formatted code is difficult because how i find the old grit code in the file? each line is prefixed with indent spaces:

language css `a { $props }` where { $props <: contains `aspect-ratio: $x` }

i need to know how much space i need to put before the code (i used to hard code this in the past commits but then it introduces other problems), thats the hard part, and i couldn't think of a way that is not hacky and breakable

i could try to find the code like this:

for i in 1..10 { let indent = " ".repeat(i); let grit_code_with_indent = prefix_each_line(indent, grit_code) if let Some(pos) = yaml_file_content.position(grit_code_with_indent) { // ... break } }

but it's a bit hacky

We actually have a lot of utils in GritQL itself for handling indentation—tested here—so I think you should be able to leverage those.

nice hint, GritQL really shines in these issues but i don't know if it's overkill or not for this, btw i updated the code, now it uses a grit pattern to find and replace yaml bodies with formatted ones

crates/cli/src/commands/format.rs

crates/cli/Cargo.toml

morgante · 2025-01-07T06:54:31Z

crates/cli/src/commands/format.rs

+    resolved.sort();
+
+    let file_path_to_resolved = group_resolved_patterns_by_group(resolved);
+    for (file_path, resovled_patterns) in file_path_to_resolved {


Instead of changing the input to make tests happy, another option is to sort the test output by line so it is stabilized.

crates/cli/src/commands/format.rs

morgante · 2025-01-07T06:55:36Z

crates/cli/src/commands/format.rs

+    Ok(())
+}
+
+fn format_yaml_file(file_content: &str) -> Result<String> {


What's the reason?

crates/cli/src/commands/format.rs

crates/cli_bin/tests/format.rs

crates/cli/src/commands/format.rs

morgante · 2025-01-12T05:35:40Z

@Arian8j2 Please make sure all tests / clippy pass.

Arian8j2 · 2025-01-12T06:08:41Z

@morgante Couldn't build biome with current cargo version of CI (1.76.0-nightly):

error[E0658]: the `#[expect]` attribute is an experimental feature
   --> .cargo/git/checkouts/biome-d49ce8898b420a50/8bec9fc/crates/biome_string_case/src/lib.rs:629:13
    |
629 |             #[expect(clippy::disallowed_methods)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #54503 <https://github.com/rust-lang/rust/issues/54503> for more information
    = help: add `#![feature(lint_reasons)]` to the crate attributes to enable

error[E0658]: the `#[expect]` attribute is an experimental feature
   --> .cargo/git/checkouts/biome-d49ce8898b420a50/8bec9fc/crates/biome_string_case/src/lib.rs:645:13
    |
645 |             #[expect(clippy::disallowed_methods)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #54503 <https://github.com/rust-lang/rust/issues/54503> for more information
    = help: add `#![feature(lint_reasons)]` to the crate attributes to enable

error[E0658]: the `#[expect]` attribute is an experimental feature
   --> .cargo/git/checkouts/biome-d49ce8898b420a50/8bec9fc/crates/biome_string_case/src/lib.rs:660:13
    |
660 |             #[expect(clippy::disallowed_methods)]
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #54503 <https://github.com/rust-lang/rust/issues/54503> for more information
    = help: add `#![feature(lint_reasons)]` to the crate attributes to enable

Should we perhaps increase the CI rust version?

morgante · 2025-01-13T21:14:43Z

I think newer Rust versions were breaking our binary on osx. Can you see about activating the feature directly?

Arian8j2 · 2025-01-14T06:50:49Z

Can you see about activating the feature directly?

I don't think we can activate feature of other crates without changing them directly, We need to fork and change if we really want to keep the rust version. stack overflow of same issue
Edit: Also we need to fork one dependency of biome, oxc-resolver because it also uses expect attribute too

I think newer Rust versions were breaking our binary on osx

Whats the issue on osx? Maybe fixing that would be easier

Arian8j2 · 2025-01-20T10:06:06Z

@morgante If you are comfortable with maintaining forks, These are the patches that needs to be applied to forks.

biome patch:

diff --git a/Cargo.toml b/Cargo.toml
index a861ceaf44..c1281620f8 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -186,7 +186,7 @@ ignore             = "0.4.23"
 indexmap           = { version = "2.7.1", features = ["serde"] }
 insta              = "1.42.0"
 natord             = "1.0.9"
-oxc_resolver       = "3.0.3"
+oxc_resolver       = { git = "our_oxc_resolver_fork"}
 proc-macro2        = "1.0.86"
 quickcheck         = "1.0.3"
 quickcheck_macros  = "1.0.0"
diff --git a/crates/biome_formatter/src/lib.rs b/crates/biome_formatter/src/lib.rs
index dc84856781..7adb5f4d5c 100644
--- a/crates/biome_formatter/src/lib.rs
+++ b/crates/biome_formatter/src/lib.rs
@@ -19,6 +19,7 @@
 //! * [`format_args!`]: Concatenates a sequence of Format objects.
 //! * [`write!`]: Writes a sequence of formatable objects into an output buffer.
 
+#![feature(lint_reasons)]
 #![deny(rustdoc::broken_intra_doc_links)]
 
 mod arguments;
diff --git a/crates/biome_grit_factory/src/lib.rs b/crates/biome_grit_factory/src/lib.rs
index 4eb787ecb1..2d19ead6fe 100644
--- a/crates/biome_grit_factory/src/lib.rs
+++ b/crates/biome_grit_factory/src/lib.rs
@@ -1,3 +1,5 @@
+#![feature(lint_reasons)]
+
 use biome_grit_syntax::GritLanguage;
 use biome_rowan::TreeBuilder;
 
diff --git a/crates/biome_grit_formatter/src/lib.rs b/crates/biome_grit_formatter/src/lib.rs
index fa7b8a3305..bb3438e08a 100644
--- a/crates/biome_grit_formatter/src/lib.rs
+++ b/crates/biome_grit_formatter/src/lib.rs
@@ -1,3 +1,5 @@
+#![feature(lint_reasons)]
+
 mod comments;
 pub mod context;
 mod cst;
diff --git a/crates/biome_rowan/src/lib.rs b/crates/biome_rowan/src/lib.rs
index 30a6cd76b3..7a7f651409 100644
--- a/crates/biome_rowan/src/lib.rs
+++ b/crates/biome_rowan/src/lib.rs
@@ -1,5 +1,6 @@
 //! A generic library for lossless syntax trees.
 //! See `examples/s_expressions.rs` for a tutorial.
+#![feature(lint_reasons, ptr_addr_eq, offset_of)]
 #![forbid(
 // missing_debug_implementations,
 unconditional_recursion,
diff --git a/crates/biome_string_case/src/lib.rs b/crates/biome_string_case/src/lib.rs
index 7d062b61ce..8d2df5a477 100644
--- a/crates/biome_string_case/src/lib.rs
+++ b/crates/biome_string_case/src/lib.rs
@@ -1,5 +1,6 @@
 //! Identify string case and convert to various string cases.
 
+#![feature(lint_reasons)]
 use std::{borrow::Cow, cmp::Ordering, ffi::OsStr};
 
 // Include the file generated by `../build.rs`
diff --git a/crates/biome_text_size/src/lib.rs b/crates/biome_text_size/src/lib.rs
index fd27c65523..34815c9aca 100644
--- a/crates/biome_text_size/src/lib.rs
+++ b/crates/biome_text_size/src/lib.rs
@@ -16,6 +16,7 @@
 //!
 //! Minimal Supported Rust Version: latest stable.
 
+#![feature(lint_reasons)]
 #![forbid(unsafe_code)]
 #![warn(missing_debug_implementations, missing_docs)]

oxc-resolver patch:

diff --git a/src/lib.rs b/src/lib.rs
index 3e7f743..77d1a65 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -47,6 +47,8 @@
 #![doc = include_str!("../examples/resolver.rs")]
 //! ```
 
+#![feature(lint_reasons)]
+
 mod builtins;
 mod cache;
 mod context;

morgante · 2025-01-25T22:46:55Z

@Arian8j2 I'e upgraded the main branch to 1.82, so hopefully you can rebase and this will be mergeable.

morgante · 2025-01-26T06:19:02Z

Looks like you need to run cargo format: https://github.com/getgrit/gritql/actions/runs/12972060346/job/36179168549

Arian8j2 · 2025-01-26T06:23:11Z

@morgante Also be careful when including this on release, It still panics on some stdlib patterns due to biome issues

crates/gritmodule/src/yaml.rs

morgante · 2025-01-27T01:06:52Z

Hi @Arian8j2 I'm going to take this over. While this initial implementation is kind of okay, it actually has massive memory usage and doesn't work remotely well.

Arian8j2 · 2025-01-27T05:32:45Z

@morgante

it actually has massive memory usage

Is the issue from grit side or biome?

doesn't work remotely well

Whats the issue?

Please feel free to make improvements, but I believe a complete rewrite of this PR isn't necessary. Additionally, will the bounty be awarded? Also i can give a hand on rewrite if you like, And also knowing what is on your mind will also help.

morgante · 2025-02-04T03:39:29Z

Hi @Arian8j2, the main issue I found is explosive memory usage. Running grit format on the stdlib uses 25GB of memory.

Obviously we can't ship it in that state. Some of the problems are obvious (we shouldn't keep all patterns in memory at once) but others will require deeper fixes. I started on a rewrite here: #606

Additionally, will the bounty be awarded?

I can only reward it once it's working viably.

Arian8j2 · 2025-02-04T14:13:10Z

@morgante

we shouldn't keep all patterns in memory at once

I don't think patterns take that much memory, Even with stdlib that has unusual amount of patterns.

I looked into this and the problem is from biome, Really small test that reproduces it:

#[test]
fn biome_memory_issue() {
    let code = "language js\n";
    biome_grit_parser::parse_grit(code);
}

Run this test in release mode and boom it will explode memory, Interestingly it doesn't explode on debug mode, Previously i just tested the debug version, So i was shocked when you said it has memory issues, One other thing that i found out is that the \n has something to do with it, If you remove that it doesn't explode.

morgante · 2025-02-04T16:42:29Z

I don't think patterns take that much memory, Even with stdlib that has unusual amount of patterns.

We still shouldn't need to keep them all in memory at the same time.

I looked into this and the problem is from biome, Really small test that reproduces it:

I've opened an issue upstream: biomejs/biome#5032

Arian8j2 requested a review from a team as a code owner January 1, 2025 16:24

algora-pbc bot mentioned this pull request Jan 1, 2025

grit format command #574

Closed

algora-pbc bot added the 🙋 Bounty claim label Jan 1, 2025

Arian8j2 marked this pull request as draft January 1, 2025 16:25

Arian8j2 force-pushed the main branch from 772526b to 3dcc719 Compare January 4, 2025 07:21

Arian8j2 commented Jan 4, 2025

View reviewed changes

crates/cli/Cargo.toml Outdated Show resolved Hide resolved

Arian8j2 commented Jan 4, 2025

View reviewed changes

Arian8j2 marked this pull request as ready for review January 4, 2025 07:30

Arian8j2 commented Jan 4, 2025

View reviewed changes

crates/cli/src/commands/format.rs Outdated Show resolved Hide resolved

morgante reviewed Jan 7, 2025

View reviewed changes

Arian8j2 force-pushed the main branch from d04e971 to 54076c7 Compare January 7, 2025 12:21

morgante reviewed Jan 11, 2025

View reviewed changes

crates/cli/src/commands/format.rs Outdated Show resolved Hide resolved

morgante reviewed Jan 11, 2025

View reviewed changes

crates/cli/src/commands/format.rs Show resolved Hide resolved

Arian8j2 changed the title ~~implement grit format command~~ feat: implement grit format command Jan 12, 2025

Arian8j2 force-pushed the main branch from ac8a0a8 to 76c4cc4 Compare January 14, 2025 05:47

morgante mentioned this pull request Jan 25, 2025

chore: increase Rust version to 1.82.0 #605

Merged

Arian8j2 added 5 commits January 26, 2025 09:38

feat: implement proof of concept of format command

467a7d2

refactor: move format for loop body to different function

855a1cb

feat: add tests for markdown and grit files (yaml currently has issues)

99f48dd

feat: add yaml support

e480a68

feat: make file path bold in print

4ed7c52

Arian8j2 added 13 commits January 26, 2025 09:38

refactor: return some errors instead of unwrapping

ca0a7f1

chore: fix typos

94a6d23

feat: format files in parallel

4d88528

refactor: remove useless reference

b805e25

test: remove format_patterns test that snapshots the stdout

163f25e

test: add test for not parsable patterns

ba78b2e

feat: use grit pattern to find and replace grit code in yaml

8eeba78

chore: fix comment

04ab53c

refactor: format_yaml_file take patterns by reference

ca69b53

chore: fix clippy warnings

af354d6

refactor: remove usesless clones

6a21264

refactor: apply hunk changes in reverse to make our life easier

bf576ac

chore: revert cargo lock to main branch one

d8c678d

Arian8j2 force-pushed the main branch from 5573a34 to d8c678d Compare January 26, 2025 06:10

chore: update cargo lock

b6261a0

chore: format mod.rs

f3d9af8

Arian8j2 added 2 commits January 26, 2025 09:57

chore: remove outdated comments

316f3a0

test: fix test returns_expected_help

cdbd4b4

morgante reviewed Jan 26, 2025

View reviewed changes

crates/gritmodule/src/yaml.rs Outdated Show resolved Hide resolved

Arian8j2 added 2 commits January 26, 2025 10:21

test: fix tests that failed due to some formats now having raw data

7994623

chore: fix typo

b5208ec

morgante merged commit 5af841f into getgrit:main Jan 26, 2025
5 checks passed

morgante mentioned this pull request Jan 27, 2025

fix: make format work better #606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement `grit format` command #595

feat: implement `grit format` command #595

Arian8j2 commented Jan 1, 2025

algora-pbc bot commented Jan 1, 2025

morgante commented Jan 3, 2025

Arian8j2 Jan 4, 2025

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025

Arian8j2 Jan 4, 2025 •

edited

Loading

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025

morgante Jan 7, 2025

Arian8j2 Jan 7, 2025

Arian8j2 Jan 9, 2025

morgante Jan 9, 2025

Arian8j2 Jan 10, 2025 •

edited

Loading

morgante Jan 7, 2025

morgante Jan 7, 2025

morgante commented Jan 12, 2025

Arian8j2 commented Jan 12, 2025

morgante commented Jan 13, 2025

Arian8j2 commented Jan 14, 2025 •

edited

Loading

Arian8j2 commented Jan 20, 2025

morgante commented Jan 25, 2025

morgante commented Jan 26, 2025

Arian8j2 commented Jan 26, 2025

morgante commented Jan 27, 2025

Arian8j2 commented Jan 27, 2025 •

edited

Loading

morgante commented Feb 4, 2025

Arian8j2 commented Feb 4, 2025

morgante commented Feb 4, 2025 •

edited

Loading

feat: implement grit format command #595

feat: implement grit format command #595

Conversation

Arian8j2 commented Jan 1, 2025

algora-pbc bot commented Jan 1, 2025

morgante commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arian8j2 Jan 4, 2025 • edited Loading

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

Arian8j2 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morgante commented Jan 12, 2025

Arian8j2 commented Jan 12, 2025

morgante commented Jan 13, 2025

Arian8j2 commented Jan 14, 2025 • edited Loading

Arian8j2 commented Jan 20, 2025

morgante commented Jan 25, 2025

morgante commented Jan 26, 2025

Arian8j2 commented Jan 26, 2025

morgante commented Jan 27, 2025

Arian8j2 commented Jan 27, 2025 • edited Loading

morgante commented Feb 4, 2025

Arian8j2 commented Feb 4, 2025

morgante commented Feb 4, 2025 • edited Loading

feat: implement `grit format` command #595

feat: implement `grit format` command #595

Arian8j2 Jan 4, 2025 •

edited

Loading

Arian8j2 Jan 10, 2025 •

edited

Loading

Arian8j2 commented Jan 14, 2025 •

edited

Loading

Arian8j2 commented Jan 27, 2025 •

edited

Loading

morgante commented Feb 4, 2025 •

edited

Loading