-
-
Notifications
You must be signed in to change notification settings - Fork 714
feat(biome_js_analyze): align no_unused_private_class_members_with_semantic_class and dynamic prop access #7684
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 |
CodSpeed Performance ReportMerging #7684 will degrade performances by 9.92%Comparing Summary
Benchmarks breakdown
Footnotes
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
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_type
only handles formal parameters, so a union-typed local likeconst action: "add" | "remove"
never registers as a meaningful read. With this change both#add
and#remove
would 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_params
andwrites
for 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.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.js.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts.snap
is excluded by!**/*.snap
and included by**
📒 Files selected for processing (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-changeset
command; 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.rs
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/src/lint/style/use_readonly_class_properties.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
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/tests/quick_test.rs
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/src/lint/style/use_readonly_class_properties.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
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_js_analyze/tests/quick_test.rs
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/invalid.ts
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
crates/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.rs
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
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/tests/quick_test.rs
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/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
Sample
class properly demonstrates the scenario where an untyped parameter inmethod(name)
should be considered too broad to validate usage ofmember
or#prop
throughthis[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
SampleAddRemove
class correctly demonstrates that a constrained union type"add" | "remove"
should only validate usage of the matching private members, leavingappend
properly 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
YesNo
type definition andSampleYesNo
class nicely demonstrate that only string literals from a union should be extracted, ignoring the object type{ ignored: number }
. This ensuresyes
andno
are considered used whilstdontKnow
remains flagged as unused, as intended by the PR objectives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_type
returnsNone
when 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.rs
crates/biome_cli/tests/snapshots/main_commands_lint/does_error_with_only_warnings.snap.new
crates/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.new
crates/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 identicalnoClassAssign
warning 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
as
assertions- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
RemoveMember
changed 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.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.js.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts.snap
is excluded by!**/*.snap
and included by**
📒 Files selected for processing (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.new
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
crates/biome_js_analyze/src/services/semantic_class.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/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.new
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
crates/biome_js_analyze/src/services/semantic_class.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/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.new
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/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.rs
crates/biome_js_analyze/src/services/semantic_class.rs
crates/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.rs
crates/biome_js_analyze/src/services/semantic_class.rs
crates/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-changeset
command; 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—
unusedProperty
now matches the test's intent.crates/biome_js_analyze/src/services/semantic_class.rs (3)
22-26
: LGTM!
NamedClassMember
cleanly encapsulates member name and range for semantic-aware analysis.
44-53
: Verify cache invalidation strategy.
SemanticClassModel
introduces aRefCell<FxHashMap>
cache forNamedClassMember
lookups. 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_type
cleanly 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_usage
correctly filters out members with meaningful reads, returning only truly unused members.
400-418
: Verify semantic_class cache hit rate.Both
member_range
andmatches_name
callsemantic_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
useReadonlyClassProperties
rule using semantic-aware member extraction..changeset/no_unused_private_class_members_dynamic_usages.md (1)
15-29
: Manually verifydontKnow
is reported as unused with dynamic union access. Confirm Biome’s new semantic analysis correctly excludesdontKnow
when 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
AnyPropertyMember
toAnyNamedClassMember
aligns 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
_
toctx
is appropriate, and usingsemantic_class.extract_named_member
aligns with the new semantic-aware approach. ReturningNone
when extraction fails is sensible (no extractable name means no actionable diagnostic).
207-283
: Action function handles both member types correctly.The pattern matching on
AnyNamedClassMember
variants (lines 217-218 and 252-253) correctly handles both class property members and constructor parameters. The cloning oforiginal_node
is appropriate to avoid ownership issues.
285-393
: Helper functions correctly updated for type migration.The collection functions (
collect_non_readonly_class_member_properties
andcollect_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
dd34d1f
to
f146954
Compare
…ndle dynamic access class members based on their ts type
f146954
to
d951e0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_aligned_with_semantic_class.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_dynamic_access.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_aligned_with_semantic_class.js.snap
is excluded by!**/*.snap
and included by**
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts.snap
is excluded by!**/*.snap
and included by**
📒 Files selected for processing (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.js
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.new
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
crates/biome_js_analyze/src/services/semantic_class.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts
crates/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.js
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.new
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
crates/biome_js_analyze/src/services/semantic_class.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid_issue_7101.ts
crates/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.js
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
crates/biome_js_analyze/tests/specs/style/useReadonlyClassProperties/invalid_checkAllPropertiesTrue.ts.snap.new
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid_dynamic_access.ts
crates/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.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/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.rs
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/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-changeset
command; 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_member
safely returnsNone
for index signatures and thefilter_map
handles 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_member
match 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
SemanticClassModel
now caches named member extraction results, which is essential given thatextract_named_member
is called repeatedly during analysis. The use ofRefCell
allows 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_context
provide 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
None
for 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.ts
Length 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
RefCell
is 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 handleas
casts 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
#add
as 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vladimir-ivanov this comment seems valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Summary
Changed
noUnusedPrivateClassMembers
to 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