feat(lint/noThisInStatic): add rule (#366)

Co-authored-by: Victorien Elvinger <[email protected]>

CHANGELOG.md

@@ -67,6 +67,10 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
 
 ### Linter
 
+#### New features
+
+- Add [noThisInStatic](https://biomejs.dev/linter/rules/no-this-in-static) rule. Contributed by @ditorodev and @Conaclos
+
 #### Bug fixes
 
 - Fix [#548](https://github.com/biomejs/biome/issues/548) which made [noSelfAssign](https://biomejs.dev/linter/rules/no-self-assign) panic.

crates/biome_diagnostics_categories/src/categories.rs

@@ -100,6 +100,7 @@ define_categories! {
     "lint/nursery/noInvalidNewBuiltin": "https://biomejs.dev/lint/rules/no-invalid-new-builtin",
     "lint/nursery/noMisleadingInstantiator": "https://biomejs.dev/linter/rules/no-misleading-instantiator",
     "lint/nursery/noMisrefactoredShorthandAssign": "https://biomejs.dev/lint/rules/no-misrefactored-shorthand-assign",
+    "lint/nursery/noThisInStatic": "https://biomejs.dev/lint/rules/no-this-in-static",
     "lint/nursery/noUnusedImports": "https://biomejs.dev/lint/rules/no-unused-imports",
     "lint/nursery/noUselessElse": "https://biomejs.dev/lint/rules/no-useless-else",
     "lint/nursery/noUselessLoneBlockStatements": "https://biomejs.dev/lint/rules/no-useless-lone-block-statements",

crates/biome_js_analyze/src/semantic_analyzers/nursery/no_this_in_static.rs

@@ -0,0 +1,193 @@
+use biome_analyze::{
+    context::RuleContext, declare_rule, ActionCategory, Ast, FixKind, Rule, RuleDiagnostic,
+};
+use biome_console::markup;
+use biome_diagnostics::Applicability;
+use biome_js_factory::make;
+use biome_js_syntax::{
+    AnyJsClass, AnyJsClassMember, AnyJsExpression, JsArrowFunctionExpression, JsSuperExpression,
+    JsSyntaxToken, JsThisExpression,
+};
+use biome_rowan::{declare_node_union, AstNode, AstNodeList, BatchMutationExt, SyntaxResult};
+
+use crate::{control_flow::AnyJsControlFlowRoot, JsRuleAction};
+
+declare_rule! {
+    /// Disallow `this` and `super` in `static` contexts.
+    ///
+    /// In JavaScript, the `this` keyword in static contexts refers to the class (the constructor) instance,
+    /// not an instance of the class. This can be confusing for developers coming from other languages where
+    /// `this` typically refers to an instance of the class, not the class itself.
+    ///
+    /// Similarly, `super` in static contexts refers to the parent class, not an instance of the class.
+    /// This can lead to unexpected behavior if not properly understood.
+    ///
+    /// This rule enforces the use of the class name itself to access static methods,
+    /// which can make the code clearer and less prone to errors. It helps to prevent
+    /// misunderstandings and bugs that can arise from the unique behavior of `this` and `super` in static contexts.
+    ///
+    /// Source: https://github.com/mysticatea/eslint-plugin/blob/master/docs/rules/no-this-in-static.md
+    ///
+    /// ## Example
+    ///
+    /// ### Invalid
+    ///
+    /// ```js,expect_diagnostic
+    ///  class A {
+    ///     static CONSTANT = 0;
+    ///
+    ///     static foo() {
+    ///         this.CONSTANT;
+    ///     }
+    ///  }
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    ///  class B extends A {
+    ///     static bar() {
+    ///         super.CONSTANT;
+    ///     }
+    ///  }
+    /// ```
+    ///
+    /// ### Valid
+    ///
+    /// ```js
+    /// class B extends A {
+    ///     static ANOTHER_CONSTANT = A.CONSTANT + 1;
+    ///
+    ///     static foo() {
+    ///         A.CONSTANT;
+    ///         B.ANOTHER_CONSTANT;
+    ///     }
+    ///
+    ///     bar() {
+    ///         this.property;
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// ```js
+    /// class A {
+    ///    static foo() {
+    ///        doSomething()
+    ///    }
+    ///
+    ///    bar() {
+    ///      A.foo()
+    ///    }
+    /// }
+    /// ```
+    ///
+    pub(crate) NoThisInStatic {
+        version: "next",
+        name: "noThisInStatic",
+        recommended: false,
+        fix_kind: FixKind::Unsafe,
+    }
+}
+
+impl Rule for NoThisInStatic {
+    type Query = Ast<JsThisSuperExpression>;
+    type State = ();
+    type Signals = Option<Self::State>;
+    type Options = ();
+
+    fn run(ctx: &RuleContext<Self>) -> Self::Signals {
+        let this_super_expression = ctx.query();
+        let static_method = this_super_expression
+            .syntax()
+            .ancestors()
+            .find(|x| {
+                AnyJsControlFlowRoot::can_cast(x.kind())
+                    && !JsArrowFunctionExpression::can_cast(x.kind())
+            })
+            .and_then(AnyJsClassMember::cast)
+            .filter(|member| match member {
+                AnyJsClassMember::JsGetterClassMember(member) => member
+                    .modifiers()
+                    .iter()
+                    .any(|modifier| modifier.as_js_static_modifier().is_some()),
+                AnyJsClassMember::JsMethodClassMember(member) => member
+                    .modifiers()
+                    .iter()
+                    .any(|modifier| modifier.as_js_static_modifier().is_some()),
+                AnyJsClassMember::JsSetterClassMember(member) => member
+                    .modifiers()
+                    .iter()
+                    .any(|modifier| modifier.as_js_static_modifier().is_some()),
+                AnyJsClassMember::JsPropertyClassMember(member) => member
+                    .modifiers()
+                    .iter()
+                    .any(|modifier| modifier.as_js_static_modifier().is_some()),
+                AnyJsClassMember::JsStaticInitializationBlockClassMember(_) => true,
+                _ => false,
+            });
+        static_method.is_some().then_some(())
+    }
+
+    fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
+        let this_super_expression = ctx.query();
+        let this_super_token = this_super_expression.token().ok()?;
+        let text = this_super_token.text_trimmed();
+        let note = if let JsThisSuperExpression::JsSuperExpression(_) = this_super_expression {
+            markup! {
+                <Emphasis>"super"</Emphasis>" refers to a parent class."
+            }
+        } else {
+            markup! {
+                <Emphasis>"this"</Emphasis>" refers to the class."
+            }
+        };
+        Some(RuleDiagnostic::new(
+            rule_category!(),
+            this_super_expression.range(),
+            markup! {
+                "Using "<Emphasis>{text}</Emphasis>" in a "<Emphasis>"static"</Emphasis>" context can be confusing."
+            },
+        ).note(note))
+    }
+
+    fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
+        let this_super_expression = ctx.query();
+        let class = this_super_expression
+            .syntax()
+            .ancestors()
+            .find_map(AnyJsClass::cast)?;
+        let suggested_class_name = if let JsThisSuperExpression::JsSuperExpression(_) =
+            this_super_expression
+        {
+            let extends_clause = class.extends_clause()?;
+            let super_class_name = extends_clause.super_class().ok()?;
+            let AnyJsExpression::JsIdentifierExpression(super_class_name) = super_class_name else {
+                return None;
+            };
+            super_class_name
+        } else {
+            let class_name = class.id()?.as_js_identifier_binding()?.name_token().ok()?;
+            make::js_identifier_expression(make::js_reference_identifier(class_name))
+        };
+        let expr = AnyJsExpression::cast_ref(this_super_expression.syntax())?;
+        let mut mutation = ctx.root().begin();
+        mutation.replace_node(expr, suggested_class_name.into());
+        Some(JsRuleAction {
+            category: ActionCategory::QuickFix,
+            applicability: Applicability::MaybeIncorrect,
+            message: markup! { "Use the class name instead." }.to_owned(),
+            mutation,
+        })
+    }
+}
+
+declare_node_union! {
+    pub(crate) JsThisSuperExpression = JsSuperExpression | JsThisExpression
+}
+
+impl JsThisSuperExpression {
+    fn token(&self) -> SyntaxResult<JsSyntaxToken> {
+        match self {
+            JsThisSuperExpression::JsSuperExpression(expr) => expr.super_token(),
+            JsThisSuperExpression::JsThisExpression(expr) => expr.this_token(),
+        }
+    }
+}

crates/biome_js_analyze/src/semantic_analyzers/suspicious/no_class_assign.rs

@@ -82,7 +82,7 @@ impl Rule for NoClassAssign {
         let node = ctx.query();
         let model = ctx.model();
 
-        if let Ok(Some(id)) = node.id() {
+        if let Some(id) = node.id() {
             if let Some(id_binding) = id.as_js_identifier_binding() {
                 return id_binding.all_writes(model).collect();
             }
@@ -94,8 +94,7 @@ impl Rule for NoClassAssign {
     fn diagnostic(ctx: &RuleContext<Self>, reference: &Self::State) -> Option<RuleDiagnostic> {
         let binding = ctx
             .query()
-            .id()
-            .ok()??
+            .id()?
             .as_js_identifier_binding()?
             .name_token()
             .ok()?;

crates/biome_js_analyze/tests/specs/nursery/noThisInStatic/invalid.js

@@ -0,0 +1,39 @@
+export default class B extends A {
+    static { this.CONSTANT += super.foo(); }
+
+    static CONSTANT = this.OTHER_CONSTANT;
+    static OTHER_CONSTANT = super.ANOTHER_CONSTANT;
+
+    static get property() {
+        /*before*/this/*after*/;
+        return /*before*/super/*after*/.x;
+    }
+
+    static set property(x) {
+        () => this;
+        () => super.x = x;
+    }
+
+    static method() {
+        return this.CONSTANT + super.ANOTHER_CONSTANT;
+    }
+}
+
+class C extends A {
+    static method() {
+        return this.CONSTANT + super.ANOTHER_CONSTANT;
+    }
+}
+
+const D = class D extends f() {
+    static method() {
+        return this.CONSTANT + super.ANOTHER_CONSTANT;
+    }
+}
+
+
+const E = class extends f() {
+    static method() {
+        return this.CONSTANT + super.ANOTHER_CONSTANT;
+    }
+}