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(biome_graphql_parser): parse object extension #2928

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 13 additions & 65 deletions crates/biome_graphql_factory/src/generated/node_factory.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

91 changes: 2 additions & 89 deletions crates/biome_graphql_factory/src/generated/syntax_factory.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion crates/biome_graphql_parser/src/parser/definitions/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use self::{
fragment::parse_fragment_definition,
input_object::parse_input_object_type_definition,
interface::parse_interface_type_definition,
object::parse_object_type_definition,
object::{parse_object_type_definition, parse_object_type_extension},
operation::{parse_operation_definition, parse_selection_set},
r#enum::parse_enum_type_definition,
scalar::{parse_scalar_type_definition, parse_scalar_type_extension},
Expand Down Expand Up @@ -98,6 +98,7 @@ fn parse_extension(p: &mut GraphqlParser) -> ParsedSyntax {
match p.nth(1) {
T![schema] => parse_schema_extension(p),
T![scalar] => parse_scalar_type_extension(p),
T![type] => parse_object_type_extension(p),
_ => Absent,
}
}
Expand Down
34 changes: 31 additions & 3 deletions crates/biome_graphql_parser/src/parser/definitions/object.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
use crate::parser::{
directive::DirectiveList, parse_description, parse_error::expected_name, parse_name,
GraphqlParser,
directive::DirectiveList,
parse_description,
parse_error::{expected_name, expected_object_extension},
parse_name, GraphqlParser,
};
use biome_graphql_syntax::{GraphqlSyntaxKind::*, T};
use biome_parser::{
parse_lists::ParseNodeList, parsed_syntax::ParsedSyntax, prelude::ParsedSyntax::*, Parser,
parse_lists::ParseNodeList, parsed_syntax::ParsedSyntax, prelude::ParsedSyntax::*,
token_source::TokenSource, Parser,
};

use super::{field::parse_fields_definition, interface::parse_implements_interface};
Expand All @@ -29,3 +32,28 @@ pub(crate) fn parse_object_type_definition(p: &mut GraphqlParser) -> ParsedSynta

Present(m.complete(p, GRAPHQL_OBJECT_TYPE_DEFINITION))
}

/// Must only be called if the next 2 token is `extend` and `type`, otherwise it will panic.
#[inline]
pub(crate) fn parse_object_type_extension(p: &mut GraphqlParser) -> ParsedSyntax {
let m = p.start();

p.bump(T![extend]);
Copy link
Contributor

Choose a reason for hiding this comment

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

bump can panic if the current token doesn't match.
It's better to use expect or have a guard like is_at_object_type_extension

Copy link
Contributor Author

@vohoanglong0107 vohoanglong0107 May 21, 2024

Choose a reason for hiding this comment

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

I have placed a check on the outer parse_extension function here, so unless this function was called outside of parse_extension it should be fine. I think panic would even be preferred in this case, cause it will fail the test and indicate that this function was called at a unintended location. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm unsure how to handle this better because I understand the cons and pros of both approaches.
I see two ways:

  1. Add is_at_object_type_extension and use it only in parse_object_type_extension. This brings some duplication because we already check the correct parser state beforehand. However, I tend to use this approach.

  2. Write documentation stating that we must check extends and type on the call sites; otherwise, it can cause a panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added documents to check for the tokens. Since this pattern is also used in some other places, I have also documented them as well.

p.bump(T![type]);

parse_name(p).or_add_diagnostic(p, expected_name);

let implements_interface_empty = parse_implements_interface(p).is_absent();

let pos = p.source().position();
DirectiveList.parse_list(p);
let directive_empty = p.source().position() == pos;

let fields_definition_empty = parse_fields_definition(p).is_absent();

if directive_empty && implements_interface_empty && fields_definition_empty {
p.error(expected_object_extension(p, p.cur_range()));
}

Present(m.complete(p, GRAPHQL_OBJECT_TYPE_EXTENSION))
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub(crate) fn parse_scalar_type_definition(p: &mut GraphqlParser) -> ParsedSynta
Present(m.complete(p, GRAPHQL_SCALAR_TYPE_DEFINITION))
}

/// Must only be called if the next 2 token is `extend` and `scalar`, otherwise it will panic.
#[inline]
pub(crate) fn parse_scalar_type_extension(p: &mut GraphqlParser) -> ParsedSyntax {
let m = p.start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub(crate) fn parse_schema_definition(p: &mut GraphqlParser) -> ParsedSyntax {
Present(m.complete(p, GRAPHQL_SCHEMA_DEFINITION))
}

/// Must only be called if the next 2 token is `extend` and `schema`, otherwise it will panic.
#[inline]
pub(crate) fn parse_schema_extension(p: &mut GraphqlParser) -> ParsedSyntax {
let m = p.start();
Expand Down
20 changes: 14 additions & 6 deletions crates/biome_graphql_parser/src/parser/parse_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ pub(crate) fn expected_any_selection(p: &GraphqlParser, range: TextRange) -> Par
expected_any(&["field", "fragment spread", "inline fragment"], range, p)
}

pub(crate) fn expected_name(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
expected_node("name", range, p)
pub(crate) fn fragment_name_must_not_be_on(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
p.err_builder("Fragment name must not be 'on'", range)
}

pub(crate) fn expected_directive(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
expected_node("directive", range, p)
pub(crate) fn expected_name(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
expected_node("name", range, p)
}

pub(crate) fn expected_named_type(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
Expand Down Expand Up @@ -63,6 +63,7 @@ pub(crate) fn expected_schema_extension(p: &GraphqlParser, range: TextRange) ->
range,
)
}

pub(crate) fn expected_root_operation_type_definition(
p: &GraphqlParser,
range: TextRange,
Expand All @@ -74,6 +75,10 @@ pub(crate) fn expected_operation_type(p: &GraphqlParser, range: TextRange) -> Pa
expected_any(&["query", "mutation", "subscription"], range, p)
}

pub(crate) fn expected_directive(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
expected_node("directive", range, p)
}

pub(crate) fn expected_directive_location(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
p.err_builder("Expected a valid directive location", range)
.with_alternatives(
Expand Down Expand Up @@ -102,6 +107,9 @@ pub(crate) fn expected_directive_location(p: &GraphqlParser, range: TextRange) -
)
}

pub(crate) fn fragment_name_must_not_be_on(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
p.err_builder("Fragment name must not be 'on'", range)
pub(crate) fn expected_object_extension(p: &GraphqlParser, range: TextRange) -> ParseDiagnostic {
p.err_builder(
"Expected at least one directive, implements interface or fields definition",
range,
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
extend type Story

extend type User
name: String
}
Loading
Loading