diff --git a/crates/oxc_linter/src/lib.rs b/crates/oxc_linter/src/lib.rs index 47512e06b..1f3dce2e3 100644 --- a/crates/oxc_linter/src/lib.rs +++ b/crates/oxc_linter/src/lib.rs @@ -1,4 +1,4 @@ -#![feature(let_chains, is_some_and)] +#![feature(let_chains, is_some_and, const_trait_impl, const_slice_index)] #[cfg(test)] mod tester; diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index f6b2c3f14..26d5e0728 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -6,5 +6,6 @@ oxc_macros::declare_all_lint_rules! { no_array_constructor, no_empty, no_empty_pattern, + no_mixed_operators, deepscan::uninvoked_array_callback, } diff --git a/crates/oxc_linter/src/rules/no_mixed_operators.rs b/crates/oxc_linter/src/rules/no_mixed_operators.rs new file mode 100644 index 000000000..15db6ad0d --- /dev/null +++ b/crates/oxc_linter/src/rules/no_mixed_operators.rs @@ -0,0 +1,336 @@ +use oxc_ast::{AstKind, GetSpan, Span}; +use oxc_diagnostics::{ + miette::{self, Diagnostic}, + thiserror::Error, +}; +use oxc_macros::declare_oxc_lint; +use oxc_semantic::AstNode; + +use crate::{context::LintContext, rule::Rule}; + +#[derive(Debug, Error, Diagnostic)] +#[error("eslint(no-mixed-operators): Unexpected mix of {0:?} with {1:?}")] +#[diagnostic( + severity(warning), + help("Use parentheses to clarify the intended order of operations.") +)] +struct NoMixedOperatorsDiagnostic( + &'static str, /*Node Operator */ + &'static str, /*Parent Operator */ + #[label] pub Span, /*Span of the node operator */ + #[label] pub Span, /*Span of the parent operator */ +); + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct NoMixedOperators { + /// Disallow Mixed operators within one group. + groups: Vec>, + /// Allow operators of the same precedence to be mixed. + allow_same_precedence: bool, +} + +impl Default for NoMixedOperators { + fn default() -> Self { + Self { groups: default_groups(), allow_same_precedence: true } + } +} + +declare_oxc_lint! { + /// ### What it does + /// Disallow mixed binary operators. + /// + /// ### Why is this bad? + /// Enclosing complex expressions by parentheses clarifies the developer’s intention, + /// which makes the code more readable. This rule warns when different operators + /// are used consecutively without parentheses in an expression. + /// + /// ### Examples + /// ```javascript + /// var foo = a && b || c || d; /*BAD: Unexpected mix of '&&' and '||'.*/ + /// var foo = (a && b) || c || d; /*GOOD*/ + /// var foo = a && (b || c || d); /*GOOD*/ + /// ``` + NoMixedOperators, + correctness +} + +impl Rule for NoMixedOperators { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let node_kind = node.get().kind(); + if !matches!(node_kind, AstKind::BinaryExpression(_) | AstKind::LogicalExpression(_)) { + return; + } + + let parent_kind = ctx.parent_kind(node); + if !matches!( + parent_kind, + AstKind::BinaryExpression(_) + | AstKind::LogicalExpression(_) + | AstKind::ConditionalExpression(_) + ) { + return; + } + + // Now we know that node is a BinaryExpression or LogicalExpression, and parent + // is a BinaryExpression or LogicalExpression or ConditionalExpression. + if Self::is_mixed_with_parent(node_kind, parent_kind) { + self.report(node_kind, parent_kind, ctx); + } + } + + fn from_configuration(value: serde_json::Value) -> Self { + Self::try_from_configuration(&value).unwrap_or_default() + } +} + +impl NoMixedOperators { + pub fn try_from_configuration(value: &serde_json::Value) -> Option { + let config = value.get(0)?; + + let mut groups = vec![]; + if let Some(groups_config) = config.get("groups") { + if let Some(groups_config) = groups_config.as_array() { + 'outer: for group_config in groups_config { + // Parse current group configuration. On failure fall through to next group. + if let Some(group_config) = group_config.as_array() { + let mut group = vec![]; + for val in group_config { + let Some(val) = val.as_str() else { + continue 'outer + }; + let Some((operator, _)) = operator_and_precedence(val) else { + continue 'outer + }; + group.push(operator); + } + groups.push(group); + } + } + } + } + + if groups.is_empty() { + groups = default_groups(); + } + + let allow_same_precedence = + config.get("allowSamePrecedence").map_or(true, |val| val.as_bool().unwrap_or_default()); + + Some(Self { groups, allow_same_precedence }) + } + + fn is_mixed_with_parent(node: AstKind, parent: AstKind) -> bool { + match (node, parent) { + (AstKind::BinaryExpression(node), AstKind::BinaryExpression(parent)) => { + node.operator != parent.operator + } + (AstKind::LogicalExpression(node), AstKind::LogicalExpression(parent)) => { + node.operator != parent.operator + } + _ => true, + } + // Note that there is not need to check for parenthesis explicitly because if an + // expression is parenthesized, its parent node is a ParenthesizedExpression and will + // never enter the code path. + } + + /// Report mixed operator pare between node and parent corresponding to configuration. + fn report(&self, node: AstKind, parent: AstKind, ctx: &LintContext<'_>) { + let (node_operator, node_left_span, node_right_span) = match node { + AstKind::BinaryExpression(expr) => { + (expr.operator.as_str(), expr.left.span(), expr.right.span()) + } + AstKind::LogicalExpression(expr) => { + (expr.operator.as_str(), expr.left.span(), expr.right.span()) + } + _ => unreachable!(), + }; + // Since we don't store the exact span of the operators, approximate that span to be between the lhs + // and rhs of the expression. + let node_operator_span = Span::new(node_left_span.end + 1, node_right_span.start - 1); + + let (parent_operator, parent_left_span, parent_right_span) = match parent { + AstKind::BinaryExpression(expr) => { + (expr.operator.as_str(), expr.left.span(), expr.right.span()) + } + AstKind::LogicalExpression(expr) => { + (expr.operator.as_str(), expr.left.span(), expr.right.span()) + } + AstKind::ConditionalExpression(expr) => { + // For conditional operators, the span covers both ? and : + ("?:", expr.test.span(), expr.alternate.span()) + } + _ => unreachable!(), + }; + let parent_operator_span = Span::new(parent_left_span.end + 1, parent_right_span.start - 1); + + let (node_operator, node_precedence) = operator_and_precedence(node_operator).unwrap(); + let (parent_operator, parent_precedence) = + operator_and_precedence(parent_operator).unwrap(); + if !(self.allow_same_precedence && node_precedence == parent_precedence) + && self.in_the_same_group(node_operator, parent_operator) + { + // Report error at both operators + ctx.diagnostic(NoMixedOperatorsDiagnostic( + node_operator, + parent_operator, + node_operator_span, + parent_operator_span, + )); + } + } + + fn in_the_same_group(&self, op1: &str, op2: &str) -> bool { + self.groups.iter().any(|group| { + let mut contains_op1 = false; + let mut contains_op2 = false; + for &op in group { + if op == op1 { + contains_op1 = true; + } + if op == op2 { + contains_op2 = true; + } + } + contains_op1 && contains_op2 + }) + } +} + +#[rustfmt::skip] +const OPERATORS: [&str; 27] = [ + "+", "-", "*", "/", "%", "**", /* Arithmetic operator: 6 */ + "&", "|", "^", "~", "<<", ">>", ">>>", /*Bitwise operator: 13 */ + "==", "!=", "===", "!==", ">", ">=", "<", "<=", /*Compare operator: 21 */ + "&&", "||", /*Logical operator: 23 */ + "in", "instanceof", /*Relational operator: 25 */ + "?:", /*Conditional operator */ + "??", /*Coalesce operator */ +]; + +/// `https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence#table` +#[rustfmt::skip] +const PRECEDENCES: [u8; 27] = [ + 11, 11, 12, 12, 12, 13, + 7, 5, 6, 14, 10, 10, 10, + 8, 8, 8, 8, 9, 9, 9, 9, + 4, 3, + 9, 9, + 2, + 9, +]; + +const ARITHMETIC: &[&str] = &OPERATORS[..6]; +const BITWISE: &[&str] = &OPERATORS[6..13]; +const COMPARE: &[&str] = &OPERATORS[13..21]; +const LOGICAL: &[&str] = &OPERATORS[21..23]; +const RELATIONAL: &[&str] = &OPERATORS[23..25]; + +const DEFAULT_OPERATORS: [&[&str]; 5] = [ARITHMETIC, BITWISE, COMPARE, LOGICAL, RELATIONAL]; + +#[inline] +fn default_groups() -> Vec> { + DEFAULT_OPERATORS.iter().map(|operators| operators.to_vec()).collect() +} + +#[inline] +fn operator_and_precedence(operator: &str) -> Option<(&'static str, u8)> { + OPERATORS.iter().position(|op| *op == operator).map(|idx| (OPERATORS[idx], PRECEDENCES[idx])) +} + +#[test] +fn test() { + use serde_json::json; + + use crate::tester::Tester; + + let pass = vec![ + ("a && b && c && d", None), + ("a || b || c || d", None), + ("(a || b) && c && d", None), + ("a || (b && c && d)", None), + ("(a || b || c) && d", None), + ("a || b || (c && d)", None), + ("a + b + c + d", None), + ("a * b * c * d", None), + ("a == 0 && b == 1", None), + ("a == 0 || b == 1", None), + ("(a == 0) && (b == 1)", Some(json!([{"groups": [["&&", "=="]]}]))), + ("a + b - c * d / e", Some(json!([{ "groups": [["&&", "||"]] }]))), + ("a + b - c", None), + ("a * b / c", None), + ("a + b - c", Some(json!([{ "allowSamePrecedence": true }]))), + ("a * b / c", Some(json!([{ "allowSamePrecedence": true }]))), + ("(a || b) ? c : d", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("a ? (b || c) : d", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("a ? b : (c || d)", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("a || (b ? c : d)", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("(a ? b : c) || d", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("a || (b ? c : d)", None), + ("(a || b) ? c : d", None), + ("a || b ? c : d", None), + ("a ? (b || c) : d", None), + ("a ? b || c : d", None), + ("a ? b : (c || d)", None), + ("a ? b : c || d", None), + ]; + + let fail = vec![ + ("a && b || c", None), + ("a && b > 0 || c", Some(json!([{ "groups": [["&&", "||", ">"]] }]))), + ("a && b > 0 || c", Some(json!([{ "groups": [["&&", "||"]] }]))), + ( + "a && b + c - d / e || f", + Some(json!([{ "groups": [["&&", "||"], ["+", "-", "*", "/"]] }])), + ), + ( + "a && b + c - d / e || f", + Some( + json!([{ "groups": [["&&", "||"], ["+", "-", "*", "/"]], "allowSamePrecedence": true }]), + ), + ), + ("a + b - c", Some(json!([{ "allowSamePrecedence": false }]))), + ("a * b / c", Some(json!([{ "allowSamePrecedence": false }]))), + ("a || b ? c : d", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("a && b ? 1 : 2", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("x ? a && b : 0", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("x ? 0 : a && b", Some(json!([{ "groups": [["&&", "||", "?:"]] }]))), + ("a + b ?? c", Some(json!([{ "groups": [["+", "??"]] }]))), + ]; + + Tester::new(NoMixedOperators::NAME, pass, fail).test_and_snapshot(); +} + +#[cfg(test)] +mod internal_tests { + use serde_json::json; + + use super::*; + + #[test] + fn test_from_configuration() { + let config = json!([{ + "groups": [ + ["+", "-", "*", "/", "%", "**"], + ["&", "|", "^", "~", "<<", ">>", ">>>"], + ["==", "!=", "===", "!==", ">", ">=", "<", "<="], + ["&&", "||"], + ["in", "instanceof"] + ], + "allowSamePrecedence": true + }]); + let rule = NoMixedOperators::try_from_configuration(&config); + assert_eq!(Some(NoMixedOperators::default()), rule); + } + + #[test] + fn test_nornmalize_configuration() { + let config = json!([ + { "allowSamePrecedence": false } + ]); + let rule = NoMixedOperators::try_from_configuration(&config); + // missing groups should fall back to default + let expected = NoMixedOperators { groups: default_groups(), allow_same_precedence: false }; + assert_eq!(Some(expected), rule); + } +} diff --git a/crates/oxc_linter/src/snapshots/no_mixed_operators.snap b/crates/oxc_linter/src/snapshots/no_mixed_operators.snap new file mode 100644 index 000000000..0c5df989c --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_mixed_operators.snap @@ -0,0 +1,111 @@ +--- +source: crates/oxc_linter/src/tester.rs +assertion_line: 53 +expression: no_mixed_operators +--- + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "||" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b || c + · ── ── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "||" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b > 0 || c + · ── ── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of ">" with "&&" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b > 0 || c + · ── ─ + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "||" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b > 0 || c + · ── ── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "||" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b + c - d / e || f + · ── ── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "/" with "-" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b + c - d / e || f + · ─ ─ + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "||" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b + c - d / e || f + · ── ── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "/" with "-" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b + c - d / e || f + · ─ ─ + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "+" with "-" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a + b - c + · ─ ─ + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "*" with "/" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a * b / c + · ─ ─ + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "||" with "?:" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a || b ? c : d + · ── ───── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "?:" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a && b ? 1 : 2 + · ── ───── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "?:" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ x ? a && b : 0 + · ─────────── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "&&" with "?:" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ x ? 0 : a && b + · ───── ── + ╰──── + help: Use parentheses to clarify the intended order of operations. + + ⚠ eslint(no-mixed-operators): Unexpected mix of "+" with "??" + ╭─[no_mixed_operators.tsx:1:1] + 1 │ a + b ?? c + · ─ ── + ╰──── + help: Use parentheses to clarify the intended order of operations. + diff --git a/tasks/coverage/babel b/tasks/coverage/babel index a547f8724..c38bf12f0 160000 --- a/tasks/coverage/babel +++ b/tasks/coverage/babel @@ -1 +1 @@ -Subproject commit a547f8724a5c6b4395b8a8f597e3edd44de74bf3 +Subproject commit c38bf12f010520ea7abe8a286f62922b2d1e1f1b diff --git a/tasks/coverage/test262 b/tasks/coverage/test262 index 53e5ef817..d216cc197 160000 --- a/tasks/coverage/test262 +++ b/tasks/coverage/test262 @@ -1 +1 @@ -Subproject commit 53e5ef817eb212d0d4f6f0ab44275094e5bf876d +Subproject commit d216cc197269fc41eb6eca14710529c3d6650535 diff --git a/tasks/coverage/typescript b/tasks/coverage/typescript index 746a6feb2..8f40d5633 160000 --- a/tasks/coverage/typescript +++ b/tasks/coverage/typescript @@ -1 +1 @@ -Subproject commit 746a6feb2e7ba6987b6c72db538dd498b35cd461 +Subproject commit 8f40d5633fc36df04b4fd4392e3877558149987f