feat(lint): add nursery rule useNamedCaptureGroup#9048
feat(lint): add nursery rule useNamedCaptureGroup#9048ff1451 wants to merge 7 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: de6612f The changes in this PR will be included in the next version bump. 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 |
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a new nursery lint rule Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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 |
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/use_named_capture_group.rs`:
- Around line 229-249: The constructor path currently returns a single
diagnostic covering the whole expression in run_regexp_constructor (return
Box::new([node.range()])), which differs from the literal path that emits one
diagnostic per unnamed group; either (A) adjust run_regexp_constructor to mirror
the literal handling by parsing pattern_text (from extract_pattern_from_args) to
find each unnamed group span, convert those spans into TextRange values relative
to the argument/string literal and return a Box<[TextRange]> containing one
range per unnamed group (use
parse_regexp_node/extract_pattern_from_args/has_unnamed_capture_groups as hooks
to locate the pattern and compute offsets), or (B) if single-diagnostic behavior
is intentional, update the rule metadata where the rule is declared (change
.same() to .inspired() at the rule setup referenced near line 76) so the
reported behavior is documented as differing from ESLint; pick one and implement
accordingly.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_named_capture_group.rs (2)
85-173: Helper functions should be placed below theimpl Ruleblock.Per project convention, all helper functions, structs, and enums must be placed below the
impl Ruleblock (the only exception being node union declarations used in theQuerytype).find_unnamed_capture_groups,has_unnamed_capture_groups,is_regexp_object,parse_regexp_node, andextract_pattern_from_argsshould all be moved below theimpl Rule for UseNamedCaptureGroupblock.Based on learnings: "In crates/biome_analyze/**/*.rs rule files, all helper functions, structs, and enums must be placed below the
impl Ruleblock."
175-208:Semantic<AnyJsExpression>as the query type is broad.This means
runis invoked for every expression node in the file. The earlymatch+Default::default()exit is cheap, so this isn't a blocker — just something to be aware of if profiling shows overhead. A narrower query viadeclare_node_union!combiningJsRegexLiteralExpression,JsNewExpression, andJsCallExpressioncould reduce invocations.
ematipico
left a comment
There was a problem hiding this comment.
Nice work! Left some comments
| /// | ||
| /// ```js | ||
| /// /(?<id>ba[rz])/; | ||
| /// ``` | ||
| /// | ||
| /// ```js | ||
| /// /(?:ba[rz])/; | ||
| /// ``` | ||
| /// | ||
| /// ```js | ||
| /// /ba[rz]/; | ||
| /// ``` | ||
| /// | ||
| /// ```js | ||
| /// /(?<year>[0-9]{4})-(?<month>[0-9]{2})/; | ||
| /// ``` | ||
| /// | ||
| /// ```js | ||
| /// new RegExp("(?<id>foo)"); | ||
| /// ``` | ||
| /// | ||
| /// ```js | ||
| /// new RegExp(pattern); |
There was a problem hiding this comment.
Valid cases can stay in one single code block
There was a problem hiding this comment.
Thanks I’ve grouped the valid cases into a single code block.
| while i < pattern.len() { | ||
| match pattern[i] { |
There was a problem hiding this comment.
See if you can use as_bytes instead. Then, evaluate the removal of the counter by using an iterator
There was a problem hiding this comment.
Thanks for the suggestion!
I’ve updated the code to use as_bytes and replaced the counter-based loop with an iterator.
| let raw_inner = &token_text[1..token_text.len() - 1]; | ||
| let inner_text = string_lit.inner_string_text().ok()?; | ||
| // If raw source and interpreted text differ, escapes are present | ||
| if raw_inner != inner_text.text() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
I don't understand when this is the case. Are there tests for this?
There was a problem hiding this comment.
Sorry for missing the test cases earlier.
raw_inner != inner_text becomes true when the string contains escape sequences.
Without this guard find_unnamed_capture_groups would run on the raw source and produce incorrect offsets.
To clarify the behavior I added tests covering both directions
-
valid:
new RegExp("\\(foo)")— using the raw source\\(foo)would incorrectly report(as an unnamed group, while the actual regex\(foo)has no capture group. -
invalid:
new RegExp("\\d+(foo)")— since escapes are present, precise mapping is skipped but the fallback path correctly detects the unnamed group(foo)using the interpreted string\d+(foo).
fcce15a to
7caad19
Compare
Summary
Added the nursery rule
useNamedCaptureGroup, which enforces using named capture groups ((?<name>...)) in regular expressions instead of numbered ones ((...)).This rule corresponds to ESLint's
prefer-named-capture-group.Supports:
/(foo)/new RegExp("(foo)")/RegExp("(foo)")constructor callsnew RegExp(pattern)) are safely skippedRegExpidentifiers are correctly ignored via semantic analysisCloses #8737
Test Plan
just test-lintrule useNamedCaptureGroup— all spec tests passnew RegExp(...),RegExp(...)constructor callsRegExpDocs
Documentation is included as rustdoc examples in the rule implementation with
expect_diagnosticannotations.