refactor(yaml): accept a list of properties#9220
Merged
l0ngvh merged 1 commit intobiomejs:mainfrom Feb 26, 2026
Merged
Conversation
|
1a4d8ce to
8d88dc3
Compare
8d88dc3 to
26c133a
Compare
Contributor
WalkthroughThis pull request refactors the YAML formatter and parser to reorganise how properties are handled in YAML block and flow nodes. The main changes involve replacing Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_yaml_parser/src/parser/property.rs (1)
21-39:recoveris unreachable today; the emptytoken_set![]is a trap for the follow-up PR.Because
is_at_list_endalways returnstrue,parse_elementandrecoverare never invoked — the list immediately completes empty. That's the correct no-op behaviour for now.However, when
parse_elementis fleshed out,is_at_list_endmust be updated at the same time. Ifis_at_list_endstarts returningfalsewhilerecoverstill holdstoken_set![], there are no synchronisation tokens for recovery to latch onto — which can stall the parser on malformed input. Consider adding a comment alongsidetoken_set![]to signal this dependency, or use a sentinel set (e.g., the property-terminating tokens) even now.💡 Suggested guard comment
fn recover( &mut self, p: &mut Self::Parser<'_>, parsed_element: ParsedSyntax, ) -> biome_parser::parse_recovery::RecoveryResult { + // TODO: replace token_set![] with actual recovery tokens (e.g. content-start tokens) + // when parse_element is implemented. An empty set means no sync point is available. parsed_element.or_recover_with_token_set( p, &ParseRecoveryTokenSet::new(YAML_BOGUS, token_set![]), |p, range| p.err_builder("expected property", range), ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_yaml_parser/src/parser/property.rs` around lines 21 - 39, The recover implementation currently uses an empty token_set![] while is_at_list_end always returns true, which makes recover unreachable and dangerous if is_at_list_end is later relaxed; update recover (the ParseRecoveryTokenSet::new(..., token_set![] ) call in recover) to either include a sentinel set of property-terminating tokens or add a clear TODO comment next to token_set![] explaining the coupling with is_at_list_end and that the token set must be kept in sync with any changes to is_at_list_end/parse_element; refer to the functions parse_element, is_at_list_end, and recover so the maintainer knows where to change the token set or add the guard comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_yaml_parser/src/parser/property.rs`:
- Around line 21-39: The recover implementation currently uses an empty
token_set![] while is_at_list_end always returns true, which makes recover
unreachable and dangerous if is_at_list_end is later relaxed; update recover
(the ParseRecoveryTokenSet::new(..., token_set![] ) call in recover) to either
include a sentinel set of property-terminating tokens or add a clear TODO
comment next to token_set![] explaining the coupling with is_at_list_end and
that the token set must be kept in sync with any changes to
is_at_list_end/parse_element; refer to the functions parse_element,
is_at_list_end, and recover so the maintainer knows where to change the token
set or add the guard comment.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (46)
crates/biome_yaml_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_yaml_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/extra_characters_in_header.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/invalid_headers.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/mapping_followed_plain.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/mapping_key_contains_multiple_values.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/mapping_missing_colon.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/multi_line_flow_collection_broken_out_of_parent_block.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/multi_line_plain_token_broken_out_parent_flow.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/plain_followed_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/plain_separated_by_comments.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/block/sequence_follow_by_map.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/flow/empty_mapping_entry.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/flow/mapping_missing_closing_bracket.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/flow/missing_closing_double_quoted.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/flow/missing_closing_single_quoted.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/flow/plain_start_with_indicator.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/err/flow/sequence_missing_closing_bracket.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/explicit_block_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/explicit_block_mapping_with_compact_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/explicit_block_mapping_with_empty_value.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/explicit_block_mapping_with_empty_value_and_mapping_as_key.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/implicit_block_mapping_empty_value.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/nested_implicit_block_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/simple_block_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/mapping/simple_block_mapping_separated_by_trivia.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/scalar/folded_scalar.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/scalar/literal_scalar.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/sequence/map_nested_in_sequence.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/sequence/nested_block_sequence.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/sequence/sequence_nested_in_map.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/sequence/sequence_with_trailing_trivia.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/block/sequence/simple_block_sequence.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/document/separated_by_doc_end.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/double_quote.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/explicit_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/flow_with_trailing_trivia.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/nested_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/simple_mapping.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/simple_plain.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/simple_sequence.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_parser/tests/yaml_test_suite/ok/flow/single_quote.yaml.snapis excluded by!**/*.snapand included by**crates/biome_yaml_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_yaml_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_yaml_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_yaml_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (19)
crates/biome_yaml_formatter/src/generated.rscrates/biome_yaml_formatter/src/yaml/any/block_in_block_content.rscrates/biome_yaml_formatter/src/yaml/any/block_in_block_node.rscrates/biome_yaml_formatter/src/yaml/any/block_node.rscrates/biome_yaml_formatter/src/yaml/any/mod.rscrates/biome_yaml_formatter/src/yaml/any/properties_combination.rscrates/biome_yaml_formatter/src/yaml/any/property.rscrates/biome_yaml_formatter/src/yaml/auxiliary/block_in_block_node.rscrates/biome_yaml_formatter/src/yaml/auxiliary/mod.rscrates/biome_yaml_formatter/src/yaml/auxiliary/properties_anchor_first.rscrates/biome_yaml_formatter/src/yaml/auxiliary/properties_tag_first.rscrates/biome_yaml_formatter/src/yaml/lists/mod.rscrates/biome_yaml_formatter/src/yaml/lists/property_list.rscrates/biome_yaml_parser/src/parser/block.rscrates/biome_yaml_parser/src/parser/flow.rscrates/biome_yaml_parser/src/parser/mod.rscrates/biome_yaml_parser/src/parser/property.rsxtask/codegen/src/yaml_kinds_src.rsxtask/codegen/yaml.ungram
💤 Files with no reviewable changes (4)
- crates/biome_yaml_formatter/src/yaml/auxiliary/properties_tag_first.rs
- crates/biome_yaml_formatter/src/yaml/any/properties_combination.rs
- crates/biome_yaml_formatter/src/yaml/any/block_in_block_node.rs
- crates/biome_yaml_formatter/src/yaml/auxiliary/properties_anchor_first.rs
Member
|
When you say schema change, do you mean a change in the grammar? |
Contributor
Author
|
My bad. Yes, it's a grammar change. Let me update the PR description |
ematipico
reviewed
Feb 24, 2026
Comment on lines
+21
to
+23
| fn parse_element(&mut self, _p: &mut Self::Parser<'_>) -> ParsedSyntax { | ||
| Absent | ||
| } |
Member
There was a problem hiding this comment.
What's the purpose of a list that doesn't parse anything?
Contributor
Author
There was a problem hiding this comment.
Right, I wanted to keep the PR lightweight, so it only contains the minimal change that passes compilation and tests. A follow up PR will handle populating the parsing logic later. What do you think?
Member
There was a problem hiding this comment.
Ah ok. It's good. I didn't understand that from the PR description
Contributor
Author
There was a problem hiding this comment.
My bad. Let me update the description to reflect this
|
|
||
| pub(crate) fn parse_flow_json_node(p: &mut YamlParser) -> CompletedMarker { | ||
| let m = p.start(); | ||
| PropertyList.parse_list(p); |
Member
There was a problem hiding this comment.
This seems like a bug. A list that doesn't parse anything
ematipico
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Our current YAML grammar enforces that there can only be at most 2 properties, which causes difficulties in parsing the properties. This PR changes the grammar to accept a list of properties. Checking the number of properties will belong to a syntax rule.
This PR also moves the property outside of block nodes, i.e. outside of the MAPPING_START/MAPPING_END pair, as from a parser's perspective, there is no way to disambiguate between properties of block maps and properties of a key of the block maps as both are inside MAPPING_START/MAPPING_END pair.
This PR is rather large due to being a grammar change. However, most of the changes are no-op, and it contains only the minimal changes that allow it to pass compilation and tests. The actual lexing and parsing the properties will belong to a future PR.
This is part of #8570, split for easier review.
Test Plan
The new test snapshot should only contain changes in the grammar
Docs
N/A