From 827cbf77e01c19254c02ead033394b597d763aee Mon Sep 17 00:00:00 2001 From: IWANABETHATGUY Date: Fri, 10 Jun 2022 01:10:39 +0800 Subject: [PATCH] feat(rome_analyze): noNegationElse (#2655) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore: 🤖 basic finish * feat: 🎸 with some log * fix: 🐛 test case * fix: 🐛 clippy * chore: 🤖 remove log * fix: 🐛 remove meaningless comment * fix: 🐛 recover new lint * chore: 🤖 code gen * fix: 🐛 cr issue * test: 💍 add more test conditionExpression not directly under StatementExpression * chore: 🤖 tweak message * fix: 🐛 cr issue * fix: 🐛 compile * chore: 🤖 fmt Co-authored-by: l3ops --- crates/rome_analyze/src/analyzers.rs | 2 + .../src/analyzers/no_negation_else.rs | 149 ++++++++++++++++++ crates/rome_analyze/src/lib.rs | 1 - crates/rome_analyze/src/registry.rs | 1 + crates/rome_analyze/src/signals.rs | 1 - crates/rome_analyze/tests/spec_tests.rs | 1 - .../tests/specs/noNegationElse.js | 12 ++ .../tests/specs/noNegationElse.js.snap | 91 +++++++++++ crates/rome_diagnostics/src/emit.rs | 1 + 9 files changed, 256 insertions(+), 3 deletions(-) create mode 100644 crates/rome_analyze/src/analyzers/no_negation_else.rs create mode 100644 crates/rome_analyze/tests/specs/noNegationElse.js create mode 100644 crates/rome_analyze/tests/specs/noNegationElse.js.snap diff --git a/crates/rome_analyze/src/analyzers.rs b/crates/rome_analyze/src/analyzers.rs index ae07d57b25c..781834f17f8 100644 --- a/crates/rome_analyze/src/analyzers.rs +++ b/crates/rome_analyze/src/analyzers.rs @@ -6,6 +6,8 @@ mod no_delete; pub(crate) use no_delete::NoDelete; mod no_double_equals; pub(crate) use no_double_equals::NoDoubleEquals; +mod no_negation_else; +pub(crate) use no_negation_else::NoNegationElse; mod no_compare_neg_zero; pub(crate) use no_compare_neg_zero::NoCompareNegZero; mod use_valid_typeof; diff --git a/crates/rome_analyze/src/analyzers/no_negation_else.rs b/crates/rome_analyze/src/analyzers/no_negation_else.rs new file mode 100644 index 00000000000..1b35fd8a663 --- /dev/null +++ b/crates/rome_analyze/src/analyzers/no_negation_else.rs @@ -0,0 +1,149 @@ +use crate::registry::{JsRuleAction, Rule, RuleAction, RuleDiagnostic}; +use crate::{ActionCategory, RuleCategory}; +use rome_console::markup; +use rome_diagnostics::{Applicability, Severity}; +use rome_js_factory::make; +use rome_js_syntax::{ + JsAnyExpression, JsAnyRoot, JsConditionalExpression, JsIfStatement, JsLanguage, JsSyntaxKind, + JsUnaryExpression, JsUnaryOperator, +}; +use rome_rowan::{AstNode, AstNodeExt}; + +pub(crate) enum NoNegationElse {} + +impl Rule for NoNegationElse { + const NAME: &'static str = "noNegationElse"; + const CATEGORY: RuleCategory = RuleCategory::Lint; + + type Query = JsAnyCondition; + type State = JsUnaryExpression; + + fn run(n: &Self::Query) -> Option { + match n { + JsAnyCondition::JsConditionalExpression(expr) => { + if is_negation(&expr.test().ok()?).unwrap_or(false) { + Some(expr.test().ok()?.as_js_unary_expression().unwrap().clone()) + } else { + None + } + } + JsAnyCondition::JsIfStatement(stmt) => { + if is_negation(&stmt.test().ok()?).unwrap_or(false) && stmt.else_clause().is_some() + { + Some(stmt.test().ok()?.as_js_unary_expression().unwrap().clone()) + } else { + None + } + } + } + } + + fn diagnostic(node: &Self::Query, _state: &Self::State) -> Option { + Some(RuleDiagnostic { + severity: Severity::Warning, + message: markup! { + "Invert blocks when performing a negation test." + } + .to_owned(), + range: node.range(), + }) + } + + fn action(root: JsAnyRoot, node: &Self::Query, state: &Self::State) -> Option { + let root = match node { + JsAnyCondition::JsConditionalExpression(expr) => { + let mut next_expr = expr + .clone() + .replace_node(expr.test().ok()?, state.argument().ok()?)?; + next_expr = next_expr + .clone() + .replace_node(next_expr.alternate().ok()?, expr.consequent().ok()?)?; + next_expr = next_expr + .clone() + .replace_node(next_expr.consequent().ok()?, expr.alternate().ok()?)?; + root.replace_node( + node.clone(), + JsAnyCondition::JsConditionalExpression(next_expr), + ) + } + JsAnyCondition::JsIfStatement(stmt) => { + let next_stmt = stmt + .clone() + .replace_node(stmt.test().ok()?, state.argument().ok()?)?; + let next_stmt = next_stmt.clone().replace_node( + next_stmt.else_clause()?, + make::js_else_clause( + stmt.else_clause()?.else_token().ok()?, + stmt.consequent().ok()?, + ), + )?; + let next_stmt = next_stmt.clone().replace_node( + next_stmt.consequent().ok()?, + stmt.else_clause()?.alternate().ok()?, + )?; + root.replace_node(node.clone(), JsAnyCondition::JsIfStatement(next_stmt)) + } + }?; + Some(RuleAction { + category: ActionCategory::QuickFix, + applicability: Applicability::MaybeIncorrect, + message: markup! { "Exchange alternate and consequent of the node" }.to_owned(), + root, + }) + } +} + +fn is_negation(node: &JsAnyExpression) -> Option { + match node { + JsAnyExpression::JsUnaryExpression(expr) => { + Some(expr.operator().ok()? == JsUnaryOperator::LogicalNot) + } + _ => Some(false), + } +} + +#[derive(Debug, Clone)] +pub enum JsAnyCondition { + JsConditionalExpression(JsConditionalExpression), + JsIfStatement(JsIfStatement), +} + +impl AstNode for JsAnyCondition { + type Language = JsLanguage; + + fn can_cast(kind: ::Kind) -> bool { + matches!( + kind, + JsSyntaxKind::JS_CONDITIONAL_EXPRESSION | JsSyntaxKind::JS_IF_STATEMENT + ) + } + + fn cast(syntax: rome_rowan::SyntaxNode) -> Option + where + Self: Sized, + { + match syntax.kind() { + JsSyntaxKind::JS_CONDITIONAL_EXPRESSION => { + JsConditionalExpression::cast(syntax).map(JsAnyCondition::JsConditionalExpression) + } + JsSyntaxKind::JS_IF_STATEMENT => { + JsIfStatement::cast(syntax).map(JsAnyCondition::JsIfStatement) + } + _ => None, + } + } + + fn syntax(&self) -> &rome_rowan::SyntaxNode { + match self { + JsAnyCondition::JsConditionalExpression(expr) => expr.syntax(), + JsAnyCondition::JsIfStatement(stmt) => stmt.syntax(), + } + } + + fn into_syntax(self) -> rome_rowan::SyntaxNode { + match self { + JsAnyCondition::JsConditionalExpression(expr) => expr.into_syntax(), + JsAnyCondition::JsIfStatement(stmt) => stmt.into_syntax(), + } + } +} diff --git a/crates/rome_analyze/src/lib.rs b/crates/rome_analyze/src/lib.rs index 0492ec39b7b..e56af553108 100644 --- a/crates/rome_analyze/src/lib.rs +++ b/crates/rome_analyze/src/lib.rs @@ -74,7 +74,6 @@ pub fn analyze( WalkEvent::Enter(node) => node, WalkEvent::Leave(_) => continue, }; - if let Some(range) = filter.range { if node.text_range().ordering(range).is_ne() { iter.skip_subtree(); diff --git a/crates/rome_analyze/src/registry.rs b/crates/rome_analyze/src/registry.rs index fbd18f7514e..a79af8b8d7f 100644 --- a/crates/rome_analyze/src/registry.rs +++ b/crates/rome_analyze/src/registry.rs @@ -39,6 +39,7 @@ impl_registry_builders!( UseWhile, NoDelete, NoDoubleEquals, + NoNegationElse, NoCompareNegZero, UseValidTypeof, UseSingleVarDeclarator, diff --git a/crates/rome_analyze/src/signals.rs b/crates/rome_analyze/src/signals.rs index 0ab2b98274b..d092075ebd0 100644 --- a/crates/rome_analyze/src/signals.rs +++ b/crates/rome_analyze/src/signals.rs @@ -59,7 +59,6 @@ where code.push_str(token.text()); } - CodeSuggestion { substitution: SuggestionChange::String(code), span: FileSpan { diff --git a/crates/rome_analyze/tests/spec_tests.rs b/crates/rome_analyze/tests/spec_tests.rs index 8d8c28f8342..642256e2204 100644 --- a/crates/rome_analyze/tests/spec_tests.rs +++ b/crates/rome_analyze/tests/spec_tests.rs @@ -122,7 +122,6 @@ where L: Language, { let output = action.root.syntax().to_string(); - markup_to_string(markup! { {Diff { mode: DiffMode::Unified, left: source, right: &output }} }) diff --git a/crates/rome_analyze/tests/specs/noNegationElse.js b/crates/rome_analyze/tests/specs/noNegationElse.js new file mode 100644 index 00000000000..ed2f2b8389b --- /dev/null +++ b/crates/rome_analyze/tests/specs/noNegationElse.js @@ -0,0 +1,12 @@ +// valid +if (!true) {consequent;}; +true ? consequent : alternate; +// invalid +if (!true) { + consequent; +} else { + alternate; +} +!condition ? consequent : alternate; + +let a = !test ? c : d; \ No newline at end of file diff --git a/crates/rome_analyze/tests/specs/noNegationElse.js.snap b/crates/rome_analyze/tests/specs/noNegationElse.js.snap new file mode 100644 index 00000000000..c0521942aca --- /dev/null +++ b/crates/rome_analyze/tests/specs/noNegationElse.js.snap @@ -0,0 +1,91 @@ +--- +source: crates/rome_analyze/tests/spec_tests.rs +assertion_line: 96 +expression: noNegationElse.js +--- +# Input +```js +// valid +if (!true) {consequent;}; +true ? consequent : alternate; +// invalid +if (!true) { + consequent; +} else { + alternate; +} +!condition ? consequent : alternate; + +let a = !test ? c : d; +``` + +# Diagnostics +``` +warning[noNegationElse]: Invert blocks when performing a negation test. + ┌─ noNegationElse.js:5:1 + │ +5 │ ┌ if (!true) { +6 │ │ consequent; +7 │ │ } else { +8 │ │ alternate; +9 │ │ } + │ └─' Invert blocks when performing a negation test. + +Exchange alternate and consequent of the node + | @@ -2,10 +2,10 @@ +1 1 | if (!true) {consequent;}; +2 2 | true ? consequent : alternate; +3 3 | // invalid +4 | - if (!true) { + 4 | + if (true) { + 5 | + alternate; + 6 | + } else { +5 7 | consequent; +6 | - } else { +7 | - alternate; +8 8 | } +9 9 | !condition ? consequent : alternate; +10 10 | + + +``` + +``` +warning[noNegationElse]: Invert blocks when performing a negation test. + ┌─ noNegationElse.js:10:1 + │ +10 │ !condition ? consequent : alternate; + │ ----------------------------------- Invert blocks when performing a negation test. + +Exchange alternate and consequent of the node + | @@ -7,6 +7,6 @@ + 6 6 | } else { + 7 7 | alternate; + 8 8 | } + 9 | - !condition ? consequent : alternate; + 9 | + condition ? alternate : consequent; +10 10 | +11 11 | let a = !test ? c : d; + + +``` + +``` +warning[noNegationElse]: Invert blocks when performing a negation test. + ┌─ noNegationElse.js:12:9 + │ +12 │ let a = !test ? c : d; + │ ------------- Invert blocks when performing a negation test. + +Exchange alternate and consequent of the node + | @@ -9,4 +9,4 @@ + 8 8 | } + 9 9 | !condition ? consequent : alternate; +10 10 | +11 | - let a = !test ? c : d; + 11 | + let a = test ? d : c; + + +``` + + diff --git a/crates/rome_diagnostics/src/emit.rs b/crates/rome_diagnostics/src/emit.rs index f0d58fb268e..182274d32a3 100644 --- a/crates/rome_diagnostics/src/emit.rs +++ b/crates/rome_diagnostics/src/emit.rs @@ -72,6 +72,7 @@ impl<'a> Display for DiagnosticPrinter<'a> { .files .name(self.d.file_id) .ok_or_else(|| io::Error::new(io::ErrorKind::NotFound, "file not found"))?; + let source_file = self .files .source(self.d.file_id)