Skip to content

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Sep 6, 2025

Summary

Fixes #7323

The rule now considers as used all TS private members if a computed access on this is found.

Test Plan

I added two tests:

  • A test that demonstrates that TS private members are no longer reported when a computed access on this is found.
  • A test that demonstrates that unused #member are still reported even if a computed access on this is found.

Docs

I added a Caveat section describing the new behavior.

Copy link

changeset-bot bot commented Sep 6, 2025

🦋 Changeset detected

Latest commit: 8e57294

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
@biomejs/biome Patch
@biomejs/cli-win32-x64 Patch
@biomejs/cli-win32-arm64 Patch
@biomejs/cli-darwin-x64 Patch
@biomejs/cli-darwin-arm64 Patch
@biomejs/cli-linux-x64 Patch
@biomejs/cli-linux-arm64 Patch
@biomejs/cli-linux-x64-musl Patch
@biomejs/cli-linux-arm64-musl Patch
@biomejs/wasm-web Patch
@biomejs/wasm-bundler Patch
@biomejs/wasm-nodejs Patch
@biomejs/backend-jsonrpc Patch

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

Copy link
Contributor

coderabbitai bot commented Sep 6, 2025

Walkthrough

The noUnusedPrivateClassMembers lint was changed to treat computed/bracket access on this (e.g. this[name]) as usage for TypeScript-style private members. Traversal was refactored to use descendants iteration, adding ts_private_count and early-exit logic. A new AnyMember::is_private_sharp() helper distinguishes ES private (#) members. On detecting computed this[...] access the code marks TS-private members as used (retaining only ES private-sharp members). Two tests (valid/invalid dynamic access) and a changeset documenting the fix were added.

Assessment against linked issues

Objective Addressed Explanation
Detect bracket/computed access to private members as usage to avoid false positives (#7323)
Cover reproduce case: private members assigned in constructor, invoked via thisaction with "add" | "remove" (#7323)
Use type information to resolve computed property expression types (string literal unions) and only mark matching private members as used; preserve conservative behaviour when unresolved (#7323) Implementation treats any computed this[...] as using TS-private members without using type-checker resolution; no evidence of literal-union resolution logic.

Suggested labels

A-Type-Inference

Suggested reviewers

  • ematipico
  • vladimir-ivanov
  • dyc3
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch conaclos/noUnusedPrivateClassMembers-omputed-access-7323

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Sep 6, 2025
@Conaclos Conaclos force-pushed the conaclos/noUnusedPrivateClassMembers-omputed-access-7323 branch 2 times, most recently from f28f207 to 0772609 Compare September 6, 2025 16:42
coderabbitai[bot]

This comment was marked as resolved.

@Conaclos Conaclos requested review from a team September 6, 2025 16:49
@Conaclos Conaclos force-pushed the conaclos/noUnusedPrivateClassMembers-omputed-access-7323 branch from 0772609 to 476f2b2 Compare September 6, 2025 16:50
@Conaclos Conaclos changed the title fix(noUnusedPrivateClassMembers): ignore all TS private members if a this[access] is found fix(noUnusedPrivateClassMembers): ignore all TS private members if a this[computed] is found Sep 6, 2025
Copy link

codspeed-hq bot commented Sep 6, 2025

CodSpeed Performance Report

Merging #7419 will not alter performance

Comparing conaclos/noUnusedPrivateClassMembers-omputed-access-7323 (8e57294) with main (53ff5ae)

Summary

✅ 133 untouched benchmarks

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are failing

@Conaclos Conaclos force-pushed the conaclos/noUnusedPrivateClassMembers-omputed-access-7323 branch from 476f2b2 to 8e57294 Compare September 6, 2025 23:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)

445-456: Good helper; docstring matches prior suggestion.

Accurately distinguishes ES private # members.

🧹 Nitpick comments (3)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (3)

68-82: Clarify caveat scope (‘this[…]’ only) and highlight ES-# exception.

Tighten wording so readers don’t infer we also handle arbitrary bracket access or optional chaining.

-    /// The rule currently considers that all TypeScript private members are used if it encounters a computed access.
+    /// The rule treats all TypeScript private members as used when it encounters a computed access on `this` (`this[...]`).
+    /// ES private members declared with `#` are not affected by this caveat and are still analysed normally.

252-258: Polish the comment for readability.

Small grammar tweak; variable name is fine, but consider remaining_ts_privates for self‑documentation.

-    // `true` is at least one member is a TypeScript private member like `private member`.
-    // The other private members are sharp members `#member`.
+    // At least one member is a TypeScript-style private (e.g. `private member`).
+    // The remaining private members are ES private-sharp (e.g. `#member`).

288-299: Exclude this[…] in nested classes when clearing TS-private usage

 for node in syntax.descendants() {
+    // Prevent a nested class’s `this[…]` from clearing outer private-member diagnostics
+    if node
+        .ancestors()
+        .any(|anc| anc.kind() == JsSyntaxKind::JS_CLASS_DECLARATION && !is_node_equal(&anc, syntax))
+    {
+        continue;
+    }
     // …existing computed-member handling…
 }

Add a test demonstrating a nested class with this[action] does not suppress diagnostics for the outer class’s unused privates.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 476f2b2 and 8e57294.

⛔ Files ignored due to path filters (2)
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap is excluded by !**/*.snap and included by **
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts.snap is excluded by !**/*.snap and included by **
📒 Files selected for processing (4)
  • .changeset/little-heads-divide.md (1 hunks)
  • crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (4 hunks)
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts (1 hunks)
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts
  • crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
  • .changeset/little-heads-divide.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Format Rust and TOML files before committing (use just f/just format).

Files:

  • crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
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/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/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 (1)
crates/biome_js_syntax/src/expr_ext.rs (4)
  • member (1590-1595)
  • object (1569-1574)
  • object (1610-1615)
  • object (1672-1678)
⏰ 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). (24)
  • GitHub Check: Bench (biome_module_graph)
  • GitHub Check: Bench (biome_configuration)
  • GitHub Check: Bench (biome_css_analyze)
  • GitHub Check: Bench (biome_package)
  • GitHub Check: Bench (biome_graphql_formatter)
  • GitHub Check: Bench (biome_json_analyze)
  • GitHub Check: Bench (biome_js_analyze)
  • GitHub Check: Bench (biome_html_parser)
  • GitHub Check: Bench (biome_html_formatter)
  • GitHub Check: Bench (biome_graphql_parser)
  • GitHub Check: Bench (biome_css_parser)
  • GitHub Check: Bench (biome_json_formatter)
  • GitHub Check: Bench (biome_json_parser)
  • GitHub Check: Bench (biome_css_formatter)
  • GitHub Check: Bench (biome_js_parser)
  • GitHub Check: Bench (biome_js_formatter)
  • GitHub Check: Documentation
  • GitHub Check: Test Node.js API
  • GitHub Check: Lint project (depot-windows-2022)
  • GitHub Check: Test (depot-ubuntu-24.04-arm-16)
  • GitHub Check: Test (depot-windows-2022-16)
  • GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
  • GitHub Check: Check Dependencies
  • GitHub Check: autofix
🔇 Additional comments (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)

13-16: Imports wired up correctly for computed access.

The new imports are used (computed member and expression checks; formal parameter).


259-287: Name-based usage pruning looks sound.

Retains write-only/update usages and removes confirmed reads; early-exit on empty set is helpful.

@Conaclos Conaclos merged commit 576baf4 into main Sep 6, 2025
30 checks passed
@Conaclos Conaclos deleted the conaclos/noUnusedPrivateClassMembers-omputed-access-7323 branch September 6, 2025 23:24
@github-actions github-actions bot mentioned this pull request Sep 6, 2025
kedevked pushed a commit to kedevked/biome that referenced this pull request Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

💅 noUnusedPrivateClassMembers does not handle bracket notation
2 participants