feat: graphql lexer #2271

vohoanglong0107 · 2024-04-02T09:52:16Z

Summary

Implement a Lexer for GraphQL
This PR also include a minimal parser to demonstrate that things work

Test Plan

All the tests for GraphQL lexer and parser pass

netlify · 2024-04-02T09:52:34Z

✅ Deploy Preview for biomejs ready!

Name	Link
🔨 Latest commit	`23546e5`
🔍 Latest deploy log	https://app.netlify.com/sites/biomejs/deploys/660f64d87b0d6600083accf6
😎 Deploy Preview	https://deploy-preview-2271--biomejs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 5 from production) Accessibility: 97 (no change from production) Best Practices: 100 (no change from production) SEO: 93 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

codspeed-hq · 2024-04-02T10:20:04Z

CodSpeed Performance Report

Merging #2271 will improve performances by 11.8%

_{Comparing vohoanglong0107:feat-graphql-lexer (23546e5) with main (2bd95dc)}

Summary

⚡ 1 improvements
✅ 92 untouched benchmarks

Benchmarks breakdown

	Benchmark	`main`	`vohoanglong0107:feat-graphql-lexer`	Change
⚡	`big5-added.json[cached]`	2.4 ms	2.1 ms	+11.8%

arendjr

I still see two todo!(), but other than that, LGTM!

Note that for the Grit lexer, I have also just left an unimplemented!() inside the rewind() method. That probably works here as well.

denbezrukov · 2024-04-02T20:50:18Z

crates/biome_graphql_parser/src/lexer/mod.rs

+    }
+}
+
+fn is_name_start(byte: u8) -> bool {


I'm wondering if we can use some functions from crates/biome_unicode_table/src/lib.rs here?

I don't think so, those function support unicode characters while graphql identity only allow ascii

ematipico

I looked at the code, and overall, it seems fine, although I found a few things we should avoid doing in production. Would you mind addressing them?

ematipico · 2024-04-03T10:20:34Z

crates/biome_graphql_parser/src/lexer/mod.rs

+    }
+
+    fn rewind(&mut self, _checkpoint: LexerCheckpoint<Self::Kind>) {
+        todo!();


Instead of todo, you should use unimplemented! and provide a brief explanation

ematipico · 2024-04-03T10:21:53Z

crates/biome_graphql_parser/src/lexer/mod.rs

+
+    /// Lexes an ellipsis.
+    fn consume_ellipsis(&mut self) -> GraphqlSyntaxKind {
+        assert_eq!(self.current_byte(), Some(b'.'));


We don't want to ship assertion in production. You should use debug_assert_eq

ematipico · 2024-04-03T10:23:26Z

crates/biome_graphql_parser/src/lexer/mod.rs

+    }
+
+    fn consume_digit(&mut self, chr: u8, state: LexNumberState) -> LexNumberState {
+        assert!(chr.is_ascii_digit());


No assertion in production. You should use debug_assert

ematipico · 2024-04-03T10:24:25Z

crates/biome_graphql_parser/src/lexer/mod.rs

+        }
+    }
+
+    fn consume_digit(&mut self, chr: u8, state: LexNumberState) -> LexNumberState {


What's the difference between a digit and a number? It's worth adding a comment that briefly explains it. Also, LexNumberState should be called LexDigitState, to avoid confusion.

ematipico · 2024-04-03T10:25:24Z

crates/biome_graphql_parser/src/lexer/mod.rs

+    }
+
+    fn consume_string(&mut self) -> GraphqlSyntaxKind {
+        assert_eq!(self.current_byte(), Some(b'"'));


use debug_assert

ematipico · 2024-04-03T10:25:38Z

crates/biome_graphql_parser/src/lexer/mod.rs

+        start: TextSize,
+        state: LexNumberState,
+    ) -> LexNumberState {
+        assert!(matches!(chr, b'e' | b'E'));


use debug_assert

ematipico · 2024-04-03T10:27:01Z

crates/biome_graphql_parser/src/lexer/mod.rs

+                    let diagnostic = ParseDiagnostic::new(
+                        "Invalid escape sequence",
+                        escape_start..self.text_position() + c.text_len(),
+                    ).with_hint(r#"Valid escape sequences are: `\\`, `\/`, `/"`, `\b\`, `\f`, `\n`, `\r`, `\t` or any unicode escape sequence `\uXXXX` where X is hexedecimal number. "#);


Consider using with_alternatives to present a list of things to the user instead.

ematipico · 2024-04-03T10:27:51Z

crates/biome_graphql_parser/src/lexer/mod.rs

+                    (state, Some(diagnostic))
+                }
+            }
+            _ => panic!("String uninitialized/terminated"),


We don't want to panic in production. Consider using a safe approach, maybe by emitting a diagnostic and/or using consume_unexpecated_character

ematipico · 2024-04-03T10:29:00Z

crates/biome_graphql_parser/src/lexer/mod.rs

+    }
+
+    fn consume_comment(&mut self) -> GraphqlSyntaxKind {
+        assert_eq!(self.current_byte(), Some(b'#'));


use debug_assert_eq

denbezrukov · 2024-04-03T20:27:20Z

crates/biome_graphql_parser/src/parser/definitions/operation.rs

+
+#[inline]
+fn parse_selection_set(p: &mut GraphqlParser) -> ParsedSyntax {
+    if !p.at(T!['{']) {


I'd like to propose removing this conditional because doing so would enable parsing of an invalid syntax without losing the set in the absence of {.

You mean something like this?

fn parse_selection_set(p: &mut GraphqlParser) -> ParsedSyntax { if !p.eat(T!['{']) {p.error("Expected `{` here")}

Yes,
I believe that we have 'p.expect(T![token])'🤔

Could you check it again e86a331

ematipico

Looks good. Just a few nits to address and we can merge it :)

crates/biome_graphql_parser/src/parser/mod.rs

crates/biome_graphql_parser/src/parser/definitions/operation.rs

crates/biome_graphql_parser/src/parser/directive.rs

crates/biome_graphql_parser/tests/graphql_test_suite/ok/operation.graphql.snap

denbezrukov · 2024-04-04T09:48:55Z

crates/biome_graphql_parser/src/parser/definitions/operation.rs

+    let m = p.start();
+    p.expect(T!['{']);
+    SelectionList::new().parse_list(p);
+    p.bump(T!['}']);


Yeah, it looks good!

Sorry, I forgot to mention that we also need to use expect here. It seems we need to check the at/at_ts token and call bump to ensure that we don't encounter a panic in production.

Suggested change

p.bump(T!['}']);

p.expect(T!['}']);

I understand. I didn't notice bump panic if the token was not found

denbezrukov

🙏

feat: graphql lexer

6086543

github-actions bot added the A-Tooling Area: internal tools label Apr 2, 2024

arendjr approved these changes Apr 2, 2024

View reviewed changes

denbezrukov reviewed Apr 2, 2024

View reviewed changes

ematipico requested changes Apr 3, 2024

View reviewed changes

vohoanglong0107 added 5 commits April 3, 2024 12:08

fix: use debug assert

805bdea

fix: use unimplemented instead of todo

16d9083

docs: explain different between number and a digit

08aa8c6

fix: remove panic with diagnostic

6898b36

fix: diagnostic with alternatives

a01d4f2

denbezrukov reviewed Apr 3, 2024

View reviewed changes

vohoanglong0107 requested a review from ematipico April 3, 2024 23:15

fix: remove the { check for selection set

e86a331

ematipico approved these changes Apr 4, 2024

View reviewed changes

vohoanglong0107 added 3 commits April 4, 2024 07:51

fix: GraphqlRoot instead of GraphqlDocument

5d25c2b

fix: graphql in tests

6f0579a

fix: derive default

760512d

vohoanglong0107 force-pushed the feat-graphql-lexer branch from 5f09443 to 760512d Compare April 4, 2024 07:59

denbezrukov reviewed Apr 4, 2024

View reviewed changes

denbezrukov approved these changes Apr 4, 2024

View reviewed changes

fix: expect instead of bump

23546e5

ematipico merged commit 2b19194 into biomejs:main Apr 5, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: graphql lexer #2271

feat: graphql lexer #2271

vohoanglong0107 commented Apr 2, 2024

netlify bot commented Apr 2, 2024 •

edited

codspeed-hq bot commented Apr 2, 2024 •

edited

arendjr left a comment

denbezrukov Apr 2, 2024

vohoanglong0107 Apr 3, 2024

ematipico left a comment

ematipico Apr 3, 2024 •

edited

ematipico Apr 3, 2024

ematipico Apr 3, 2024

ematipico Apr 3, 2024

ematipico Apr 3, 2024

ematipico Apr 3, 2024

ematipico Apr 3, 2024

ematipico Apr 3, 2024

ematipico Apr 3, 2024

denbezrukov Apr 3, 2024

vohoanglong0107 Apr 3, 2024

denbezrukov Apr 4, 2024

vohoanglong0107 Apr 4, 2024

ematipico left a comment

denbezrukov Apr 4, 2024

vohoanglong0107 Apr 5, 2024

denbezrukov left a comment

feat: graphql lexer #2271

feat: graphql lexer #2271

Conversation

vohoanglong0107 commented Apr 2, 2024

Summary

Test Plan

netlify bot commented Apr 2, 2024 • edited

✅ Deploy Preview for biomejs ready!

codspeed-hq bot commented Apr 2, 2024 • edited

Merging #2271 will improve performances by 11.8%

Summary

Benchmarks breakdown

arendjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

ematipico Apr 3, 2024 • edited

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

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

denbezrukov left a comment

Choose a reason for hiding this comment

netlify bot commented Apr 2, 2024 •

edited

codspeed-hq bot commented Apr 2, 2024 •

edited

ematipico Apr 3, 2024 •

edited