crates/biome_js_analyze/src/lint/style/use_readonly_class_properties.rs (2)

20-20: Minor import tidy-up for new HashSet usage

If you take the refactor above, pull HashSet into scope.

-use std::iter::once;
+use std::{collections::HashSet, iter::once};

---

328-330: Doc nit: duplicated word and clearer phrasing

Small clean-up; makes the comment easier to scan.

-/// Collects all all mutable (non-readonly) constructor parameters from a given class member list. If private_only is true, it only includes parameters with private visibility.
-/// It returns a Vec<PropOrParam> representing these parameters, which are candidates for being marked as readonly.
+/// Collects all mutable (non-`readonly`) constructor parameters from a given class member list.
+/// If `private_only` is true, only parameters with private visibility are included.
+/// Returns a `Vec<AnyPropertyMember>` of parameters that are candidates for `readonly`.

crates/biome_js_analyze/src/services/semantic_class.rs (3)

653-657: Use the member token range for precision

Diagnostic ranges are sharper if we point at the property name, not the whole this.member expression.

-        reads.insert(ClassMemberReference {
-            name: member.to_trimmed_text(),
-            range: static_member.syntax().text_trimmed_range(),
-        });
+        let name_text = member.to_trimmed_text();
+        reads.insert(ClassMemberReference {
+            range: member.syntax().text_trimmed_range(),
+            name: name_text,
+        });

---

686-698: Consider covering more compound assignment operators

You handle many, but miss some common ones (e.g. -= and shifts/xor). Low effort to extend while we’re here.

             && matches!(
                 operator.kind(),
                 JsSyntaxKind::PIPE2EQ
                     | JsSyntaxKind::AMP2EQ
+                    | JsSyntaxKind::CARET_EQ
                     | JsSyntaxKind::SLASHEQ
                     | JsSyntaxKind::STAREQ
                     | JsSyntaxKind::PERCENTEQ
                     | JsSyntaxKind::PLUSEQ
+                    | JsSyntaxKind::MINUSEQ
+                    | JsSyntaxKind::LT2EQ
+                    | JsSyntaxKind::GT2EQ
+                    | JsSyntaxKind::GT3EQ
                     | JsSyntaxKind::QUESTION2EQ
             )

---

1-13: Small import hygiene

You can drop a couple of unused imports after the refactors (if any remain post-diff). Let rustfmt/clippy guide this.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : If a rule provides a code action, declare fix_kind in declare_lint_rule!

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When porting a lint rule, set sources in declare_lint_rule! (RuleSource::Eslint(...).same() or .inspired())

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Set the language field in declare_lint_rule! to the most specific applicable language (js/jsx/ts/tsx)

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When returning a code action, pass category via ctx.action_category(ctx.category(), ctx.group()) and applicability via ctx.metadata().applicability()

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/lint/nursery/**/*.rs : Place all new rules in the nursery group under biome_js_analyze/lib/src/lint/nursery

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : Use rule_category!() when creating RuleDiagnostic instead of dynamic string categories

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : In declare_lint_rule! metadata, set version: "next"

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-05T09:13:58.901Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/**/src/**/*.rs : When deprecating a rule, add deprecated: "<reason>" to declare_lint_rule!

crates/biome_js_analyze/src/services/semantic_class.rs (4)

299-299: Nix the commented debug println

It’s noise in the visitor.

-                // println!("leave node in ThisScopeVisitor {:?}", node);

---

71-79: Emit the Syntax-phase match so rules’ run() actually executes

You’re collecting refs but never emitting a Syntax-phase match. Call ctx.match_query when entering a class. This fixes the “run not executed” issue.

-    fn visit(&mut self, event: &WalkEvent<JsSyntaxNode>, _ctx: VisitorContext<JsLanguage>) {
+    fn visit(&mut self, event: &WalkEvent<JsSyntaxNode>, ctx: VisitorContext<JsLanguage>) {
         if let WalkEvent::Enter(node) = event
             && let Some(js_class_declaration) = JsClassDeclaration::cast_ref(node)
         {
+            // Emit the Syntax-phase query match for this class
+            ctx.match_query(node.clone());
             let class_member_list = js_class_declaration.members();
             let refs = class_member_references(&class_member_list);
             self.references.reads.extend(refs.reads);
             self.references.writes.extend(refs.writes);
         }
     }

#!/bin/bash
# Expect: match_query is called inside ClassMemberReferencesVisitor; no SemanticClassVisitor present.
rg -nP -C2 'impl\s+Visitor\s+for\s+ClassMemberReferencesVisitor|match_query\(' crates/biome_js_analyze/src/services/semantic_class.rs
rg -nP -C1 'SemanticClassVisitor' crates/biome_js_analyze/src/services/semantic_class.rs || true

---

99-114: Delete the redundant Semantic-phase visitor and its registration

With Syntax-phase matching in place, this visitor just confuses things and risks double work.

-struct SemanticClassVisitor;
-
-impl Visitor for SemanticClassVisitor {
-    type Language = JsLanguage;
-
-    fn visit(&mut self, event: &WalkEvent<JsSyntaxNode>, mut ctx: VisitorContext<JsLanguage>) {
-        match event {
-            WalkEvent::Enter(node) => {
-                if JsClassDeclaration::can_cast(node.kind()) {
-                    ctx.match_query(node.clone());
-                }
-            }
-            WalkEvent::Leave(_) => {}
-        };
-    }
-}
@@
     fn build_visitor(analyzer: &mut impl AddVisitor<JsLanguage>, root: &AnyJsRoot) {
         analyzer.add_visitor(Phases::Syntax, || ClassMemberReferencesVisitor::new(root));
-        analyzer.add_visitor(Phases::Semantic, || SemanticClassVisitor);
     }

Also applies to: 126-129

---

19-28: Track references per class (don’t mix all classes together)

The service currently aggregates reads/writes for the whole root. That breaks when multiple classes exist. Store refs keyed by class range (or node key) and expose a query by range, aligning with earlier feedback.

-#[derive(Clone)]
-pub struct SemanticClassServices {
-    class_member_references: ClassMemberReferences,
-}
+#[derive(Clone)]
+pub struct SemanticClassServices {
+    class_member_references: std::collections::BTreeMap<TextRange, ClassMemberReferences>,
+}
@@
-impl SemanticClassServices {
-    pub fn class_member_references(&self) -> &ClassMemberReferences {
-        &self.class_member_references
-    }
-}
+impl SemanticClassServices {
+    pub fn class_member_references(&self, range: TextRange) -> Option<&ClassMemberReferences> {
+        self.class_member_references.get(&range)
+    }
+}
@@
-#[derive(Debug, Clone)]
-pub struct SemanticClassModel {
-    pub references: ClassMemberReferences,
-}
+#[derive(Debug, Clone)]
+pub struct SemanticClassModel {
+    pub references_by_class: std::collections::BTreeMap<TextRange, ClassMemberReferences>,
+}
@@
         let service: &SemanticClassModel = services.get_service().ok_or_else(|| {
             ServicesDiagnostic::new(rule_key.rule_name(), &["SemanticClassModel"])
         })?;
         Ok(Self {
-            class_member_references: service.references.clone(),
+            class_member_references: service.references_by_class.clone(),
         })
     }
 }
@@
-pub struct ClassMemberReferencesVisitor {
-    references: ClassMemberReferences,
-}
+pub struct ClassMemberReferencesVisitor {
+    by_class: std::collections::BTreeMap<TextRange, ClassMemberReferences>,
+}
@@
 impl ClassMemberReferencesVisitor {
     pub(crate) fn new(_root: &AnyJsRoot) -> Self {
         Self {
-            references: ClassMemberReferences::default(),
+            by_class: Default::default(),
         }
     }
 }
@@
-        {
-            let class_member_list = js_class_declaration.members();
-            let refs = class_member_references(&class_member_list);
-            self.references.reads.extend(refs.reads);
-            self.references.writes.extend(refs.writes);
+        {
+            let class_member_list = js_class_declaration.members();
+            let refs = class_member_references(&class_member_list);
+            let class_range = js_class_declaration.syntax().text_trimmed_range();
+            let entry = self.by_class.entry(class_range).or_default();
+            entry.reads.extend(refs.reads);
+            entry.writes.extend(refs.writes);
         }
@@
-        ctx.services.insert_service(SemanticClassModel {
-            references: self.references,
-        });
+        ctx.services.insert_service(SemanticClassModel {
+            references_by_class: self.by_class,
+        });

If you prefer NodeKey over TextRange for stability, happy to sketch that too.

Also applies to: 30-48, 56-66, 82-86

crates/biome_js_analyze/src/services/semantic_class.rs (5)

647-656: Prefer the member’s range over the whole expression

This makes diagnostics crisper (highlights foo, not this.foo).

-        reads.insert(ClassMemberReference {
-            name: member.to_trimmed_text(),
-            range: static_member.syntax().text_trimmed_range(),
-        });
+        reads.insert(ClassMemberReference {
+            name: member.to_trimmed_text(),
+            range: member.syntax().text_trimmed_range(),
+        });

---

682-701: Cover all compound assignments (or centralise the check)

The operator list misses some tokens (e.g. -= and shifts). Either expand it fully or add a helper like “is_compound_assignment”.

Would you like me to propose a complete token set based on JsSyntaxKind?

---

2-7: Remove unused union and import

AnyPropertyMember isn’t used. Trim it and the TsPropertyParameter import.

-    JsVariableDeclarator, TextRange, TsPropertyParameter,
+    JsVariableDeclarator, TextRange,
@@
-declare_node_union! {
-    pub AnyPropertyMember = JsPropertyClassMember | TsPropertyParameter
-}
+// (removed unused AnyPropertyMember)

Also applies to: 152-155

---

56-66: Minor: Visitor can be Default

Root isn’t used; consider impl Default + analyzer.add_visitor(..., ClassMemberReferencesVisitor::default).

Happy to wire it if you prefer.

---

10-13: Naming: singular ‘Service’ reads better

Most codebases use FooService (singular). Consider SemanticClassService for consistency with SemanticModel.

If you want, I’ll prep a follow-up PR-wide rename.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-10T08:05:22.867Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/{lint,assist}/**/*.rs : When banning globals (e.g., `noConsoleLog`), check the semantic model to avoid false positives from locally shadowed bindings

crates/biome_js_analyze/src/services/semantic_class.rs (1)

19-37: Naming: align with existing service/model conventions
Current split reads as “Services” (plural) holding a “Model”. Consider ClassMemberReferencesService (service) + ClassMemberReferencesModel (model), or adopt reviewer’s suggestion to mirror SemanticModel.

crates/biome_js_analyze/src/services/semantic_class.rs (5)

577-599: Only record the bound property name (avoid key: alias text)
declarator.to_trimmed_text() will include aliases (foo: local). Extract the property key instead so the recorded read is the class field name.

-    for declarator in binding.properties() {
-        if let Some(declarator) = declarator.ok()
-            && is_this_reference(&expression, scoped_this_references)
-        {
-            reads.insert(ClassMemberReference {
-                name: declarator.to_trimmed_text(),
-                range: declarator.syntax().text_trimmed_range(),
-            });
-        }
-    }
+    for prop in binding.properties() {
+        if let Some(prop) = prop.ok()
+            && is_this_reference(&expression, scoped_this_references)
+            && let Some(p) = prop.as_js_object_binding_pattern_property()
+            && let Ok(key) = p.key()
+        {
+            reads.insert(ClassMemberReference {
+                name: key.to_trimmed_text(),
+                range: key.syntax().text_trimmed_range(),
+            });
+        }
+    }

---

319-344: Alias discovery misses nested block declarations
collect_local_this_references only scans top-level statements, so const self = this; inside a block is ignored. Consider descending into blocks to catch aliases with block scope.

---

274-274: Remove debug noise
Stray commented println! left behind.

-                // println!("leave node in ThisScopeVisitor {:?}", node);

---

127-129: Dead union?
AnyPropertyMember isn’t used. If truly unused, drop it and the TsPropertyParameter import.

---

807-1131: Add a few missing tests
Please add tests for:

• Compound ops -=, **=, <<= to validate the generalised check.
• Object binding with aliasing (const { foo: local } = this) to ensure only foo is recorded.

Happy to draft the cases if useful.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-10T08:05:22.867Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/{lint,assist}/**/*.rs : When banning globals (e.g., `noConsoleLog`), check the semantic model to avoid false positives from locally shadowed bindings

crates/biome_js_analyze/src/services/semantic_class.rs (3)

274-274: Remove the debug print statement.

-                // println!("leave node in ThisScopeVisitor {:?}", node);

---

33-37: Model doesn't store any class data.

The SemanticClassModel is empty and can't provide class member references for specific classes. Rules need to query references by class range/identity.

Store class references in the model:

 #[derive(Debug, Clone)]
-pub struct SemanticClassModel {}
+pub struct SemanticClassModel {
+    class_references: FxHashMap<TextRange, ClassMemberReferences>,
+}

 impl SemanticClassModel {
-    pub fn class_member_references(&self, members: &JsClassMemberList) -> ClassMemberReferences {
-        class_member_references(members)
+    pub fn class_member_references(&self, class_range: TextRange) -> Option<&ClassMemberReferences> {
+        self.class_references.get(&class_range)
     }
 }

---

77-79: The service should be created once per root, not once per class.

The current approach inserts a new SemanticClassModel {} for every class encountered. This will overwrite previous instances, and rules won't be able to access data from other classes in the same file.

Move the service creation to happen once per file:

     fn finish(self: Box<Self>, ctx: VisitorFinishContext<JsLanguage>) {
-        ctx.services.insert_service(SemanticClassModel {});
+        // Service should be created once per root with collected data
+        // Consider storing class references in the visitor and creating
+        // the model here with all collected data
     }

crates/biome_js_analyze/src/services/semantic_class.rs (1)

83-83: Naming: Consider SemanticClass → ClassSemanticQuery.

Given the existing feedback about naming and the pattern of other services, a more descriptive name might help distinguish this from general class handling.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-09-10T08:05:22.867Z
Learning: Applies to crates/biome_analyze/crates/biome_js_analyze/lib/src/{lint,assist}/**/*.rs : When banning globals (e.g., `noConsoleLog`), check the semantic model to avoid false positives from locally shadowed bindings