feat(linter): Implement no_this_before_super with cfg (#2254)

Implements `eslint/no-this-before-super` in #479.

Closes #2279
This commit is contained in:
Tzvi Melamed 2024-02-04 00:51:04 -05:00 committed by GitHub
parent d2b304b1f8
commit 0060d6a730
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 350 additions and 138 deletions

View file

@ -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,

View file

@ -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::<NodeIndex>::new();
let mut basic_blocks_with_local_violations = HashMap::<NodeIndex, Vec<AstNodeId>>::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();

View file

@ -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.