Skip to content

Commit

Permalink
Merge branch 'main' into c/08-23-feat_linter_add_partial_fixer_for_pr…
Browse files Browse the repository at this point in the history 
…efer-array-flat_
  • Loading branch information
@DonIsaac
DonIsaac authored Aug 24, 2024
2 parents 58931c6 + fd1031a commit be193de
Show file tree
Hide file tree
Showing 4 changed files with 273 additions and 163 deletions.
20 changes: 19 additions & 1 deletion crates/oxc_linter/src/ast_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::hash::{Hash, Hasher};

use oxc_ast::ast::BindingIdentifier;
use oxc_ast::AstKind;
use oxc_semantic::{AstNode, SymbolId};
use oxc_semantic::{AstNode, AstNodeId, SymbolId};
use oxc_span::{GetSpan, Span};
use oxc_syntax::operator::{AssignmentOperator, BinaryOperator, LogicalOperator, UnaryOperator};
use rustc_hash::FxHasher;
Expand Down Expand Up @@ -251,6 +251,24 @@ pub fn nth_outermost_paren_parent<'a, 'b>(
.filter(|parent| !matches!(parent.kind(), AstKind::ParenthesizedExpression(_)))
.nth(n)
}
/// Iterate over parents of `node`, skipping nodes that are also ignored by
/// [`Expression::get_inner_expression`].
pub fn iter_outer_expressions<'a, 'ctx>(
ctx: &'ctx LintContext<'a>,
node_id: AstNodeId,
) -> impl Iterator<Item = &'ctx AstNode<'a>> + 'ctx {
ctx.nodes().iter_parents(node_id).skip(1).filter(|parent| {
!matches!(
parent.kind(),
AstKind::ParenthesizedExpression(_)
| AstKind::TSAsExpression(_)
| AstKind::TSSatisfiesExpression(_)
| AstKind::TSInstantiationExpression(_)
| AstKind::TSNonNullExpression(_)
| AstKind::TSTypeAssertion(_)
)
})
}

pub fn get_declaration_of_variable<'a, 'b>(
ident: &IdentifierReference,
Expand Down
229 changes: 144 additions & 85 deletions crates/oxc_linter/src/rules/unicorn/no_null.rs
View file
Original file line number Diff line number Diff line change
@@ -1,37 +1,31 @@
use oxc_ast::{
ast::{
Argument, BinaryExpression, CallExpression, Expression, NullLiteral, VariableDeclarator,
Argument, BinaryExpression, CallExpression, Expression, NullLiteral, SwitchStatement,
VariableDeclarator,
},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::{GetSpan, Span};
use oxc_syntax::operator::BinaryOperator;
use serde_json::Value;

use crate::{
ast_util::is_method_call,
ast_util::{is_method_call, iter_outer_expressions},
context::LintContext,
fixer::{RuleFix, RuleFixer},
rule::Rule,
AstNode,
};

fn replace_null_diagnostic(span0: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow the use of the `null` literal")
.with_help("Replace the `null` literal with `undefined`.")
.with_label(span0)
}

fn remove_null_diagnostic(span0: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow the use of the `null` literal")
.with_help("Remove the `null` literal.")
.with_label(span0)
fn no_null_diagnostic(null: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("Do not use `null` literals").with_label(null)
}

#[derive(Debug, Default, Clone)]
pub struct NoNull {
check_strict_equality: Option<bool>,
check_strict_equality: bool,
}

declare_oxc_lint!(
Expand All @@ -56,48 +50,51 @@ declare_oxc_lint!(
/// ```
NoNull,
style,
fix
conditional_fix
);

fn match_null_arg(call_expr: &CallExpression, index: usize, span: Span) -> bool {
call_expr.arguments.get(index).map_or(false, |arg| {
if let Argument::NullLiteral(null_lit) = arg {
return null_lit.span == span;
}

false
})
}

fn diagnose_binary_expression(
no_null: &NoNull,
ctx: &LintContext,
null_literal: &NullLiteral,
binary_expr: &BinaryExpression,
) {
// checkStrictEquality=false && `if (foo !== null) {}`
if !no_null.check_strict_equality.is_some_and(|val| val)
&& matches!(
binary_expr.operator,
BinaryOperator::StrictEquality | BinaryOperator::StrictInequality
)
match call_expr
.arguments
.get(index)
.and_then(Argument::as_expression)
.map(Expression::get_inner_expression)
{
return;
Some(Expression::NullLiteral(null_lit)) => span.contains_inclusive(null_lit.span),
_ => false,
}
}

// `if (foo != null) {}`
if matches!(binary_expr.operator, BinaryOperator::Equality | BinaryOperator::Inequality) {
ctx.diagnostic_with_fix(replace_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
impl NoNull {
fn diagnose_binary_expression(
&self,
ctx: &LintContext,
null_literal: &NullLiteral,
binary_expr: &BinaryExpression,
) {
match binary_expr.operator {
// `if (foo != null) {}`
BinaryOperator::Equality | BinaryOperator::Inequality => {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
}

return;
// `if (foo !== null) {}`
BinaryOperator::StrictEquality | BinaryOperator::StrictInequality => {
if self.check_strict_equality {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
}
}
_ => {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
}
}
}

// checkStrictEquality=true && `if (foo !== null) {}`
ctx.diagnostic_with_fix(replace_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
}

fn diagnose_variable_declarator(
Expand All @@ -110,15 +107,15 @@ fn diagnose_variable_declarator(
if matches!(&variable_declarator.init, Some(Expression::NullLiteral(expr)) if expr.span == null_literal.span)
&& matches!(parent_kind, Some(AstKind::VariableDeclaration(var_declaration)) if !var_declaration.kind.is_const() )
{
ctx.diagnostic_with_fix(remove_null_diagnostic(null_literal.span), |fixer| {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fixer.delete_range(Span::new(variable_declarator.id.span().end, null_literal.span.end))
});

return;
}

// `const foo = null`
ctx.diagnostic_with_fix(replace_null_diagnostic(null_literal.span), |fixer| {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
}
Expand Down Expand Up @@ -164,12 +161,13 @@ fn match_call_expression_pass_case(null_literal: &NullLiteral, call_expr: &CallE
}

impl Rule for NoNull {
fn from_configuration(value: serde_json::Value) -> Self {
fn from_configuration(value: Value) -> Self {
Self {
check_strict_equality: value
.get(0)
.and_then(|v| v.get("checkStrictEquality"))
.and_then(serde_json::Value::as_bool),
.and_then(Value::as_bool)
.unwrap_or_default(),
}
}

Expand All @@ -178,52 +176,67 @@ impl Rule for NoNull {
return;
};

if let Some(parent_node) = ctx.nodes().parent_node(node.id()) {
let grand_parent_kind = ctx.nodes().parent_kind(parent_node.id());
let mut parents = iter_outer_expressions(ctx, node.id());
let Some(parent_kind) = parents.next().map(AstNode::kind) else {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
return;
};

if matches!(parent_node.kind(), AstKind::Argument(_)) {
if let Some(AstKind::CallExpression(call_expr)) = grand_parent_kind {
if match_call_expression_pass_case(null_literal, call_expr) {
return;
}
}
let grandparent_kind = parents.next().map(AstNode::kind);
match (parent_kind, grandparent_kind) {
(AstKind::Argument(_), Some(AstKind::CallExpression(call_expr)))
if match_call_expression_pass_case(null_literal, call_expr) =>
{
// no violation
}

if let AstKind::BinaryExpression(binary_expr) = parent_node.kind() {
diagnose_binary_expression(self, ctx, null_literal, binary_expr);
return;
(AstKind::BinaryExpression(binary_expr), _) => {
self.diagnose_binary_expression(ctx, null_literal, binary_expr);
}

if let AstKind::VariableDeclarator(variable_declarator) = parent_node.kind() {
diagnose_variable_declarator(
ctx,
null_literal,
variable_declarator,
grand_parent_kind,
);

return;
(AstKind::VariableDeclarator(decl), _) => {
diagnose_variable_declarator(ctx, null_literal, decl, grandparent_kind);
}

// `function foo() { return null; }`,
if matches!(parent_node.kind(), AstKind::ReturnStatement(_)) {
ctx.diagnostic_with_fix(remove_null_diagnostic(null_literal.span), |fixer| {
fixer.delete_range(null_literal.span)
(AstKind::ReturnStatement(_), _) => {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fixer.delete(null_literal)
});
}
(AstKind::SwitchCase(_), Some(AstKind::SwitchStatement(switch))) => {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
try_fix_case(fixer, null_literal, switch)
});
}
_ => {
ctx.diagnostic_with_fix(no_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});

return;
}
}

ctx.diagnostic_with_fix(replace_null_diagnostic(null_literal.span), |fixer| {
fix_null(fixer, null_literal)
});
}
}

fn fix_null<'a>(fixer: RuleFixer<'_, 'a>, null: &NullLiteral) -> RuleFix<'a> {
fixer.replace(null.span, "undefined")
}

fn try_fix_case<'a>(
fixer: RuleFixer<'_, 'a>,
null: &NullLiteral,
switch: &SwitchStatement<'a>,
) -> RuleFix<'a> {
let also_has_undefined = switch
.cases
.iter()
.filter_map(|case| case.test.as_ref())
.any(|test| test.get_inner_expression().is_undefined());
if also_has_undefined {
fixer.noop()
} else {
fixer.replace(null.span, "undefined")
}
}

#[test]
fn test() {
use crate::tester::Tester;
Expand All @@ -237,13 +250,16 @@ fn test() {
let pass = vec![
("let foo", None),
("Object.create(null)", None),
("Object.create(null as any)", None),
("Object.create(null, {foo: {value:1}})", None),
("let insertedNode = parentNode.insertBefore(newNode, null)", None),
("const foo = \"null\";", None),
("Object.create()", None),
("Object.create(bar)", None),
("Object.create(\"null\")", None),
("useRef(null)", None),
("useRef(((((null)))))", None),
("useRef(null as unknown as HTMLElement)", None),
("React.useRef(null)", None),
("if (foo === null) {}", None),
("if (null === foo) {}", None),
Expand All @@ -254,6 +270,7 @@ fn test() {
("if (null === foo) {}", Some(check_strict_equality(false))),
("if (foo !== null) {}", Some(check_strict_equality(false))),
("if (null !== foo) {}", Some(check_strict_equality(false))),
("if (foo === null || foo === undefined) {}", None),
];

let fail = vec![
Expand All @@ -263,6 +280,7 @@ fn test() {
("if (foo != null) {}", None),
("if (null == foo) {}", None),
("if (null != foo) {}", None),
("let curr;\nwhile (curr != null) { curr = stack.pop() }", None),
// Suggestion `ReturnStatement`
(
"function foo() {
Expand Down Expand Up @@ -304,12 +322,53 @@ fn test() {
("Object.create(...[null])", None),
("Object.create(null, bar, extraArgument)", None),
("foo.insertBefore(null)", None),
("foo.insertBefore(null as any)", None),
("foo.insertBefore(foo, null, bar)", None),
("foo.insertBefore(...[foo], null)", None),
// Not in right position
("foo.insertBefore(null, bar)", None),
("Object.create(bar, null)", None),
];

Tester::new(NoNull::NAME, pass, fail).test_and_snapshot();
let fix = vec![
("let x = null;", "let x;", None),
("let x = null as any;", "let x = undefined as any;", None),
("if (foo == null) {}", "if (foo == undefined) {}", None),
("if (foo != null) {}", "if (foo != undefined) {}", None),
("if (foo == null) {}", "if (foo == undefined) {}", Some(check_strict_equality(true))),
(
"
let isNullish;
switch (foo) {
case null:
case undefined:
isNullish = true;
break;
default:
isNullish = false;
break;
}
",
"
let isNullish;
switch (foo) {
case null:
case undefined:
isNullish = true;
break;
default:
isNullish = false;
break;
}
",
None,
),
// FIXME
(
"if (foo === null || foo === undefined) {}",
"if (foo === undefined || foo === undefined) {}",
Some(check_strict_equality(true)),
),
];
Tester::new(NoNull::NAME, pass, fail).expect_fix(fix).test_and_snapshot();
}
Loading

0 comments on commit be193de

Please sign in to comment.