From 0c1ffc1d1fdfd73bf326d1e7a886f59367374dc3 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sun, 4 Nov 2018 10:02:49 +0200 Subject: [PATCH 1/3] Fix `possible_missing_comma` false positives `possible_missing_comma` should only trigger when the binary operator has unary equivalent. Otherwise, it's not possible to insert a comma without breaking compilation. The operators identified were `+`, `&`, `*` and `-`. This fixes the specific examples given in issues #3244 and #3396 but doesn't address the conflict this lint has with the style of starting a line with a binary operator. --- clippy_lints/src/formatting.rs | 36 ++++++++++++++++++++++------------ tests/ui/formatting.rs | 12 ++++++++++++ 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/formatting.rs b/clippy_lints/src/formatting.rs index 7f5715ef6913..8649834e267e 100644 --- a/clippy_lints/src/formatting.rs +++ b/clippy_lints/src/formatting.rs @@ -173,24 +173,34 @@ fn check_else_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { } } +fn has_unary_equivalent(bin_op: ast::BinOpKind) -> bool { + //+, &, *, - + bin_op == ast::BinOpKind::Add + || bin_op == ast::BinOpKind::And + || bin_op == ast::BinOpKind::Mul + || bin_op == ast::BinOpKind::Sub +} + /// Implementation of the `POSSIBLE_MISSING_COMMA` lint for array fn check_array(cx: &EarlyContext<'_>, expr: &ast::Expr) { if let ast::ExprKind::Array(ref array) = expr.node { for element in array { if let ast::ExprKind::Binary(ref op, ref lhs, _) = element.node { - if !differing_macro_contexts(lhs.span, op.span) { - let space_span = lhs.span.between(op.span); - if let Some(space_snippet) = snippet_opt(cx, space_span) { - let lint_span = lhs.span.with_lo(lhs.span.hi()); - if space_snippet.contains('\n') { - span_note_and_lint( - cx, - POSSIBLE_MISSING_COMMA, - lint_span, - "possibly missing a comma here", - lint_span, - "to remove this lint, add a comma or write the expr in a single line", - ); + if has_unary_equivalent(op.node) { + if !differing_macro_contexts(lhs.span, op.span) { + let space_span = lhs.span.between(op.span); + if let Some(space_snippet) = snippet_opt(cx, space_span) { + let lint_span = lhs.span.with_lo(lhs.span.hi()); + if space_snippet.contains('\n') { + span_note_and_lint( + cx, + POSSIBLE_MISSING_COMMA, + lint_span, + "possibly missing a comma here", + lint_span, + "to remove this lint, add a comma or write the expr in a single line", + ); + } } } } diff --git a/tests/ui/formatting.rs b/tests/ui/formatting.rs index 0dca8c8585b3..88f6e497d12e 100644 --- a/tests/ui/formatting.rs +++ b/tests/ui/formatting.rs @@ -112,4 +112,16 @@ fn main() { 1 + 2, 3 + 4, 5 + 6, ]; + + // don't lint for bin op without unary equiv + // issue 3244 + vec![ + 1 + / 2, + ]; + // issue 3396 + vec![ + true + | false, + ]; } From a3ab512576c77e08646ba731e8906a02983da2c8 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sun, 4 Nov 2018 10:48:24 +0200 Subject: [PATCH 2/3] Fix `collapsible_if` error --- clippy_lints/src/formatting.rs | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/formatting.rs b/clippy_lints/src/formatting.rs index 8649834e267e..69aabcb7949a 100644 --- a/clippy_lints/src/formatting.rs +++ b/clippy_lints/src/formatting.rs @@ -186,21 +186,19 @@ fn check_array(cx: &EarlyContext<'_>, expr: &ast::Expr) { if let ast::ExprKind::Array(ref array) = expr.node { for element in array { if let ast::ExprKind::Binary(ref op, ref lhs, _) = element.node { - if has_unary_equivalent(op.node) { - if !differing_macro_contexts(lhs.span, op.span) { - let space_span = lhs.span.between(op.span); - if let Some(space_snippet) = snippet_opt(cx, space_span) { - let lint_span = lhs.span.with_lo(lhs.span.hi()); - if space_snippet.contains('\n') { - span_note_and_lint( - cx, - POSSIBLE_MISSING_COMMA, - lint_span, - "possibly missing a comma here", - lint_span, - "to remove this lint, add a comma or write the expr in a single line", - ); - } + if has_unary_equivalent(op.node) && !differing_macro_contexts(lhs.span, op.span) { + let space_span = lhs.span.between(op.span); + if let Some(space_snippet) = snippet_opt(cx, space_span) { + let lint_span = lhs.span.with_lo(lhs.span.hi()); + if space_snippet.contains('\n') { + span_note_and_lint( + cx, + POSSIBLE_MISSING_COMMA, + lint_span, + "possibly missing a comma here", + lint_span, + "to remove this lint, add a comma or write the expr in a single line", + ); } } } From c20e17f8ee44e05067402960dc12794c377fbe03 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 6 Nov 2018 07:05:13 +0200 Subject: [PATCH 3/3] Remove `+` from `has_unary_equivalent` Rust doesn't has a unary + operator! --- clippy_lints/src/formatting.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/formatting.rs b/clippy_lints/src/formatting.rs index 69aabcb7949a..d06183cb52f1 100644 --- a/clippy_lints/src/formatting.rs +++ b/clippy_lints/src/formatting.rs @@ -174,9 +174,8 @@ fn check_else_if(cx: &EarlyContext<'_>, expr: &ast::Expr) { } fn has_unary_equivalent(bin_op: ast::BinOpKind) -> bool { - //+, &, *, - - bin_op == ast::BinOpKind::Add - || bin_op == ast::BinOpKind::And + // &, *, - + bin_op == ast::BinOpKind::And || bin_op == ast::BinOpKind::Mul || bin_op == ast::BinOpKind::Sub }