diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 0c0422b0b..8ec31b02f 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -82,7 +82,7 @@ mod eslint { pub mod no_setter_return; pub mod no_shadow_restricted_names; pub mod no_sparse_arrays; - // pub mod no_this_before_super; + pub mod no_this_before_super; pub mod no_undef; pub mod no_unsafe_finally; pub mod no_unsafe_negation; @@ -329,7 +329,7 @@ oxc_macros::declare_all_lint_rules! { eslint::eqeqeq, eslint::for_direction, eslint::getter_return, - // eslint::no_this_before_super, + eslint::no_this_before_super, eslint::no_array_constructor, eslint::no_async_promise_executor, eslint::no_bitwise, diff --git a/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs b/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs index 7535625b9..03fee5341 100644 --- a/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs +++ b/crates/oxc_linter/src/rules/eslint/no_this_before_super.rs @@ -1,7 +1,7 @@ -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use oxc_ast::{ - ast::{ArrowExpression, Expression, Function, MethodDefinitionKind}, + ast::{Expression, MethodDefinitionKind}, AstKind, }; use oxc_diagnostics::{ @@ -10,8 +10,7 @@ use oxc_diagnostics::{ }; use oxc_macros::declare_oxc_lint; use oxc_semantic::{ - pg::neighbors_filtered_by_edge_weight, AssignmentValue, BasicBlockElement, CallType, - CalleeWithArgumentsAssignmentValue, EdgeType, ObjectPropertyAccessAssignmentValue, + petgraph::stable_graph::NodeIndex, pg::neighbors_filtered_by_edge_weight, AstNodeId, EdgeType, }; use oxc_span::{GetSpan, Span}; @@ -27,16 +26,19 @@ pub struct NoThisBeforeSuper; declare_oxc_lint!( /// ### What it does - /// Requires all getters to have a return statement + /// Requires calling `super()` before using `this` or `super`. /// /// ### Why is this bad? - /// Getters should always return a value. If they don't, it's probably a mistake. + /// Getters should always return a value. + /// If they don't, it's probably a mistake. /// /// ### Example /// ```javascript - /// class Person{ - /// get name(){ - /// // no return + /// class A1 extends B { + /// constructor() { + /// // super() needs to be called first + /// this.a = 0; + /// super(); /// } /// } /// ``` @@ -66,137 +68,95 @@ impl NoThisBeforeSuper { false } - - fn run_diagnostic<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) { - if !Self::is_wanted_node(node, ctx) { - return; - } - - let cfg = ctx.semantic().cfg(); - - let mut registers_currently_with_super_in_them = HashSet::new(); - let mut registers_currently_with_this_in_them = HashSet::new(); - - neighbors_filtered_by_edge_weight( - &cfg.graph, - cfg.function_to_node_ix[&node.id()], - &|edge| match edge { - EdgeType::Normal => None, - // We don't need to handle backedges because we would have already visited - // them on the forward pass - | EdgeType::Backedge - // We don't need to visit NewFunction edges because it's not going to be evaluated - // immediately, and we are only doing a pass over things that will be immediately evaluated - | EdgeType::NewFunction - // By returning Some(X), - // we signal that we don't walk to this path any farther. - // - // The actual value that we return here doesn't matter because - // we don't use the final value of cfg paths in this rule. - => Some(CalledSuperBeforeThis::Initial), - }, - // The state_going_into_this_rule represents whether or not we have seen an expression - // call super(). - &mut |basic_block_id, state_going_into_this_rule| { - let mut state = state_going_into_this_rule; - // Scan through the values in this basic block. - for entry in &cfg.basic_blocks[*basic_block_id] { - let mut should_clear = true; - match entry { - // If the element is an assignment. - // - // Everything you can write in javascript that would have - // the function continue are expressed as assignments in the cfg. - BasicBlockElement::Assignment(to_reg, val) => { - match val { - // If the assignment value is super, we are either just - // accessing this, or going to do something further with it, - // so let's take note of the register holding the super. - AssignmentValue::Super => { - should_clear = false; - registers_currently_with_super_in_them.insert(to_reg); - } - // Same as super, but for this - AssignmentValue::This => { - should_clear = false; - registers_currently_with_this_in_them.insert(to_reg); - } - AssignmentValue::CalleeWithArguments(b) - if matches!(b.call_type, CallType::CallExpression) => - { - let CalleeWithArgumentsAssignmentValue { callee, .. } = &**b; - // If we see a CallExpression with a callee that is a super, - // we know that this path has now called super() and is free - // to use super and this. - // - // We could also flag a diagnostic if the path calls this(), - // however this isn't semantically meaningful or tested so - // we do not. - if registers_currently_with_super_in_them.contains(callee) { - state = CalledSuperBeforeThis::SuperCalled; - } - } - // If we see an object property access, check if we are accessing - // this or super, if so check if we have already called super(). - // If not, flag the diagnostic. - AssignmentValue::ObjectPropertyAccess(b) => { - let ObjectPropertyAccessAssignmentValue { - id, access_on, .. - } = &**b; - if !matches!(state, CalledSuperBeforeThis::SuperCalled) - && (registers_currently_with_super_in_them - .contains(access_on) - || registers_currently_with_this_in_them - .contains(access_on)) - { - ctx.diagnostic(NoThisBeforeSuperDiagnostic( - ctx.nodes().get_node(*id).kind().span(), - )); - } - } - _ => {} - } - - // Any assignments to registers other than super / this mean - // that this register is no longer significant to us. - if should_clear { - registers_currently_with_super_in_them.remove(to_reg); - registers_currently_with_this_in_them.remove(to_reg); - } - } - // No need to keep following this path if there is an unreachable as - // we simply can not do anything this rule checks for if there is an - // unreachable or throw. - BasicBlockElement::Unreachable | BasicBlockElement::Throw(_) => { - return (state, false); - } - } - } - - // Return the current state going into the next basic block on this path, - // returning true to indicate continue walking this path. - (state, true) - }, - ); - } -} - -#[derive(Default, Copy, Clone, Debug)] -enum CalledSuperBeforeThis { - #[default] - Initial, - SuperCalled, } impl Rule for NoThisBeforeSuper { - fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { - match node.kind() { - AstKind::Function(Function { .. }) - | AstKind::ArrowExpression(ArrowExpression { .. }) => { - Self::run_diagnostic(node, ctx); - } + fn run_once(&self, ctx: &LintContext) { + let semantic = ctx.semantic(); + let cfg = semantic.cfg(); - _ => {} + // first pass -> find super calls and local violations + let mut wanted_nodes = Vec::new(); + let mut basic_blocks_with_super_called = HashSet::::new(); + let mut basic_blocks_with_local_violations = HashMap::>::new(); + for node in semantic.nodes().iter() { + match node.kind() { + AstKind::Function(_) | AstKind::ArrowExpression(_) => { + if Self::is_wanted_node(node, ctx) { + wanted_nodes.push(node); + } + } + AstKind::Super(_) => { + let basic_block_id = node.cfg_ix(); + if let Some(parent) = semantic.nodes().parent_node(node.id()) { + if let AstKind::CallExpression(_) = parent.kind() { + // Note: we don't need to worry about also having invalid + // usage in the same callexpression, because arguments are visited + // before the callee in generating the semantic nodes. + basic_blocks_with_super_called.insert(basic_block_id); + } + } + if !basic_blocks_with_super_called.contains(&basic_block_id) { + basic_blocks_with_local_violations + .entry(basic_block_id) + .or_default() + .push(node.id()); + } + } + AstKind::ThisExpression(_) => { + let basic_block_id = node.cfg_ix(); + if !basic_blocks_with_super_called.contains(&basic_block_id) { + basic_blocks_with_local_violations + .entry(basic_block_id) + .or_default() + .push(node.id()); + } + } + _ => {} + } + } + + // second pass, walk cfg for wanted nodes and propagate + // cross-block super calls: + for node in wanted_nodes { + let output = neighbors_filtered_by_edge_weight( + &cfg.graph, + node.cfg_ix(), + &|edge| match edge { + EdgeType::Normal => None, + EdgeType::Backedge | EdgeType::NewFunction => { + Some(DefinitelyCallsThisBeforeSuper::No) + } + }, + &mut |basic_block_id, _| { + let super_called = basic_blocks_with_super_called.contains(basic_block_id); + if basic_blocks_with_local_violations.contains_key(basic_block_id) { + // super was not called before this in the current code path: + return (DefinitelyCallsThisBeforeSuper::Yes, false); + } + + if super_called { + (DefinitelyCallsThisBeforeSuper::No, false) + } else { + (DefinitelyCallsThisBeforeSuper::No, true) + } + }, + ); + + // Deciding whether we definitely call this before super in all + // codepaths is as simple as seeing if any individual codepath + // definitely calls this before super. + let violation_in_any_codepath = + output.into_iter().any(|y| matches!(y, DefinitelyCallsThisBeforeSuper::Yes)); + + // If not, flag it as a diagnostic. + if violation_in_any_codepath { + // the parent must exist, because of Self::is_wanted_node + // so the unwrap() is safe here. The parent node is the + // AstKind::MethodDefinition for `constructor`. + let parent_span = ctx.nodes().parent_node(node.id()).unwrap().kind().span(); + ctx.diagnostic(NoThisBeforeSuperDiagnostic(parent_span)); + } } } @@ -205,6 +165,13 @@ impl Rule for NoThisBeforeSuper { } } +#[derive(Default, Copy, Clone, Debug)] +enum DefinitelyCallsThisBeforeSuper { + #[default] + No, + Yes, +} + #[test] fn test() { use crate::tester::Tester; @@ -316,6 +283,51 @@ fn test() { ("class A extends B { constructor() { foo &&= super().a; this.c(); } }", None), ("class A extends B { constructor() { foo ||= super().a; this.c(); } }", None), ("class A extends B { constructor() { foo ??= super().a; this.c(); } }", None), + ("class A extends B { constructor() { if (foo) { if (bar) { } super(); } this.a(); }}", None), + ("class A extends B { + constructor() { + if (foo) { + } else { + super(); + } + this.a(); + } + }", None), + ("class A extends B { + constructor() { + try { + call(); + } finally { + this.a(); + } + } + }", None), + ("class A extends B { + constructor() { + while (foo) { + super(); + } + this.a(); + } + }", None), + ("class A extends B { + constructor() { + while (foo) { + this.a(); + super(); + } + } + }", None), + ("class A extends B { + constructor() { + while (foo) { + if (init) { + this.a(); + super(); + } + } + } + }", None), ]; Tester::new(NoThisBeforeSuper::NAME, pass, fail).test_and_snapshot(); diff --git a/crates/oxc_linter/src/snapshots/no_this_before_super.snap b/crates/oxc_linter/src/snapshots/no_this_before_super.snap new file mode 100644 index 000000000..a394e4871 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_this_before_super.snap @@ -0,0 +1,200 @@ +--- +source: crates/oxc_linter/src/tester.rs +expression: no_this_before_super +--- + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { this.c = 0; } } + · ───────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { this.c(); } } + · ─────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { super.c(); } } + · ──────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { this.c = 0; super(); } } + · ────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { this.c(); super(); } } + · ──────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { super.c(); super(); } } + · ───────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { super(this.c); } } + · ──────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { super(this.c()); } } + · ────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { super(super.c()); } } + · ─────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { class C extends D { constructor() { super(); this.e(); } } this.f(); super(); } } + · ─────────────────────────────────────────────────────────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:57] + 1 │ class A extends B { constructor() { class C extends D { constructor() { this.e(); super(); } } super(); this.f(); } } + · ──────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { if (a) super(); this.a(); } } + · ─────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { try { super(); } finally { this.a; } } } + · ────────────────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { try { super(); } catch (err) { } this.a; } } + · ────────────────────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { foo &&= super().a; this.c(); } } + · ────────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { foo ||= super().a; this.c(); } } + · ────────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { foo ??= super().a; this.c(); } } + · ────────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:1:21] + 1 │ class A extends B { constructor() { if (foo) { if (bar) { } super(); } this.a(); }} + · ────────────────────────────────────────────────────────────── + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:2:17] + 1 │ class A extends B { + 2 │ ╭─▶ constructor() { + 3 │ │ if (foo) { + 4 │ │ } else { + 5 │ │ super(); + 6 │ │ } + 7 │ │ this.a(); + 8 │ ╰─▶ } + 9 │ } + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:2:17] + 1 │ class A extends B { + 2 │ ╭─▶ constructor() { + 3 │ │ try { + 4 │ │ call(); + 5 │ │ } finally { + 6 │ │ this.a(); + 7 │ │ } + 8 │ ╰─▶ } + 9 │ } + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:2:17] + 1 │ class A extends B { + 2 │ ╭─▶ constructor() { + 3 │ │ while (foo) { + 4 │ │ super(); + 5 │ │ } + 6 │ │ this.a(); + 7 │ ╰─▶ } + 8 │ } + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:2:17] + 1 │ class A extends B { + 2 │ ╭─▶ constructor() { + 3 │ │ while (foo) { + 4 │ │ this.a(); + 5 │ │ super(); + 6 │ │ } + 7 │ ╰─▶ } + 8 │ } + ╰──── + help: Call super() before this/super property access. + + ⚠ eslint(no-this-before-super): Expected to always call super() before this/super property access. + ╭─[no_this_before_super.tsx:2:17] + 1 │ class A extends B { + 2 │ ╭─▶ constructor() { + 3 │ │ while (foo) { + 4 │ │ if (init) { + 5 │ │ this.a(); + 6 │ │ super(); + 7 │ │ } + 8 │ │ } + 9 │ ╰─▶ } + 10 │ } + ╰──── + help: Call super() before this/super property access. +