refactor(linter): move run on regex node to utils#10772
refactor(linter): move run on regex node to utils#10772graphite-app[bot] merged 1 commit intomainfrom
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughThe changes introduce a refactoring of the regular expression handling logic within the linter's "no_control_regex" rule. The previous approach, which involved manual extraction and parsing of regex patterns from different AST node types (regex literals, 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (12)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/oxc_linter/src/utils/regex.rs (2)
14-19: Literal branch: consider allowing multiple visits
run_on_regex_nodetakescb: FnOnce, which is fine because a single AST node can expose at most one regex literal.
However, if the helper is reused for more complex nodes (e.g. an array of regex literals) we would have to re-enter the utility. Switching toFnMutwould make the helper future-proof with negligible cost:-pub fn run_on_regex_node<'a, 'b, M>(node: &'a AstNode<'b>, ctx: &'a LintContext<'b>, cb: M) -where - M: FnOnce(&Pattern<'_>, Span), +pub fn run_on_regex_node<'a, 'b, M>(node: &'a AstNode<'b>, ctx: &'a LintContext<'b>, mut cb: M) +where + M: FnMut(&Pattern<'_>, Span),
20-41:new RegExp()parsing logic duplicated – DRY opportunityThe two big match arms for
NewExpressionandCallExpressionare almost identical. Extracting the common code into a small helper will halve the branchy code and make future fixes (e.g. template-literal support) one-liners:+fn parse_and_apply<'a>( + allocator: &Allocator, + pattern_arg: &Argument<'a>, + flags_arg: Option<&Argument<'a>>, + ctx: &'a LintContext<'_>, + cb: &mut impl FnMut(&Pattern<'_>, Span), +) { + if let Argument::StringLiteral(pattern) = pattern_arg { + let flags_span = flags_arg.and_then(|a| match a { + Argument::StringLiteral(f) => Some(f.span), + _ => None, + }); + if let Some(pat) = parse_regex(allocator, pattern.span, flags_span, ctx) { + cb(&pat, pattern.span); + } + } +}Then call this helper from both arms.
This keepsrun_on_regex_nodeconcise and easier to audit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/oxc_linter/src/rules/eslint/no_control_regex.rs(2 hunks)crates/oxc_linter/src/utils/mod.rs(2 hunks)crates/oxc_linter/src/utils/regex.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Benchmark (formatter)
- GitHub Check: Benchmark (codegen)
- GitHub Check: Benchmark (minifier)
- GitHub Check: Benchmark (transformer)
- GitHub Check: Build Linter Benchmark
- GitHub Check: Test (macos-latest)
- GitHub Check: Benchmark (parser)
- GitHub Check: Test (ubuntu-latest)
🔇 Additional comments (3)
crates/oxc_linter/src/utils/mod.rs (1)
10-20: New regex module is wired-in correctly – nice!The module declaration (
mod regex;) and the public re-export (regex::*) look consistent with the rest of the file and maintain alphabetical order, so downstream crates can now simplyuse utils::run_on_regex_node.
No further action needed.crates/oxc_linter/src/utils/regex.rs (1)
70-88: Potential offset bug when noflags_spanis present
flags_span_offsetfalls back to0whenflags_spanisNone:flags_span_offset: flags_span.map_or(0, |span| span.start)If the constructor parser expects an offset relative to the pattern (or to the beginning of the file),
0might be misleading and could shift diagnostic positions. Consider forwardingpattern_span.end(or the actual location where the flags would start) instead, or confirm that0is explicitly accepted byConstructorParser.Do you mind double-checking the expected semantics in
ConstructorParser::new? If an offset of0is invalid we can patch it quickly.crates/oxc_linter/src/rules/eslint/no_control_regex.rs (1)
71-75: Great simplification of the rule bodySwapping ~50 lines of bespoke extraction logic for one call to
run_on_regex_nodeis a big readability win and removes duplicated parsing code. All existing tests still pass, so the refactor looks safe.Nice job!
CodSpeed Instrumentation Performance ReportMerging #10772 will not alter performanceComparing Summary
|
bf3ccc8 to
731d8d6
Compare
Merge activity
|
731d8d6 to
2efe3f0
Compare
No description provided.