fix(noParameterAssign): prevent false positive when parameter is used as R-value #8259
Conversation
… as R-value Closes biomejs#8254.
🦋 Changeset detectedLatest commit: 837cc7e 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
WalkthroughAdds a changelog patch and tightens the Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.js (1)
1-6: Good coverage of the false-positive scenario; consider aligning filename with spec conventionsThe test nicely exercises both static and computed member assignments with the parameter only on the RHS, which is exactly what bit this rule before – good stuff.
One minor consistency nit: test specs under
biome_*_analyze/tests/specsgenerally usevalid/invalidas filename prefixes to signal expectation; you might consider something likevalid.parameterMutationDeny.js(and updating the snapshot name accordingly) for easier scanning.Based on learnings, this folder prefers
valid/invalidprefixes for spec files..changeset/light-mugs-speak.md (1)
1-16: Nice concise changeset; tiny wording/link polishThis reads well and is very user-facing. Two small nits you may want to tweak:
- “The rule should only flag…” could be tightened to something like “Biome now only flags…” to state the current behaviour rather than the intention.
- Consider adding a link to the
noParameterAssignrule docs alongside the issue link for quick reference.As per coding guidelines, changesets prefer present tense for Biome’s behaviour and include both issue and rule links where handy.
crates/biome_js_analyze/src/lint/style/no_parameter_assign.rs (2)
151-175: Guarding on the callee object name fixes the false positive; small DRY opportunityThe
get_callee_object_name()equality check againstbinding.name_token()is exactly what was missing to stop RHS-only usages (local.a = param) from being treated as parameter property assignments. This should narrow diagnostics to genuineparam.*writes.The two arms for
JsComputedMemberAssignmentandJsStaticMemberAssignmentduplicate the same object/name logic and callbinding.name_token()twice each; if you feel like a tiny tidy-up, you could factor out the parameter name and reuse a helper along these lines:-AnyJsAssignment::JsComputedMemberAssignment(assignment) => { - if assignment - .object() - .ok()? - .get_callee_object_name()? - .token_text_trimmed() - == binding.name_token().ok()?.token_text_trimmed() - { - return assignment.object().ok(); - } - - None -} +AnyJsAssignment::JsComputedMemberAssignment(assignment) => { + let param_name = binding.name_token().ok()?.token_text_trimmed(); + let object = assignment.object().ok()?; + + if object + .get_callee_object_name()? + .token_text_trimmed() + == param_name + { + return Some(object); + } + + None +}and mirror that pattern in the static-member arm. Entirely optional, but it does make the intent a bit crisper.
24-26: Rule docs still claim property assignments cannot be configuredThe doc block still says this rule “cannot be configured to report assignments to a property of a parameter”, which now contradicts the
propertyAssignmentoption documented just below and exercised in this PR.Might be worth updating these lines to avoid confusion, for example:
-/// In contrast to the _ESLint_ rule, this rule cannot be configured to report -/// assignments to a property of a parameter. +/// Property assignments on function parameters can be additionally controlled +/// via the `propertyAssignment` option described below.That keeps the comment aligned with the current options without having to restate all the details here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/light-mugs-speak.md(1 hunks)crates/biome_js_analyze/src/lint/style/no_parameter_assign.rs(1 hunks)crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.js(1 hunks)crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rustdbg!()macro for debugging output during test execution, and pass the--show-outputflag tocargo testto display debug output.
Use snapshot testing with theinstacrate for testing in Rust projects. Accept or reject snapshots usingcargo insta accept,cargo insta reject, orcargo insta review.
Write doc comments as doc tests in Rust using code blocks with assertions that will be executed during the testing phase.
Use rustdoc inline documentation for rules, assists, and their options. Create corresponding documentation PRs for other documentation updates against thenextbranch of the website repository.
Set theversionmetadata field in linter rule implementations to'next'for newly created rules. Update this field to the new version number when releasing a minor or major version.
Files:
crates/biome_js_analyze/src/lint/style/no_parameter_assign.rs
**/.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Changeset descriptions should be user-facing, use past tense for actions taken (e.g., 'Added new feature'), and present tense for Biome behavior (e.g., 'Biome now supports...'). Include issue links, rule links, and code examples where applicable.
Files:
.changeset/light-mugs-speak.md
🧠 Learnings (12)
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/tests/specs/**/*.{js,ts,tsx,jsx,json,css,graphql} : Test files should use 'invalid' or 'valid' prefixes to indicate whether they contain code reported by the rule
Applied to files:
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.jsoncrates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.js
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
Repo: biomejs/biome PR: 7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.json
📚 Learning: 2025-11-24T18:05:20.343Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.343Z
Learning: Applies to crates/biome_formatter/**/biome_*_formatter/tests/specs/**/options.json : Create an `options.json` file (formatted as `biome.json`) in test specification folders to apply non-default formatting options to all test files in that folder
Applied to files:
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.json
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/tests/specs/**/*.{js,ts,tsx,jsx,json,css,graphql} : Test files for rules should be placed inside 'tests/specs/' directory organized by group and rule name (e.g., 'tests/specs/nursery/myRuleName/')
Applied to files:
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.json
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/lib/src/**/!(mod).rs : Code blocks in documentation marked with 'options' should contain only rule-specific options in JSON/JSONC format, while 'full_options' contains complete biome.json configuration
Applied to files:
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.json
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/lib/src/**/!(mod).rs : The 'language' field in 'declare_lint_rule!' should be set to the specific JavaScript dialect (jsx, ts, tsx) if the rule only applies to that dialect, otherwise use 'js'
Applied to files:
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.jsoncrates/biome_js_analyze/src/lint/style/no_parameter_assign.rs.changeset/light-mugs-speak.md
📚 Learning: 2025-11-21T01:10:53.036Z
Learnt from: dyc3
Repo: biomejs/biome PR: 8171
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:125-137
Timestamp: 2025-11-21T01:10:53.036Z
Learning: In the Biome codebase, each lint rule has its own options type declaration (e.g., `type Options = RuleNameOptions`) as part of the codegen process, even if the options struct is empty or unused. This is standard practice and should not be flagged as an issue.
Applied to files:
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.json
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/lib/src/**/!(mod).rs : Rules that ban functions or variables should use the semantic model to check if the variable is global before reporting, to avoid false positives on locally redeclared variables
Applied to files:
crates/biome_js_analyze/src/lint/style/no_parameter_assign.rs
📚 Learning: 2025-11-24T18:05:27.784Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.784Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Do not attempt to 'fix' the code; if a token/node is known to be mandatory but is missing, return `None` instead
Applied to files:
crates/biome_js_analyze/src/lint/style/no_parameter_assign.rs
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/lib/src/**/!(mod).rs : Rules that promote equivalent compact syntax should use the 'useShorthand<Concept>' naming convention (e.g., `useShorthandAssign`)
Applied to files:
crates/biome_js_analyze/src/lint/style/no_parameter_assign.rs
📚 Learning: 2025-11-24T18:05:27.784Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:27.784Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : When formatting AST nodes, use mandatory tokens from the AST instead of hardcoding token strings (e.g., use `node.l_paren_token().format()` instead of `token("(")`)
Applied to files:
crates/biome_js_analyze/src/lint/style/no_parameter_assign.rs
📚 Learning: 2025-11-24T18:03:52.013Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:03:52.013Z
Learning: Applies to **/.changeset/*.md : Changeset descriptions should be user-facing, use past tense for actions taken (e.g., 'Added new feature'), and present tense for Biome behavior (e.g., 'Biome now supports...'). Include issue links, rule links, and code examples where applicable.
Applied to files:
.changeset/light-mugs-speak.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Validate rules documentation
- GitHub Check: Test Node.js API
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Check Dependencies
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: autofix
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
🔇 Additional comments (1)
crates/biome_js_analyze/tests/specs/style/noParameterAssign/parameterMutationDenyValid.options.json (1)
1-15: Config matches the intended reproThe options file cleanly scopes
noParameterAssignwithpropertyAssignment: "deny"for this spec, and the schema reference looks correct. Nothing to change here from my side.
CodSpeed Performance ReportMerging #8259 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/light-mugs-speak.md (1)
5-7: Changeset looks solid.The description is user-facing, uses appropriate tense (past for action, present for Biome behaviour), includes the issue link, and provides a clear code example demonstrating the fixed scenario.
As an optional enhancement, consider adding an explicit link to the rule documentation (e.g.,
[noParameterAssign](https://biomejs.dev/linter/rules/no-parameter-assign/#propertyassignment)) to help users navigate to the full rule reference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/light-mugs-speak.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Changeset descriptions should be user-facing, use past tense for actions taken (e.g., 'Added new feature'), and present tense for Biome behavior (e.g., 'Biome now supports...'). Include issue links, rule links, and code examples where applicable.
Files:
.changeset/light-mugs-speak.md
🧠 Learnings (5)
📚 Learning: 2025-11-24T18:03:52.013Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:03:52.013Z
Learning: Applies to **/.changeset/*.md : Changeset descriptions should be user-facing, use past tense for actions taken (e.g., 'Added new feature'), and present tense for Biome behavior (e.g., 'Biome now supports...'). Include issue links, rule links, and code examples where applicable.
Applied to files:
.changeset/light-mugs-speak.md
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/lib/src/**/!(mod).rs : The 'language' field in 'declare_lint_rule!' should be set to the specific JavaScript dialect (jsx, ts, tsx) if the rule only applies to that dialect, otherwise use 'js'
Applied to files:
.changeset/light-mugs-speak.md
📚 Learning: 2025-11-24T18:03:52.014Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:03:52.014Z
Learning: For bugfix/feature PRs visible to Biome toolchain users or affecting published crates, create a changeset using the `just new-changeset` command with appropriate package selection, change type (major/minor/patch), and description.
Applied to files:
.changeset/light-mugs-speak.md
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/lib/src/**/!(mod).rs : Rules that ban functions or variables should use the semantic model to check if the variable is global before reporting, to avoid false positives on locally redeclared variables
Applied to files:
.changeset/light-mugs-speak.md
📚 Learning: 2025-11-24T18:04:42.146Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:04:42.146Z
Learning: Applies to crates/biome_analyze/**/biome_*_analyze/lib/src/**/!(mod).rs : Rule names should follow the 'no<Concept>' prefix when a rule's sole intention is to forbid a single concept (e.g., `noDebugger` for disallowing debugger statements)
Applied to files:
.changeset/light-mugs-speak.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Test Node.js API
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: autofix
- GitHub Check: Documentation
- GitHub Check: End-to-end tests
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
2 participants
Summary
The
noParameterAssignrule withpropertyAssignment: "deny"was incorrectly reporting an error when a function parameter was used on the right-hand side (R-value) of an assignment to a local variable's property.The rule should only flag assignments that modify the parameter binding or its properties (L-value), not the use of its value.
Closes #8254.
Test Plan
Added:
tests/specs/style/noParameterAssign/parameterMutationDenyValid.jstests/specs/style/noParameterAssign/parameterMutationDenyValid.js.snaptests/specs/style/noParameterAssign/parameterMutationDenyValid.options.jsonDocs