feat(biome-js-analyzer): adds class member analyzer to extract mutated properties in a class#7015
Conversation
|
github-actions
bot
added
A-Linter
vladimir-ivanov
force-pushed
the
feat/classMemberAnalyzer
branch
from
0c7fb7d to
821bd3c
Compare
CodSpeed Performance ReportMerging #7015 will not alter performanceComparing Summary
|
|
I don't really understand what you're trying to get at. I don't have very much context into the problem you're trying to solve. My comment was asking whether the whole file should go into js_analyze, because at a glance, this does analysis things. |
vladimir-ivanov
changed the title
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
I didn't have time to look at all the code. This PR needs a bit more love, and here a few suggestions to make it better:
- The PR description is very poor, compared to what the code does. A PR description is meant to present the code and all its business logic, so the reviewers know what they are about to review. As for myself, I really don't know what I'm reviewing and how the code is supposed to work
- The code is over-engineered. We used traits, callbacks with trait bounds, and the visitor pattern isn't implemented correctly. I think all of this isn't needed, and we can use plain functions
- Inconsistent documentation format. We should stick to one, and put a bit more care into what we document. Some comments lack context, some comments show examples differently.
|
@ematipico thanks for the feedback. Does the idea make sense to proceed or not at all please? It is the main question here.
Step 1 is largely needed to resolve a this.scope to aliases e.g. const self = this; and later self.scope is checked against its valid scope. when 3. is ready we can reuse in no_unused_private_class_members. Else I can clean up the pr if there is any value of creating this class analyser? |
|
You might want to consider expanding the semantic model at this point, or creating a new one just for classes. I know I said that the current semantic model is more about scopes and such, but after reviewing the code, I think that's the right approach. |
thanks, my feeling is do it incremental - one for classes at first. And then potentially merge with the existing. Can do a few things on this pr here so it keeps similar shape as currently (will be much easier to turn it into a semantic model alike. Traits - I tried (and failed) to use them instead of namespaces - as grouping smaller pieces of code together, and only expose the public fns out. So it becomes much simpler to refactor later when I learn more. e.g. struct ThisAliases {} and attach methods to it. Can also clean up the write part of the visitor, now is quite hard to maintain. Instead will use the same walk interface as in read? (downside is it will be slightly slower as it will traverse a bit more nodes -> node.syntax() but will be much simpler to maintain. And of course I do not mind sctrapping the whole thing if result is a disaster. Or redo it to a better one.. As it stands I just wanted to make things work. And fix all bugs with no_unused_private_properties. Step 2 is to make things more pretty :-) Appreciate your feedback so far. And please do feel free always to say if it wont work. My goal is not to merge this if the outcome is not easily readable and maintainable code. Or if the implementation is fundamentally wrong. |
Yeah that's a valid use-case, however empty struts are usually created because they eventually implement traits. However, if you design to create a new semantic model for classes, I don't see a compelling argument for using the previous approach. The model will need to save data because consumers will need to query them. For example if you identify a "write", you might want to save the node where it happens (or the text range). Just food for thought |
|
@ematipico I have addressed most of the issues, added comments etc. now is ok to be looked at. I would say maybe we make this mergeable. |
vladimir-ivanov
force-pushed
the
feat/classMemberAnalyzer
branch
from
4813afa to
b0843a2
Compare
github-actions
bot
added
A-Core
|
Note Generated docstrings for this pull request at #7308 |
![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
♻️ Duplicate comments (3)
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (1)
11-15: Imports look clean now — the E0432 saga is over.The stray identifiers reported earlier are gone; the import list matches actual exports.
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
236-262: ReturnTextRangewith a robust fallback.Switch from
Option<TextRange>toTextRange, preferring the name’s span and falling back to the node’s range. Mirrors the earlier bot suggestion.Apply:
- fn property_range(&self) -> Option<TextRange> { + fn property_range(&self) -> TextRange { match self { - Self::AnyJsClassMember(member) => match member { - AnyJsClassMember::JsGetterClassMember(member) => Some(member.name().ok()?.range()), - AnyJsClassMember::JsMethodClassMember(member) => Some(member.name().ok()?.range()), - AnyJsClassMember::JsPropertyClassMember(member) => { - Some(member.name().ok()?.range()) - } - AnyJsClassMember::JsSetterClassMember(member) => Some(member.name().ok()?.range()), - _ => None, - }, - Self::TsPropertyParameter(ts_property) => match ts_property.formal_parameter().ok()? { - AnyJsFormalParameter::JsBogusParameter(_) - | AnyJsFormalParameter::JsMetavariable(_) => None, - AnyJsFormalParameter::JsFormalParameter(param) => Some( - param - .binding() - .ok()? - .as_any_js_binding()? - .as_js_identifier_binding()? - .name_token() - .ok()? - .text_range(), - ), - }, + Self::AnyJsClassMember(member) => { + let name_range = match member { + AnyJsClassMember::JsGetterClassMember(m) => m.name().ok().map(|n| n.range()), + AnyJsClassMember::JsMethodClassMember(m) => m.name().ok().map(|n| n.range()), + AnyJsClassMember::JsPropertyClassMember(m) => m.name().ok().map(|n| n.range()), + AnyJsClassMember::JsSetterClassMember(m) => m.name().ok().map(|n| n.range()), + _ => None, + }; + name_range.unwrap_or_else(|| member.syntax().text_range()) + } + Self::TsPropertyParameter(ts_property) => { + let name_range = ts_property + .formal_parameter() + .ok() + .and_then(|param| match param { + AnyJsFormalParameter::JsFormalParameter(p) => p + .binding() + .ok()? + .as_any_js_binding()? + .as_js_identifier_binding()? + .name_token() + .ok() + .map(|t| t.text_range()), + _ => None, + }); + name_range.unwrap_or_else(|| ts_property.syntax().text_range()) + } } }
123-131: Compile blocker:RuleDiagnostic::newexpectsTextRange, notOption<TextRange>.
state.property_range()returnsOption<TextRange>, so this won’t compile. Return aTextRangewith a sensible fallback (member node span) and keep the call site unchanged.Apply:
- fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { - Some(RuleDiagnostic::new( - rule_category!(), - state.property_range(), + fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> { + Some(RuleDiagnostic::new( + rule_category!(), + state.property_range(), markup! { "This private class member is defined but never used." }, )) }and update
property_range()below (see next comment).
🧹 Nitpick comments (7)
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (4)
149-172: Cut allocations and scans in the hot path.We clone the
writesset and scan it per member; we also clone items we already own. Use iterators and precompute a lookup set.Apply:
- .filter_map(|prop_or_param| { - if writes - .clone() - .into_iter() - .any(|ClassMemberReference { name, .. }| { - if let Some(TextAndRange { text, .. }) = - extract_property_or_param_range_and_text(&prop_or_param.clone()) - { - return name.eq(&text); - } - - false - }) - { - None - } else { - Some(prop_or_param.clone()) - } - }) + .filter_map(|prop_or_param| { + let is_written = writes.iter().any(|ClassMemberReference { name, .. }| { + extract_property_or_param_range_and_text(&prop_or_param) + .is_some_and(|TextAndRange { text, .. }| name.eq(&text)) + }); + if is_written { None } else { Some(prop_or_param) } + })Optionally, dedupe against constructor params without repeated clones:
- 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() - }) - }), - ) + let ctor_names: std::collections::HashSet<_> = constructor_params + .iter() + .filter_map(|p| extract_property_or_param_range_and_text(p).map(|t| t.text)) + .collect(); + + constructor_params + .into_iter() + .chain( + non_readonly_class_property_members.filter(|m| { + extract_property_or_param_range_and_text(m) + .is_some_and(|t| !ctor_names.contains(&t.text)) + }), + )
196-208: Avoid re‑casting from syntax; pattern‑match the union directly.
nodeis alreadyAnyPropertyMember. Casting the syntax back toAnyPropertyMemberis redundant and brittle.Apply:
- if let Some(AnyPropertyMember::JsPropertyClassMember(member)) = - AnyPropertyMember::cast(original_node.clone()) + if let AnyPropertyMember::JsPropertyClassMember(member) = node { … - } else if let Some(AnyPropertyMember::TsPropertyParameter(parameter)) = - AnyPropertyMember::cast(original_node.clone()) + } else if let AnyPropertyMember::TsPropertyParameter(parameter) = node { … }This also removes an extra clone of
original_node.Also applies to: 241-263
268-269: Wording: “modifier”, not “decorator”.TS
readonlyis a modifier, not a decorator. Tiny thing, big peace of mind for readers.Apply:
- markup! { "Add "<Emphasis>"readonly"</Emphasis>" decorator." }.to_owned(), + markup! { "Add "<Emphasis>"readonly"</Emphasis>" modifier." }.to_owned(),
392-424: Simplify: match on the union instead of re‑casting.Inside
extract_property_or_param_range_and_text, you already have&AnyPropertyMember. The extracastviainto()is unnecessary.Apply:
-fn extract_property_or_param_range_and_text( - property_or_param: &AnyPropertyMember, -) -> Option<TextAndRange> { - if let Some(AnyPropertyMember::JsPropertyClassMember(member)) = - AnyPropertyMember::cast(property_or_param.clone().into()) - { +fn extract_property_or_param_range_and_text( + property_or_param: &AnyPropertyMember, +) -> Option<TextAndRange> { + if let AnyPropertyMember::JsPropertyClassMember(member) = property_or_param { if let Ok(member_name) = member.name() { return Some(TextAndRange { text: member_name.to_trimmed_text(), range: member_name.range(), }); } return None; } - if let Some(AnyPropertyMember::TsPropertyParameter(parameter)) = - AnyPropertyMember::cast(property_or_param.clone().into()) - { + if let AnyPropertyMember::TsPropertyParameter(parameter) = property_or_param { let name = parameter .formal_parameter() .ok()? .as_js_formal_parameter()? .binding() .ok()?; return Some(TextAndRange { text: name.to_trimmed_text(), range: name.range(), }); } None }crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.ts (1)
29-36: Two exported classes with the same name may confuse tooling.Having two
export class ToastServicedeclarations in one module can trip bundlers or TS language services, even if our test harness doesn’t care.Apply:
-export class ToastService { +export class ToastService2 { private _toastId = 0; show(message: string, type: string, autoClose: boolean): void { const id = this._toastId++; this.activeToasts.push({ id, message, type, autoClose }); } }crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
90-121: Overall flow looks good; one style nit.
get_constructor_paramsreturnsFxHashSetwhileget_all_declared_private_memberscollects intoHashSet. It’s harmless, but we can useHashSetconsistently unless perf dictates otherwise.If you prefer
FxHashSet, mirror it inget_all_declared_private_members; otherwise, switchget_constructor_paramstoHashSet.
264-307: Name matching is clean; consider normalising once.You strip
#per comparison. If hotspots show up, pre‑normalise thereads/writesnames (or the private member names) to avoid repeated allocation/trim logic. Non‑blocking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
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/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (6)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs(5 hunks)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs(13 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js(2 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.tscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.tscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js
**/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.tscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/*.rs : For new JavaScript lint rules generated by `just new-js-lintrule`, implement the rule in the generated file under `biome_js_analyze/lib/src/lint/nursery/`
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/src/**/*.rs : Represent links between types using TypeReference (not Arc) to avoid cross-module retention and recursive structures; store type data in linear vectors
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/biome_module_graph/src/js_module_info/collector.rs : Thin (module-level) inference may resolve qualifiers to local declarations or globals (TypeReference::Resolved), mark imported bindings as TypeReference::Import, and set unresolved to TypeReference::Unknown; it must not look beyond the current module
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/*.rs : For new JavaScript lint rules generated by `just new-js-lintrule`, implement the rule in the generated file under `biome_js_analyze/lib/src/lint/nursery/`
Applied to files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rscrates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/biome_module_graph/src/js_module_info/collector.rs : Thin (module-level) inference may resolve qualifiers to local declarations or globals (TypeReference::Resolved), mark imported bindings as TypeReference::Import, and set unresolved to TypeReference::Unknown; it must not look beyond the current module
Applied to files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/tests/quick_test.rs : Use `biome_js_analyze/tests/quick_test.rs` for quick, ad-hoc testing; un-ignore the test and adjust the rule filter as needed
Applied to files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
📚 Learning: 2025-08-11T11:50:12.090Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/src/**/*.rs : Use TypeData::Unknown for unimplemented inference and TypeData::UnknownKeyword for the explicit TypeScript unknown keyword; treat them semantically the same but keep them distinct for measurement
Applied to files:
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/**/lib/src/**/nursery/**/*.rs : If a rule provides a code action, add `fix_kind` in `declare_lint_rule!` and use `ctx.action_category(ctx.category(), ctx.group())` and `ctx.metadata().applicability()` when constructing actions
Applied to files:
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/biome_rule_options/lib/**/*.rs : Define per-rule options in the `biome_rule_options` crate under `lib/`, with serde- and schemars-compatible derives and `#[serde(rename_all = "camelCase", deny_unknown_fields, default)]`
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/class_member_references.rs (1)
class_member_references(39-95)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (2)
crates/biome_js_analyze/src/class_member_references.rs (2)
class_member_references(39-95)new(194-199)crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (3)
run(132-179)ctx(133-133)member(211-217)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_package)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
🔇 Additional comments (5)
crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (1)
141-178: Double‑check: are constructor writes excluded from “writes”?The rule promises “readonly if never modified outside the constructor”, but the filter currently drops any member that appears in
writes, regardless of origin. Ifclass_member_referencesincludes constructor writes, properties set only in the constructor will be incorrectly excluded from diagnostics. Please verify the semantics, or filter out constructor-origin writes here.Would you like me to wire a small flag on
ClassMemberReference(e.g.,origin: MemberOrigin::{Constructor, NonConstructor}) and adjust the filter?Example patch at call‑site (assuming
writes_outside_ctoris available):- let ClassMemberReferences { writes, .. } = class_member_references(&members); + let ClassMemberReferences { writes_outside_ctor: writes, .. } = + class_member_references(&members);crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.ts (1)
6-9: Nice coverage ofthisdestructuring.This exercises alias‑free reads and closes a common hole in member‑usage analysis.
crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/invalid.js (2)
26-34: Good: inner class reads from a non‑alias parameter.This guards against false positives where
ais mistaken forthis.
44-50: Write‑only assignment expression is correctly treated as “unused”.
return this.#val = 1still doesn’t “use” the field’s value; it’s a pure write — perfect for catching regressions like #6994.crates/biome_js_analyze/tests/specs/correctness/noUnusedPrivateClassMembers/valid.js (1)
187-193: Nice addition: post‑increment use case for ES private fields.Covers the update‑expr path; complements the existing pre/post cases elsewhere.
|
@ematipico could you please review one last time? |
…eads and writes of class members and props
…eads and writes of class members and props
…eads and writes of class members and props
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/biome_lsp/src/server.tests.rs (3)
1006-1022: Accept numeric diagnostic codes and avoid relying on borrowed strs.Some servers may emit numeric codes; converting to owned
Strings makes the check robust and avoids lifetime gotchas. Also switchcontainsto an iterator predicate to match the new type.Apply this diff:
- // collect the diagnostic codes for easier comparison - let codes: Vec<_> = params.diagnostics.iter().filter_map(|diagnostic| { - if let Some(lsp::NumberOrString::String(code)) = &diagnostic.code { - Some(code.as_str()) - } else { - None - } - }).collect(); - - assert!( - codes.contains(&"syntax/correctness/noDuplicatePrivateClassMembers"), - "Expected duplicate private member diagnostic, got {codes:?}" - ); - assert!( - codes.contains(&"lint/correctness/noUnusedVariables"), - "Expected unused variable diagnostic, got {codes:?}" - ); + // Collect diagnostic codes as owned strings (support both string and numeric codes) + let codes: Vec<String> = params + .diagnostics + .iter() + .filter_map(|d| d.code.as_ref().map(|c| match c { + lsp::NumberOrString::String(s) => s.clone(), + lsp::NumberOrString::Number(n) => n.to_string(), + })) + .collect(); + + assert!( + codes.iter().any(|c| c == "syntax/correctness/noDuplicatePrivateClassMembers"), + "Expected duplicate private member diagnostic, got {codes:?}" + ); + assert!( + codes.iter().any(|c| c == "lint/correctness/noUnusedVariables"), + "Expected unused variable diagnostic, got {codes:?}" + );
1024-1033: Make message checks case-insensitive and include context on failure.Minor tweak to avoid casing brittleness and improve debug output. Also the comment says “Optionally”, but these are hard asserts.
Apply this diff:
- // Optionally also check messages (ignoring positions/ranges to avoid brittleness) - let messages: Vec<_> = params.diagnostics.iter().map(|diagnostic| diagnostic.message.as_str()).collect(); - assert!( - messages.iter().any(|message| message.contains("Duplicate private class member")), - "Expected duplicate member message" - ); - assert!( - messages.iter().any(|message| message.contains("unused")), - "Expected unused variable message" - ); + // Also check messages (ignoring positions/ranges to avoid brittleness) + let messages_lower: Vec<String> = params + .diagnostics + .iter() + .map(|d| d.message.to_lowercase()) + .collect(); + assert!( + messages_lower + .iter() + .any(|m| m.contains("duplicate private class member")), + "Expected duplicate member message. Got: {messages_lower:?}" + ); + assert!( + messages_lower.iter().any(|m| m.contains("unused")), + "Expected unused variable message. Got: {messages_lower:?}" + );
1002-1014: Potential flake: assumes both diagnostics arrive in a single PublishDiagnostics.If the server ever emits syntax and lint diagnostics in separate notifications, this will occasionally fail. Consider aggregating diagnostics for a short window before asserting.
Example (replace the single-use
params.diagnosticsbelow withall_diags):// After receiving the first PublishDiagnostics as `params`: let mut all_diags = params.diagnostics.clone(); // Drain additional PublishDiagnostics for a short window (e.g., 200ms idle). loop { let next = tokio::select! { msg = receiver.next() => msg, _ = sleep(Duration::from_millis(200)) => None, }; match next { Some(ServerNotification::PublishDiagnostics(next_params)) => { all_diags.extend(next_params.diagnostics); } _ => break, } } // Then build `codes`/`messages` from `all_diags.iter()` rather than `params.diagnostics.iter()`.Would you like me to wire this into the test with minimal churn?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/biome_lsp/src/server.tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_lsp/src/server.tests.rs
crates/biome_service/../biome_lsp/src/server.tests.rs
📄 CodeRabbit inference engine (crates/biome_service/CONTRIBUTING.md)
Keep end-to-end LSP tests in biome_lsp’s server.tests.rs
Files:
crates/biome_lsp/src/server.tests.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_lsp/src/server.tests.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/*.rs : For new JavaScript lint rules generated by `just new-js-lintrule`, implement the rule in the generated file under `biome_js_analyze/lib/src/lint/nursery/`
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/src/**/*.rs : Represent links between types using TypeReference (not Arc) to avoid cross-module retention and recursive structures; store type data in linear vectors
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:50:12.090Z
Learning: Applies to crates/biome_js_type_info/biome_module_graph/src/js_module_info/collector.rs : Thin (module-level) inference may resolve qualifiers to local declarations or globals (TypeReference::Resolved), mark imported bindings as TypeReference::Import, and set unresolved to TypeReference::Unknown; it must not look beyond the current module
📚 Learning: 2025-08-11T11:53:15.299Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:53:15.299Z
Learning: Applies to crates/biome_service/../biome_lsp/src/server.tests.rs : Keep end-to-end LSP tests in biome_lsp’s server.tests.rs
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-08-17T08:56:30.831Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-08-17T08:56:30.831Z
Learning: Applies to crates/biome_analyze/crates/**/lib/src/**/nursery/**/*.rs : Use the local `rule_category!()` macro in diagnostics for the rule’s category, not string-parsed categories
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-08-11T11:46:05.836Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_diagnostics/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:46:05.836Z
Learning: Applies to crates/biome_diagnostics/**/*.rs : Types implementing Diagnostic must also implement Debug (e.g., use #[derive(Debug, Diagnostic)])
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-08-11T11:46:05.836Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_diagnostics/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:46:05.836Z
Learning: Applies to crates/biome_diagnostics/**/*.rs : Specify category and severity using #[diagnostic(...)] on the type or derive them from fields
Applied to files:
crates/biome_lsp/src/server.tests.rs
🧬 Code graph analysis (1)
crates/biome_lsp/src/server.tests.rs (2)
crates/biome_js_analyze/src/lint/correctness/no_unused_private_class_members.rs (1)
diagnostic(123-131)crates/biome_js_analyze/src/lint/correctness/no_unused_variables.rs (1)
diagnostic(306-348)
⏰ 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). (25)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: End-to-end tests
- GitHub Check: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: autofix
🔇 Additional comments (2)
crates/biome_lsp/src/server.tests.rs (2)
1002-1004: Solid pattern-match; less brittle than full-struct equality.Graceful destructuring of the notification improves resilience and keeps the failure mode explicit.
1035-1035: Nice touch: explicit close before shutdown.Keeps the lifecycle tidy and mirrors other tests.
To be honest, I'm feeling a bit lost about all the changes. The PR description says that it should address only one lint rule, however it seems the scope of the PR has changed. I also thought you were trying to address a couple of bugs of a nursery rule. My advice is to take a step back, and keep the changes scoped to that single rule and do not span into others. I really don't understand why the scope changed, and maybe it shouldn't. |
…eads and writes of class members and props
vladimir-ivanov
force-pushed
the
feat/classMemberAnalyzer
branch
from
66143a9 to
9608e47
Compare
@ematipico thanks for the comment. Reduced the scope of the pr is now ONLY class member analyser. it will fix many other bugs in useReadonlyClassProperties such as the recently opened one here: let me know if we want to keep or scrap entirely the pr please. Also do we want to move this into service bag at all as discussed above? if so, do we add it to semantic model or we have separate semantic class model service? thanks |
…eads and writes of class members and props
There's no need, as long as the scope of the pr doesn't change
Yes I think it should. Do you think it shouldn't?
I think it will be another service, because this one works only for classes |
will give it a shot. thank you RE: Yes I think it should. Do you think it shouldn't? - I do not assume anything anymore, too many folks have very different opinions, some can surprise me sometimes :-). So there is no harm to ask even obvious questions :-) |
…d properties in a class (#7015)
…d properties in a class (biomejs#7015)
…d properties in a class (biomejs#7015)
3 participants
Summary
Adds class member analyser that returns all properties mutations in a class member.
Exposes a single fn:
which returns all reads and writes found in any class member (constructors, property and method members).
Different rules apply for extracting writes and reads from a constructor (consistent with eslint)
The code internally collects the
thiskeyword and its aliases if any (e.g. const thisAlias = this;) with the scope/ range next to it.It is useful to avoid matching false positives of this.something in nested constructs.