feat(biome_js_analyze): align no_unused_private_class_members_with_semantic_class#7610
Conversation
🦋 Changeset detectedLatest commit: 38035b9 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
force-pushed
the
feat/no_unused_private_class_members_semantic_class
branch
from
2577f8f to
19ee305
Compare
vladimir-ivanov
changed the title
CodSpeed Performance ReportMerging #7610 will degrade performances by 10.43%Comparing Summary
Benchmarks breakdown
Footnotes
|
vladimir-ivanov
force-pushed
the
feat/no_unused_private_class_members_semantic_class
branch
from
dae7a42 to
0b4b6be
Compare
WalkthroughThe PR integrates SemanticClassModel and named-member extraction into unused-private-member analysis. It replaces the old traversal with semantic-aware traversals (traverse_meaningful_read_members_usage and traverse_computed_members_usage), threads Arc through diagnostics/actions, extends AnyMember with semantic-aware APIs (member_range, matches_name), introduces AnyNamedClassMember/NamedClassMember, migrates use_readonly_class_properties to the named-member model, and updates tests and changelog to clarify the “meaningful read” definition. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 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)
⏰ 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). (11)
Comment |
vladimir-ivanov
requested review from
Conaclos,
ematipico and
togami2864
and removed request for
Conaclos,
ematipico and
togami2864
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.js (1)
4-9: LGTM. Consider one more variant for coverage.This nicely exercises a meaningful read via +=. Optional: add prefix/postfix increment and direct read/write variants to future‑proof the rule against similar patterns.
.changeset/no_unused_private_class_members_amended.md (1)
25-27: Grammar nit and duplicated example.
- Comment grammar: “a is not reference to this scope” → “a is not a reference to this scope.”
- The “UsedMember/#usedInInnerClass” block appears twice with the same point; consider dropping one to reduce noise.
- // not really used, a is not reference to this scope + // Not really used; `a` is not a reference to this scope.- class UsedMember { - #usedInInnerClass; - - method(a) { - return class { - foo = a.#usedInInnerClass; - } - } - } + // (Duplicate example removed for brevity.)Also applies to: 44-48
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts (1)
16-18: Small wording tweak for clarity.“a is not reference to this scope” → “a is not a reference to this scope.”
- // not really used, a is not reference to this scope + // Not really used; `a` is not a reference to this scope.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
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/invalid_aligned_with_semantic_class.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (10)
.changeset/no_unused_private_class_members_amended.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(12 hunks)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs(11 hunks)crates/biome_js_analyze/src/services/semantic_class.rs(13 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.js(0 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js(0 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.js(1 hunks)
💤 Files with no reviewable changes (2)
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.js
🧰 Additional context used
📓 Path-based instructions (5)
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/invalid_aligned_with_semantic_class.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.jscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/services/semantic_class.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.jscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/services/semantic_class.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts
**/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/invalid_aligned_with_semantic_class.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts
.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_amended.md
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/services/semantic_class.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
🧠 Learnings (5)
📚 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 : If a rule returns a code action (implements `action`), add `fix_kind` in `declare_lint_rule!` and use `ctx.metadata().applicability()` when building the action
Applied to files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
📚 Learning: 2025-08-11T11:48:52.001Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:48:52.001Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : When a token is mandatory and present in the AST, use the AST-provided token (e.g., node.l_paren_token().format()) instead of hardcoded tokens
Applied to files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.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/correctness/no_unused_private_class_members.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/**/src/{lint,assist}/**/*.rs : Follow rule naming conventions: use `no<Concept>` to forbid and `use<Concept>` to mandate; prefer consistent prefixes (e.g., `noDuplicate<Concept>`, `useConsistent<Concept>`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.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_rule_options/lib/**/*.rs : Define per-rule options types in `biome_rule_options` crate (one file per rule, e.g., `lib/use_my_rule.rs`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
🧬 Code graph analysis (2)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
crates/biome_rowan/src/ast/mod.rs (1)
cast_ref(142-151)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)
crates/biome_js_analyze/src/services/semantic_class.rs (5)
new(48-50)new(426-431)default(42-44)class_member_references(52-54)class_member_references(271-327)
⏰ 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). (18)
- GitHub Check: Validate PR title
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_css_parser)
🔇 Additional comments (4)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts (1)
10-10: Rename improves intent.Switching to
unusedPropertyclarifies the case. Please ensure any snapshots/inline expectations referencing the old name are updated.crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts (2)
2-6: Good clarification of the constructor case.This cleanly isolates two private param properties as unused while keeping
usedPropertypublic. Looks spot on for the regression.Please confirm this still reproduces the intended diagnostics for issue 7101 with the new semantics.
14-17: Helpful inline note.The comment crisply explains why this doesn’t count as a member read/write. No changes needed.
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts (1)
1-9: Accessor write‑only case captured well.This is a solid invalid example for “no meaningful read” with a setter. All good.
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
Show resolved
Hide resolved
...e/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
Outdated
Show resolved
Hide resolved
| Ok(_) => {} | ||
| Err(node) => { | ||
| if ts_private_count != 0 | ||
| && let Some(computed_member) = AnyJsComputedMember::cast(node) |
There was a problem hiding this comment.
will do another / final pr to address this area - I would suggest we either do:
- move to semantic_class and perhaps introduce another type of entity there - so far we have AccessKind { MeaningfulRead, TrivialRead, Write} - we can add another DynamicRead, or similar.
- keep the logic here and expand so we read the type of the argument AND check its definition -> if TS Union type, then we check the values and match against class members.
- any other ideas welcome :-)
There was a problem hiding this comment.
Will you create a new task for that?
There was a problem hiding this comment.
thinking yes. maybe just option 2 as a start. will play with it tomorrow
There was a problem hiding this comment.
#7682 ticket raised, will do a pr shortly, perhaps will wait for this one to get resolved or rejected first to avoid conflicts.
There was a problem hiding this comment.
#7684 pr raised, but in draft mode as it stands, depends on this one
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…eClassMembers/invalid_aligned_with_semantic_class.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…_class_members.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
164-174: Unsafe fix: demoting TS parameter properties whilethis.<name>is used.We always propose
RemovePrivateModifierforTsPropertyParameter, but we don’t gate it on property reads/writes. If the class writesthis.foo = ...(or reads it) anywhere, removing the private modifier drops the class field and breaks those sites.Gate the fix on “no property reads and no property writes” and fall back to a diagnostic‑only path otherwise.
Suggested minimal change:
@@ - AnyMember::TsPropertyParameter(ts_property_param) => { - // Check if the parameter is also unused in constructor body using semantic analysis - let should_rename = - check_ts_property_parameter_usage(ctx, ts_property_param); - results.push(UnusedMemberAction::RemovePrivateModifier { - member, - semantic_class: semantic_class.clone(), - rename_with_underscore: should_rename, - }); - } + AnyMember::TsPropertyParameter(ts_property_param) => { + let has_prop_refs = + has_property_references(&semantic_class, &member, &class_member_references); + if has_prop_refs { + // Report only; avoid an unsafe demotion that would break `this.<name>` sites. + results.push(UnusedMemberAction::RemoveMember { + member, + semantic_class: semantic_class.clone(), + }); + } else { + // Safe to demote to a plain parameter. Rename if the binding itself is unused. + let should_rename = + check_ts_property_parameter_usage(ctx, ts_property_param); + results.push(UnusedMemberAction::RemovePrivateModifier { + member, + semantic_class: semantic_class.clone(), + rename_with_underscore: should_rename, + }); + } + }…and add this helper (place near other helpers):
+fn has_property_references( + semantic_class: &SemanticClassModel, + member: &AnyMember, + refs: &ClassMemberReferences, +) -> bool { + let matches = |r: &ClassMemberReference| member.matches_name(semantic_class, &r.name); + refs.reads.iter().any(matches) || refs.writes.iter().any(matches) +}This keeps the diagnostic while avoiding an unsound quick‑fix.
214-226: Avoid deleting constructor parameters for the diagnostic‑only path.With the gating above, some
TsPropertyParametercases will useRemoveMemberpurely to carry the diagnostic. Makeactiona no‑op for that specific shape.@@ - UnusedMemberAction::RemoveMember { member, .. } => { - mutation.remove_node(member.clone()); + UnusedMemberAction::RemoveMember { member, .. } => { + if matches!(member, AnyMember::TsPropertyParameter(_)) { + // Diagnostic only; no safe fix for TS parameter properties with property uses. + return None; + } + mutation.remove_node(member.clone()); Some(JsRuleAction::new(This preserves existing fixes for regular class members.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
321-351: Computed‑access scan risks false negatives via nested classes.
syntax.descendants()will also traverse into inner classes;this[...]inside them doesn’t refer to the outer class. That can incorrectly mark TS privates as used.Follow‑up suggestion:
- Either restrict the walk to bodies belonging to the current class’ members, or
- Leverage
SemanticClassModeland tag such accesses as a separateAccessKind(e.g. DynamicRead) and handle them upstream, as discussed in the PR thread.This is non‑blocking for this PR but worth tightening.
180-212: Message clarity for TS parameter properties.Once the gating is in place:
- When
rename_with_underscoreis true: consider “This parameter is never used.”.- When false (there are constructor‑binding references): “This parameter is never used outside of the constructor.”
Optional polish, but helps match the actual fix offered.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/no_unused_private_class_members_amended.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(12 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/no_unused_private_class_members_amended.md
🧰 Additional context used
📓 Path-based instructions (4)
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/invalid_aligned_with_semantic_class.tscrates/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/invalid_aligned_with_semantic_class.tscrates/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/invalid_aligned_with_semantic_class.ts
**/*.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 (5)
📚 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 : If a rule returns a code action (implements `action`), add `fix_kind` in `declare_lint_rule!` and use `ctx.metadata().applicability()` when building the action
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.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/correctness/no_unused_private_class_members.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/**/src/{lint,assist}/**/*.rs : Follow rule naming conventions: use `no<Concept>` to forbid and `use<Concept>` to mandate; prefer consistent prefixes (e.g., `noDuplicate<Concept>`, `useConsistent<Concept>`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.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_rule_options/lib/**/*.rs : Define per-rule options types in `biome_rule_options` crate (one file per rule, e.g., `lib/use_my_rule.rs`)
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.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/**/src/{lint,assist}/**/*.rs : Set the `language` field in `declare_lint_rule!` to the primary language (`js`, `jsx`, `ts`, or `tsx`) the rule targets
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 (1)
crates/biome_js_analyze/src/services/semantic_class.rs (5)
new(48-50)new(426-431)default(42-44)class_member_references(52-54)class_member_references(271-327)
⏰ 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). (19)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Validate PR title
🔇 Additional comments (6)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts (4)
1-9: Good coverage for accessor-only writes.This clearly exercises “setter-only” paths; matches the updated meaningful‑read semantics. Nice.
14-20: Inner class “a.#field” examples hit the right edge case.These confirm that an inner class not bound to the outer
thisshouldn’t count as usage. Solid negative tests.Also applies to: 34-39
22-29: Setter‑only accessor used in member access is invalid (as intended).Good to assert that
[this.#accessorUsedInMemberAccess] = adoesn’t constitute a read without a getter.
41-50: Nit: keep comments consistent with other cases.You already phrase it as “no meaningful read” elsewhere; this one matches now. All good.
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
148-154: Nice: no longer discarding the computed‑usage filter result.Passing
unused_membersintotraverse_meaningful_read_members_usageresolves the earlier bypass. Thanks for addressing it.
85-92: Good: fix kind + applicability wired.
fix_kind: FixKind::Unsafeand usingctx.metadata().applicability()in actions are in line with the project’s guidance. Based on learnings
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
579-617: LHS object destructuring is a write, not a read
collect_object_assignment_namesmarks({ a: this.z } = obj)as a MeaningfulRead; it should be a Write (we assign tothis.z). Current logic then feeds it intoreadsvia the branch at Lines 841–851, which can incorrectly “use” write‑only members.Apply this minimal fix:
- AccessKind::MeaningfulRead, + AccessKind::Write,No other branches need to change; the existing split on
access_kindwill now route towrites.Also applies to: 841-851
🧹 Nitpick comments (6)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts.snap.new (1)
29-36: Snapshot intent matches “meaningful read” alignmentThe cases capture setter-only writes, inner-class without outer
this, and computed LHS without a getter. Looks good and reads crisply.Consider adding a tiny positive case showing a compound assignment on a private accessor (e.g.
this.#x += 1) to make the new “read+write” semantics explicit in snapshots.Also applies to: 48-57
crates/biome_js_analyze/src/services/semantic_class.rs (5)
1003-1009: Missed meaningful-read contexts: variable initialisers
const x = this.foois a genuine read, butis_used_in_expression_contextdoesn’t consider variable initialisers. Please extend to cover variable declarator initialiser contexts (or a general “initialiser clause” when parent isn’t a class property).Suggested approach:
- Treat
JS_INITIALIZER_CLAUSEas meaningful unless its parent isJS_PROPERTY_CLASS_MEMBER(already handled byis_class_initializer_rhs).- Or include
JS_VARIABLE_DECLARATORwith a check that you’re on the RHS.Also applies to: 1065-1085
452-477: Alias detection only scans top-level statements; misses aliases in nested blocks
collect_local_this_referenceswalksbody.statements()and only variable statements, soif (...) { const alias = this }(then used inside the block) is missed. This can undercount reads/writes via aliases.Consider a lightweight preorder over
body.syntax()to catchconst/let alias = thisanywhere in the function body, not just top level. Optionally pick up simplealias = thisassignments too.
772-782: Inconsistent ranges for member readsHere you use the whole static member (
this.foo) range; elsewhere you use the member token’s range. Normalise on the member token’s trimmed range for sharper diagnostics.Apply:
- range: static_member.syntax().text_trimmed_range(), + range: member.syntax().text_trimmed_range(),Also applies to: 957-964
191-201: Tiny nit: comment typo and a TODOThe doc hints at adding
accessorsupport; there’s a “claas” typo. Maybe add a tracking issue or TODO so it doesn’t drift.
211-262: Minor: prefer consistent range extraction inextract_named_memberFor consistency with other sites and to avoid node‑specific
range()quirks, prefername_node.syntax().text_trimmed_range().- range: name_node.range(), + range: name_node.syntax().text_trimmed_range(),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/biome_js_analyze/src/services/semantic_class.rs(13 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts.snap.new(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap.new(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/invalid_aligned_with_semantic_class.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap.newcrates/biome_js_analyze/src/services/semantic_class.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap.newcrates/biome_js_analyze/src/services/semantic_class.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/invalid_aligned_with_semantic_class.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap.new
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/services/semantic_class.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
crates/biome_rowan/src/ast/mod.rs (1)
cast_ref(142-151)
⏰ 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). (18)
- GitHub Check: Test Node.js API
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: autofix
🔇 Additional comments (3)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap.new (1)
8-20: LGTM: dynamic access doesn’t imply private name usageThe diagnostic and unsafe fix are appropriate for
#propwhen onlythis[name]is read.Also applies to: 26-46
crates/biome_js_analyze/src/services/semantic_class.rs (2)
1290-1334: Test expectation questions: destructuring LHS counted as readThe test expects a meaningful read for
({ a: this.z } = obj). Per JS semantics, that’s a write tothis.z(no prior value read). Recommend flipping this to a write to stay consistent with “setter‑only usage is not enough.”If you agree, I can draft the test update after the code change above.
811-827: Extend compound assignment operator match
Currentmatches!only handlesPIPE2EQ,AMP2EQ,SLASHEQ,STAREQ,PERCENTEQ,PLUSEQ,QUESTION2EQ. Add the missing variants for-=(MINUSEQ),^=(CARETEQ),<<=(LTLTEQ),>>=(GTGTEQ),>>>=(GT2GTEQ) and the exponent‐assignment token for**=. Confirm the exactJsSyntaxKindnames and update thematches!accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
481-489: Diagnostic anchoring can still fail.The past review comment on lines 481-490 remains unaddressed.
member_rangereturnsNonewhenextract_named_membercan't resolve, which could cause diagnostics to lose their anchor. Falling back toself.syntax().text_range()ensures diagnostics always have a range.Apply this diff:
fn member_range(&self, semantic_class: &SemanticClassModel) -> Option<TextRange> { if let Some(any_named_class_member) = AnyNamedClassMember::cast(self.syntax().clone()) && let Some(prop_name) = semantic_class.extract_named_member(&any_named_class_member) { return Some(prop_name.range); } - None + Some(self.syntax().text_range()) }
145-154: Critical: Still passing wrong vector to meaningful read traversal.The past review comment on lines 146-154 remains unaddressed. Line 146 filters out computed-usage members into
unused_members, but line 151 then passes the originalprivate_members(notunused_members) totraverse_meaningful_read_members_usage, discarding the computed-usage filtering.Apply this diff:
let mut unused_members = traverse_computed_members_usage(node.syntax(), private_members.clone()); if !unused_members.is_empty() { unused_members = traverse_meaningful_read_members_usage( &semantic_class, - private_members, + unused_members, &class_member_references, ); }
🧹 Nitpick comments (3)
crates/biome_js_analyze/src/services/semantic_class.rs (2)
21-25: Consider adding PartialEq and Eq derives.Since
NamedClassMembercontainsTextandTextRange(both likelyEq), adding#[derive(PartialEq, Eq)]would enable direct equality checks and potentially allow use in hash-based collections for deduplication scenarios.
191-201: Address the TODO for accessor support.The comment notes that accessor class members need to be added. This could affect completeness of the semantic analysis.
Do you want me to open an issue to track adding accessor support to
AnyNamedClassMember?crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)
331-346: Empty match arm may indicate incomplete refactoring.Line 333's
Ok(_) => {}suggests thatAnyJsNamenodes are intentionally skipped, but the empty block provides no clarity. Consider adding a comment explaining why names are excluded from computed-member checks, or removing the match if it serves no purpose.
📜 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/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(12 hunks)crates/biome_js_analyze/src/services/semantic_class.rs(12 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/services/semantic_class.rscrates/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/services/semantic_class.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format all Rust source files before committing (just f)
Files:
crates/biome_js_analyze/src/services/semantic_class.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.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 : If a rule returns a code action (implements `action`), add `fix_kind` in `declare_lint_rule!` and use `ctx.metadata().applicability()` when building the action
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.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/correctness/no_unused_private_class_members.rs
🧬 Code graph analysis (2)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
crates/biome_rowan/src/ast/mod.rs (1)
cast_ref(142-151)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
crates/biome_js_syntax/src/expr_ext.rs (8)
member(1541-1546)new(1400-1402)name(108-110)name(168-170)name(1698-1712)name(1743-1764)name(1795-1815)name(1872-1898)crates/biome_js_analyze/src/services/semantic_class.rs (5)
new(48-50)new(426-431)default(42-44)class_member_references(52-54)class_member_references(271-327)
⏰ 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_package)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test Node.js API
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: autofix
🔇 Additional comments (10)
crates/biome_js_analyze/src/services/semantic_class.rs (6)
41-50: LGTM!The
Defaultand constructor implementations follow Rust conventions correctly.
56-61: LGTM!Clean delegation pattern. The public method provides a stable API while keeping implementation details private.
213-262: LGTM!The extraction logic correctly handles each member type, with appropriate
Nonereturn for index signatures and proper error propagation via?.
845-850: LGTM!The access kind discrimination properly routes references to either writes or reads based on
AccessKind.
862-876: LGTM!This correctly implements the meaningful read enhancement: assignments used in expression contexts (e.g.,
return this.#val = 3) are both writes and meaningful reads.
1011-1085: LGTM!The three expression context helpers correctly identify meaningful reads:
is_class_initializer_rhs: RHS of property initialisersis_assignment_expression_context: RHS and LHS object destructuring (both are reads)is_general_expression_context: comprehensive coverage of expression contextscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (4)
1-23: LGTM!Imports properly support the semantic class integration with
Arcfor shared ownership.
100-110: LGTM!Wrapping
SemanticClassModelinArcenables efficient shared ownership across multiple actions.
275-302: LGTM!The filtering logic correctly retains only members without meaningful reads, using
AccessKind::MeaningfulReaddiscrimination and semantic name matching.
491-499: LGTM!
matches_namecorrectly returnsfalsewhen name extraction fails, providing safe fallback behaviour for comparison.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
| Ok(_) => {} | ||
| Err(node) => { | ||
| if ts_private_count != 0 | ||
| && let Some(computed_member) = AnyJsComputedMember::cast(node) |
There was a problem hiding this comment.
Will you create a new task for that?
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/services/semantic_class.rs (2)
41-45: Is theDefaultimplementation necessary?You've added an explicit
new()constructor. TheDefaulttrait implementation delegates tonew(), which is fine, but consider whether callers actually needDefault::default()or if they can just callnew()directly. IfDefaultisn't required by a trait bound elsewhere, this might be unnecessary indirection.
1107-1138: Consider restoringAccessKindchecks in test assertions.The updated assertions at lines 1107–1117 and 1125–1137 now ignore the
AccessKind(second element of the tuple). Whilst this simplifies the tests, it also means you won't catch bugs where the wrongAccessKindis returned (e.g.,TrivialReadinstead ofMeaningfulRead). If the distinction matters for correctness, consider restoring those checks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_js_analyze/src/services/semantic_class.rs(15 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/services/semantic_class.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/services/semantic_class.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Before committing, format Rust and TOML files (e.g., via
just f/just format)
Files:
crates/biome_js_analyze/src/services/semantic_class.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Document rules, assists, and options via inline rustdoc in Rust source
Files:
crates/biome_js_analyze/src/services/semantic_class.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/services/semantic_class.rs (2)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
TextRange(9205-9205)crates/biome_rowan/src/ast/mod.rs (1)
cast_ref(142-151)
⏰ 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). (11)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Test Node.js API
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: autofix
- GitHub Check: Documentation
- GitHub Check: Check Dependencies
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
🔇 Additional comments (4)
crates/biome_js_analyze/src/services/semantic_class.rs (4)
1036-1059: Verify object destructuring on LHS logic.Lines 1051–1056 treat the LHS of an assignment as a read when it's an object assignment pattern. Typically, the LHS of an assignment is the write target, not a read. For example, in
({ a: this.z } = obj),this.zis being written to, not read.However, if the intent is to detect reads from
obj.abeing assigned intothis.z, that's handled by the RHS check (lines 1045–1049). Please verify this logic is correct, as it might flag false positives.
1564-1566: Test case expectation may be incomplete.Line 1564 has the code
for(this.i = 0; this.i < 10; this.i++) {}, which referencesthis.ithree times. The expected result only lists("this.i", true)once. If the test helperparse_this_member_nodes_from_codereturns one node per reference, you'd expect three entries. Please verify this is intentional and that the test is checking what you intend.
1004-1085: Nice refactor ofis_used_in_expression_context.Breaking out the three helper functions (
is_class_initializer_rhs,is_assignment_expression_context,is_general_expression_context) makes the logic much clearer and easier to extend. The new test coverage is comprehensive, too. Well done.
211-262:extract_named_memberimplementation looks correct.The match arms correctly extract the name and range for each member type, and you're returning
Nonefor index signatures, which makes sense. The new tests inextract_named_member_testsconfirm the behaviour. Good work.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
1107-1138: Consider enhancing test assertions.Lines 1107 and 1126 now ignore the
AccessKindcomponent of expected tuples (changed from(expected_name, expected_access_kind)to(expected_name, _)). This simplifies the assertions but reduces test coverage—correctness ofAccessKind(MeaningfulRead vs TrivialRead) is no longer verified.If
AccessKindcorrectness is critical, restore the checks:- for (expected_name, _) in expected { + for (expected_name, expected_kind) in expected { reads .iter() - .find(|r| r.name.clone().text() == *expected_name) + .find(|r| r.name.clone().text() == *expected_name && &r.access_kind == expected_kind) .unwrap_or_else(|| { panic!( - "Case '{}' failed: expected to find read '{}', but none was found in {:#?}", + "Case '{}' failed: expected to find read '{}' with {:?}, but none was found in {:#?}", - description, expected_name, reads + description, expected_name, expected_kind, reads ) }); }Apply similar changes to
assert_writesat lines 1125-1137.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_js_analyze/src/services/semantic_class.rs(15 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/services/semantic_class.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/services/semantic_class.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Before committing, format Rust and TOML files (e.g., via
just f/just format)
Files:
crates/biome_js_analyze/src/services/semantic_class.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Document rules, assists, and options via inline rustdoc in Rust source
Files:
crates/biome_js_analyze/src/services/semantic_class.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
crates/biome_rowan/src/ast/mod.rs (1)
cast_ref(142-151)
⏰ 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). (10)
- GitHub Check: Test Node.js API
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Documentation
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
🔇 Additional comments (10)
crates/biome_js_analyze/src/services/semantic_class.rs (10)
21-25: LGTM!The
NamedClassMemberstruct is well-structured for holding a class member's name and range.
41-50: LGTM!Standard Rust idiom for providing a default constructor via
Defaulttrait.
56-61: LGTM!Clean delegation to the private extraction logic.
192-201: LGTM with a note.The union covers the expected named member types. The inline TODO on line 200 regarding
accessoris noted for future work.
204-205: LGTM!Adding
JsStaticMemberAssignmentto the union aligns with the new extraction capabilities.
213-262: LGTM!The extraction logic correctly handles each member variant, with index signatures appropriately returning
None.
845-876: LGTM!The changes correctly distinguish between writes and reads in destructuring patterns (lines 845-850) and appropriately propagate
MeaningfulReadwhen an assignment is used in an expression context (lines 862-875).
1003-1009: LGTM!Refactoring into smaller helper functions improves readability and maintainability.
1020-1085: LGTM!The three new helper functions correctly identify meaningful read contexts:
is_class_initializer_rhsdetects RHS of class property initialisers.is_assignment_expression_contextdetects RHS and LHS object destructuring.is_general_expression_contextcovers a comprehensive list of expression contexts.
1211-1637: LGTM!The test additions and modifications correctly reflect the new behaviour:
- Test cases updated to align with new read/write semantics (lines 1211-1350).
- New test case (lines 1314-1319) validates assignment-in-expression as a
MeaningfulRead.is_used_in_expression_context_testsmodule (lines 1441-1582) provides comprehensive coverage of expression contexts.extract_named_member_testsmodule (lines 1584-1637) validates extraction logic for all member variants and edge cases.
vladimir-ivanov
force-pushed
the
feat/no_unused_private_class_members_semantic_class
branch
from
f31bbc2 to
dcdad70
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/no_unused_private_class_members_amended.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/**/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/**/*.md: Create changesets using thejust new-changesetcommand; do not author them manually
In changeset markdown, only use headers #### or #####
Changeset descriptions must end every sentence with a full stop (.)
For bug fixes, start the changeset description with a linked issue reference like “Fixed #1234”
Prefer past tense for what was done and present tense for current behavior in changesets
Files:
.changeset/no_unused_private_class_members_amended.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). (11)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_formatter)
|
are we getting this reviewed anytime soon please? will be ready to do another pr soon, which will handle the dynamic this access. And then want to add additional checks to read return types of fns (e.g. parse esnext.d.ts files from typescript, and use the types in ts queries to deal with Object.clone return type (this) and others inside semantic_class. |
|
@vladimir-ivanov not sure I'll be able to review the PR soon, but have you looked at the performance degradation? |
|
thanks, in that case will merge the follow up pr into this one. handleDynamicAcees e.g. this[action] |
|
closing in favour of #7684 |
2 participants
Changed
noUnusedPrivateClassMembersto align more fully with meaningful reads.Amended slightly the meaningful reads logic to include some extra cases.
Reshuffled some previously valid and invalid cases like:
Invalid examples (previously valid)
Valid example (Previously invalid)
closes #7499