feat(biome_js_analyze): align no_unused_private_class_members_with_semantic_class and dynamic prop access #7684
Conversation
🦋 Changeset detectedLatest commit: 24d4595 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 |
CodSpeed Performance ReportMerging #7684 will improve performances by 6.37%Comparing Summary
Benchmarks breakdown
Footnotes
|
d7e33ef to
47b9b5b
Compare
WalkthroughReworks no_unused_private_class_members to be semantic-aware: introduces SemanticClassModel and NamedClassMember, swaps syntax-only traversal for semantic ClassMemberReferences and traverse_meaningful_read_members_usage so only meaningful reads count as usage (pure writes excluded), updates UnusedMemberAction/AnyMember APIs to accept semantic_class for name/range resolution, updates related lints (use_readonly_class_properties) to use named-member semantics, and adds/adjusts tests and changesets for dynamic Possibly related PRs
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)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-20T16:24:59.781ZApplied to files:
⏰ 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 |
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/lint/correctness/no_unused_private_class_members.rs (1)
807-823: Computed access misses union-typed locals
resolve_formal_param_typeonly handles formal parameters, so a union-typed local likeconst action: "add" | "remove"never registers as a meaningful read. With this change both#addand#removewould now be flagged as unused—previously the rule suppressed the warning. Please resolve the identifier’s type for any binding (variables, class fields, parameters) before extracting literals. A quick fix is to generalise the helper to acceptJsIdentifierBinding(and friends) and look up their type annotation via the semantic model. Example to repro:class Demo { #add() {} #remove() {} run() { const action: "add" | "remove" = "add"; this[action](); } }This now fires, which is a regression.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (1)
145-184: Trim the clones; one pass is plenty.We keep cloning
constructor_paramsandwritesfor every candidate, so the work balloons with class size. Let’s reuse the original vectors and iterate them by reference instead:- let constructor_params: Vec<_> = - collect_non_readonly_constructor_parameters(&members, private_only); - let non_readonly_class_property_members = - collect_non_readonly_class_member_properties(&members, private_only); - - constructor_params - .clone() - .into_iter() - .chain( - non_readonly_class_property_members.filter(|class_property_member| { - !constructor_params.clone().into_iter().any(|node| { - node.to_trimmed_text() == class_property_member.to_trimmed_text() - }) - }), - ) - .filter_map(|prop_or_param| { - if writes.clone().into_iter().any( - |ClassMemberReference { - name: class_member_ref_name, - .. - }| { - if let Some(NamedClassMember { - name: member_name, .. - }) = ctx - .semantic_class() - .extract_named_member(&prop_or_param.clone()) - { - return class_member_ref_name.eq(&member_name); - } - - false - }, - ) { - None - } else { - Some(prop_or_param.clone()) - } - }) + let constructor_params: Vec<_> = + collect_non_readonly_constructor_parameters(&members, private_only); + let constructor_param_texts: Vec<_> = constructor_params + .iter() + .map(|member| member.to_trimmed_text()) + .collect(); + let non_readonly_class_property_members = + collect_non_readonly_class_member_properties(&members, private_only); + + constructor_params + .into_iter() + .chain( + non_readonly_class_property_members.filter(|class_property_member| { + !constructor_param_texts + .iter() + .any(|text| text == &class_property_member.to_trimmed_text()) + }), + ) + .filter_map(|prop_or_param| { + let Some(NamedClassMember { + name: member_name, .. + }) = ctx.semantic_class().extract_named_member(&prop_or_param) + else { + return Some(prop_or_param); + }; + + if writes.iter().any( + |ClassMemberReference { + name: class_member_ref_name, + .. + }| class_member_ref_name == &member_name, + ) { + None + } else { + Some(prop_or_param) + } + }) </blockquote></details> <details> <summary>crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts (1)</summary><blockquote> `44-72`: **Consider adding inline comments for consistency.** The last three classes (`SampleYesNoString`, `SampleYesNoAny`, `SampleYesNoUnknown`) correctly test overly broad types (`string`, `any`, `unknown`) that should result in all private members being flagged as unused. For consistency with the earlier classes, consider adding inline comments to clarify expected behaviour, e.g., `// <- all unused`. Apply this diff to improve test clarity: ```diff export class SampleYesNoString { - private yes: () => void; - private no: () => void; - private dontKnow: () => void; + private yes: () => void; // <- unused + private no: () => void; // <- unused + private dontKnow: () => void; // <- unused on(action: string): void { this[action](); } } export class SampleYesNoAny { - private yes: () => void; - private no: () => void; - private dontKnow: () => void; + private yes: () => void; // <- unused + private no: () => void; // <- unused + private dontKnow: () => void; // <- unused on(action: any): void { this[action](); } } export class SampleYesNoUnknown { - private yes: () => void; - private no: () => void; - private dontKnow: () => void; + private yes: () => void; // <- unused + private no: () => void; // <- unused + private dontKnow: () => void; // <- unused on(action: unknown): void { this[action](); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
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**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (14)
.changeset/no_unused_private_class_members_amended.md(1 hunks).changeset/no_unused_private_class_members_dynamic_usages.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(14 hunks)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs(12 hunks)crates/biome_js_analyze/src/services/semantic_class.rs(40 hunks)crates/biome_js_analyze/tests/quick_test.rs(1 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_dynamic_access.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)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts(2 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 (6)
.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_dynamic_usages.md.changeset/no_unused_private_class_members_amended.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/quick_test.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.tscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.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_aligned_with_semantic_class.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.tscrates/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/quick_test.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.tscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.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_aligned_with_semantic_class.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.tscrates/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/quick_test.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
**/*.{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/tests/quick_test.rscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/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/tests/quick_test.rscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/biome_js_analyze/src/services/semantic_class.rs
🧠 Learnings (2)
📚 Learning: 2025-10-02T12:57:33.228Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-02T12:57:33.228Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/tests/quick_test.rs : Use tests/quick_test.rs for ad-hoc testing: un-ignore the test and set the rule filter (e.g., RuleFilter::Rule("nursery", "<ruleName>"))
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
📚 Learning: 2025-10-02T12:57:33.228Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-02T12:57:33.228Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/*.rs : Place all new JavaScript lint rules in the nursery group under biome_js_analyze/lib/src/lint/nursery/<rule_name>.rs
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
🧬 Code graph analysis (5)
crates/biome_js_analyze/tests/quick_test.rs (1)
crates/biome_service/src/file_handlers/mod.rs (1)
rule_filter(1127-1127)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts (2)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts (2)
SampleAddRemove(3-14)SampleYesNo(18-25)crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)
action(194-252)
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)
diagnostic(160-192)crates/biome_js_analyze/src/services/semantic_class.rs (3)
new(51-53)new(94-96)new(425-430)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)
crates/biome_js_analyze/src/services/semantic_class.rs (6)
semantic_class(36-38)class_member_references(55-57)class_member_references(280-328)new(51-53)new(94-96)new(425-430)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts (1)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts (2)
SampleAddRemove(15-29)SampleYesNo(34-42)
⏰ 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). (7)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
🔇 Additional comments (3)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts (3)
1-13: Good baseline test case for untyped parameter.The
Sampleclass properly demonstrates the scenario where an untyped parameter inmethod(name)should be considered too broad to validate usage ofmemberor#propthroughthis[name]. Both members should be flagged as unused, aligning with the PR objective to avoid false negatives when the key type is untyped.
15-29: Excellent coverage of string literal union constraint.The
SampleAddRemoveclass correctly demonstrates that a constrained union type"add" | "remove"should only validate usage of the matching private members, leavingappendproperly flagged as unused. This aligns precisely with the PR's goal to recognise only string literal unions as valid usages.
31-42: Well-crafted test for mixed union type filtering.The
YesNotype definition andSampleYesNoclass nicely demonstrate that only string literals from a union should be extracted, ignoring the object type{ ignored: number }. This ensuresyesandnoare considered used whilstdontKnowremains flagged as unused, as intended by the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
1205-1217: Consider adding error context for type resolution failures.The function
resolve_formal_param_typereturnsNonewhen it cannot resolve a parameter's type, but this could be due to several reasons (no binding, not a formal parameter, no type annotation). For debugging and telemetry, consider logging or tracking why resolution failed.This is optional, as the current behaviour (silent fallback) is acceptable for production use.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/biome_cli/tests/snapshots/main_commands_check/does_error_with_only_warnings.snap.new(1 hunks)crates/biome_cli/tests/snapshots/main_commands_check/should_pass_if_there_are_only_warnings.snap.new(1 hunks)crates/biome_cli/tests/snapshots/main_commands_ci/does_error_with_only_warnings.snap.new(1 hunks)crates/biome_cli/tests/snapshots/main_commands_lint/does_error_with_only_warnings.snap.new(1 hunks)crates/biome_cli/tests/snapshots/main_commands_lint/lint_syntax_rules.snap.new(1 hunks)crates/biome_cli/tests/snapshots/main_commands_lint/should_pass_if_there_are_only_warnings.snap.new(1 hunks)crates/biome_js_analyze/src/services/semantic_class.rs(40 hunks)
✅ Files skipped from review due to trivial changes (4)
- crates/biome_cli/tests/snapshots/main_commands_ci/does_error_with_only_warnings.snap.new
- crates/biome_cli/tests/snapshots/main_commands_lint/should_pass_if_there_are_only_warnings.snap.new
- crates/biome_cli/tests/snapshots/main_commands_check/does_error_with_only_warnings.snap.new
- crates/biome_cli/tests/snapshots/main_commands_check/should_pass_if_there_are_only_warnings.snap.new
🧰 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/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.rscrates/biome_cli/tests/snapshots/main_commands_lint/does_error_with_only_warnings.snap.newcrates/biome_cli/tests/snapshots/main_commands_lint/lint_syntax_rules.snap.new
**/*.{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
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_cli/tests/snapshots/main_commands_lint/does_error_with_only_warnings.snap.newcrates/biome_cli/tests/snapshots/main_commands_lint/lint_syntax_rules.snap.new
⏰ 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). (9)
- GitHub Check: Test Node.js API
- GitHub Check: Documentation
- GitHub Check: autofix
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (4)
crates/biome_cli/tests/snapshots/main_commands_lint/does_error_with_only_warnings.snap.new (1)
42-80: Confirm duplicate noClassAssign warnings
Please verify if the two identicalnoClassAssignwarning blocks in crates/biome_cli/tests/snapshots/main_commands_lint/does_error_with_only_warnings.snap.new are intentional.crates/biome_js_analyze/src/services/semantic_class.rs (3)
1731-1784: Good test coverage for extract_named_member.The tests verify method, property, getter, setter, and index signature extraction. This provides solid coverage for the new named-member extraction API.
1260-1263: Fix duplicate text in panic message."but none was found" appears twice.
Apply this diff:
panic!( - "Case '{}' failed: expected to find read '{}', but none was found in {:#?}", + "Case '{}' failed: expected to find read '{}' in {:#?}", description, expected_name, reads )Likely an incorrect or invalid review comment.
815-849: Add missing test coverage for dynamic member type-narrowing edge casesCurrent tests only cover simple string-literal unions. Please add specs for:
- type guards or
asassertions- template literal types expanding to specific strings
- enums that compile to string unions
crates/biome_cli/tests/snapshots/main_commands_lint/lint_syntax_rules.snap.new
Outdated
Show resolved
Hide resolved
01e5bc1 to
16c66aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/biome_js_analyze/src/services/semantic_class.rs (2)
1186-1204: Refactor: extract literal string trimming to helper.Lines 1195-1198 contain
trim_matches(&['"', '\''][..])logic that could be extracted to a small helper for clarity and reuse.Apply this diff:
+/// Trims quotes from a string literal type's text representation +fn trim_literal_quotes(text: &str) -> &str { + text.trim_matches(&['"', '\''][..]) +} + fn extract_literal_types(union: &TsUnionType) -> Vec<Text> { extract_shallow_union_members(union) .iter() .filter_map(|item| { if let Some(literal_type) = TsStringLiteralType::cast(item.syntax().clone()) { return Some(Text::new_owned(Box::from( - literal_type - .to_trimmed_text() - .trim_matches(&['"', '\''][..]), + trim_literal_quotes(&literal_type.to_trimmed_text()), ))); } None }) .collect() }
824-860: extract_literal_types skips nested unions
As documented, nested union members are ignored—consider adding recursive extraction or clearly documenting this limitation.crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)
98-106: Refactor: consider if struct variant adds value.
RemoveMemberchanged fromRemoveMember(AnyMember)toRemoveMember { member: AnyMember }. Does the named field improve clarity for this single-field variant?If the struct variant doesn't provide meaningful documentation benefit, consider reverting to the tuple variant for brevity:
pub enum UnusedMemberAction { - RemoveMember { - member: AnyMember, - }, + RemoveMember(AnyMember), RemovePrivateModifier { member: AnyMember, rename_with_underscore: bool, }, }Adjust the pattern matches accordingly:
- Self::RemoveMember { member } => member.member_range(semantic_class), + Self::RemoveMember(member) => member.member_range(semantic_class),crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (1)
161-177: Consider simplifying the nested filter logic.The logic is correct but quite verbose with multiple
.clone()calls. Whilst cloning might be necessary for ownership, extracting the inner closure into a helper function could improve readability.Example refactor:
fn has_write_reference( prop_or_param: &AnyNamedClassMember, writes: &[ClassMemberReference], semantic_class: &SemanticClassModel, ) -> bool { writes.iter().any(|ClassMemberReference { name: class_member_ref_name, .. }| { semantic_class .extract_named_member(prop_or_param) .is_some_and(|NamedClassMember { name: member_name, .. }| { class_member_ref_name.eq(&member_name) }) }) }Then use:
.filter_map(|prop_or_param| { if has_write_reference(&prop_or_param, &writes, ctx.semantic_class()) { None } else { Some(prop_or_param) } })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
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**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (15)
.changeset/no_unused_private_class_members_amended.md(1 hunks).changeset/no_unused_private_class_members_dynamic_usages.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(14 hunks)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs(12 hunks)crates/biome_js_analyze/src/services/semantic_class.rs(41 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_dynamic_access.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)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts(2 hunks)crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid.ts.snap.new(1 hunks)crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.new(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
✅ Files skipped from review due to trivial changes (1)
- .changeset/no_unused_private_class_members_amended.md
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.js
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
🧰 Additional context used
📓 Path-based instructions (6)
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/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.tscrates/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/style/useReadonlyClassProperties/invalid.ts.snap.new
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.tscrates/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/style/useReadonlyClassProperties/invalid.ts.snap.new
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.tscrates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid.ts.snap.new
**/*.{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/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
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Document rules, assists, and options via inline rustdoc in Rust source
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
.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_dynamic_usages.md
🧠 Learnings (1)
📚 Learning: 2025-10-02T12:57:33.228Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-02T12:57:33.228Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/*.rs : Place all new JavaScript lint rules in the nursery group under biome_js_analyze/lib/src/lint/nursery/<rule_name>.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/lint/style/use_readonly_class_properties.rs (1)
crates/biome_js_analyze/src/services/semantic_class.rs (4)
semantic_class(35-37)new(51-53)new(101-103)new(440-445)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
crates/biome_js_analyze/src/services/semantic_class.rs (6)
semantic_class(35-37)class_member_references(55-57)class_member_references(293-341)new(51-53)new(101-103)new(440-445)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (3)
member(222-228)ctx(133-133)diagnostic(187-205)
⏰ 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 (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: End-to-end tests
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: autofix
🔇 Additional comments (14)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts (1)
10-10: LGTM!Typo corrected—
unusedPropertynow matches the test's intent.crates/biome_js_analyze/src/services/semantic_class.rs (3)
22-26: LGTM!
NamedClassMembercleanly encapsulates member name and range for semantic-aware analysis.
44-53: Verify cache invalidation strategy.
SemanticClassModelintroduces aRefCell<FxHashMap>cache forNamedClassMemberlookups. Confirm that the cache doesn't retain stale entries if the AST is mutated during analysis phases.Based on learnings
1216-1228: LGTM!
resolve_formal_param_typecleanly navigates from identifier → binding → formal parameter → type annotation.crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
255-282: LGTM!
traverse_meaningful_read_members_usagecorrectly filters out members with meaningful reads, returning only truly unused members.
400-418: Verify semantic_class cache hit rate.Both
member_rangeandmatches_namecallsemantic_class.extract_named_member(), which hits the cache. Confirm the cache is effective during typical analysis runs.Based on learnings
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts (1)
22-29: LGTM!This test correctly validates that a setter-only accessor (without getter) in destructuring is not a meaningful read.
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid.ts.snap.new (1)
1-356: LGTM!Snapshot correctly documents diagnostics for the updated
useReadonlyClassPropertiesrule using semantic-aware member extraction..changeset/no_unused_private_class_members_dynamic_usages.md (1)
15-29: Manually verifydontKnowis reported as unused with dynamic union access. Confirm Biome’s new semantic analysis correctly excludesdontKnowwhen callingthis[action]()foraction: YesNo.crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.new (1)
1-279: LGTM! Comprehensive test snapshot.The snapshot correctly validates the rule's behaviour with
checkAllProperties: true, covering property fields, constructor parameters, and various visibility modifiers. Diagnostics and fixes are consistent with the semantic-class-based implementation.crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (4)
2-5: State type migration looks good.The shift from
AnyPropertyMembertoAnyNamedClassMemberaligns with the semantic-class integration. The new imports support semantic-aware member extraction and reference tracking.Also applies to: 128-128
187-205: Diagnostic function correctly uses semantic class.The signature change from
_toctxis appropriate, and usingsemantic_class.extract_named_memberaligns with the new semantic-aware approach. ReturningNonewhen extraction fails is sensible (no extractable name means no actionable diagnostic).
207-283: Action function handles both member types correctly.The pattern matching on
AnyNamedClassMembervariants (lines 217-218 and 252-253) correctly handles both class property members and constructor parameters. The cloning oforiginal_nodeis appropriate to avoid ownership issues.
285-393: Helper functions correctly updated for type migration.The collection functions (
collect_non_readonly_class_member_propertiesandcollect_non_readonly_constructor_parameters) now consistently returnAnyNamedClassMember, maintaining type safety throughout the refactor.
...e/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
Show resolved
Hide resolved
f146954 to
d951e0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
229-239: Typo in comment (already noted in past reviews).Line 238: "claas" should be "class". This was flagged previously and marked as addressed, but appears to still be present in the code.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
1198-1255: Add tests for union type edge casesCurrent specs only cover simple literal unions (e.g.
YesNo). Add tests for:
- generic type parameters involving unions
- intersections containing unions (e.g.
A & (B | C))- conditional types with union branches
- mapped types over union keys
Place new cases under
crates/biome_js_analyze/tests/specs/correctness.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
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**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (15)
.changeset/no_unused_private_class_members_amended.md(1 hunks).changeset/no_unused_private_class_members_dynamic_usages.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(14 hunks)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs(12 hunks)crates/biome_js_analyze/src/services/semantic_class.rs(40 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_dynamic_access.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)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts(2 hunks)crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid.ts.snap.new(1 hunks)crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.new(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/valid.js
✅ Files skipped from review due to trivial changes (1)
- .changeset/no_unused_private_class_members_dynamic_usages.md
🚧 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/invalid_aligned_with_semantic_class.ts
- crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid.ts.snap.new
🧰 Additional context used
📓 Path-based instructions (6)
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_aligned_with_semantic_class.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.tscrates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.tscrates/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_issue_7101.tscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/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/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.tscrates/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_issue_7101.tscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.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_aligned_with_semantic_class.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.tscrates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.newcrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts
**/*.{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.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.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.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
.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
🧠 Learnings (1)
📚 Learning: 2025-10-02T12:57:33.228Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-02T12:57:33.228Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/*.rs : Place all new JavaScript lint rules in the nursery group under biome_js_analyze/lib/src/lint/nursery/<rule_name>.rs
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
🧬 Code graph analysis (4)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts (1)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts (2)
SampleAddRemove(15-29)SampleYesNo(34-42)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
crates/biome_js_analyze/src/services/semantic_class.rs (6)
semantic_class(38-40)class_member_references(61-63)class_member_references(301-349)new(54-59)new(109-111)new(452-457)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (3)
member(222-228)ctx(133-133)diagnostic(187-205)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts (1)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts (1)
foo(36-39)
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (1)
crates/biome_js_analyze/src/services/semantic_class.rs (4)
semantic_class(38-40)new(54-59)new(109-111)new(452-457)
⏰ 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: Documentation
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (8)
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts (1)
3-25: Good call on literal unionsNice to see the rule exercised with a narrowed union so we stop hand-waving every computed access. Keep them coming.
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (2)
187-205: Good: Diagnostic now uses semantic context.The diagnostic correctly derives member names via
semantic_class.extract_named_member(), handling cases where extraction fails by returningNone. This is more robust than the previous approach.
160-182: Semantic-aware filtering logic is correct.extract_named_membersafely returnsNonefor index signatures and thefilter_maphandles this case as intended.crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
258-282: Good: Semantic‑aware filtering improves accuracy.The function now filters based on
AccessKind::MeaningfulRead, which correctly distinguishes between writes, meaningful reads, and trivial reads. This aligns well with the PR objectives to reduce false negatives.
400-418: TsPropertyParameter is handled by extract_named_member
Theextract_named_membermatch includes an arm forAnyNamedClassMember::TsPropertyParameter, so this variant is covered.crates/biome_js_analyze/src/services/semantic_class.rs (3)
48-80: Good: Caching improves performance.The
SemanticClassModelnow caches named member extraction results, which is essential given thatextract_named_memberis called repeatedly during analysis. The use ofRefCellallows interior mutability without requiring&mut self.
1100-1196: Good: Comprehensive meaningful read detection.The helper functions
is_class_initializer_rhs,is_assignment_expression_context, andis_general_expression_contextprovide a robust way to determine whether a read is meaningful. This correctly handles:
- Class property initialisers
- Assignment RHS
- Control flow contexts (if, switch, for)
- Expression contexts (return, call args, etc.)
1752-1805: Good: Comprehensive test coverage for extract_named_member.The tests verify name extraction for methods, properties, getters, setters, and correctly return
Nonefor index signatures. This provides confidence in the core extraction logic.
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: 1
📜 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(40 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(9264-9264)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: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
🔇 Additional comments (29)
crates/biome_js_analyze/src/services/semantic_class.rs (29)
25-29: LGTM!Clean struct definition for named class members.
31-45: LGTM!Service struct properly exposes semantic data.
47-80: LGTM!Caching logic is sound.
RefCellis appropriate here as Biome's analyzer runs single-threaded within each file analysis.
82-96: LGTM!Service initialization follows the established pattern.
104-130: LGTM!Visitor wiring correctly propagates semantic data into the service bag.
132-152: LGTM!Root discovery logic correctly handles the first class declaration.
173-180: LGTM!Visitor construction properly initialized with root.
229-239: LGTM!Union covers all named class member variants comprehensively.
249-292: LGTM!Member name extraction handles all variants correctly, including the special case for index signatures.
301-349: LGTM!Semantic model correctly propagated through all reference collection paths.
351-506: LGTM!This-aliasing logic correctly handles scope inheritance, nested functions, and arrow functions. The skipping of nested classes prevents false positives.
508-543: LGTM!Alias resolution correctly validates scope boundaries before confirming a reference.
682-702: LGTM!Reference collection correctly initialised with semantic context.
710-759: LGTM!Match statement improves readability over cascading if-let chains.
776-805: LGTM!Boolean return value correctly signals whether the node was handled.
823-843: LGTM!Handler return semantics are consistent with the refactored pattern.
845-881: Past concern: narrowed literal casts.A previous review requested test coverage for narrowed literal casts via
as(e.g.,this[action as "add"]). The current implementation attempts to resolve the type but may not handleascasts explicitly.Generate a test case to confirm narrowed casts are recognised:
class Example { #add() {} #remove() {} method(action: "add" | "remove") { // Should recognise #add as used via narrowing this[action as "add"](); } }Verify the dynamic access resolution correctly identifies
#addas read when the type is narrowed viaas.
1039-1059: LGTM!Constructor reference collection correctly incorporates semantic context.
1067-1085: LGTM!Static member read collection properly categorised by access kind.
1088-1107: LGTM!Scope validation correctly handles shadowing by stopping at lexical boundaries.
1118-1129: LGTM!Expression context detection comprehensively checks multiple ancestor contexts.
1131-1206: LGTM!Helper predicates comprehensively identify meaningful read contexts. The logic correctly distinguishes class initialisers, assignment contexts, and general expression usage.
1207-1265: LGTM!Type resolution utilities correctly extract string literal types from unions and resolve type aliases. The shallow union extraction aligns with the PR objective to handle string-literal unions without over-matching.
1266-1326: LGTM!Test infrastructure is well structured and provides clear assertion failures.
1337-1456: LGTM!Test coverage for object destructuring and static member access is thorough.
1458-1528: LGTM!Assignment expression tests comprehensively cover compound assignments and destructuring patterns, including the return expression case.
1530-1616: LGTM!Update expression tests verify correct read/write detection across various contexts.
1618-1759: LGTM!Expression context tests are extensive and verify meaningful read detection across a wide range of language constructs.
1761-1814: LGTM!Named member extraction tests verify correct behaviour for all member types, including the special index signature case.
dyc3
left a comment
There was a problem hiding this comment.
By no means an in depth review, but at a glance it looks good. 👍
| - A *meaningful read* is any access that affects program behavior. | ||
| - For example, `this.#x += 1` both reads and writes `#x`, so it counts as usage. | ||
| - Pure writes without a read (e.g. `this.#x = 1` with no getter) are no longer treated as usage. | ||
|
|
||
| This change ensures that private members are only considered “used” when they are actually read in a way that influences execution. | ||
|
|
||
| ***Invalid examples (previously valid)*** | ||
|
|
||
| ```ts | ||
| class UsedMember { | ||
| set #x(value) { | ||
| doSomething(value); | ||
| } | ||
|
|
||
| foo() { | ||
| // This assignment does not actually read #x, because there is no getter. | ||
| // Previously, this was considered a usage, but now it’s correctly flagged. | ||
| this.#x = 1; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ***Valid example (Previously invalid)*** | ||
|
|
||
| ```js | ||
| class Foo { | ||
| #usedOnlyInWriteStatement = 5; | ||
|
|
||
| method() { | ||
| // This counts as a meaningful read because we both read and write the value. | ||
| this.#usedOnlyInWriteStatement += 42; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ***Summary*** | ||
| • Only accesses that read a value are considered meaningful for the purpose of this rule. | ||
| • Simple assignments to a setter without a corresponding getter no longer count as usage. | ||
| • Operations like +=, method calls returning a value, or reading the property for computation are considered meaningful reads. |
There was a problem hiding this comment.
nit: I feel like this is just a little too verbose for what users actually care about. Maybe others feel differently.
needs a lot of work still, but thanks, experimenting with perf optimisation. Wont be ready for review until I make it green. |
416d1ae to
fba354c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
887-896: Missing compound assignment operators.The compound assignment detection at lines 887-896 only covers:
||=,&&=,/=,*=,%=,+=,??=It's missing:
-=(MINUSEQ)**=(exponentiation assignment)<<=,>>=,>>>=(bitwise shift assignments)&=,|=,^=(bitwise assignments)All compound assignments read the old value before writing, so they should all trigger meaningful reads.
As per the past review comment from dyc3, extend the
matches!to include all compound operators:&& matches!( operator.kind(), JsSyntaxKind::PIPE2EQ | JsSyntaxKind::AMP2EQ | JsSyntaxKind::SLASHEQ | JsSyntaxKind::STAREQ | JsSyntaxKind::PERCENTEQ | JsSyntaxKind::PLUSEQ | JsSyntaxKind::QUESTION2EQ + | JsSyntaxKind::MINUSEQ // -= + | JsSyntaxKind::STARSTAREQ // **= (check exact name) + | JsSyntaxKind::LT2EQ // <<= + | JsSyntaxKind::GT2EQ // >>= + | JsSyntaxKind::GT3EQ // >>>= + | JsSyntaxKind::AMPEQ // &= + | JsSyntaxKind::PIPEEQ // |= + | JsSyntaxKind::CARETEQ // ^= )Note: Verify the exact
JsSyntaxKindvariant names for**=and the bitwise operators in your codebase.
📜 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(40 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)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
TextRange(9264-9264)
⏰ 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: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: End-to-end tests
- GitHub Check: Check Dependencies
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_formatter)
🔇 Additional comments (7)
crates/biome_js_analyze/src/services/semantic_class.rs (7)
25-29: Well-defined struct for named members.The
NamedClassMemberstructure clearly separates the name from its source range, which is essential for accurate diagnostics.
47-80: Caching strategy looks sound.Using
RefCell<FxHashMap<SyntaxNodePtr, Option<NamedClassMember>>>for caching named members is appropriate for interior mutability in a query-based system. Theextract_named_membermethod correctly checks the cache before computing.
223-233: Clear documentation with examples.The inline comments for each variant of
AnyNamedClassMemberare helpful. Note: Line 232 still contains the typo "claas" → should be "class" (though marked as addressed in a previous commit).
243-286:extract_named_memberhandles all member types correctly.The function properly returns
Nonefor index signatures (which lack a traditional name) and extracts names for all other member types. The use ofto_namedhelper reduces duplication.
1091-1165: Expression context helpers are comprehensive.The
is_class_initializer_rhs,is_assignment_expression_context, andis_general_expression_contextfunctions cover a wide range of cases where a read is meaningful. The logic correctly distinguishes between RHS (reads) and LHS contexts.
1167-1224: Type resolution logic is well-structured.The functions
extract_literal_types,resolve_formal_param_type, andresolve_reference_to_unionproperly handle:
- Extracting string literals from union types
- Resolving formal parameter type annotations
- Following type aliases to their underlying unions
This aligns with the PR objective to handle string-literal unions.
1226-1774: Comprehensive test coverage.The test suite covers:
- Object binding patterns
- Static member expressions
- Assignment expressions (including compound assignments)
- Pre/post update expressions
- Expression context detection
- Named member extraction
Good use of
TestCasestructs and helper functions.
|
big win, no more perf issues, interesting how small optimisations make big difference. |
…ndle dynamic access class members based on their ts type
…ndle dynamic access class members based on their ts type
…ndle dynamic access class members based on their ts type
59dfee8 to
2c83ce7
Compare
ematipico
left a comment
There was a problem hiding this comment.
Great work. @vladimir-ivanov
…ndle dynamic access class members based on their ts type
0c57f9c to
e5f2c63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
879-888: Address the missing compound assignment operators.As noted in the previous review, this matches clause is still missing several compound assignment operators (
-=,**=,<<=,>>=,>>>=,&=,|=,^=). All compound assignments should read the old value before writing.Apply this diff to include all compound operators:
&& matches!( operator.kind(), JsSyntaxKind::PIPE2EQ | JsSyntaxKind::AMP2EQ | JsSyntaxKind::SLASHEQ | JsSyntaxKind::STAREQ | JsSyntaxKind::PERCENTEQ | JsSyntaxKind::PLUSEQ + | JsSyntaxKind::MINUSEQ + | JsSyntaxKind::STAR2EQ + | JsSyntaxKind::LT2EQ + | JsSyntaxKind::RT2EQ + | JsSyntaxKind::RT3EQ + | JsSyntaxKind::AMPEQ + | JsSyntaxKind::PIPEEQ + | JsSyntaxKind::CARETEQ | JsSyntaxKind::QUESTION2EQ )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/suspicious/no_class_assign.rs(1 hunks)crates/biome_js_analyze/src/services/semantic_class.rs(39 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: arendjr
Repo: biomejs/biome PR: 7593
File: crates/biome_service/src/workspace/server.rs:1306-1306
Timestamp: 2025-09-25T12:32:59.003Z
Learning: In the biomejs/biome project, do not flag compilation errors during code review as they are handled by the existing test infrastructure and CI. Focus on other code quality aspects instead.
📚 Learning: 2025-08-20T16:24:59.781Z
Learnt from: arendjr
Repo: biomejs/biome PR: 7266
File: crates/biome_js_type_info/src/type.rs:94-102
Timestamp: 2025-08-20T16:24:59.781Z
Learning: In crates/biome_js_type_info/src/type.rs, the flattened_union_variants() method returns TypeReference instances that already have the correct module IDs applied to them. These references should be used directly with resolver.resolve_reference() without applying additional module ID transformations, as variant references may originate from nested unions in different modules.
Applied to files:
crates/biome_js_analyze/src/services/semantic_class.rs
⏰ 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: Documentation
- GitHub Check: End-to-end tests
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
🔇 Additional comments (7)
crates/biome_js_analyze/src/services/semantic_class.rs (7)
22-42: Well-structured public API.The
NamedClassMemberstruct andSemanticClassServicesupdates provide a clean interface for semantic-aware class member analysis.
816-848: Dynamic member handling correctly implements PR objectives.This function properly resolves formal parameter types to string-literal unions and records reads only for resolved literals, which addresses the main goal of avoiding false positives with broad types like
stringorany.
101-140: Solid semantic model integration.The visitor correctly wires up the semantic event extraction and model building, enabling semantic-aware analysis throughout the class member reference collection.
44-77: Efficient caching strategy.The
RefCell<FxHashMap>cache for named members is a sensible optimisation, avoiding repeated AST traversal for member name extraction.
1159-1216: Type resolution logic looks correct.The functions properly extract string literals from union types and resolve type aliases, which enables precise tracking of dynamic member access.
1061-1157: Comprehensive expression context detection.These helpers correctly distinguish meaningful reads (used in logic/computation) from trivial ones (standalone statements), which is crucial for the rule's accuracy.
1218-1767: Thorough test coverage.The test suite comprehensively verifies read/write detection, this-aliasing, and the distinction between meaningful and trivial reads across various contexts.
…ndle dynamic access class members based on their ts type
244a10e to
e7ea8c4
Compare
Co-authored-by: Emanuele Stoppa <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/no_unused_private_class_members_amended.md (1)
1-42: Consider condensing examples per prior feedback on verbosity.Previous reviewers flagged that the changelog was too verbose and suggested removing detailed examples. The current entry still includes two full code blocks and expanded explanations. Whilst the content is accurate and technically sound, consider whether end users need this level of detail in the changelog, or whether a shorter summary with a link to migration documentation would better serve the audience.
A condensed version might look like:
- One-line summary of the change
- Bullet point of key behaviour shifts
- Link to updated documentation for migration guidance
This would align with the principle that changelogs should highlight what changed, whilst detailed examples belong in guides or rule documentation.
📜 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
🧠 Learnings (1)
📓 Common learnings
Learnt from: arendjr
Repo: biomejs/biome PR: 7593
File: crates/biome_service/src/workspace/server.rs:1306-1306
Timestamp: 2025-09-25T12:32:59.003Z
Learning: In the biomejs/biome project, do not flag compilation errors during code review as they are handled by the existing test infrastructure and CI. Focus on other code quality aspects instead.
⏰ 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: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: End-to-end tests
- 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: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: Validate rules documentation
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: autofix
…mantic_class and dynamic prop access (biomejs#7684) Co-authored-by: Emanuele Stoppa <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Summary
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 #7682
closes #7499