diff --git a/Cargo.lock b/Cargo.lock index 52e4c6a15..72bf935f9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -934,6 +934,7 @@ dependencies = [ "oxc_diagnostics", "oxc_macros", "oxc_parser", + "oxc_printer", "oxc_semantic", "phf", "rustc-hash", diff --git a/crates/oxc_linter/Cargo.toml b/crates/oxc_linter/Cargo.toml index e167c7f67..655892726 100644 --- a/crates/oxc_linter/Cargo.toml +++ b/crates/oxc_linter/Cargo.toml @@ -14,6 +14,7 @@ oxc_ast = { path = "../oxc_ast" } oxc_diagnostics = { path = "../oxc_diagnostics" } oxc_macros = { path = "../oxc_macros" } oxc_semantic = { path = "../oxc_semantic" } +oxc_printer = { path = "../oxc_printer" } lazy_static = { workspace = true } serde_json = { workspace = true } diff --git a/crates/oxc_linter/src/context.rs b/crates/oxc_linter/src/context.rs index eeb7d4b6e..1ca8f7ade 100644 --- a/crates/oxc_linter/src/context.rs +++ b/crates/oxc_linter/src/context.rs @@ -3,6 +3,7 @@ use std::{cell::RefCell, rc::Rc}; use indextree::{Ancestors, NodeId}; use oxc_ast::{ast::IdentifierReference, AstKind, SourceType}; use oxc_diagnostics::Error; +use oxc_printer::{Printer, PrinterOptions}; use oxc_semantic::{AstNodes, Scope, ScopeTree, Semantic, SemanticNode}; use crate::{ @@ -120,4 +121,9 @@ impl<'a> LintContext<'a> { pub fn is_reference_to_global_variable(&self, _ident: &IdentifierReference) -> bool { true } + + #[allow(clippy::unused_self)] + pub fn printer(&self) -> Printer { + Printer::new(0, PrinterOptions::default()) + } } diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 253cd460e..d5385f2cf 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -17,6 +17,7 @@ oxc_macros::declare_all_lint_rules! { no_mixed_operators, no_constant_binary_expression, no_compare_neg_zero, + no_unsafe_negation, deepscan::uninvoked_array_callback, use_isnan, } diff --git a/crates/oxc_linter/src/rules/no_unsafe_negation.rs b/crates/oxc_linter/src/rules/no_unsafe_negation.rs new file mode 100644 index 000000000..872f61ef9 --- /dev/null +++ b/crates/oxc_linter/src/rules/no_unsafe_negation.rs @@ -0,0 +1,143 @@ +use oxc_ast::{ + ast::{BinaryExpression, BinaryOperator, Expression, UnaryOperator}, + AstKind, GetSpan, Span, +}; +use oxc_diagnostics::{ + miette::{self, Diagnostic}, + thiserror::{self, Error}, +}; +use oxc_macros::declare_oxc_lint; +use oxc_printer::Gen; + +use crate::{context::LintContext, fixer::Fix, rule::Rule, AstNode}; + +#[derive(Debug, Error, Diagnostic)] +#[error("Unexpected logical not in the left hand side of '{0}' operator")] +#[diagnostic( + severity(warning), + help( + "use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '{0}'" + ) +)] +struct NoUnsafeNegationDiagnostic(&'static str, #[label] pub Span); + +#[derive(Debug, Default, Clone)] +pub struct NoUnsafeNegation { + /// true: disallow negation of the left-hand side of ordering relational operators + /// false: allow negation of the left-hand side of ordering relational operators (<, >, <=, >=) + enforce_for_ordering_relations: bool, +} + +declare_oxc_lint!( + /// ### What it does + /// Disallow negating the left operand of relational operators + /// + /// ### Why is this bad? + /// Just as developers might type -a + b when they mean -(a + b) for the negative of a sum, + /// they might type !key in object by mistake when they almost certainly mean !(key in object) + /// to test that a key is not in an object. !obj instanceof Ctor is similar. + /// + /// ### Example + /// ```javascript + /// if (!key in object) { + /// //operator precedence makes it equivalent to (!key) in object + /// //and type conversion makes it equivalent to (key ? "false" : "true") in object + /// } + /// ``` + NoUnsafeNegation, + correctness +); + +impl Rule for NoUnsafeNegation { + fn from_configuration(value: serde_json::Value) -> Self { + let enforce_for_ordering_relations = value + .get(0) + .and_then(|config| config.get("enforceForOrderingRelations")) + .and_then(serde_json::Value::as_bool) + .unwrap_or_default(); + Self { enforce_for_ordering_relations } + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + let AstKind::BinaryExpression(expr) = node.get().kind() else { return; }; + if self.should_check(expr.operator) { + let Expression::UnaryExpression(left) = &expr.left else { return; }; + if left.operator == UnaryOperator::LogicalNot { + Self::report_with_fix(expr, ctx); + } + } + } +} + +impl NoUnsafeNegation { + fn should_check(&self, op: BinaryOperator) -> bool { + op.is_relational() || (self.enforce_for_ordering_relations && op.is_compare()) + } + + /// Precondition: + /// expr.left is `UnaryExpression` whose operator is '!' + fn report_with_fix(expr: &BinaryExpression, ctx: &LintContext<'_>) { + // Diagnostic points at the unexpected negation + let diagnostic = NoUnsafeNegationDiagnostic(expr.operator.as_str(), expr.left.span()); + + let fix_producer = || { + // modify `!a instance of B` to `!(a instanceof B)` + let modified_code = { + let mut printer = ctx.printer(); + printer.print(b'!'); + let Expression::UnaryExpression(left) = &expr.left else { unreachable!() }; + printer.print(b'('); + left.argument.gen(&mut printer); + expr.operator.gen(&mut printer); + expr.right.gen(&mut printer); + printer.print(b')'); + printer.into_code() + }; + Fix::new(modified_code, expr.span) + }; + + ctx.diagnostic_with_fix(diagnostic, fix_producer); + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ("a in b", None), + ("a in b === false", None), + ("!(a in b)", None), + ("(!a) in b", None), + ("a instanceof b", None), + ("a instanceof b === false", None), + ("!(a instanceof b)", None), + ("(!a) instanceof b", None), + ("if (! a < b) {}", None), + ("while (! a > b) {}", None), + ("foo = ! a <= b;", None), + ("foo = ! a >= b;", None), + ("! a <= b", Some(serde_json::json!([{}]))), + ("foo = ! a >= b;", Some(serde_json::json!([{ "enforceForOrderingRelations": false }]))), + ("foo = (!a) >= b;", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ("a <= b", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ("!(a < b)", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ("foo = a > b;", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ]; + + let fail = vec![ + ("!a in b", None), + ("(!a in b)", None), + ("!(a) in b", None), + ("!a instanceof b", None), + ("(!a instanceof b)", None), + ("!(a) instanceof b", None), + ("if (! a < b) {}", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ("while (! a > b) {}", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ("foo = ! a <= b;", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ("foo = ! a >= b;", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ("! a <= b", Some(serde_json::json!([{ "enforceForOrderingRelations": true }]))), + ]; + + Tester::new(NoUnsafeNegation::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_unsafe_negation.snap b/crates/oxc_linter/src/snapshots/no_unsafe_negation.snap new file mode 100644 index 000000000..4d4359593 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_unsafe_negation.snap @@ -0,0 +1,82 @@ +--- +source: crates/oxc_linter/src/tester.rs +expression: no_unsafe_negation +--- + + ⚠ Unexpected logical not in the left hand side of 'in' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ !a in b + · ── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'in' + + ⚠ Unexpected logical not in the left hand side of 'in' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ (!a in b) + · ── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'in' + + ⚠ Unexpected logical not in the left hand side of 'in' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ !(a) in b + · ──── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'in' + + ⚠ Unexpected logical not in the left hand side of 'instanceof' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ !a instanceof b + · ── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'instanceof' + + ⚠ Unexpected logical not in the left hand side of 'instanceof' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ (!a instanceof b) + · ── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'instanceof' + + ⚠ Unexpected logical not in the left hand side of 'instanceof' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ !(a) instanceof b + · ──── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than 'instanceof' + + ⚠ Unexpected logical not in the left hand side of '<' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ if (! a < b) {} + · ─── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '<' + + ⚠ Unexpected logical not in the left hand side of '>' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ while (! a > b) {} + · ─── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '>' + + ⚠ Unexpected logical not in the left hand side of '<=' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ foo = ! a <= b; + · ─── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '<=' + + ⚠ Unexpected logical not in the left hand side of '>=' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ foo = ! a >= b; + · ─── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '>=' + + ⚠ Unexpected logical not in the left hand side of '<=' operator + ╭─[no_unsafe_negation.tsx:1:1] + 1 │ ! a <= b + · ─── + ╰──── + help: use parenthesis to express the negation of the whole boolean expression, as '!' binds more closely than '<=' + diff --git a/crates/oxc_printer/src/gen.rs b/crates/oxc_printer/src/gen.rs index 420de21ed..2db4e92a3 100644 --- a/crates/oxc_printer/src/gen.rs +++ b/crates/oxc_printer/src/gen.rs @@ -1141,18 +1141,25 @@ impl<'a> Gen for UnaryExpression<'a> { impl<'a> Gen for BinaryExpression<'a> { fn gen(&self, p: &mut Printer) { self.left.gen(p); - let operator = self.operator.as_str().as_bytes(); - if self.operator.is_keyword() { + self.operator.gen(p); + self.right.gen(p); + } +} + +impl Gen for BinaryOperator { + fn gen(&self, p: &mut Printer) { + let operator = self.as_str().as_bytes(); + if self.is_keyword() { p.print(b' '); p.print_str(operator); p.print(b' '); } else { - p.print_space_before_operator(self.operator.into()); + let op: Operator = (*self).into(); + p.print_space_before_operator(op); p.print_str(operator); - p.prev_op = Some(self.operator.into()); + p.prev_op = Some(op); p.prev_op_end = p.code().len(); } - self.right.gen(p); } } diff --git a/crates/oxc_printer/src/lib.rs b/crates/oxc_printer/src/lib.rs index bfc645eba..64d274bf1 100644 --- a/crates/oxc_printer/src/lib.rs +++ b/crates/oxc_printer/src/lib.rs @@ -8,7 +8,7 @@ mod gen; #[allow(clippy::wildcard_imports)] use oxc_ast::ast::*; -use crate::gen::Gen; +pub use crate::gen::Gen; #[derive(Debug, Clone, Copy)] pub struct PrinterOptions { @@ -65,6 +65,12 @@ impl Printer { #[must_use] pub fn build(mut self, program: &Program<'_>) -> String { program.gen(&mut self); + self.into_code() + } + + #[must_use] + #[inline] + pub fn into_code(self) -> String { unsafe { String::from_utf8_unchecked(self.code) } } diff --git a/tasks/coverage/printer.snap b/tasks/coverage/printer.snap index 3ce25c700..d4075064a 100644 --- a/tasks/coverage/printer.snap +++ b/tasks/coverage/printer.snap @@ -1,3 +1,3 @@ Printer Summary: -AST Parsed : 44519/44519 (100.00%) -Positive Passed: 44519/44519 (100.00%) +AST Parsed : 44469/44469 (100.00%) +Positive Passed: 44469/44469 (100.00%)