feat(css_parser): introduce grit metavariable #3340

ah-yu · 2024-07-03T02:36:15Z

Summary

Part of #3334

Grammar: https://github.com/getgrit/gritql/blob/main/resources/language-metavariables/tree-sitter-css/grammar.js

Test Plan

Add new tests

codspeed-hq · 2024-07-03T03:08:31Z

CodSpeed Performance Report

Merging #3340 will degrade performances by 20.68%

_{Comparing ah-yu:feat/metavar (89ee4bb) with main (29280e3)}

Summary

❌ 1 regressions
✅ 107 untouched benchmarks

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

Benchmarks breakdown

	Benchmark	`main`	`ah-yu:feat/metavar`	Change
❌	`eucjp_4223654609205405644.json[uncached]`	965 µs	1,216.6 µs	-20.68%

denbezrukov · 2024-07-03T04:31:29Z

Great start!

I'm wondering if we can solve the problem in general. It seems all we need is to have a node that can be anywhere and not raise an invalid syntax error. We already have a bogus node type that can be anywhere in the tree; however, it produces a tree with errors.

What do you think about having something like a bogus type that doesn't break the syntax tree?

ah-yu · 2024-07-03T04:53:59Z

What do you think about having something like a bogus type that doesn't break the syntax tree?

Sounds good! However, I am wondering if it will meet the needs of the plugin side, as they would like to know the exact node type of metavariable. @arendjr

biome/crates/biome_grit_patterns/src/grit_target_language.rs

Lines 230 to 231 in f663e6b

    
               /// Returns the syntax kind for metavariables. 
        
               fn metavariable_kind() -> Self::Kind;

crates/biome_css_parser/src/lexer/mod.rs

arendjr · 2024-07-03T06:20:48Z

However, I am wondering if it will meet the needs of the plugin side, as they would like to know the exact node type of metavariable.

We do need to be able to check for the node type indeed. But as long as it’s a distinct kind that should be covered.

I’m just not entirely sure how I should understand this sentence:

It seems all we need is to have a node that can be anywhere and not raise an invalid syntax error

How does that differ from the current metavariable implementation exactly? We can let it appear anywhere we want and it doesn’t raise an error. I guess the subtlety lies in how we make it appear where we want? I would be fine with an alternative approach to using specialized syntax (like the μ character), but I’m not sure I see a better way?

denbezrukov · 2024-07-03T08:48:52Z

However, I am wondering if it will meet the needs of the plugin side, as they would like to know the exact node type of metavariable.

We do need to be able to check for the node type indeed. But as long as it’s a distinct kind that should be covered.

I’m just not entirely sure how I should understand this sentence:

It seems all we need is to have a node that can be anywhere and not raise an invalid syntax error

How does that differ from the current metavariable implementation exactly? We can let it appear anywhere we want and it doesn’t raise an error. I guess the subtlety lies in how we make it appear where we want? I would be fine with an alternative approach to using specialized syntax (like the μ character), but I’m not sure I see a better way?

I agree, but I have slight concerns that we would have to adjust the grammar in many places to include CssGritMetavariable because we want to have it almost anywhere in the syntax tree.

I'm trying to understand if it's possible to implement something similar to what we have for the bogus node.

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

I'm with @denbezrukov on this one. Considering the approach of the metavariables (grit is one, and we will have more too, and they will be sparse across languages), I believe we should strike for a more generic approach using known nodes, very much like the bogus nodes.

We've done this for other nodes, such as Any*, *List, and *Bogus. *Meta could be just one of them.

ah-yu · 2024-07-04T03:03:58Z

If I understand correctly, @denbezrukov and @ematipico are proposing something like the following:

CssMetaValue = CssGritMetavar | AnotherQueryLanguageMetavar
CssMetaSelector = CssGritMetavar | AnotherQueryLanguageMetavar

AnyCssValue = 
    ...
    CssMetaValue

AnyCssSelector = 
    ...
    CssMetaSelector

Is that correct?

arendjr · 2024-07-04T06:15:33Z

Yes, I think that’s the gist of it. And now I see what @denbezrukov meant, I agree too. It would be best if we had a more generic way of handling these. Probably the solution indeed is that we should be able to specify in the .ungram where they might occur and then the codegen should pretty much take care of the rest.

denbezrukov · 2024-07-04T07:22:19Z

We can try to improve it in another PR:)

ematipico · 2024-07-05T06:57:38Z

crates/biome_css_parser/src/lexer/mod.rs

@@ -1326,6 +1353,7 @@ impl<'src> ReLexer<'src> for CssLexer<'src> {
            Some(current) => match context {
                CssReLexContext::Regular => self.consume_token(current),
                CssReLexContext::UnicodeRange => self.consume_unicode_range_token(current),
+                CssReLexContext::GritMetavariable => self.re_lex_grit_metavariable(old_position),


Why "re lex" and not "consume"? I don't see any re lexing inside the code

ematipico · 2024-07-05T06:59:04Z

crates/biome_css_parser/src/lexer/mod.rs

@@ -1315,6 +1317,31 @@ impl<'src> CssLexer<'src> {
            _ => false,
        }
    }
+
+    fn re_lex_grit_metavariable(&mut self, current_end: usize) -> CssSyntaxKind {


If we have the same logic inside JavaScript, this function should become part of the lexer trait. Not a blocker

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

crates/biome_css_parser/src/syntax/selector/mod.rs

-    // After parsing the compound selector, it then checks if this compound selector is a part of a complex selector.
-    parse_compound_selector(p).and_then(|selector| parse_complex_selector(p, selector))
+    if p.options().is_grit_metavariable_enabled() {
+        p.re_lex(CssReLexContext::GritMetavariable);


I'm not sure we need the re-lexing here, because we aren't doing any backwards operation. Instead, you should use parsing with a different context

We could try to move this logic inside the lexer.
By passing a new option to the lexer and, without any additional context, if the next token is μ (Επιτέλους, τα ελληνικά μου ήταν χρήσιμα!) and the option is enabled, we can process it as a meta variable.

Make sense. Addressed in 0ca8bd9

crates/biome_css_parser/src/lexer/mod.rs

Sec-ant

Just another nitpick, thanks!

crates/biome_css_parser/src/parser.rs

github-actions bot added A-Project Area: project A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools L-CSS Language: CSS labels Jul 3, 2024

arendjr reviewed Jul 3, 2024

View reviewed changes

crates/biome_css_parser/src/lexer/mod.rs Outdated Show resolved Hide resolved

ah-yu force-pushed the feat/metavar branch from 84a89e0 to d698550 Compare July 5, 2024 02:47

arendjr approved these changes Jul 5, 2024

View reviewed changes

ematipico reviewed Jul 5, 2024

View reviewed changes

ah-yu added 12 commits July 8, 2024 16:06

add grammar

ca64b68

gen formatter

311de89

implement metavar formatting

9cb8a4f

as any value

be54c82

as block item

da35f93

as any media query

b5de569

as any selector

564ed02

add option

0416c8a

add tests

46ec9ff

clippy

20d5380

add comments

fc7e3d5

$ => μ

74e00d4

ah-yu added 2 commits July 8, 2024 16:07

support μ...

dcc180a

move logic to the lexer

0ca8bd9

ah-yu force-pushed the feat/metavar branch from d698550 to 0ca8bd9 Compare July 8, 2024 08:08

update snapshot

0dc8f1a

denbezrukov approved these changes Jul 8, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat/metavar

90b5cbe

ah-yu requested a review from ematipico July 9, 2024 02:23

ematipico approved these changes Jul 9, 2024

View reviewed changes

Sec-ant reviewed Jul 9, 2024

View reviewed changes

crates/biome_css_parser/src/lexer/mod.rs Outdated Show resolved Hide resolved

typo

9f6adab

ah-yu requested a review from Sec-ant July 9, 2024 07:19

Sec-ant approved these changes Jul 9, 2024

View reviewed changes

crates/biome_css_parser/src/parser.rs Outdated Show resolved Hide resolved

typo

89ee4bb

ah-yu merged commit a77c00b into biomejs:main Jul 9, 2024
12 of 13 checks passed

ah-yu deleted the feat/metavar branch July 9, 2024 08:41

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(css_parser): introduce grit metavariable #3340

feat(css_parser): introduce grit metavariable #3340

ah-yu commented Jul 3, 2024

codspeed-hq bot commented Jul 3, 2024 •

edited

Loading

denbezrukov commented Jul 3, 2024

ah-yu commented Jul 3, 2024

arendjr commented Jul 3, 2024

denbezrukov commented Jul 3, 2024

ematipico commented Jul 3, 2024

ah-yu commented Jul 4, 2024

arendjr commented Jul 4, 2024 •

edited

Loading

denbezrukov commented Jul 4, 2024

ematipico Jul 5, 2024

ematipico Jul 5, 2024

ematipico Jul 5, 2024

denbezrukov Jul 7, 2024

ah-yu Jul 8, 2024

Sec-ant left a comment

feat(css_parser): introduce grit metavariable #3340

feat(css_parser): introduce grit metavariable #3340

Conversation

ah-yu commented Jul 3, 2024

Summary

Test Plan

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

CodSpeed Performance Report

Merging #3340 will degrade performances by 20.68%

Summary

Benchmarks breakdown

denbezrukov commented Jul 3, 2024

ah-yu commented Jul 3, 2024

arendjr commented Jul 3, 2024

denbezrukov commented Jul 3, 2024

ematipico commented Jul 3, 2024

ah-yu commented Jul 4, 2024

arendjr commented Jul 4, 2024 • edited Loading

denbezrukov commented Jul 4, 2024

ematipico Jul 5, 2024

Choose a reason for hiding this comment

ematipico Jul 5, 2024

Choose a reason for hiding this comment

ematipico Jul 5, 2024

Choose a reason for hiding this comment

denbezrukov Jul 7, 2024

Choose a reason for hiding this comment

ah-yu Jul 8, 2024

Choose a reason for hiding this comment

Sec-ant left a comment

Choose a reason for hiding this comment

codspeed-hq bot commented Jul 3, 2024 •

edited

Loading

arendjr commented Jul 4, 2024 •

edited

Loading