Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
feat(rome_analyze): noNegationElse (#2655)
Browse files Browse the repository at this point in the history
* 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 <leops@users.noreply.github.com>
  • Loading branch information
IWANABETHATGUY and leops committed Jun 9, 2022
1 parent f795014 commit 827cbf7
Show file tree
Hide file tree
Showing 9 changed files with 256 additions and 3 deletions.
2 changes: 2 additions & 0 deletions crates/rome_analyze/src/analyzers.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

149 changes: 149 additions & 0 deletions crates/rome_analyze/src/analyzers/no_negation_else.rs
Original file line number Diff line number Diff line change
@@ -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<Self::State> {
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<RuleDiagnostic> {
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<JsRuleAction> {
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<bool> {
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: <Self::Language as rome_rowan::Language>::Kind) -> bool {
matches!(
kind,
JsSyntaxKind::JS_CONDITIONAL_EXPRESSION | JsSyntaxKind::JS_IF_STATEMENT
)
}

fn cast(syntax: rome_rowan::SyntaxNode<Self::Language>) -> Option<Self>
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<Self::Language> {
match self {
JsAnyCondition::JsConditionalExpression(expr) => expr.syntax(),
JsAnyCondition::JsIfStatement(stmt) => stmt.syntax(),
}
}

fn into_syntax(self) -> rome_rowan::SyntaxNode<Self::Language> {
match self {
JsAnyCondition::JsConditionalExpression(expr) => expr.into_syntax(),
JsAnyCondition::JsIfStatement(stmt) => stmt.into_syntax(),
}
}
}
1 change: 0 additions & 1 deletion crates/rome_analyze/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ pub fn analyze<B>(
WalkEvent::Enter(node) => node,
WalkEvent::Leave(_) => continue,
};

if let Some(range) = filter.range {
if node.text_range().ordering(range).is_ne() {
iter.skip_subtree();
Expand Down
1 change: 1 addition & 0 deletions crates/rome_analyze/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ impl_registry_builders!(
UseWhile,
NoDelete,
NoDoubleEquals,
NoNegationElse,
NoCompareNegZero,
UseValidTypeof,
UseSingleVarDeclarator,
Expand Down
1 change: 0 additions & 1 deletion crates/rome_analyze/src/signals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ where

code.push_str(token.text());
}

CodeSuggestion {
substitution: SuggestionChange::String(code),
span: FileSpan {
Expand Down
1 change: 0 additions & 1 deletion crates/rome_analyze/tests/spec_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
})
Expand Down
12 changes: 12 additions & 0 deletions crates/rome_analyze/tests/specs/noNegationElse.js
Original file line number Diff line number Diff line change
@@ -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;
91 changes: 91 additions & 0 deletions crates/rome_analyze/tests/specs/noNegationElse.js.snap
Original file line number Diff line number Diff line change
@@ -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;
```


1 change: 1 addition & 0 deletions crates/rome_diagnostics/src/emit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 827cbf7

Please sign in to comment.