fix(grit): support positional arguments in pattern, function, and node calls#10281
Open
Zelys-DFKH wants to merge 2 commits into
Open
fix(grit): support positional arguments in pattern, function, and node calls#10281Zelys-DFKH wants to merge 2 commits into
Zelys-DFKH wants to merge 2 commits into
Conversation
…e calls Fixes biomejs#10114. GritQL allows omitting parameter names when arguments are given in order, but Biome rejected all positional args. - Parser: parse_maybe_named_arg now uses a one-token lookahead to distinguish a positional pattern call from a named arg, instead of unconditionally routing GRIT_NAME to parse_named_arg - Compiler: node_to_args_pairs now accepts any AnyGritPattern as a positional arg and resolves the parameter name by position, instead of rejecting non-GritVariable args with ExpectedVariable - Compiler: node_pattern_from_node_with_name_and_kind applies the same fix for AST node patterns
Contributor
WalkthroughThe change enables positional (unkeyed) arguments in GritQL patterns, functions, predicates and AST node patterns. The parser's 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/biome_grit_patterns/src/pattern_compiler/call_compiler.rs (1)
207-218: 💤 Low valueTiny nit:
expected_params.as_ref().map_or(0, |p| p.len())could be0literally.We only enter this
ok_or_elsewhenexpected_params.as_ref().and_then(...)returnedNone. That's either becauseexpected_paramswasNone(→ 0) or becauseparams.get(i)was out of range (→params.len() <= i, but reportingparams.len()is exactly what we want). So the current expression is correct — it's theexpected_params is Nonecase where reportingmax_args: 0feels a bit terse for an unknown callee. Not blocking; just flagging in case you'd rather route theNonecase throughMissingArgumentNameor skip the error here entirely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/biome_grit_patterns/src/pattern_compiler/call_compiler.rs` around lines 207 - 218, The current ok_or_else builds NodeLikeArgumentError::TooManyArguments with max_args computed as expected_params.as_ref().map_or(0, |p| p.len()), which yields 0 when expected_params is None; instead, first test expected_params: if expected_params.is_none() return a clearer error such as NodeLikeArgumentError::MissingArgumentName { name: fn_name.to_string() } and only construct NodeLikeArgumentError::TooManyArguments when expected_params is Some—using params.len() for max_args; update the branch around expected_params, NodeLikeArgumentError::TooManyArguments, and fn_name in call_compiler.rs accordingly..changeset/fix-gritql-positional-args.md (1)
5-5: ⚡ Quick winUse the
Fixed [#NUMBER](issue link): …prefix.Issue
#10114exists for this fix, so per the changeset convention the description should lead with the linked-issue prefix instead of plain prose.📝 Suggested tweak
-Fixed a bug where GritQL patterns rejected positional (unkeyed) arguments. +Fixed [`#10114`](https://github.com/biomejs/biome/issues/10114): GritQL patterns rejected positional (unkeyed) arguments.As per coding guidelines: "For bug fixes, start with 'Fixed [
#NUMBER](issue link): ...'".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/fix-gritql-positional-args.md at line 5, Replace the changeset description line so it follows the convention for bug fixes: change the current sentence "Fixed a bug where GritQL patterns rejected positional (unkeyed) arguments." to start with the linked-issue prefix, e.g. "Fixed [`#10114`](issue link): GritQL patterns rejected positional (unkeyed) arguments.", ensuring the issue number 10114 is used and the rest of the text remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.changeset/fix-gritql-positional-args.md:
- Line 5: Replace the changeset description line so it follows the convention
for bug fixes: change the current sentence "Fixed a bug where GritQL patterns
rejected positional (unkeyed) arguments." to start with the linked-issue prefix,
e.g. "Fixed [`#10114`](issue link): GritQL patterns rejected positional (unkeyed)
arguments.", ensuring the issue number 10114 is used and the rest of the text
remains the same.
In `@crates/biome_grit_patterns/src/pattern_compiler/call_compiler.rs`:
- Around line 207-218: The current ok_or_else builds
NodeLikeArgumentError::TooManyArguments with max_args computed as
expected_params.as_ref().map_or(0, |p| p.len()), which yields 0 when
expected_params is None; instead, first test expected_params: if
expected_params.is_none() return a clearer error such as
NodeLikeArgumentError::MissingArgumentName { name: fn_name.to_string() } and
only construct NodeLikeArgumentError::TooManyArguments when expected_params is
Some—using params.len() for max_args; update the branch around expected_params,
NodeLikeArgumentError::TooManyArguments, and fn_name in call_compiler.rs
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f760091-2a20-4acb-92c4-1768b9a35428
⛔ Files ignored due to path filters (3)
crates/biome_grit_parser/tests/grit_test_suite/ok/positional_args.grit.snapis excluded by!**/*.snapand included by**crates/biome_grit_parser/tests/grit_test_suite/ok/positional_args_pattern_call.grit.snapis excluded by!**/*.snapand included by**crates/biome_grit_patterns/tests/specs/ts/positional_args.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (8)
.changeset/fix-gritql-positional-args.mdcrates/biome_grit_parser/src/parser/mod.rscrates/biome_grit_parser/tests/grit_test_suite/ok/positional_args.gritcrates/biome_grit_parser/tests/grit_test_suite/ok/positional_args_pattern_call.gritcrates/biome_grit_patterns/src/pattern_compiler/call_compiler.rscrates/biome_grit_patterns/src/pattern_compiler/node_like_compiler.rscrates/biome_grit_patterns/tests/specs/ts/positional_args.gritcrates/biome_grit_patterns/tests/specs/ts/positional_args.ts
When a non-variable positional argument is passed to a function with no known parameter list, emit MissingArgumentName instead of TooManyArguments with max_args=0. When the parameter list is known but the index is out of range, use the actual parameter count in TooManyArguments.
🦋 Changeset detectedLatest commit: 7de0f51 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_grit_patterns/src/pattern_compiler/call_compiler.rs (1)
174-228: 💤 Low valueLogic looks correct — consider deduping the positional fallback.
The two arms of the inner
match &patternshare the same "look upexpected_params[i], otherwise emitTooManyArguments(when params are known) orMissingArgumentName(when they aren't)" routine. Pulling that into a small closure tightens the code and keeps the two error policies in one place.♻️ Sketch of a shared fallback helper
Ok(AnyGritMaybeNamedArg::AnyGritPattern(pattern)) => { + let positional_name = |missing_var: &str| -> Result<String, NodeLikeArgumentError> { + expected_params + .as_ref() + .and_then(|params| params.get(i)) + .map(|p| { + p.strip_prefix(lang.metavariable_prefix()) + .unwrap_or(p) + .to_owned() + }) + .ok_or_else(|| { + if let Some(params) = expected_params.as_ref() { + NodeLikeArgumentError::TooManyArguments { + name: fn_name.to_string(), + max_args: params.len(), + } + } else { + NodeLikeArgumentError::MissingArgumentName { + name: fn_name.to_string(), + variable: missing_var.to_owned(), + } + } + }) + }; let name = match &pattern { AnyGritPattern::GritVariable(var) => { let var_name = var.to_trimmed_string(); - let name = var_name + match var_name .strip_prefix(lang.metavariable_prefix()) .filter(|stripped| { expected_params.as_ref().is_none_or(|expected| { expected.iter().any(|exp| exp == &var_name || exp == stripped) }) }) - .or_else(|| { ... }) - .ok_or_else(|| { ... })?; - name.to_owned() + { + Some(name) => name.to_owned(), + None => positional_name(&var_name)?, + } } - _ => { ... } + _ => positional_name("")?, }; Ok((name, pattern)) }Purely cosmetic — feel free to ignore if you'd rather not touch it again.
A couple of small things worth noting (no action required):
- For a variable whose name doesn't match any expected param (e.g. a typo like
$methdo), the new behaviour silently rebinds it positionally rather than raisingUnknownVariable. That's the natural cost of supporting positional variable binding, and matches the issue's intent — just flagging it so you're not surprised by it later in a bug report.- Line 222's
variable: String::new()only fires whenexpected_paramsisNoneand the positional arg is not a variable; downstream you'd typically hitUnknownFunctionOrPatternfirst, so it's effectively defensive. Leaving as-is is fine.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/biome_grit_patterns/src/pattern_compiler/call_compiler.rs` around lines 174 - 228, The two branches inside the AnyGritMaybeNamedArg::AnyGritPattern handling duplicate the logic for resolving expected_params[i] and producing NodeLikeArgumentError variants; extract that positional-fallback lookup into a small helper/closure (e.g., let resolve_positional = |i: usize| -> Result<String, NodeLikeArgumentError> { ... }) that captures expected_params and fn_name and returns the stripped name or the appropriate TooManyArguments/MissingArgumentName error, then call this helper from both the AnyGritPattern::GritVariable fallback path and the non-variable arm to eliminate duplication and centralize the error policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/biome_grit_patterns/src/pattern_compiler/call_compiler.rs`:
- Around line 174-228: The two branches inside the
AnyGritMaybeNamedArg::AnyGritPattern handling duplicate the logic for resolving
expected_params[i] and producing NodeLikeArgumentError variants; extract that
positional-fallback lookup into a small helper/closure (e.g., let
resolve_positional = |i: usize| -> Result<String, NodeLikeArgumentError> { ...
}) that captures expected_params and fn_name and returns the stripped name or
the appropriate TooManyArguments/MissingArgumentName error, then call this
helper from both the AnyGritPattern::GritVariable fallback path and the
non-variable arm to eliminate duplication and centralize the error policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 854eef94-010a-4f8b-9d4e-dc5a5e15cd8f
📒 Files selected for processing (1)
crates/biome_grit_patterns/src/pattern_compiler/call_compiler.rs
dyc3
added
M-Likely Agent
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.
Fixes #10114.
The GritQL spec says positional arguments are valid, so
console_method_to_info(\log`)should work the same asconsole_method_to_info(method = `log`)`. Biome was rejecting them.The fix needed two places. In the parser,
parse_maybe_named_argsent everyGRIT_NAMEtoken toparse_named_arg, which requires=. A positional call likeis_log()would get misparsed as a broken named arg. One token of lookahead fixes it.In the pattern compiler,
node_to_args_pairsrejected any positional arg that wasn't aGritVariable— backtick snippets and other patterns were all refused withExpectedVariable. The same restriction innode_pattern_from_node_with_name_and_kindblocked AST node patterns too. Both now resolve the parameter name by position.Test Plan
ok/positional_args.grit— the exact GritQL from the issue parses without errorsok/positional_args_pattern_call.grit— positional pattern call (is_log()) parses correctlyspecs/ts/positional_args.{grit,ts,snap}— end-to-end: the pattern compiles and matchesconsole.log(...)but notconsole.warn(...)