diff --git a/bin/tests/data/useless_parens.nix b/bin/tests/data/useless_parens.nix index cf264416..c72009f1 100644 --- a/bin/tests/data/useless_parens.nix +++ b/bin/tests/data/useless_parens.nix @@ -1,16 +1,51 @@ let # parens around primitives - a = { + primitive = { b = ("hello"); c = (d); e = ({ f = 2; }); }; # parens around let-value - g = (1 + 2); - h = ({ inherit i; }); + value1 = (1 + 2); + value2 = ({ inherit i; }); - # TODO: binary exprs, function args etc. + binop = + let id = x: x; in + (id [3]) + ++ (id [1] ++ [2]) + ; + binop_no = + (1 + 1) + * (2 + 2) + ; + + # precedence + arith1 = + 4 + (5 * 3) + ; + arith2 = + (4 * 5) / 5 + ; + arith3_no = + 4 * (5 / 5) + ; + + primitive_binop = + [1] ++ ([2]) + ; + + string_concat = + (builtins.readFile ./x.txt) + + (lib.optionalString true '' + foo + '') + + (lib.optionalString true '' + bar + '') + ; + + # TODO: function args etc. in # parens around let body (null) diff --git a/bin/tests/snapshots/main__useless_parens_fix.snap b/bin/tests/snapshots/main__useless_parens_fix.snap index 1738da84..a50284ba 100644 --- a/bin/tests/snapshots/main__useless_parens_fix.snap +++ b/bin/tests/snapshots/main__useless_parens_fix.snap @@ -1,13 +1,14 @@ --- source: bin/tests/main.rs expression: "& stdout" + --- --- tests/data/useless_parens.nix +++ tests/data/useless_parens.nix [fixed] -@@ -1,16 +1,16 @@ +@@ -1,51 +1,51 @@ let # parens around primitives - a = { + primitive = { - b = ("hello"); - c = (d); - e = ({ f = 2; }); @@ -17,13 +18,60 @@ expression: "& stdout" }; # parens around let-value -- g = (1 + 2); -- h = ({ inherit i; }); -+ g = 1 + 2; -+ h = { inherit i; }; +- value1 = (1 + 2); +- value2 = ({ inherit i; }); ++ value1 = 1 + 2; ++ value2 = { inherit i; }; - # TODO: binary exprs, function args etc. + binop = + let id = x: x; in +- (id [3]) +- ++ (id [1] ++ [2]) ++ id [3] ++ ++ id [1] ++ [2] + ; + binop_no = + (1 + 1) + * (2 + 2) + ; + + # precedence + arith1 = +- 4 + (5 * 3) ++ 4 + 5 * 3 + ; + arith2 = +- (4 * 5) / 5 ++ 4 * 5 / 5 + ; + arith3_no = + 4 * (5 / 5) + ; + + primitive_binop = +- [1] ++ ([2]) ++ [1] ++ [2] + ; + + string_concat = +- (builtins.readFile ./x.txt) +- + (lib.optionalString true '' ++ builtins.readFile ./x.txt ++ + lib.optionalString true '' + foo +- '') +- + (lib.optionalString true '' ++ '' ++ + lib.optionalString true '' + bar +- '') ++ '' + ; + + # TODO: function args etc. in # parens around let body - (null) + null + + diff --git a/bin/tests/snapshots/main__useless_parens_lint.snap b/bin/tests/snapshots/main__useless_parens_lint.snap index 0b716a0f..1944ef39 100644 --- a/bin/tests/snapshots/main__useless_parens_lint.snap +++ b/bin/tests/snapshots/main__useless_parens_lint.snap @@ -1,11 +1,12 @@ --- source: bin/tests/main.rs expression: "& out" + --- [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:16:3] + ╭─[data/useless_parens.nix:51:3] │ - 16 │ (null) + 51 │ (null) · ───┬── · ╰──── Useless parentheses around body of let expression ────╯ @@ -31,16 +32,67 @@ expression: "& out" · ╰─────── Useless parentheses around value in binding ───╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:10:7] + ╭─[data/useless_parens.nix:10:12] │ - 10 │ g = (1 + 2); - · ───┬─── - · ╰───── Useless parentheses around value in binding + 10 │ value1 = (1 + 2); + · ───┬─── + · ╰───── Useless parentheses around value in binding ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:11:7] + ╭─[data/useless_parens.nix:11:12] │ - 11 │ h = ({ inherit i; }); - · ────────┬─────── - · ╰───────── Useless parentheses around value in binding + 11 │ value2 = ({ inherit i; }); + · ────────┬─────── + · ╰───────── Useless parentheses around value in binding ────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:15:5] + │ + 15 │ (id [3]) + · ────┬─── + · ╰───── Useless parentheses in operand of binary operator + 16 │ ++ (id [1] ++ [2]) + · ───────┬─────── + · ╰───────── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:25:9] + │ + 25 │ 4 + (5 * 3) + · ───┬─── + · ╰───── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:28:5] + │ + 28 │ (4 * 5) / 5 + · ───┬─── + · ╰───── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:35:12] + │ + 35 │ [1] ++ ([2]) + · ──┬── + · ╰──── Useless parentheses around primitive expression +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:43:7] + │ + 43 │ ╭─▶ + (lib.optionalString true '' + 45 │ ├─▶ '') + · │ + · ╰───────────── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:39:5] + │ + 39 │ (builtins.readFile ./x.txt) + · ─────────────┬───────────── + · ╰─────────────── Useless parentheses in operand of binary operator + 40 │ ╭─▶ + (lib.optionalString true '' + 42 │ ├─▶ '') + · │ + · ╰───────────── Useless parentheses in operand of binary operator +────╯ + diff --git a/lib/src/lints/useless_parens.rs b/lib/src/lints/useless_parens.rs index 17561961..04d71ef7 100644 --- a/lib/src/lints/useless_parens.rs +++ b/lib/src/lints/useless_parens.rs @@ -2,8 +2,8 @@ use crate::{Diagnostic, Metadata, Report, Rule, Suggestion, session::SessionInfo use macros::lint; use rnix::{ - NodeOrToken, SyntaxElement, SyntaxKind, - types::{KeyValue, LetIn, Paren, ParsedType, TypedNode, Wrapper}, + NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode, + types::{Apply, BinOp, BinOpKind, KeyValue, LetIn, Paren, ParsedType, TypedNode, Wrapper}, }; /// ## What it does @@ -39,10 +39,25 @@ use rnix::{ SyntaxKind::NODE_KEY_VALUE, SyntaxKind::NODE_PAREN, SyntaxKind::NODE_LET_IN, + SyntaxKind::NODE_BIN_OP, ] )] struct UselessParens; +enum OneOrMany { + One(A), + Many(Vec), +} + +type Prec = i8; + +#[derive(Eq, PartialEq)] +enum Assoc { + Left, + Right, + NoAssoc, +} + impl Rule for UselessParens { fn validate(&self, node: &SyntaxElement, _sess: &SessionInfo) -> Option { if let NodeOrToken::Node(node) = node @@ -50,7 +65,10 @@ impl Rule for UselessParens { && let Some(diagnostic) = do_thing(parsed_type_node) { let mut report = self.report(); - report.diagnostics.push(diagnostic); + match diagnostic { + OneOrMany::One(x) => report.diagnostics.push(x), + OneOrMany::Many(mut xs) => report.diagnostics.append(&mut xs), + } Some(report) } else { None @@ -58,7 +76,7 @@ impl Rule for UselessParens { } } -fn do_thing(parsed_type_node: ParsedType) -> Option { +fn do_thing(parsed_type_node: ParsedType) -> Option> { match parsed_type_node { ParsedType::KeyValue(kv) => { if let Some(value_node) = kv.value() @@ -69,11 +87,11 @@ fn do_thing(parsed_type_node: ParsedType) -> Option { let at = value_range; let message = "Useless parentheses around value in binding"; let replacement = inner; - Some(Diagnostic::suggest( + Some(OneOrMany::One(Diagnostic::suggest( at, message, Suggestion::new(at, replacement), - )) + ))) } else { None } @@ -87,15 +105,80 @@ fn do_thing(parsed_type_node: ParsedType) -> Option { let at = body_range; let message = "Useless parentheses around body of `let` expression"; let replacement = inner; - Some(Diagnostic::suggest( + Some(OneOrMany::One(Diagnostic::suggest( at, message, Suggestion::new(at, replacement), - )) + ))) } else { None } } + ParsedType::BinOp(binop) => { + let maybe_diagnostic = |(node, is_left): (SyntaxNode, bool)| -> Option { + let range = node.text_range(); + let as_parens = Paren::cast(node)?; + let inner = as_parens.inner()?; + + let suggestion = Diagnostic::suggest( + range, + "Useless parentheses in operand of binary operator", + Suggestion::new(range, inner.clone()), + ); + + // https://nix.dev/manual/nix/2.29/language/operators + let prec_of = |op: BinOp| -> Option<(Prec, Assoc)> { + Some(match op.operator()? { + BinOpKind::IsSet => (4, Assoc::NoAssoc), + BinOpKind::Concat => (5, Assoc::Right), + BinOpKind::Mul | BinOpKind::Div => (6, Assoc::Left), + BinOpKind::Add | BinOpKind::Sub => (7, Assoc::Left), + BinOpKind::Update => (9, Assoc::Right), + BinOpKind::Less + | BinOpKind::LessOrEq + | BinOpKind::More + | BinOpKind::MoreOrEq => (10, Assoc::NoAssoc), + BinOpKind::Equal | BinOpKind::NotEqual => (11, Assoc::NoAssoc), + BinOpKind::And => (12, Assoc::Left), + BinOpKind::Or => (13, Assoc::Left), + BinOpKind::Implication => (14, Assoc::Right), + }) + }; + + if Apply::cast(inner.clone()).is_some() { + Some(suggestion) + } else if let Some(inner_binop) = BinOp::cast(inner.clone()) { + let (outer_prec, outer_assoc) = prec_of(binop.clone())?; + let (inner_prec, _) = prec_of(inner_binop.clone())?; + if inner_prec < outer_prec + || (inner_prec == outer_prec && (is_left && outer_assoc == Assoc::Left) + || (!is_left && outer_assoc == Assoc::Right)) + { + Some(suggestion) + } else { + None + } + } else { + None + } + }; + + // Fix rhs then lhs otherwise the position will drift + let diagnostics = vec![ + binop.rhs().map(|node| (node, false)), + binop.lhs().map(|node| (node, true)), + ] + .into_iter() + .flatten() + .filter_map(maybe_diagnostic) + .collect::>(); + + if diagnostics.is_empty() { + None + } else { + Some(OneOrMany::Many(diagnostics)) + } + } ParsedType::Paren(paren_expr) => { let paren_expr_range = paren_expr.node().text_range(); if let Some(father_node) = paren_expr.node().parent() @@ -105,7 +188,9 @@ fn do_thing(parsed_type_node: ParsedType) -> Option { // ensure that we don't lint inside let-bodies // if this primitive is a let-body, we have already linted it - && LetIn::cast(father_node).is_none() + && LetIn::cast(father_node.clone()).is_none() + + // allow checking primitive expressions in binops && let Some(inner_node) = paren_expr.inner() && let Some(parsed_inner) = ParsedType::cast(inner_node) @@ -121,11 +206,11 @@ fn do_thing(parsed_type_node: ParsedType) -> Option { let at = paren_expr_range; let message = "Useless parentheses around primitive expression"; let replacement = parsed_inner.node().clone(); - Some(Diagnostic::suggest( + Some(OneOrMany::One(Diagnostic::suggest( at, message, Suggestion::new(at, replacement), - )) + ))) } else { None }