feat(biome-js-analyze): adjusts members update assignments to be marked as valid in noUnusedPrivateClassMembers #7500
Conversation
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
…ed as valid updates and no errors triggered
🦋 Changeset detectedLatest commit: 4a50b81 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
vladimir-ivanov
changed the title
CodSpeed Performance ReportMerging #7500 will not alter performanceComparing Summary
|
|
I would like to take some time to understand why we want this. This looks strange to me. I see update assignments as atomic operations. To me reporting them as unused is fine. Let wait some more discussions in #7192 |
WalkthroughThe PR updates the noUnusedPrivateClassMembers lint to treat compound assignments as valid usage of private members. It imports JsAssignmentOperator and expands is_in_update_expression to detect non-plain assignments (e.g., +=, -=, **=, &&=, ||=, ??=) via JsAssignmentExpression. The traversal logic splits handling: update expressions drop the member from the unused set; write-only plain assignments keep it. Tests are adjusted by removing previously invalid self-update cases and adding comprehensive valid cases covering all compound operators. A patch-level changeset entry documents the fix linked to issue #7192. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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)
.changeset/no_unused_private_class_members_bug.md (1)
5-18: Tighten wording, punctuation, and fix the example to match the reported bug (use ??= with a Promise).
- End sentences with a period.
- Replace “correctly flagged” (ambiguous) with “no diagnostic”.
- Use the original false‑positive shape from #7192 with ??= and a Promise.
Apply this diff:
-Fixed [#7192](https://github.com/biomejs/biome/issues/7192): -`noUnusedPrivateClassMembers` now treats private members in compound assignments (+=, -=, ??=, etc.) as used, -while plain assignments (=) do not count as usage. +Fixed [#7192](https://github.com/biomejs/biome/issues/7192): `noUnusedPrivateClassMembers` now treats private members in compound assignments (+=, -=, **=, %=, <<=, >>=, >>>=, &=, |=, ^=, &&=, ||=, ??=) as used. Plain assignments (=) do not count as usage. -Example that is now correctly flagged: +Example (no diagnostic): ```typescript class App { - #persistenceRequest: Promise<boolean> | undefined; + #persistenceRequest?: Promise<boolean>; saveData() { - this.#persistenceRequest += 2; + this.#persistenceRequest ??= navigator.storage.persist(); } }</blockquote></details> <details> <summary>crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)</summary><blockquote> `430-443`: **Broaden detection and tidy comments.** - Minor: The comment “grand_parent can also be js expression statement” is confusing; the parent of the assignment/update is the expression statement. - Consider also counting compound assignments as “used” even when not a direct expression statement (e.g. inside `if (this.#x &&= y) {}`); the general path will mark them used, but an explicit test would guard against future regressions. Proposed small tidy: ```diff - // grand_parent can also be js expression statement + // Treat standalone update/compound assignments as usage (statement form only here).And please add tests that cover:
return (this.#x += 1);if (this.#x ??= getVal()) {}This ensures both statement and expression contexts remain green.
📜 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/correctness/noUnusedPrivateClassMembers/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
.changeset/no_unused_private_class_members_bug.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(3 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.js(0 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts(0 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js(1 hunks)
💤 Files with no reviewable changes (2)
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.js
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
🧰 Additional context used
📓 Path-based instructions (5)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changeset files, only use #### or ##### headers; avoid other header levels
Changeset descriptions should use past tense for what you did (e.g., "Added...")
Describe current Biome behavior in present tense within changesets (e.g., "Biome now supports...")
For bug fixes in changesets, start with a link to the issue (e.g., "Fixed #1234: ...")
When referencing rules or assists in changesets, include links to their documentation pages
Include a minimal code block in the changeset when applicable to demonstrate the change
End every sentence in the changeset description with a period
Files:
.changeset/no_unused_private_class_members_bug.md
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/tests/specs/correctness/noUnusedPrivateClassMembers/valid.jscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.jscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.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/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/correctness/no_unused_private_class_members.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
crates/biome_rowan/src/syntax/node.rs (1)
grand_parent(254-256)crates/biome_rowan/src/ast/mod.rs (6)
cast(135-137)cast(381-381)cast(385-387)cast(824-829)cast(873-878)cast(1319-1324)
🔇 Additional comments (3)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
14-16: Importing JsAssignmentOperator is spot on.Required for distinguishing plain vs compound assignments.
271-276: Early‑return on update expressions correctly treats them as usage.This cleanly separates update vs write‑only cases and resolves the false positive for compound ops.
Mind adding one spec that exercises compound assignment used as an expression (e.g.
return (this.#x ||= 1);) to lock this in?.changeset/no_unused_private_class_members_bug.md (1)
5-7: Add Biome docs link for noUnusedPrivateClassMembersConfirmed docs URL: https://v1.biomejs.dev/linter/rules/no-unused-private-class-members/ — add it next to the rule name in .changeset/no_unused_private_class_members_bug.md (lines 5–7).
Comment on lines
+207
to
+366
| class AppSelfAdd { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest += this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppSelfSubtract { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest -= this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppSelfMultiply { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest *= this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppSelfDivide { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest /= this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppSelfExponent { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest **= this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppSelfAnd { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest &&= this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppSelfOr { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest ||= this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppSelfNullish { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest ??= this.#persistenceRequest; | ||
| } | ||
| } | ||
|
|
||
| class AppAddAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest += 1; | ||
| } | ||
| } | ||
|
|
||
| class AppSubtractAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest -= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppMultiplyAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest *= 2; | ||
| } | ||
| } | ||
|
|
||
| class AppDivideAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest /= 2; | ||
| } | ||
| } | ||
|
|
||
| class AppExponentAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest **= 2; | ||
| } | ||
| } | ||
|
|
||
| class AppModuloAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest %= 2; | ||
| } | ||
| } | ||
|
|
||
| class AppAndAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest &= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppOrAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest |= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppXorAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest ^= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppLeftShiftAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest <<= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppRightShiftAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest >>= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppUnsignedRightShiftAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest >>>= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppAndLogicalAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest &&= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppOrLogicalAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest ||= 1; | ||
| } | ||
| } | ||
|
|
||
| class AppNullishAssignment { | ||
| #persistenceRequest = 0; | ||
| saveData() { | ||
| this.#persistenceRequest ??= 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add expression‑context cases for compound assignments.
Great coverage of statement forms. Let’s add a couple of expression forms to prevent regressions.
Apply this diff near the end of the file:
class AppNullishAssignment {
#persistenceRequest = 0;
saveData() {
this.#persistenceRequest ??= 1;
}
}
+class AppOrLogicalAssignmentInIf {
+ #persistenceRequest = 0;
+ saveData() {
+ if (this.#persistenceRequest ||= 1) {
+ return true;
+ }
+ }
+}
+
+class AppNullishAssignmentInReturn {
+ #persistenceRequest = 0;
+ saveData() {
+ return (this.#persistenceRequest ??= 1);
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class AppSelfAdd { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest += this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfSubtract { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest -= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfMultiply { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest *= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfDivide { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest /= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfExponent { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest **= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfAnd { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest &&= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfOr { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ||= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfNullish { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ??= this.#persistenceRequest; | |
| } | |
| } | |
| class AppAddAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest += 1; | |
| } | |
| } | |
| class AppSubtractAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest -= 1; | |
| } | |
| } | |
| class AppMultiplyAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest *= 2; | |
| } | |
| } | |
| class AppDivideAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest /= 2; | |
| } | |
| } | |
| class AppExponentAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest **= 2; | |
| } | |
| } | |
| class AppModuloAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest %= 2; | |
| } | |
| } | |
| class AppAndAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest &= 1; | |
| } | |
| } | |
| class AppOrAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest |= 1; | |
| } | |
| } | |
| class AppXorAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ^= 1; | |
| } | |
| } | |
| class AppLeftShiftAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest <<= 1; | |
| } | |
| } | |
| class AppRightShiftAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest >>= 1; | |
| } | |
| } | |
| class AppUnsignedRightShiftAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest >>>= 1; | |
| } | |
| } | |
| class AppAndLogicalAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest &&= 1; | |
| } | |
| } | |
| class AppOrLogicalAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ||= 1; | |
| } | |
| } | |
| class AppNullishAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ??= 1; | |
| } | |
| } | |
| class AppSelfAdd { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest += this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfSubtract { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest -= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfMultiply { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest *= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfDivide { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest /= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfExponent { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest **= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfAnd { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest &&= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfOr { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ||= this.#persistenceRequest; | |
| } | |
| } | |
| class AppSelfNullish { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ??= this.#persistenceRequest; | |
| } | |
| } | |
| class AppAddAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest += 1; | |
| } | |
| } | |
| class AppSubtractAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest -= 1; | |
| } | |
| } | |
| class AppMultiplyAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest *= 2; | |
| } | |
| } | |
| class AppDivideAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest /= 2; | |
| } | |
| } | |
| class AppExponentAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest **= 2; | |
| } | |
| } | |
| class AppModuloAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest %= 2; | |
| } | |
| } | |
| class AppAndAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest &= 1; | |
| } | |
| } | |
| class AppOrAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest |= 1; | |
| } | |
| } | |
| class AppXorAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ^= 1; | |
| } | |
| } | |
| class AppLeftShiftAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest <<= 1; | |
| } | |
| } | |
| class AppRightShiftAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest >>= 1; | |
| } | |
| } | |
| class AppUnsignedRightShiftAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest >>>= 1; | |
| } | |
| } | |
| class AppAndLogicalAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest &&= 1; | |
| } | |
| } | |
| class AppOrLogicalAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ||= 1; | |
| } | |
| } | |
| class AppNullishAssignment { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| this.#persistenceRequest ??= 1; | |
| } | |
| } | |
| class AppOrLogicalAssignmentInIf { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| if (this.#persistenceRequest ||= 1) { | |
| return true; | |
| } | |
| } | |
| } | |
| class AppNullishAssignmentInReturn { | |
| #persistenceRequest = 0; | |
| saveData() { | |
| return (this.#persistenceRequest ??= 1); | |
| } | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
noUnusedPrivateClassMembersnow treats private members in compound assignments (+=, -=, ??=, etc.) as used,while plain assignments (=) do not count as usage.
Example that is now correctly flagged:
closes #7192