Skip to content

Commit

Permalink
feat(linter): eslint-lugin-unicorn no_useless_length_check (#1541)
Browse files Browse the repository at this point in the history 
I have an issue with this rule. I am able to identify the condition that
breaks the rule, but I can't manage to get the fix done. I was thinking
of concatenating the valid conditions with the common operator, but I am
not sure how to do that (from `Expression` to turn to `string` to be
passed as a parameter to the fix). any help is appreciated.

NOTE: I will probably do some refactorization for this code after
implementing the fix

---------

Co-authored-by: Radu Baston <radu.baston@sectorlabs.ro>
  • Loading branch information
@radu2147
radu2147 and Radu Baston authored Nov 27, 2023
1 parent e1704d9 commit afeed17
Show file tree
Hide file tree
Showing 4 changed files with 497 additions and 0 deletions.
5 changes: 5 additions & 0 deletions crates/oxc_ast/src/ast/js.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ impl<'a> Expression<'a> {
matches!(self, Self::NumberLiteral(lit) if lit.value == 0.0)
}

/// Determines whether the given numeral literal's raw value is exactly val
pub fn is_specific_raw_number_literal(&self, val: &str) -> bool {
matches!(self, Self::NumberLiteral(lit) if lit.raw == val)
}

/// Determines whether the given expr evaluate to `undefined`
pub fn evaluate_to_undefined(&self) -> bool {
self.is_undefined() || self.is_void()
Expand Down
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ mod unicorn {
pub mod no_typeof_undefined;
pub mod no_unnecessary_await;
pub mod no_useless_fallback_in_spread;
pub mod no_useless_length_check;
pub mod no_useless_promise_resolve_reject;
pub mod no_useless_switch_case;
pub mod no_zero_fractions;
Expand Down Expand Up @@ -347,6 +348,7 @@ oxc_macros::declare_all_lint_rules! {
unicorn::no_typeof_undefined,
unicorn::no_unnecessary_await,
unicorn::no_useless_fallback_in_spread,
unicorn::no_useless_length_check,
unicorn::no_useless_promise_resolve_reject,
unicorn::no_useless_switch_case,
unicorn::no_zero_fractions,
Expand Down
296 changes: 296 additions & 0 deletions crates/oxc_linter/src/rules/unicorn/no_useless_length_check.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,296 @@
use itertools::concat;
use oxc_diagnostics::{
miette::{self, Diagnostic},
thiserror::Error,
};
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use oxc_syntax::operator::{BinaryOperator, LogicalOperator};
use std::fmt::Debug;

use oxc_ast::{
ast::{Expression, LogicalExpression},
AstKind,
};

use crate::{context::LintContext, rule::Rule, AstNode};

#[derive(Debug, Error, Diagnostic)]
enum NoUselessLengthCheckDiagnostic {
#[error("eslint-plugin-unicorn(no-useless-length-check)")]
#[diagnostic(
severity(warning),
help(
"The non-empty check is useless as `Array#some()` returns `false` for an empty array."
)
)]
Some(#[label] Span),
#[error("eslint-plugin-unicorn(no-useless-length-check)")]
#[diagnostic(
severity(warning),
help("The empty check is useless as `Array#every()` returns `true` for an empty array.")
)]
Every(#[label] Span),
}

#[derive(Debug, Default, Clone)]
pub struct NoUselessLengthCheck;

declare_oxc_lint!(
/// ### What it does
/// It checks for an unnecessary array length check in a logical expression
/// The cases are:
/// array.length === 0 || array.every(Boolean) (array.every returns true if array is has elements)
/// array.length > 0 && array.some(Boolean) (array.some returns false if array is empty)
///
/// ### Why is this bad?
/// An extra unnecessary length check is done
///
/// ### Example
/// ```javascript
/// if(array.length === 0 || array.every(Boolean)){
/// do something!
/// }
///
/// ```
NoUselessLengthCheck,
correctness
);

struct ConditionDTO<T: ToString> {
property_name: T,
binary_operators: Vec<BinaryOperator>,
}

fn is_useless_check<'a>(
left: &'a Expression<'a>,
right: &'a Expression<'a>,
operator: LogicalOperator,
) -> Option<NoUselessLengthCheckDiagnostic> {
let every_condition = ConditionDTO {
property_name: "every",
binary_operators: vec![BinaryOperator::StrictEquality],
};
let some_condition = ConditionDTO {
property_name: "some",
binary_operators: vec![BinaryOperator::StrictInequality, BinaryOperator::GreaterThan],
};

let mut array_name: &str = "";
let active_condition = {
if operator == LogicalOperator::Or {
every_condition
} else {
some_condition
}
};
let mut binary_expression_span: Option<Span> = None;
let mut call_expression_span: Option<Span> = None;

let l = match left.without_parenthesized() {
Expression::BinaryExpression(expr) => {
let Expression::MemberExpression(left_expr) = expr.left.get_inner_expression() else {
return None;
};
array_name = left_expr.object().get_identifier_reference()?.name.as_str();
binary_expression_span = Some(expr.span);

active_condition.binary_operators.contains(&expr.operator)
&& left_expr.is_specific_member_access(array_name, "length")
&& expr.right.is_specific_raw_number_literal("0")
}
Expression::CallExpression(expr) => {
array_name =
expr.callee.get_member_expr()?.object().get_identifier_reference()?.name.as_str();
let property_name = expr.callee.get_member_expr()?.static_property_name()?;
call_expression_span = Some(expr.span);

let is_same_method = property_name == active_condition.property_name;
let is_optional = expr.optional;

is_same_method && !is_optional
}
_ => false,
};

let r = match right.without_parenthesized() {
Expression::BinaryExpression(expr) => {
let Expression::MemberExpression(left_expr) = expr.left.get_inner_expression() else {
return None;
};
let ident_name = left_expr.object().get_identifier_reference()?.name.as_str();
if binary_expression_span.is_some() {
return None;
}
binary_expression_span = Some(expr.span);

active_condition.binary_operators.contains(&expr.operator)
&& left_expr.is_specific_member_access(array_name, "length")
&& expr.right.is_specific_raw_number_literal("0")
&& ident_name == array_name
}
Expression::CallExpression(expr) => {
let is_same_name =
expr.callee.get_member_expr()?.object().get_identifier_reference()?.name.as_str()
== array_name;

if call_expression_span.is_some() {
return None;
}
let property_name = expr.callee.get_member_expr()?.static_property_name()?;
let is_same_method = property_name == active_condition.property_name;
let is_optional = expr.optional;

is_same_method && !is_optional && is_same_name
}
_ => false,
};

if l && r {
Some(if active_condition.property_name == "every" {
NoUselessLengthCheckDiagnostic::Every(binary_expression_span?)
} else {
NoUselessLengthCheckDiagnostic::Some(binary_expression_span?)
})
} else {
None
}
}

impl Rule for NoUselessLengthCheck {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
if let AstKind::LogicalExpression(log_expr) = node.kind() {
if ![LogicalOperator::And, LogicalOperator::Or].contains(&log_expr.operator) {
return;
}
let flat_expr = flat_logical_expression(log_expr);
for i in 0..flat_expr.len() - 1 {
if let Some(diag) =
is_useless_check(flat_expr[i], flat_expr[i + 1], log_expr.operator)
{
ctx.diagnostic(diag);
}
}
};
}
}

fn flat_logical_expression<'a>(node: &'a LogicalExpression<'a>) -> Vec<&'a Expression<'a>> {
let left = match &node.left.without_parenthesized() {
Expression::LogicalExpression(le) => {
if le.operator == node.operator {
flat_logical_expression(le)
} else {
vec![&node.left]
}
}
_ => vec![&node.left],
};

let right = match &node.right.without_parenthesized() {
Expression::LogicalExpression(le) => {
if le.operator == node.operator {
flat_logical_expression(le)
} else {
vec![&node.right]
}
}
_ => vec![&node.right],
};

concat(vec![left, right])
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
"array.length === 0 ?? array.every(Boolean)",
"array.length === 0 && array.every(Boolean)",
"(array.length === 0) + (array.every(Boolean))",
"array.length === 1 || array.every(Boolean)",
"array.length === \"0\" || array.every(Boolean)",
"array.length === 0. || array.every(Boolean)",
"array.length === 0x0 || array.every(Boolean)",
"array.length !== 0 || array.every(Boolean)",
"array.length == 0 || array.every(Boolean)",
"0 === array.length || array.every(Boolean)",
"array?.length === 0 || array.every(Boolean)",
"array.notLength === 0 || array.every(Boolean)",
"array[length] === 0 || array.every(Boolean)",
"array.length === 0 || array.every?.(Boolean)",
"array.length === 0 || array?.every(Boolean)",
"array.length === 0 || array.every",
"array.length === 0 || array[every](Boolean)",
"array1.length === 0 || array2.every(Boolean)",
"array.length !== 0 ?? array.some(Boolean)",
"array.length !== 0 || array.some(Boolean)",
"(array.length !== 0) - (array.some(Boolean))",
"array.length !== 1 && array.some(Boolean)",
"array.length !== \"0\" && array.some(Boolean)",
"array.length !== 0. && array.some(Boolean)",
"array.length !== 0x0 && array.some(Boolean)",
"array.length === 0 && array.some(Boolean)",
"array.length <= 0 && array.some(Boolean)",
"array.length != 0 && array.some(Boolean)",
"0 !== array.length && array.some(Boolean)",
"array?.length !== 0 && array.some(Boolean)",
"array.notLength !== 0 && array.some(Boolean)",
"array[length] !== 0 && array.some(Boolean)",
"array.length !== 0 && array.some?.(Boolean)",
"array.length !== 0 && array?.some(Boolean)",
"array.length !== 0 && array.some",
"array.length !== 0 && array.notSome(Boolean)",
"array.length !== 0 && array[some](Boolean)",
"array1.length !== 0 && array2.some(Boolean)",
"array.length > 0 ?? array.some(Boolean)",
"array.length > 0 || array.some(Boolean)",
"(array.length > 0) - (array.some(Boolean))",
"array.length > 1 && array.some(Boolean)",
"array.length > \"0\" && array.some(Boolean)",
"array.length > 0. && array.some(Boolean)",
"array.length > 0x0 && array.some(Boolean)",
"array.length >= 0 && array.some(Boolean)",
"0 > array.length && array.some(Boolean)",
"0 < array.length && array.some(Boolean)",
"array?.length > 0 && array.some(Boolean)",
"array.notLength > 0 && array.some(Boolean)",
"array.length > 0 && array.some?.(Boolean)",
"array.length > 0 && array?.some(Boolean)",
"array.length > 0 && array.some",
"array.length > 0 && array.notSome(Boolean)",
"array.length > 0 && array[some](Boolean)",
"array1.length > 0 && array2.some(Boolean)",
"(foo && array.length === 0) || array.every(Boolean) && foo",
"array.length === 0 || (array.every(Boolean) && foo)",
"(foo || array.length > 0) && array.some(Boolean)",
"array.length > 0 && (array.some(Boolean) || foo)",
"array.length === 0 || array.length === 0",
"array.some(Boolean) && array.some(Boolean)",
];

let fail = vec![
"array.length === 0 || array.every(Boolean)",
"array.length > 0 && array.some(Boolean)",
"array.length !== 0 && array.some(Boolean)",
"if ((( array.length > 0 )) && array.some(Boolean));",
"(array.length === 0 || array.every(Boolean)) || foo",
"foo || (array.length === 0 || array.every(Boolean))",
"(array.length > 0 && array.some(Boolean)) && foo",
"foo && (array.length > 0 && array.some(Boolean))",
"array.every(Boolean) || array.length === 0",
"array.some(Boolean) && array.length !== 0",
"array.some(Boolean) && array.length > 0",
"foo && array.length > 0 && array.some(Boolean)",
"foo || array.length === 0 || array.every(Boolean)",
"(foo || array.length === 0) || array.every(Boolean)",
"array.length === 0 || (array.every(Boolean) || foo)",
"(foo && array.length > 0) && array.some(Boolean)",
"array.length > 0 && (array.some(Boolean) && foo)",
"array.every(Boolean) || array.length === 0 || array.every(Boolean)",
"array.length === 0 || array.every(Boolean) || array.length === 0",
];

Tester::new_without_config(NoUselessLengthCheck::NAME, pass, fail).test_and_snapshot();
}
Loading

0 comments on commit afeed17

Please sign in to comment.