From ec795b5ff1346102e633df2f1f164ac298c6a8a2 Mon Sep 17 00:00:00 2001 From: ematipico Date: Fri, 28 Oct 2022 12:03:43 +0100 Subject: [PATCH] fix(rome_js_analyze): keep tokens when simplifying logical expression --- .../use_simplified_logic_expression.rs | 26 +++++-- crates/rome_js_analyze/src/lib.rs | 6 +- .../useSimplifiedLogicExpression.js | 10 +++ .../useSimplifiedLogicExpression.js.snap | 78 ++++++++++++++++++- crates/rome_rowan/src/ast/mutation.rs | 4 +- .../rules/useSimplifiedLogicExpression.md | 4 +- 6 files changed, 114 insertions(+), 14 deletions(-) diff --git a/crates/rome_js_analyze/src/analyzers/correctness/use_simplified_logic_expression.rs b/crates/rome_js_analyze/src/analyzers/correctness/use_simplified_logic_expression.rs index 8213b631034..3676bd141f1 100644 --- a/crates/rome_js_analyze/src/analyzers/correctness/use_simplified_logic_expression.rs +++ b/crates/rome_js_analyze/src/analyzers/correctness/use_simplified_logic_expression.rs @@ -217,15 +217,29 @@ fn simplify_de_morgan(node: &JsLogicalExpression) -> Option { let left = node.left().ok()?; let right = node.right().ok()?; let operator_token = node.operator_token().ok()?; + let right_argument_trivia = right + .syntax() + .first_leading_trivia() + .map(|trivia| trivia.pieces()); match (left, right) { (JsAnyExpression::JsUnaryExpression(left), JsAnyExpression::JsUnaryExpression(right)) => { let mut next_logic_expression = match operator_token.kind() { - T![||] => node - .clone() - .replace_token(operator_token, make::token(T![&&])), - T![&&] => node - .clone() - .replace_token(operator_token, make::token(T![||])), + T![||] => { + let mut token = make::token(T![&&]); + if let Some(right_argument_trivia) = right_argument_trivia { + token = token.with_leading_trivia_pieces(right_argument_trivia); + } + node.clone() + .replace_token_discard_trivia(operator_token, token) + } + T![&&] => { + let mut token = make::token(T![||]); + if let Some(right_argument_trivia) = right_argument_trivia { + token = token.with_leading_trivia_pieces(right_argument_trivia); + } + node.clone() + .replace_token_discard_trivia(operator_token, token) + } _ => return None, }?; next_logic_expression = next_logic_expression.with_left(left.argument().ok()?); diff --git a/crates/rome_js_analyze/src/lib.rs b/crates/rome_js_analyze/src/lib.rs index 60f7cfc5053..72f56b435e1 100644 --- a/crates/rome_js_analyze/src/lib.rs +++ b/crates/rome_js_analyze/src/lib.rs @@ -141,7 +141,11 @@ mod tests { String::from_utf8(buffer).unwrap() } - const SOURCE: &str = r#" + const SOURCE: &str = r#"if ( + !firstCondition && + // comment + !secondCondition + ) {} "#; let parsed = parse(SOURCE, FileId::zero(), SourceType::jsx()); diff --git a/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js b/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js index 45ff371a71d..88b330092f9 100644 --- a/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js +++ b/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js @@ -16,3 +16,13 @@ const r3 = null ?? nonNullExp; const boolExpr1 = true; const boolExpr2 = false; const r4 = !boolExpr1 || !boolExpr2; +if ( + !boolExpr1 || + // comment + !boolExpr2 +) {} +if ( + !boolExpr1 || + // comment + !boolExpr2 +) {} diff --git a/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js.snap b/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js.snap index f0465b73611..42e8022c847 100644 --- a/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js.snap +++ b/crates/rome_js_analyze/tests/specs/correctness/useSimplifiedLogicExpression.js.snap @@ -22,6 +22,16 @@ const r3 = null ?? nonNullExp; const boolExpr1 = true; const boolExpr2 = false; const r4 = !boolExpr1 || !boolExpr2; +if ( + !boolExpr1 || + // comment + !boolExpr2 +) {} +if ( + !boolExpr1 || + // comment + !boolExpr2 +) {} ``` @@ -92,15 +102,77 @@ useSimplifiedLogicExpression.js:18:12 lint/correctness/useSimplifiedLogicExpress 17 │ const boolExpr2 = false; > 18 │ const r4 = !boolExpr1 || !boolExpr2; │ ^^^^^^^^^^^^^^^^^^^^^^^^ - 19 │ + 19 │ if ( + 20 │ !boolExpr1 || i Suggested fix: Reduce the complexity of the logical expression. 16 16 │ const boolExpr1 = true; 17 17 │ const boolExpr2 = false; 18 │ - const·r4·=·!boolExpr1·||·!boolExpr2; - 18 │ + const·r4·=·!(boolExpr1·&&·boolExpr2); - 19 19 │ + 18 │ + const·r4·=·!(boolExpr1·&&boolExpr2); + 19 19 │ if ( + 20 20 │ !boolExpr1 || + + +``` + +``` +useSimplifiedLogicExpression.js:20:5 lint/correctness/useSimplifiedLogicExpression FIXABLE ━━━━━━━━━━ + + ! Logical expression contains unnecessary complexity. + + 18 │ const r4 = !boolExpr1 || !boolExpr2; + 19 │ if ( + > 20 │ !boolExpr1 || + │ ^^^^^^^^^^^^^ + > 21 │ // comment + > 22 │ !boolExpr2 + │ ^^^^^^^^^^ + 23 │ ) {} + 24 │ if ( + + i Suggested fix: Reduce the complexity of the logical expression. + + 18 18 │ const r4 = !boolExpr1 || !boolExpr2; + 19 19 │ if ( + 20 │ - ····!boolExpr1·|| + 20 │ + ····!(boolExpr1· + 21 21 │ // comment + 22 │ - ····!boolExpr2 + 22 │ + ····&&boolExpr2) + 23 23 │ ) {} + 24 24 │ if ( + + +``` + +``` +useSimplifiedLogicExpression.js:25:5 lint/correctness/useSimplifiedLogicExpression FIXABLE ━━━━━━━━━━ + + ! Logical expression contains unnecessary complexity. + + 23 │ ) {} + 24 │ if ( + > 25 │ !boolExpr1 || + │ ^^^^^^^^^^^^^ + > 26 │ // comment + > 27 │ !boolExpr2 + │ ^^^^^^^^^^ + 28 │ ) {} + 29 │ + + i Suggested fix: Reduce the complexity of the logical expression. + + 23 23 │ ) {} + 24 24 │ if ( + 25 │ - ····!boolExpr1·|| + 25 │ + ····!(boolExpr1· + 26 26 │ // comment + 27 │ - ····!boolExpr2 + 27 │ + ····&&boolExpr2) + 28 28 │ ) {} + 29 29 │ ``` diff --git a/crates/rome_rowan/src/ast/mutation.rs b/crates/rome_rowan/src/ast/mutation.rs index bbf4d154509..e3d9ef59b5f 100644 --- a/crates/rome_rowan/src/ast/mutation.rs +++ b/crates/rome_rowan/src/ast/mutation.rs @@ -14,7 +14,7 @@ pub trait AstNodeExt: AstNode { Self: Sized; /// Return a new version of this node with the node `prev_node` replaced with `next_node`, - /// transfering the leading and trailing trivia of `prev_node` to `next_node` + /// transferring the leading and trailing trivia of `prev_node` to `next_node` /// /// `prev_node` can be a direct child of this node, or an indirect child through any descendant node /// @@ -38,7 +38,7 @@ pub trait AstNodeExt: AstNode { Self: Sized; /// Return a new version of this node with the token `prev_token` replaced with `next_token`, - /// transfering the leading and trailing trivia of `prev_token` to `next_token` + /// transferring the leading and trailing trivia of `prev_token` to `next_token` /// /// `prev_token` can be a direct child of this node, or an indirect child through any descendant node /// diff --git a/website/src/docs/lint/rules/useSimplifiedLogicExpression.md b/website/src/docs/lint/rules/useSimplifiedLogicExpression.md index d3d4a37c011..8e6ff38487c 100644 --- a/website/src/docs/lint/rules/useSimplifiedLogicExpression.md +++ b/website/src/docs/lint/rules/useSimplifiedLogicExpression.md @@ -93,8 +93,8 @@ const r4 = !boolExpr1 || !boolExpr2; 1 1 │ const boolExpr1 = true; 2 2 │ const boolExpr2 = false; - 3 │ - const·r4·=·!boolExpr1·||·!boolExpr2; - 3 │ + const·r4·=·!(boolExpr1·&&·boolExpr2); + 3 │ - const·r4·=·!boolExpr1·||·!boolExpr2; + 3 │ + const·r4·=·!(boolExpr1·&&boolExpr2); 4 4 │ {% endraw %}