feat(biome_js_analyze): align no_unused_private_class_members_with_semantic_class and dynamic prop access #7684
Conversation
🦋 Changeset detectedLatest commit: b446f6c 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
CodSpeed Performance ReportMerging #7684 will degrade performances by 9.92%Comparing Summary
Benchmarks breakdown
Footnotes
|
vladimir-ivanov
force-pushed
the
feat/no_unused_private_class_members_dynamic_prop_access
branch
from
d7e33ef to
47b9b5b
Compare
WalkthroughRefactors no_unused_private_class_members and use_readonly_class_properties to be semantic-aware by introducing SemanticClassModel, NamedClassMember/AnyNamedClassMember and ClassMemberReferences. Changes detection to count only "meaningful reads" (reads that affect program behaviour; compound ops like += count; pure writes without a getter do not). Updates rule Query types, UnusedMemberAction variant, AnyMember/AnyNamedClassMember APIs and many signatures to accept semantic_class/semantic. Rewires visitors/services to populate semantic data, updates diagnostics, tests and changesets to reflect the new semantics-aware behaviour. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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.
vladimir-ivanov
changed the title
vladimir-ivanov
changed the title
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
vladimir-ivanov
force-pushed
the
feat/no_unused_private_class_members_dynamic_prop_access
branch
3 times, most recently
from
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
Outdated
Show resolved
Hide resolved
vladimir-ivanov
force-pushed
the
feat/no_unused_private_class_members_dynamic_prop_access
branch
2 times, most recently
from
dd34d1f to
f146954
Compare
…ndle dynamic access class members based on their ts type
vladimir-ivanov
force-pushed
the
feat/no_unused_private_class_members_dynamic_prop_access
branch
from
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.
| RemoveMember { | ||
| member: AnyMember, | ||
| }, | ||
| RemovePrivateModifier { | ||
| member: AnyMember, | ||
| rename_with_underscore: bool, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Breaking change: enum variant signature altered.
The RemoveMember variant changed from a tuple to a struct, which is a breaking change for any code pattern‑matching on this enum. As this is likely internal‑only, ensure no external consumers are affected.
Based on coding guidelines
🤖 Prompt for AI Agents
In
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
around lines 99 to 106, the enum variant RemoveMember was changed from a tuple
variant to a struct variant which is a breaking change for existing pattern
matches; either restore the original tuple form (RemoveMember(AnyMember)) to
preserve API/behavior or update every internal match site to use the new struct
pattern (RemoveMember { member }) and run a compile pass to update all usages;
choose restore-if-public-or-update-all-usages-if-internal, and ensure cargo
build/test passes after the change.
| /// we assume that any usage in an expression context is meaningful read, and writes are much less likely | ||
| /// so skip the dynamic writes | ||
| fn handle_dynamic_member_expression( | ||
| node: &SyntaxNode<JsLanguage>, | ||
| scoped_this_references: &[FunctionThisAliases], | ||
| semantic: &SemanticModel, | ||
| reads: &mut FxHashSet<ClassMemberReference>, | ||
| ) -> bool { | ||
| if let Some(dynamic_member) = AnyJsComputedMember::cast(node.clone()) | ||
| && let Ok(object) = dynamic_member.object() | ||
| && is_this_reference(&object, scoped_this_references) | ||
| && let Ok(member_expr) = dynamic_member.member() | ||
| && let Some(id_expr) = JsIdentifierExpression::cast_ref(member_expr.syntax()) | ||
| && let Some(ty) = resolve_formal_param_type(semantic, &id_expr) | ||
| && let Some(ts_union_type) = TsUnionType::cast(ty.syntax().clone()) | ||
| .or_else(|| resolve_reference_to_union(semantic, &ty)) | ||
| { | ||
| let items: Vec<_> = extract_literal_types(&ts_union_type); | ||
|
|
||
| for item in items.iter() { | ||
| reads.insert(ClassMemberReference { | ||
| // we keep the range of the dynamic accessed member | ||
| range: member_expr.range(), | ||
| // swap the name for the actual resolved type | ||
| name: item.clone(), | ||
|
|
||
| access_kind: get_read_access_kind(&AnyCandidateForUsedInExpressionNode::from( | ||
| dynamic_member.clone(), | ||
| )), | ||
| }); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify type resolution for dynamic member access.
The handle_dynamic_member_expression function resolves union types to extract string literals for dynamic property access. This is a key feature for the PR objectives. Ensure:
- Non‑string‑literal unions are correctly ignored
- Type resolution handles aliased types
- Narrowed types (e.g.,
as "add") are recognised
Run the following script to verify dynamic access handling in tests:
🏁 Script executed:
#!/bin/bash
# Find test cases for dynamic property access
rg -nP --type=ts 'this\[.*\]' \
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/Length of output: 984
🏁 Script executed:
#!/bin/bash
# Inspect type definitions in dynamic access tests to confirm coverage of required cases
sed -n '1,200p' crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
sed -n '1,200p' crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.tsLength of output: 1634
🏁 Script executed:
#!/bin/bash
# Check for narrowed literal casts in tests
rg -nP "as\s*['\"][^'\"]+['\"]" -n crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/Length of output: 0
Add a test for narrowed literal casts via as
Existing specs cover union literals and type aliases but none exercise this[action as "add"]. Add a case in noUnusedPrivateClassMembers to confirm narrowed casts are recognised.
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/services/semantic_class.rs around lines 836 to
872, the review asks for a new unit test to cover narrowed literal casts via
`as` (e.g., this[action as "add"]) so the dynamic member handling logic
correctly recognises narrowed union literal types; add a test case in the
noUnusedPrivateClassMembers spec that includes a class using this[action as
"add"] (and similar narrowed-cast patterns) so the analyzer reports the member
as read (not unused), asserting the expected diagnostics/absence thereof; ensure
the test constructs a union literal type, performs the `as` narrowing on the
computed member access, and verifies reads are collected for the resolved
literal name.
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.
| fn handle_assignment_expression( | ||
| node: &SyntaxNode<JsLanguage>, | ||
| scoped_this_references: &[FunctionThisReferences], | ||
| scoped_this_references: &[FunctionThisAliases], | ||
| reads: &mut FxHashSet<ClassMemberReference>, | ||
| writes: &mut FxHashSet<ClassMemberReference>, | ||
| ) { | ||
| if let Some(assignment) = JsAssignmentExpression::cast_ref(node) | ||
| && let Ok(left) = assignment.left() | ||
| ) -> bool { | ||
| let assignment = match JsAssignmentExpression::cast_ref(node) { | ||
| Some(assignment) => assignment, | ||
| None => return false, | ||
| }; | ||
| let left = match assignment.left() { | ||
| Ok(left) => left, | ||
| Err(_) => return false, | ||
| }; | ||
| let operator = match assignment.operator_token() { | ||
| Ok(operator) => operator, | ||
| Err(_) => return false, | ||
| }; | ||
|
|
||
| // Shorthand: store `as_any_js_assignment` once | ||
| let any_assignment = left.as_any_js_assignment(); | ||
| let mut is_handled = false; | ||
|
|
||
| // Compound assignment -> meaningful read | ||
| if let Some(operand) = any_assignment | ||
| && matches!( | ||
| operator.kind(), | ||
| JsSyntaxKind::PIPE2EQ | ||
| | JsSyntaxKind::AMP2EQ | ||
| | JsSyntaxKind::SLASHEQ | ||
| | JsSyntaxKind::STAREQ | ||
| | JsSyntaxKind::PERCENTEQ | ||
| | JsSyntaxKind::PLUSEQ | ||
| | JsSyntaxKind::QUESTION2EQ | ||
| ) | ||
| && let Some(name) = ThisPatternResolver::extract_this_member_reference( | ||
| operand.as_js_static_member_assignment(), | ||
| scoped_this_references, | ||
| AccessKind::MeaningfulRead, | ||
| ) | ||
| { | ||
| if let Ok(operator) = assignment.operator_token() | ||
| && let Some(operand) = left.as_any_js_assignment() | ||
| && matches!( | ||
| operator.kind(), | ||
| JsSyntaxKind::PIPE2EQ | ||
| | JsSyntaxKind::AMP2EQ | ||
| | JsSyntaxKind::SLASHEQ | ||
| | JsSyntaxKind::STAREQ | ||
| | JsSyntaxKind::PERCENTEQ | ||
| | JsSyntaxKind::PLUSEQ | ||
| | JsSyntaxKind::QUESTION2EQ | ||
| ) | ||
| && let Some(name) = ThisPatternResolver::extract_this_member_reference( | ||
| operand.as_js_static_member_assignment(), | ||
| scoped_this_references, | ||
| AccessKind::MeaningfulRead, | ||
| ) | ||
| reads.insert(name); | ||
| is_handled = true; | ||
| } | ||
|
|
||
| // Array assignment pattern | ||
| if let Some(array) = left.as_js_array_assignment_pattern() { | ||
| for class_member_reference in | ||
| ThisPatternResolver::collect_array_assignment_names(array, scoped_this_references) | ||
| { | ||
| reads.insert(name.clone()); | ||
| writes.insert(class_member_reference); | ||
| } | ||
| is_handled = true; | ||
| } | ||
|
|
||
| if let Some(array) = left.as_js_array_assignment_pattern().cloned() { | ||
| for class_member_reference in | ||
| ThisPatternResolver::collect_array_assignment_names(&array, scoped_this_references) | ||
| { | ||
| writes.insert(class_member_reference.clone()); | ||
| } | ||
| // Object assignment pattern | ||
| if let Some(object) = left.as_js_object_assignment_pattern() { | ||
| for class_member_reference in | ||
| ThisPatternResolver::collect_object_assignment_names(object, scoped_this_references) | ||
| { | ||
| match class_member_reference.access_kind { | ||
| AccessKind::Write => writes.insert(class_member_reference), | ||
| _ => reads.insert(class_member_reference), | ||
| }; | ||
| } | ||
| is_handled = true; | ||
| } | ||
|
|
||
| if let Some(object) = left.as_js_object_assignment_pattern().cloned() { | ||
| for class_member_reference in ThisPatternResolver::collect_object_assignment_names( | ||
| &object, | ||
| scoped_this_references, | ||
| ) { | ||
| writes.insert(class_member_reference.clone()); | ||
| } | ||
| } | ||
| // Plain assignment | ||
| if let Some(operand) = any_assignment | ||
| && let Some(name) = ThisPatternResolver::extract_this_member_reference( | ||
| operand.as_js_static_member_assignment(), | ||
| scoped_this_references, | ||
| AccessKind::Write, | ||
| ) | ||
| { | ||
| writes.insert(name.clone()); | ||
|
|
||
| if let Some(assignment) = left.as_any_js_assignment().cloned() | ||
| && let Some(name) = ThisPatternResolver::extract_this_member_reference( | ||
| assignment.as_js_static_member_assignment(), | ||
| scoped_this_references, | ||
| AccessKind::Write, | ||
| ) | ||
| if let Some(reference) = AnyCandidateForUsedInExpressionNode::cast_ref(operand.syntax()) | ||
| && is_used_in_expression_context(&reference) | ||
| { | ||
| writes.insert(name.clone()); | ||
| reads.insert(ClassMemberReference { | ||
| name: name.name, | ||
| range: name.range, | ||
| access_kind: AccessKind::MeaningfulRead, | ||
| }); | ||
|
|
||
| is_handled = true; | ||
| } | ||
| } | ||
|
|
||
| is_handled | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify compound assignment operators are complete.
Lines 926-934 check for compound assignments but may be missing operators:
- Present:
||=,&&=,/=,*=,%=,+=,??= - Potentially missing:
-=,<<=,>>=,>>>=,&=,|=,^=
Confirm whether all compound assignment operators should trigger meaningful reads, or if the current subset is intentional.
Run the following script to find compound assignment usage in tests:
🏁 Script executed:
#!/bin/bash
# Search for compound assignments in test files
rg -nP 'this\.[^=]+=(?!=)' crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/Length of output: 5309
Include all JS compound assignments in meaningful-read logic
In handle_assignment_expression (lines 926–934), extend the matches!(operator.kind(), …) to cover every compound operator—-= **= <<= >>= >>>= &= |= ^=—so each reads the old value before writing.
🤖 Prompt for AI Agents
crates/biome_js_analyze/src/services/semantic_class.rs around lines 900–992
(specifically the compound-assignment matches at ~926–934): the
matches!(operator.kind(), …) branch only covers some compound operators and must
include every JS compound operator so they count as meaningful reads; update
that matches! to also include the JsSyntaxKind variants for -=, **=, <<=, >>=,
>>>=, &=, |=, and ^= (i.e. add the corresponding JsSyntaxKind::MINUSEQ,
JsSyntaxKind variant for exponentiation assignment,
JsSyntaxKind::LT2EQ/RT2EQ/RT3EQ or the project’s exact names for <<=, >>=, >>>=,
and JsSyntaxKind::AMPEQ, PIPEEQ, CARETEQ) so all compound assignments read the
old value before write.
There was a problem hiding this comment.
@vladimir-ivanov this comment seems valid
| - 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. |
2 participants
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