fix(lint): only highlight function names in useMaxParams rule #7608
Conversation
🦋 Changeset detectedLatest commit: e729c6b 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 |
github-actions
bot
added
A-Linter
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Have you tried using the range of the parameters? It should be possible to create a range from the node list.
Regardless, a changeset is missing and it's required
WalkthroughAdded a Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs (2)
211-213: Diagnostic now points to the name — good; tiny micro‑nitUsing the name range markedly improves UX. Micro‑nit:
TextRangeisCopy, sounwrap_or(node.range())would avoid the closure. Not worth a separate commit unless you touch this again.Also applies to: 217-217
103-106: Arrow functions — optional: highlight binding/property name instead of full nodeCurrently Self::JsArrowFunctionExpression(_) returns None and falls back to a full-node squiggle; consider resolving the containing declarator or property key (e.g.
const foo = () => {}or{ foo: () => {} }) and pointing the diagnostic at that identifier, keeping the current behaviour for inline callbacks.File: crates/biome_js_analyze/src/lint/nursery/use_max_params.rs (lines 103–106)
Self::JsArrowFunctionExpression(_) => { // Arrow functions don't have names None }Ripgrep of the test suite shows many arrow-in-declarator cases (eg. crates/biome_grit_patterns/tests/specs/ts/treeSitterCompatibility.ts, crates/biome_js_parser/tests/js_test_suite/ok/jsx_element_on_arrow_function.jsx) — update snapshots if you change diagnostic ranges.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalidCustomMax.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
🧠 Learnings (1)
📚 Learning: 2025-09-10T08:05:22.867Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-10T08:05:22.867Z
Learning: Applies to crates/biome_analyze/**/src/{lint,assist}/**/*.rs : Avoid deep indentation and panics; prefer `?`, `ok()?`, and combinators (`map`, `and_then`, `filter`) over nested `if let`/`unwrap`/`expect`
Applied to files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
🔇 Additional comments (3)
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs (3)
11-11: Import change looks goodBrings
TextRangeanddeclare_node_unioninto scope; required for the new API and macro usage.
87-137: Solid, panic‑free traversal of AST for name rangesNice use of
ok()?/?to avoid unwraps and deep nesting; covers decls, exprs, methods, constructors, TS declare fn and type aliases. This aligns with our guidance to avoid panics.Based on learnings
109-111: Constructor naming: double‑check the highlighted tokenconstructor.name() should return the range for the 'constructor' keyword/token — confirm the snapshot underlines only that token, not the whole header.
Location: crates/biome_js_analyze/src/lint/nursery/use_max_params.rs:109–111
| Self::JsFunctionExpression(func) => func | ||
| .id()? | ||
| .as_js_identifier_binding()? | ||
| .name_token() | ||
| .ok() | ||
| .map(|token| token.text_range()), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Consider preferring the binding name over the internal name for function expressions
When a named function expression is assigned (const foo = function bar(...) {}), highlighting bar is less helpful than foo. Optional follow‑up: if id() is None or even when present, try resolving the enclosing JsVariableDeclarator/property name to highlight the LHS binding/property key instead. This would bring function expressions in line with the “underline what the developer sees” principle.
If you want to gauge impact, search snapshots for these cases:
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g 'crates/**/tests/**' -e $'const\\s+\\w+\\s*=\\s*function\\b' -e $'\\w+\\s*:\\s*function\\b'Length of output: 202398
Prefer binding name over internal identifier in use_max_params lint
• Resolve the enclosing variable/property binding (e.g. fn1/fn2) and use its text range instead of func.id() for both anonymous and named function expressions.
• Update snapshots in crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalid.js.snap (around lines 95 and 117) to underline fn1/fn2 rather than the parameter list or internal name.
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/use_max_params.rs around lines 97 to
102, the code uses func.id() to get the internal function identifier text range;
instead, resolve the enclosing binding (variable or property) that holds the
function expression and return that binding's text_range for both anonymous and
named function expressions so the diagnostic highlights the external name (e.g.,
fn1/fn2) rather than the internal id or the parameter list. Modify the logic to
walk to the parent binding (variable declarator or property) and extract its
identifier/token range when present, falling back to the existing func.id()
behavior only if no enclosing binding exists. After changing the lint, update
the snapshots in
crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalid.js.snap (around
lines ~95 and ~117) to underline the external binding names fn1/fn2 instead of
the parameter list or internal name.
I can try that out..would you like me to update the implementation that way |
|
Yes. And please update the changeset by following our guidelines |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs (1)
207-209: Align diagnostic range with “prefer name, then params, else node”At the moment, diagnostics always favour the parameter list. Switch to name-first to match the PR title and issue intent.
- // Use the parameter list range if available, otherwise fall back to the whole node - let range = node.parameter_range().unwrap_or_else(|| node.range()); + // Prefer the visible name; then parameters; finally the whole node + let range = node + .name_range() + .or_else(|| node.parameter_range()) + .unwrap_or_else(|| node.range());If you intentionally chose parameters (per discussion with maintainers), please update the PR title/description to avoid surprises in release notes.
Also applies to: 213-213
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMaxParams/invalidCustomMax.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
.changeset/ten-donkeys-raise.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/use_max_params.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/ten-donkeys-raise.md
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
🧠 Learnings (2)
📚 Learning: 2025-09-10T08:05:22.867Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-10T08:05:22.867Z
Learning: Applies to crates/biome_analyze/**/src/{lint,assist}/**/*.rs : Avoid deep indentation and panics; prefer `?`, `ok()?`, and combinators (`map`, `and_then`, `filter`) over nested `if let`/`unwrap`/`expect`
Applied to files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
📚 Learning: 2025-09-10T08:05:22.867Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-10T08:05:22.867Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/{lint,assist}/**/*.rs : When banning globals (e.g., `noConsoleLog`), check the semantic model to avoid false positives from locally shadowed bindings
Applied to files:
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs
🔇 Additional comments (2)
crates/biome_js_analyze/src/lint/nursery/use_max_params.rs (2)
11-11: Import looks good
TextRangeis used by the new helpers; the import set is tidy.
87-133: Prefer highlighting the visible name (binding/method/alias) with params as fallbackCurrent impl returns the parameter list range for all variants. That’s an improvement, but it doesn’t match the PR title (“only highlight function names”) nor the linked issue’s “prefer the function name”. It also misses the common case of function/arrow expressions where the user-visible name is the LHS binding (e.g.
const foo = (...) => {}).Suggestion: add a lightweight
name_range()that:
- Decls: underline
id.- Methods: underline the method/property name; constructors: the
constructorkeyword.- Function/arrow expressions: prefer enclosing variable/property name; fall back to internal
id(if any), then params.- TS:
declare functionusesid;type Fn = (…)=>…underlines the aliasFn.Then pick
name_range().or(parameter_range()).unwrap_or(node.range())in diagnostics.Minimal diff sketch:
impl AnyFunctionLike { + pub fn name_range(&self) -> Option<TextRange> { + match self { + Self::JsFunctionDeclaration(func) => func.id().ok().map(|id| id.range()), + Self::JsMethodClassMember(m) => m.name().ok().map(|n| n.range()), + Self::JsMethodObjectMember(m) => m.name().ok().map(|n| n.range()), + Self::JsConstructorClassMember(c) => Some(c.range()), // or the `constructor` token if available + Self::TsDeclareFunctionDeclaration(d) => d.id().ok().map(|id| id.range()), + Self::TsTypeAliasDeclaration(d) => d.id().ok().map(|id| id.range()), + // For expressions and arrows, try the enclosing binding/property first + Self::JsFunctionExpression(func) => enclosing_binding_or_property_name_range(func.syntax()), + Self::JsArrowFunctionExpression(func) => enclosing_binding_or_property_name_range(func.syntax()), + } + } + pub fn parameter_range(&self) -> Option<TextRange> { match self { Self::JsFunctionDeclaration(func) => func .parameters() .ok() .map(|params| params.range()), /* …unchanged for the other variants… */ } } } + +fn enclosing_binding_or_property_name_range(node: &biome_rowan::SyntaxNode) -> Option<TextRange> { + use biome_js_syntax::{JsVariableDeclarator, JsPropertyObjectMember}; + let mut cur = Some(node.clone()); + while let Some(n) = cur { + if let Some(decl) = JsVariableDeclarator::cast(n.clone()) { + return decl.id().ok().map(|id| id.range()); + } + if let Some(prop) = JsPropertyObjectMember::cast(n.clone()) { + return prop.name().ok().map(|name| name.range()); + } + cur = n.parent(); + } + None +}This keeps squiggles compact and matches user expectations across forms.
The useMaxParams rule was highlighting entire function bodies instead of just the function names, causing noisy error squiggles. Changes: - Add name_range() method to AnyFunctionLike union - Update diagnostic to use function name ranges - Handle all function types including TS declarations - Update test snapshots to reflect new behavior
ritoban23
force-pushed
the
fix/use-max-params-highlight
branch
from
601e9b4 to
ffcd20b
Compare
CodSpeed Performance ReportMerging #7608 will not alter performanceComparing Summary
|
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Summary
The useMaxParams rule was highlighting entire function bodies instead of just the function names, causing noisy error squiggles that made it difficult to quickly identify which function was problematic.
Fixes #7604
Changes:
Test Plan
function myFunc(...)→ highlights onlymyFuncmethod(...)→ highlights onlymethodconstructor(...)→ highlights onlyconstructordeclare function makeDate(...)→ highlights onlymakeDatefunction(...)→ highlights entire function (correct fallback)(...) => ...→ highlights entire function (correct fallback)Docs