Skip to content

Commit

Permalink
feat(linter): Implement no_this_before_super with cfg (#2254)
Browse files Browse the repository at this point in the history 
Implements `eslint/no-this-before-super` in #479.

Closes #2279
  • Loading branch information
@TzviPM
TzviPM authored Feb 4, 2024
1 parent d2b304b commit 0060d6a
Show file tree
Hide file tree
Showing 3 changed files with 345 additions and 133 deletions.
4 changes: 2 additions & 2 deletions crates/oxc_linter/src/rules.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
274 changes: 143 additions & 131 deletions crates/oxc_linter/src/rules/eslint/no_this_before_super.rs
View file
Original file line number Diff line number Diff line change
@@ -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::{
Expand All @@ -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};

Expand All @@ -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();
/// }
/// }
/// ```
Expand Down Expand Up @@ -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(),
));
}
}
_ => {}
}
impl Rule for NoThisBeforeSuper {
fn run_once(&self, ctx: &LintContext) {
let semantic = ctx.semantic();
let cfg = semantic.cfg();

// 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);
// 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());
}
}
_ => {}
}
}

// Return the current state going into the next basic block on this path,
// returning true to indicate continue walking this path.
(state, true)
},
);
}
}
// 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);
}

#[derive(Default, Copy, Clone, Debug)]
enum CalledSuperBeforeThis {
#[default]
Initial,
SuperCalled,
}
if super_called {
(DefinitelyCallsThisBeforeSuper::No, false)
} else {
(DefinitelyCallsThisBeforeSuper::No, true)
}
},
);

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);
}
// 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));
}
}
}

Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Loading

0 comments on commit 0060d6a

Please sign in to comment.