From af9196d61e4eb1ec8634b608305b1b3e672577b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 09:59:10 +0200 Subject: [PATCH 1/8] test: useless parens in binops --- bin/tests/data/useless_parens.nix | 27 ++++++++++++++++++- .../snapshots/main__useless_parens_fix.snap | 12 +++++++-- .../snapshots/main__useless_parens_lint.snap | 6 +++-- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/bin/tests/data/useless_parens.nix b/bin/tests/data/useless_parens.nix index cf264416..ed2ea9cb 100644 --- a/bin/tests/data/useless_parens.nix +++ b/bin/tests/data/useless_parens.nix @@ -10,7 +10,32 @@ let g = (1 + 2); h = ({ inherit i; }); - # TODO: binary exprs, function args etc. + # binary exprs with superflous parens + # TODO: we could implement associativity check to remove more redundant parens in the future + f = + let id = x: x; in + (id [3]) + ++ (id [1] ++ [2]) + ; + + # binary exprs with necessary parens + u = + (1 + 1) + * (2 + 2) + ; + + # string concat + s = + (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..9d8ee718 100644 --- a/bin/tests/snapshots/main__useless_parens_fix.snap +++ b/bin/tests/snapshots/main__useless_parens_fix.snap @@ -1,10 +1,11 @@ --- source: bin/tests/main.rs expression: "& stdout" + --- --- tests/data/useless_parens.nix +++ tests/data/useless_parens.nix [fixed] -@@ -1,16 +1,16 @@ +@@ -1,15 +1,15 @@ let # parens around primitives a = { @@ -22,8 +23,15 @@ expression: "& stdout" + g = 1 + 2; + h = { inherit i; }; - # TODO: binary exprs, function args etc. + # binary exprs with superflous parens + # TODO: we could implement associativity check to remove more redundant parens in the future + f = +@@ -37,5 +37,5 @@ + + # 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..290195e6 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:41:3] │ - 16 │ (null) + 41 │ (null) · ───┬── · ╰──── Useless parentheses around body of let expression ────╯ @@ -44,3 +45,4 @@ expression: "& out" · ────────┬─────── · ╰───────── Useless parentheses around value in binding ────╯ + From 07e0cfcfaa587400b5ddfd581da7589f166ae7a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 10:13:41 +0200 Subject: [PATCH 2/8] feat: useless parens in binops --- lib/src/lints/useless_parens.rs | 70 +++++++++++++++++++++++++++------ 1 file changed, 59 insertions(+), 11 deletions(-) diff --git a/lib/src/lints/useless_parens.rs b/lib/src/lints/useless_parens.rs index 17561961..801f3e74 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, KeyValue, LetIn, Paren, ParsedType, TypedNode, Wrapper}, }; /// ## What it does @@ -39,10 +39,16 @@ 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), +} + impl Rule for UselessParens { fn validate(&self, node: &SyntaxElement, _sess: &SessionInfo) -> Option { if let NodeOrToken::Node(node) = node @@ -50,7 +56,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 +67,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 +78,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,11 +96,47 @@ 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(bin_op) => { + let maybe_diagnostic = |node: SyntaxNode| -> Option { + let range = node.text_range(); + let as_parens = Paren::cast(node)?; + let inner = as_parens.inner()?; + + // TODO: + // it would be nice to compare operator precedence + // currently we only check if inner is function + if Apply::cast(inner.clone()).is_some() { + let at = range; + let message = "Useless parentheses in operand of binary operator"; + let replacement = inner; + Some(Diagnostic::suggest( + at, + message, + Suggestion::new(at, replacement), + )) + } else { + None + } + }; + + // Fix rhs then lhs otherwise the position will drift + let diagnostics = vec![bin_op.rhs(), bin_op.lhs()] + .into_iter() + .flatten() + .filter_map(maybe_diagnostic) + .collect::>(); + + if diagnostics.len() > 0 { + Some(OneOrMany::Many(diagnostics)) } else { None } @@ -105,7 +150,10 @@ 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() + + // ensure that we don't lint inside of neither bin-op branches + && BinOp::cast(father_node).is_none() && let Some(inner_node) = paren_expr.inner() && let Some(parsed_inner) = ParsedType::cast(inner_node) @@ -121,11 +169,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 } From a6c8f1ce1dd36dcd19c799518024f7d9b1cfefb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 10:14:06 +0200 Subject: [PATCH 3/8] test: accept new useless parens in binops behaviour --- .../snapshots/main__useless_parens_fix.snap | 28 +++++++++++++++++-- .../snapshots/main__useless_parens_lint.snap | 26 +++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/bin/tests/snapshots/main__useless_parens_fix.snap b/bin/tests/snapshots/main__useless_parens_fix.snap index 9d8ee718..65e0373b 100644 --- a/bin/tests/snapshots/main__useless_parens_fix.snap +++ b/bin/tests/snapshots/main__useless_parens_fix.snap @@ -5,7 +5,7 @@ expression: "& stdout" --- --- tests/data/useless_parens.nix +++ tests/data/useless_parens.nix [fixed] -@@ -1,15 +1,15 @@ +@@ -1,21 +1,21 @@ let # parens around primitives a = { @@ -26,7 +26,31 @@ expression: "& stdout" # binary exprs with superflous parens # TODO: we could implement associativity check to remove more redundant parens in the future f = -@@ -37,5 +37,5 @@ + let id = x: x; in +- (id [3]) ++ id [3] + ++ (id [1] ++ [2]) + ; + + # binary exprs with necessary parens +@@ -25,17 +25,17 @@ + ; + + # string concat + s = +- (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 diff --git a/bin/tests/snapshots/main__useless_parens_lint.snap b/bin/tests/snapshots/main__useless_parens_lint.snap index 290195e6..30b705f1 100644 --- a/bin/tests/snapshots/main__useless_parens_lint.snap +++ b/bin/tests/snapshots/main__useless_parens_lint.snap @@ -45,4 +45,30 @@ expression: "& out" · ────────┬─────── · ╰───────── Useless parentheses around value in binding ────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:17:5] + │ + 17 │ (id [3]) + · ────┬─── + · ╰───── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:33:7] + │ + 33 │ ╭─▶ + (lib.optionalString true '' + 35 │ ├─▶ '') + · │ + · ╰───────────── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:29:5] + │ + 29 │ (builtins.readFile ./x.txt) + · ─────────────┬───────────── + · ╰─────────────── Useless parentheses in operand of binary operator + 30 │ ╭─▶ + (lib.optionalString true '' + 32 │ ├─▶ '') + · │ + · ╰───────────── Useless parentheses in operand of binary operator +────╯ From a1fa246e104c39c5ef942db25473663f11217b78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 10:18:50 +0200 Subject: [PATCH 4/8] lints/useless_parens: apply clippy lints --- lib/src/lints/useless_parens.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/src/lints/useless_parens.rs b/lib/src/lints/useless_parens.rs index 801f3e74..4a493636 100644 --- a/lib/src/lints/useless_parens.rs +++ b/lib/src/lints/useless_parens.rs @@ -135,10 +135,10 @@ fn do_thing(parsed_type_node: ParsedType) -> Option> { .filter_map(maybe_diagnostic) .collect::>(); - if diagnostics.len() > 0 { - Some(OneOrMany::Many(diagnostics)) - } else { + if diagnostics.is_empty() { None + } else { + Some(OneOrMany::Many(diagnostics)) } } ParsedType::Paren(paren_expr) => { From 749f6120fc7df9a6118f0eeebe830bbd7b30a24a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 16:38:15 +0200 Subject: [PATCH 5/8] lints/useless_parens: handle binop precedence and associativity --- bin/tests/data/useless_parens.nix | 11 +++ .../snapshots/main__useless_parens_fix.snap | 21 +++++- .../snapshots/main__useless_parens_lint.snap | 35 +++++++--- lib/src/lints/useless_parens.rs | 67 ++++++++++++++++--- 4 files changed, 112 insertions(+), 22 deletions(-) diff --git a/bin/tests/data/useless_parens.nix b/bin/tests/data/useless_parens.nix index ed2ea9cb..3322324c 100644 --- a/bin/tests/data/useless_parens.nix +++ b/bin/tests/data/useless_parens.nix @@ -24,6 +24,17 @@ let * (2 + 2) ; + # precedence + prec1 = + 4 + (5 * 3) + ; + prec2 = + (4 * 5) / 5 + ; + prec3_no = + 4 * (5 / 5) + ; + # string concat s = (builtins.readFile ./x.txt) diff --git a/bin/tests/snapshots/main__useless_parens_fix.snap b/bin/tests/snapshots/main__useless_parens_fix.snap index 65e0373b..da4fc98a 100644 --- a/bin/tests/snapshots/main__useless_parens_fix.snap +++ b/bin/tests/snapshots/main__useless_parens_fix.snap @@ -5,7 +5,7 @@ expression: "& stdout" --- --- tests/data/useless_parens.nix +++ tests/data/useless_parens.nix [fixed] -@@ -1,21 +1,21 @@ +@@ -1,22 +1,22 @@ let # parens around primitives a = { @@ -28,12 +28,27 @@ expression: "& stdout" f = let id = x: x; in - (id [3]) +- ++ (id [1] ++ [2]) + id [3] - ++ (id [1] ++ [2]) ++ ++ id [1] ++ [2] ; # binary exprs with necessary parens -@@ -25,17 +25,17 @@ + u = +@@ -25,28 +25,28 @@ + ; + + # precedence + prec1 = +- 4 + (5 * 3) ++ 4 + 5 * 3 + ; + prec2 = +- (4 * 5) / 5 ++ 4 * 5 / 5 + ; + prec3_no = + 4 * (5 / 5) ; # string concat diff --git a/bin/tests/snapshots/main__useless_parens_lint.snap b/bin/tests/snapshots/main__useless_parens_lint.snap index 30b705f1..9ac024cb 100644 --- a/bin/tests/snapshots/main__useless_parens_lint.snap +++ b/bin/tests/snapshots/main__useless_parens_lint.snap @@ -4,9 +4,9 @@ expression: "& out" --- [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:41:3] + ╭─[data/useless_parens.nix:52:3] │ - 41 │ (null) + 52 │ (null) · ───┬── · ╰──── Useless parentheses around body of let expression ────╯ @@ -51,23 +51,40 @@ expression: "& out" 17 │ (id [3]) · ────┬─── · ╰───── Useless parentheses in operand of binary operator + 18 │ ++ (id [1] ++ [2]) + · ───────┬─────── + · ╰───────── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:33:7] + ╭─[data/useless_parens.nix:29:9] │ - 33 │ ╭─▶ + (lib.optionalString true '' - 35 │ ├─▶ '') + 29 │ 4 + (5 * 3) + · ───┬─── + · ╰───── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:32:5] + │ + 32 │ (4 * 5) / 5 + · ───┬─── + · ╰───── Useless parentheses in operand of binary operator +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:44:7] + │ + 44 │ ╭─▶ + (lib.optionalString true '' + 46 │ ├─▶ '') · │ · ╰───────────── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:29:5] + ╭─[data/useless_parens.nix:40:5] │ - 29 │ (builtins.readFile ./x.txt) + 40 │ (builtins.readFile ./x.txt) · ─────────────┬───────────── · ╰─────────────── Useless parentheses in operand of binary operator - 30 │ ╭─▶ + (lib.optionalString true '' - 32 │ ├─▶ '') + 41 │ ╭─▶ + (lib.optionalString true '' + 43 │ ├─▶ '') · │ · ╰───────────── Useless parentheses in operand of binary operator ────╯ diff --git a/lib/src/lints/useless_parens.rs b/lib/src/lints/useless_parens.rs index 4a493636..96eddd60 100644 --- a/lib/src/lints/useless_parens.rs +++ b/lib/src/lints/useless_parens.rs @@ -3,7 +3,7 @@ use crate::{Diagnostic, Metadata, Report, Rule, Suggestion, session::SessionInfo use macros::lint; use rnix::{ NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode, - types::{Apply, BinOp, KeyValue, LetIn, Paren, ParsedType, TypedNode, Wrapper}, + types::{Apply, BinOp, BinOpKind, KeyValue, LetIn, Paren, ParsedType, TypedNode, Wrapper}, }; /// ## What it does @@ -49,6 +49,15 @@ enum OneOrMany { 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 @@ -106,14 +115,12 @@ fn do_thing(parsed_type_node: ParsedType) -> Option> { } } ParsedType::BinOp(bin_op) => { - let maybe_diagnostic = |node: SyntaxNode| -> Option { + 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()?; - // TODO: - // it would be nice to compare operator precedence - // currently we only check if inner is function + // TODO: unify this with binops if Apply::cast(inner.clone()).is_some() { let at = range; let message = "Useless parentheses in operand of binary operator"; @@ -123,17 +130,57 @@ fn do_thing(parsed_type_node: ParsedType) -> Option> { message, Suggestion::new(at, replacement), )) + } else if let Some(nested) = BinOp::cast(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), + }) + }; + + let (outer_prec, outer_assoc) = prec_of(bin_op.clone())?; + let (inner_prec, _inner_assoc) = prec_of(nested.clone())?; + if inner_prec < outer_prec + || (inner_prec == outer_prec && (is_left && outer_assoc == Assoc::Left) + || (!is_left && outer_assoc == Assoc::Right)) + { + let at = range; + let message = "Useless parentheses in operand of binary operator"; + let replacement = inner; + Some(Diagnostic::suggest( + at, + message, + Suggestion::new(at, replacement), + )) + } else { + None + } } else { None } }; // Fix rhs then lhs otherwise the position will drift - let diagnostics = vec![bin_op.rhs(), bin_op.lhs()] - .into_iter() - .flatten() - .filter_map(maybe_diagnostic) - .collect::>(); + let diagnostics = vec![ + bin_op.rhs().map(|node| (node, false)), + bin_op.lhs().map(|node| (node, true)), + ] + .into_iter() + .flatten() + .filter_map(maybe_diagnostic) + .collect::>(); if diagnostics.is_empty() { None From 34dc821c404d59ad87be870fcad7871270aafe1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 16:49:16 +0200 Subject: [PATCH 6/8] lints/useless_parens: refactor --- lib/src/lints/useless_parens.rs | 75 +++++++++++++++------------------ 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/lib/src/lints/useless_parens.rs b/lib/src/lints/useless_parens.rs index 96eddd60..77ab6148 100644 --- a/lib/src/lints/useless_parens.rs +++ b/lib/src/lints/useless_parens.rs @@ -114,56 +114,47 @@ fn do_thing(parsed_type_node: ParsedType) -> Option> { None } } - ParsedType::BinOp(bin_op) => { + 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()?; - // TODO: unify this with binops - if Apply::cast(inner.clone()).is_some() { - let at = range; - let message = "Useless parentheses in operand of binary operator"; - let replacement = inner; - Some(Diagnostic::suggest( - at, - message, - Suggestion::new(at, replacement), - )) - } else if let Some(nested) = BinOp::cast(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), - }) - }; + 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), + }) + }; - let (outer_prec, outer_assoc) = prec_of(bin_op.clone())?; - let (inner_prec, _inner_assoc) = prec_of(nested.clone())?; + 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)) { - let at = range; - let message = "Useless parentheses in operand of binary operator"; - let replacement = inner; - Some(Diagnostic::suggest( - at, - message, - Suggestion::new(at, replacement), - )) + Some(suggestion) } else { None } @@ -174,8 +165,8 @@ fn do_thing(parsed_type_node: ParsedType) -> Option> { // Fix rhs then lhs otherwise the position will drift let diagnostics = vec![ - bin_op.rhs().map(|node| (node, false)), - bin_op.lhs().map(|node| (node, true)), + binop.rhs().map(|node| (node, false)), + binop.lhs().map(|node| (node, true)), ] .into_iter() .flatten() From 78a640f239a43bafd5cac84ff22fb6494af31ad1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 16:58:09 +0200 Subject: [PATCH 7/8] lints/useless_parens: allow primitive lint in binop --- bin/tests/data/useless_parens.nix | 5 ++++ .../snapshots/main__useless_parens_fix.snap | 8 +++++- .../snapshots/main__useless_parens_lint.snap | 25 ++++++++++++------- lib/src/lints/useless_parens.rs | 3 +-- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/bin/tests/data/useless_parens.nix b/bin/tests/data/useless_parens.nix index 3322324c..489bcae5 100644 --- a/bin/tests/data/useless_parens.nix +++ b/bin/tests/data/useless_parens.nix @@ -35,6 +35,11 @@ let 4 * (5 / 5) ; + # primitive in binop + primitive_binop = + [1] ++ ([2]) + ; + # string concat s = (builtins.readFile ./x.txt) diff --git a/bin/tests/snapshots/main__useless_parens_fix.snap b/bin/tests/snapshots/main__useless_parens_fix.snap index da4fc98a..5f39f9aa 100644 --- a/bin/tests/snapshots/main__useless_parens_fix.snap +++ b/bin/tests/snapshots/main__useless_parens_fix.snap @@ -35,7 +35,7 @@ expression: "& stdout" # binary exprs with necessary parens u = -@@ -25,28 +25,28 @@ +@@ -25,33 +25,33 @@ ; # precedence @@ -51,6 +51,12 @@ expression: "& stdout" 4 * (5 / 5) ; + # primitive in binop + primitive_binop = +- [1] ++ ([2]) ++ [1] ++ [2] + ; + # string concat s = - (builtins.readFile ./x.txt) diff --git a/bin/tests/snapshots/main__useless_parens_lint.snap b/bin/tests/snapshots/main__useless_parens_lint.snap index 9ac024cb..ae71aeb2 100644 --- a/bin/tests/snapshots/main__useless_parens_lint.snap +++ b/bin/tests/snapshots/main__useless_parens_lint.snap @@ -4,9 +4,9 @@ expression: "& out" --- [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:52:3] + ╭─[data/useless_parens.nix:57:3] │ - 52 │ (null) + 57 │ (null) · ───┬── · ╰──── Useless parentheses around body of let expression ────╯ @@ -70,21 +70,28 @@ expression: "& out" · ╰───── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:44:7] + ╭─[data/useless_parens.nix:40:12] │ - 44 │ ╭─▶ + (lib.optionalString true '' - 46 │ ├─▶ '') + 40 │ [1] ++ ([2]) + · ──┬── + · ╰──── Useless parentheses around primitive expression +────╯ +[W08] Warning: These parentheses can be omitted + ╭─[data/useless_parens.nix:49:7] + │ + 49 │ ╭─▶ + (lib.optionalString true '' + 51 │ ├─▶ '') · │ · ╰───────────── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:40:5] + ╭─[data/useless_parens.nix:45:5] │ - 40 │ (builtins.readFile ./x.txt) + 45 │ (builtins.readFile ./x.txt) · ─────────────┬───────────── · ╰─────────────── Useless parentheses in operand of binary operator - 41 │ ╭─▶ + (lib.optionalString true '' - 43 │ ├─▶ '') + 46 │ ╭─▶ + (lib.optionalString true '' + 48 │ ├─▶ '') · │ · ╰───────────── Useless parentheses in operand of binary operator ────╯ diff --git a/lib/src/lints/useless_parens.rs b/lib/src/lints/useless_parens.rs index 77ab6148..04d71ef7 100644 --- a/lib/src/lints/useless_parens.rs +++ b/lib/src/lints/useless_parens.rs @@ -190,8 +190,7 @@ fn do_thing(parsed_type_node: ParsedType) -> Option> { // if this primitive is a let-body, we have already linted it && LetIn::cast(father_node.clone()).is_none() - // ensure that we don't lint inside of neither bin-op branches - && BinOp::cast(father_node).is_none() + // allow checking primitive expressions in binops && let Some(inner_node) = paren_expr.inner() && let Some(parsed_inner) = ParsedType::cast(inner_node) From deb7c9a562d190d0b7786bd78ac49d520f30cf9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9ana=20=E6=B1=9F?= Date: Mon, 18 Aug 2025 17:01:56 +0200 Subject: [PATCH 8/8] test: clean up tests for useless parens --- bin/tests/data/useless_parens.nix | 24 ++++----- .../snapshots/main__useless_parens_fix.snap | 33 +++++------- .../snapshots/main__useless_parens_lint.snap | 52 +++++++++---------- 3 files changed, 49 insertions(+), 60 deletions(-) diff --git a/bin/tests/data/useless_parens.nix b/bin/tests/data/useless_parens.nix index 489bcae5..c72009f1 100644 --- a/bin/tests/data/useless_parens.nix +++ b/bin/tests/data/useless_parens.nix @@ -1,47 +1,41 @@ 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; }); - # binary exprs with superflous parens - # TODO: we could implement associativity check to remove more redundant parens in the future - f = + binop = let id = x: x; in (id [3]) ++ (id [1] ++ [2]) ; - - # binary exprs with necessary parens - u = + binop_no = (1 + 1) * (2 + 2) ; # precedence - prec1 = + arith1 = 4 + (5 * 3) ; - prec2 = + arith2 = (4 * 5) / 5 ; - prec3_no = + arith3_no = 4 * (5 / 5) ; - # primitive in binop primitive_binop = [1] ++ ([2]) ; - # string concat - s = + string_concat = (builtins.readFile ./x.txt) + (lib.optionalString true '' foo diff --git a/bin/tests/snapshots/main__useless_parens_fix.snap b/bin/tests/snapshots/main__useless_parens_fix.snap index 5f39f9aa..a50284ba 100644 --- a/bin/tests/snapshots/main__useless_parens_fix.snap +++ b/bin/tests/snapshots/main__useless_parens_fix.snap @@ -5,10 +5,10 @@ expression: "& stdout" --- --- tests/data/useless_parens.nix +++ tests/data/useless_parens.nix [fixed] -@@ -1,22 +1,22 @@ +@@ -1,51 +1,51 @@ let # parens around primitives - a = { + primitive = { - b = ("hello"); - c = (d); - e = ({ f = 2; }); @@ -18,47 +18,42 @@ 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; }; - # binary exprs with superflous parens - # TODO: we could implement associativity check to remove more redundant parens in the future - f = + binop = let id = x: x; in - (id [3]) - ++ (id [1] ++ [2]) + id [3] + ++ id [1] ++ [2] ; - - # binary exprs with necessary parens - u = -@@ -25,33 +25,33 @@ + binop_no = + (1 + 1) + * (2 + 2) ; # precedence - prec1 = + arith1 = - 4 + (5 * 3) + 4 + 5 * 3 ; - prec2 = + arith2 = - (4 * 5) / 5 + 4 * 5 / 5 ; - prec3_no = + arith3_no = 4 * (5 / 5) ; - # primitive in binop primitive_binop = - [1] ++ ([2]) + [1] ++ [2] ; - # string concat - s = + string_concat = - (builtins.readFile ./x.txt) - + (lib.optionalString true '' + builtins.readFile ./x.txt diff --git a/bin/tests/snapshots/main__useless_parens_lint.snap b/bin/tests/snapshots/main__useless_parens_lint.snap index ae71aeb2..1944ef39 100644 --- a/bin/tests/snapshots/main__useless_parens_lint.snap +++ b/bin/tests/snapshots/main__useless_parens_lint.snap @@ -4,9 +4,9 @@ expression: "& out" --- [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:57:3] + ╭─[data/useless_parens.nix:51:3] │ - 57 │ (null) + 51 │ (null) · ───┬── · ╰──── Useless parentheses around body of let expression ────╯ @@ -32,66 +32,66 @@ 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:17:5] + ╭─[data/useless_parens.nix:15:5] │ - 17 │ (id [3]) + 15 │ (id [3]) · ────┬─── · ╰───── Useless parentheses in operand of binary operator - 18 │ ++ (id [1] ++ [2]) + 16 │ ++ (id [1] ++ [2]) · ───────┬─────── · ╰───────── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:29:9] + ╭─[data/useless_parens.nix:25:9] │ - 29 │ 4 + (5 * 3) + 25 │ 4 + (5 * 3) · ───┬─── · ╰───── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:32:5] + ╭─[data/useless_parens.nix:28:5] │ - 32 │ (4 * 5) / 5 + 28 │ (4 * 5) / 5 · ───┬─── · ╰───── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:40:12] + ╭─[data/useless_parens.nix:35:12] │ - 40 │ [1] ++ ([2]) + 35 │ [1] ++ ([2]) · ──┬── · ╰──── Useless parentheses around primitive expression ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:49:7] + ╭─[data/useless_parens.nix:43:7] │ - 49 │ ╭─▶ + (lib.optionalString true '' - 51 │ ├─▶ '') + 43 │ ╭─▶ + (lib.optionalString true '' + 45 │ ├─▶ '') · │ · ╰───────────── Useless parentheses in operand of binary operator ────╯ [W08] Warning: These parentheses can be omitted - ╭─[data/useless_parens.nix:45:5] + ╭─[data/useless_parens.nix:39:5] │ - 45 │ (builtins.readFile ./x.txt) + 39 │ (builtins.readFile ./x.txt) · ─────────────┬───────────── · ╰─────────────── Useless parentheses in operand of binary operator - 46 │ ╭─▶ + (lib.optionalString true '' - 48 │ ├─▶ '') + 40 │ ╭─▶ + (lib.optionalString true '' + 42 │ ├─▶ '') · │ · ╰───────────── Useless parentheses in operand of binary operator ────╯