From 8552c61c5a8e91b30bd35e4711d39d470c83cbb1 Mon Sep 17 00:00:00 2001 From: Kevin Leimkuhler Date: Mon, 1 Oct 2018 22:32:26 -0700 Subject: [PATCH 1/5] Add initial impl of check_pat() for UnusedParens This uses a copied version of `check_unused_parens_expr` that is specific to `ast::Pat`. `check_unused_parens_` could possibly be made more generic to work with any `ast::*` that has `node` and `span` fields. This also only checks for the case of parens around the wildcard pattern. It covers the case highlighted in the issue, but could check for a lot more. --- src/librustc_lint/unused.rs | 72 ++++++++++++++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 4 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index ae178888b6a1c..b559b7b8cd8c0 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -264,7 +264,7 @@ declare_lint! { pub struct UnusedParens; impl UnusedParens { - fn check_unused_parens_core(&self, + fn check_unused_parens_expr(&self, cx: &EarlyContext, value: &ast::Expr, msg: &str, @@ -313,6 +313,56 @@ impl UnusedParens { } } } + + fn check_unused_parens_pat(&self, + cx: &EarlyContext, + value: &ast::Pat, + msg: &str, + struct_lit_needs_parens: bool) { + if let ast::PatKind::Paren(_) = value.node { + // Does there need to be a check similar to `parser::contains_exterior_struct_lit` + // here? + if !struct_lit_needs_parens { + let span_msg = format!("unnecessary parentheses around {}", msg); + let mut err = cx.struct_span_lint(UNUSED_PARENS, + value.span, + &span_msg); + // Remove exactly one pair of parentheses (rather than naïvely + // stripping all paren characters) + let mut ate_left_paren = false; + let mut ate_right_paren = false; + let parens_removed = pprust::pat_to_string(value) + .trim_matches(|c| { + match c { + '(' => { + if ate_left_paren { + false + } else { + ate_left_paren = true; + true + } + }, + ')' => { + if ate_right_paren { + false + } else { + ate_right_paren = true; + true + } + }, + _ => false, + } + }).to_owned(); + err.span_suggestion_short_with_applicability( + value.span, + "remove these parentheses", + parens_removed, + Applicability::MachineApplicable + ); + err.emit(); + } + } + } } impl LintPass for UnusedParens { @@ -354,18 +404,32 @@ impl EarlyLintPass for UnusedParens { } let msg = format!("{} argument", call_kind); for arg in args_to_check { - self.check_unused_parens_core(cx, arg, &msg, false); + self.check_unused_parens_expr(cx, arg, &msg, false); } return; } }; - self.check_unused_parens_core(cx, &value, msg, struct_lit_needs_parens); + self.check_unused_parens_expr(cx, &value, msg, struct_lit_needs_parens); + } + + fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat) { + use ast::PatKind::*; + let (value, msg) = match p.node { + Paren(ref pat) => { + match pat.node { + Wild => (p, "wildcard pattern"), + _ => return, + } + } + _ => return, + }; + self.check_unused_parens_pat(cx, &value, msg, false); } fn check_stmt(&mut self, cx: &EarlyContext, s: &ast::Stmt) { if let ast::StmtKind::Local(ref local) = s.node { if let Some(ref value) = local.init { - self.check_unused_parens_core(cx, &value, "assigned value", false); + self.check_unused_parens_expr(cx, &value, "assigned value", false); } } } From 5217527a5b2e09342e00cf8571e8af22f50546c3 Mon Sep 17 00:00:00 2001 From: Kevin Leimkuhler Date: Thu, 4 Oct 2018 09:36:55 -0700 Subject: [PATCH 2/5] Share outer paren trimming logic --- src/librustc_lint/unused.rs | 130 +++++++++++++----------------------- 1 file changed, 46 insertions(+), 84 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index b559b7b8cd8c0..a176bd8302609 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -273,43 +273,8 @@ impl UnusedParens { let necessary = struct_lit_needs_parens && parser::contains_exterior_struct_lit(&inner); if !necessary { - let span_msg = format!("unnecessary parentheses around {}", msg); - let mut err = cx.struct_span_lint(UNUSED_PARENS, - value.span, - &span_msg); - // Remove exactly one pair of parentheses (rather than naïvely - // stripping all paren characters) - let mut ate_left_paren = false; - let mut ate_right_paren = false; - let parens_removed = pprust::expr_to_string(value) - .trim_matches(|c| { - match c { - '(' => { - if ate_left_paren { - false - } else { - ate_left_paren = true; - true - } - }, - ')' => { - if ate_right_paren { - false - } else { - ate_right_paren = true; - true - } - }, - _ => false, - } - }).to_owned(); - err.span_suggestion_short_with_applicability( - value.span, - "remove these parentheses", - parens_removed, - Applicability::MachineApplicable - ); - err.emit(); + let pattern = pprust::expr_to_string(value); + Self::remove_outer_parens(cx, value.span, &pattern, msg) } } } @@ -320,49 +285,48 @@ impl UnusedParens { msg: &str, struct_lit_needs_parens: bool) { if let ast::PatKind::Paren(_) = value.node { - // Does there need to be a check similar to `parser::contains_exterior_struct_lit` - // here? if !struct_lit_needs_parens { - let span_msg = format!("unnecessary parentheses around {}", msg); - let mut err = cx.struct_span_lint(UNUSED_PARENS, - value.span, - &span_msg); - // Remove exactly one pair of parentheses (rather than naïvely - // stripping all paren characters) - let mut ate_left_paren = false; - let mut ate_right_paren = false; - let parens_removed = pprust::pat_to_string(value) - .trim_matches(|c| { - match c { - '(' => { - if ate_left_paren { - false - } else { - ate_left_paren = true; - true - } - }, - ')' => { - if ate_right_paren { - false - } else { - ate_right_paren = true; - true - } - }, - _ => false, - } - }).to_owned(); - err.span_suggestion_short_with_applicability( - value.span, - "remove these parentheses", - parens_removed, - Applicability::MachineApplicable - ); - err.emit(); + let pattern = pprust::pat_to_string(value); + Self::remove_outer_parens(cx, value.span, &pattern, msg) } } } + + fn remove_outer_parens(cx: &EarlyContext, span: Span, pattern: &str, msg: &str) { + let span_msg = format!("unnecessary parentheses around {}", msg); + let mut err = cx.struct_span_lint(UNUSED_PARENS, span, &span_msg); + let mut ate_left_paren = false; + let mut ate_right_paren = false; + let parens_removed = pattern + .trim_matches(|c| { + match c { + '(' => { + if ate_left_paren { + false + } else { + ate_left_paren = true; + true + } + }, + ')' => { + if ate_right_paren { + false + } else { + ate_right_paren = true; + true + } + }, + _ => false, + } + }).to_owned(); + err.span_suggestion_short_with_applicability( + span, + "remove these parentheses", + parens_removed, + Applicability::MachineApplicable + ); + err.emit(); + } } impl LintPass for UnusedParens { @@ -414,16 +378,14 @@ impl EarlyLintPass for UnusedParens { fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat) { use ast::PatKind::*; - let (value, msg) = match p.node { - Paren(ref pat) => { - match pat.node { - Wild => (p, "wildcard pattern"), - _ => return, - } - } + let (value, msg, struct_lit_needs_parens) = match p.node { + Ident(.., Some(ref pat)) => (pat, "optional subpattern", false), + Ref(ref pat, _) => (pat, "reference pattern", false), + Slice(_, Some(ref pat), _) => (pat, "optional position pattern", false), + Paren(_) => (p, "pattern", false), _ => return, }; - self.check_unused_parens_pat(cx, &value, msg, false); + self.check_unused_parens_pat(cx, &value, msg, struct_lit_needs_parens); } fn check_stmt(&mut self, cx: &EarlyContext, s: &ast::Stmt) { From 46b07d670a168e832391bba35d58f823cd00b76d Mon Sep 17 00:00:00 2001 From: Kevin Leimkuhler Date: Fri, 5 Oct 2018 19:09:14 -0700 Subject: [PATCH 3/5] Simply unused_parens check and add tests --- src/librustc_lint/unused.rs | 27 +++++------- src/test/run-pass/binding/pat-tuple-7.rs | 2 +- .../ui/lint/issue-54538-unused-parens-lint.rs | 42 +++++++++++++++++++ .../issue-54538-unused-parens-lint.stderr | 42 +++++++++++++++++++ 4 files changed, 95 insertions(+), 18 deletions(-) create mode 100644 src/test/ui/lint/issue-54538-unused-parens-lint.rs create mode 100644 src/test/ui/lint/issue-54538-unused-parens-lint.stderr diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index a176bd8302609..0ef46e77280db 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -274,7 +274,7 @@ impl UnusedParens { parser::contains_exterior_struct_lit(&inner); if !necessary { let pattern = pprust::expr_to_string(value); - Self::remove_outer_parens(cx, value.span, &pattern, msg) + Self::remove_outer_parens(cx, value.span, &pattern, msg); } } } @@ -282,13 +282,10 @@ impl UnusedParens { fn check_unused_parens_pat(&self, cx: &EarlyContext, value: &ast::Pat, - msg: &str, - struct_lit_needs_parens: bool) { + msg: &str) { if let ast::PatKind::Paren(_) = value.node { - if !struct_lit_needs_parens { - let pattern = pprust::pat_to_string(value); - Self::remove_outer_parens(cx, value.span, &pattern, msg) - } + let pattern = pprust::pat_to_string(value); + Self::remove_outer_parens(cx, value.span, &pattern, msg); } } @@ -355,7 +352,9 @@ impl EarlyLintPass for UnusedParens { // first "argument" is self (which sometimes needs parens) MethodCall(_, ref args) => (&args[1..], "method"), // actual catch-all arm - _ => { return; } + _ => { + return; + } }; // Don't lint if this is a nested macro expansion: otherwise, the lint could // trigger in situations that macro authors shouldn't have to care about, e.g., @@ -377,15 +376,9 @@ impl EarlyLintPass for UnusedParens { } fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat) { - use ast::PatKind::*; - let (value, msg, struct_lit_needs_parens) = match p.node { - Ident(.., Some(ref pat)) => (pat, "optional subpattern", false), - Ref(ref pat, _) => (pat, "reference pattern", false), - Slice(_, Some(ref pat), _) => (pat, "optional position pattern", false), - Paren(_) => (p, "pattern", false), - _ => return, - }; - self.check_unused_parens_pat(cx, &value, msg, struct_lit_needs_parens); + if let ast::PatKind::Paren(_) = p.node { + self.check_unused_parens_pat(cx, &p, "pattern"); + } } fn check_stmt(&mut self, cx: &EarlyContext, s: &ast::Stmt) { diff --git a/src/test/run-pass/binding/pat-tuple-7.rs b/src/test/run-pass/binding/pat-tuple-7.rs index c32b52eac33d1..f02e3198984f4 100644 --- a/src/test/run-pass/binding/pat-tuple-7.rs +++ b/src/test/run-pass/binding/pat-tuple-7.rs @@ -12,6 +12,6 @@ fn main() { match 0 { - (pat) => assert_eq!(pat, 0) + pat => assert_eq!(pat, 0) } } diff --git a/src/test/ui/lint/issue-54538-unused-parens-lint.rs b/src/test/ui/lint/issue-54538-unused-parens-lint.rs new file mode 100644 index 0000000000000..5702a8941d466 --- /dev/null +++ b/src/test/ui/lint/issue-54538-unused-parens-lint.rs @@ -0,0 +1,42 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// compile-pass + +#![allow(unreachable_patterns)] +#![allow(unused_variables)] +#![warn(unused_parens)] + +struct A { + field: Option, +} + +fn main() { + let x = 3; + match x { + (_) => {} //~ WARNING: unnecessary parentheses around pattern + (y) => {} //~ WARNING: unnecessary parentheses around pattern + (ref r) => {} //~ WARNING: unnecessary parentheses around pattern + e @ 1...2 | (e @ (3...4)) => {} + //~^ WARNING: unnecessary parentheses around pattern (3 ... 4) + //~^ WARNING: unnecessary parentheses around pattern (e @ _) + } + + let field = "foo".to_string(); + let x: Option = Some(A { field: Some(field) }); + match x { + Some(A { + field: (ref a @ Some(_)), + //~^ WARNING: unnecessary parentheses around pattern + .. + }) => {} + _ => {} + } +} diff --git a/src/test/ui/lint/issue-54538-unused-parens-lint.stderr b/src/test/ui/lint/issue-54538-unused-parens-lint.stderr new file mode 100644 index 0000000000000..7d9eb697fd7ea --- /dev/null +++ b/src/test/ui/lint/issue-54538-unused-parens-lint.stderr @@ -0,0 +1,42 @@ +warning: unnecessary parentheses around pattern + --> $DIR/issue-54538-unused-parens-lint.rs:24:9 + | +LL | (_) => {} //~ WARNING: unnecessary parentheses around pattern + | ^^^ help: remove these parentheses + | +note: lint level defined here + --> $DIR/issue-54538-unused-parens-lint.rs:15:9 + | +LL | #![warn(unused_parens)] + | ^^^^^^^^^^^^^ + +warning: unnecessary parentheses around pattern + --> $DIR/issue-54538-unused-parens-lint.rs:25:9 + | +LL | (y) => {} //~ WARNING: unnecessary parentheses around pattern + | ^^^ help: remove these parentheses + +warning: unnecessary parentheses around pattern + --> $DIR/issue-54538-unused-parens-lint.rs:26:9 + | +LL | (ref r) => {} //~ WARNING: unnecessary parentheses around pattern + | ^^^^^^^ help: remove these parentheses + +warning: unnecessary parentheses around pattern + --> $DIR/issue-54538-unused-parens-lint.rs:27:21 + | +LL | e @ 1...2 | (e @ (3...4)) => {} + | ^^^^^^^^^^^^^ help: remove these parentheses + +warning: unnecessary parentheses around pattern + --> $DIR/issue-54538-unused-parens-lint.rs:27:26 + | +LL | e @ 1...2 | (e @ (3...4)) => {} + | ^^^^^^^ help: remove these parentheses + +warning: unnecessary parentheses around pattern + --> $DIR/issue-54538-unused-parens-lint.rs:36:20 + | +LL | field: (ref a @ Some(_)), + | ^^^^^^^^^^^^^^^^^ help: remove these parentheses + From 47014df79083f837323aa60021b893d2a33a9828 Mon Sep 17 00:00:00 2001 From: Kevin Leimkuhler Date: Sat, 6 Oct 2018 19:48:04 -0700 Subject: [PATCH 4/5] Fix Range warning and improve tests --- src/librustc_lint/unused.rs | 8 +++- .../ui/lint/issue-54538-unused-parens-lint.rs | 38 +++++++++---------- .../issue-54538-unused-parens-lint.stderr | 30 +++++++-------- 3 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 0ef46e77280db..050e52d2e0cdd 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -376,8 +376,12 @@ impl EarlyLintPass for UnusedParens { } fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat) { - if let ast::PatKind::Paren(_) = p.node { - self.check_unused_parens_pat(cx, &p, "pattern"); + use ast::PatKind::{Paren, Range}; + if let Paren(ref pat) = p.node { + match pat.node { + Range(..) => {} + _ => self.check_unused_parens_pat(cx, &p, "pattern") + } } } diff --git a/src/test/ui/lint/issue-54538-unused-parens-lint.rs b/src/test/ui/lint/issue-54538-unused-parens-lint.rs index 5702a8941d466..97a2dd59a6209 100644 --- a/src/test/ui/lint/issue-54538-unused-parens-lint.rs +++ b/src/test/ui/lint/issue-54538-unused-parens-lint.rs @@ -14,29 +14,25 @@ #![allow(unused_variables)] #![warn(unused_parens)] -struct A { - field: Option, -} - fn main() { - let x = 3; - match x { - (_) => {} //~ WARNING: unnecessary parentheses around pattern - (y) => {} //~ WARNING: unnecessary parentheses around pattern - (ref r) => {} //~ WARNING: unnecessary parentheses around pattern - e @ 1...2 | (e @ (3...4)) => {} - //~^ WARNING: unnecessary parentheses around pattern (3 ... 4) - //~^ WARNING: unnecessary parentheses around pattern (e @ _) + match 1 { + (_) => {} //~ WARNING: unnecessary parentheses around pattern + (y) => {} //~ WARNING: unnecessary parentheses around pattern + (ref r) => {} //~ WARNING: unnecessary parentheses around pattern + (e @ 1..=2) => {} //~ WARNING: unnecessary parentheses around outer pattern + (1..=2) => {} // Non ambiguous range pattern should not warn + e @ (3..=4) => {} // Non ambiguous range pattern should not warn + } + + match &1 { + (e @ &(1...2)) => {} //~ WARNING: unnecessary parentheses around outer pattern + &(_) => {} //~ WARNING: unnecessary parentheses around pattern + e @ &(1...2) => {} // Ambiguous range pattern should not warn + &(1..=2) => {} // Ambiguous range pattern should not warn } - let field = "foo".to_string(); - let x: Option = Some(A { field: Some(field) }); - match x { - Some(A { - field: (ref a @ Some(_)), - //~^ WARNING: unnecessary parentheses around pattern - .. - }) => {} - _ => {} + match &1 { + e @ &(1...2) | e @ &(3..=4) => {} // Complex ambiguous pattern should not warn + &_ => {} } } diff --git a/src/test/ui/lint/issue-54538-unused-parens-lint.stderr b/src/test/ui/lint/issue-54538-unused-parens-lint.stderr index 7d9eb697fd7ea..b76b969fd2b1a 100644 --- a/src/test/ui/lint/issue-54538-unused-parens-lint.stderr +++ b/src/test/ui/lint/issue-54538-unused-parens-lint.stderr @@ -1,7 +1,7 @@ warning: unnecessary parentheses around pattern - --> $DIR/issue-54538-unused-parens-lint.rs:24:9 + --> $DIR/issue-54538-unused-parens-lint.rs:19:9 | -LL | (_) => {} //~ WARNING: unnecessary parentheses around pattern +LL | (_) => {} //~ WARNING: unnecessary parentheses around pattern | ^^^ help: remove these parentheses | note: lint level defined here @@ -11,32 +11,32 @@ LL | #![warn(unused_parens)] | ^^^^^^^^^^^^^ warning: unnecessary parentheses around pattern - --> $DIR/issue-54538-unused-parens-lint.rs:25:9 + --> $DIR/issue-54538-unused-parens-lint.rs:20:9 | -LL | (y) => {} //~ WARNING: unnecessary parentheses around pattern +LL | (y) => {} //~ WARNING: unnecessary parentheses around pattern | ^^^ help: remove these parentheses warning: unnecessary parentheses around pattern - --> $DIR/issue-54538-unused-parens-lint.rs:26:9 + --> $DIR/issue-54538-unused-parens-lint.rs:21:9 | -LL | (ref r) => {} //~ WARNING: unnecessary parentheses around pattern +LL | (ref r) => {} //~ WARNING: unnecessary parentheses around pattern | ^^^^^^^ help: remove these parentheses warning: unnecessary parentheses around pattern - --> $DIR/issue-54538-unused-parens-lint.rs:27:21 + --> $DIR/issue-54538-unused-parens-lint.rs:22:9 | -LL | e @ 1...2 | (e @ (3...4)) => {} - | ^^^^^^^^^^^^^ help: remove these parentheses +LL | (e @ 1..=2) => {} //~ WARNING: unnecessary parentheses around outer pattern + | ^^^^^^^^^^^ help: remove these parentheses warning: unnecessary parentheses around pattern - --> $DIR/issue-54538-unused-parens-lint.rs:27:26 + --> $DIR/issue-54538-unused-parens-lint.rs:28:9 | -LL | e @ 1...2 | (e @ (3...4)) => {} - | ^^^^^^^ help: remove these parentheses +LL | (e @ &(1...2)) => {} //~ WARNING: unnecessary parentheses around outer pattern + | ^^^^^^^^^^^^^^ help: remove these parentheses warning: unnecessary parentheses around pattern - --> $DIR/issue-54538-unused-parens-lint.rs:36:20 + --> $DIR/issue-54538-unused-parens-lint.rs:29:10 | -LL | field: (ref a @ Some(_)), - | ^^^^^^^^^^^^^^^^^ help: remove these parentheses +LL | &(_) => {} //~ WARNING: unnecessary parentheses around pattern + | ^^^ help: remove these parentheses From 0e411c2597dcfe75dd76acefcecec916a950ac6e Mon Sep 17 00:00:00 2001 From: Kevin Leimkuhler Date: Tue, 9 Oct 2018 17:37:45 -0700 Subject: [PATCH 5/5] Add clarifying pattern lint comment and revert test --- src/librustc_lint/unused.rs | 4 ++++ src/test/run-pass/binding/pat-tuple-7.rs | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs index 050e52d2e0cdd..297b60d57289f 100644 --- a/src/librustc_lint/unused.rs +++ b/src/librustc_lint/unused.rs @@ -377,6 +377,10 @@ impl EarlyLintPass for UnusedParens { fn check_pat(&mut self, cx: &EarlyContext, p: &ast::Pat) { use ast::PatKind::{Paren, Range}; + // The lint visitor will visit each subpattern of `p`. We do not want to lint any range + // pattern no matter where it occurs in the pattern. For something like `&(a..=b)`, there + // is a recursive `check_pat` on `a` and `b`, but we will assume that if there are + // unnecessry parens they serve a purpose of readability. if let Paren(ref pat) = p.node { match pat.node { Range(..) => {} diff --git a/src/test/run-pass/binding/pat-tuple-7.rs b/src/test/run-pass/binding/pat-tuple-7.rs index f02e3198984f4..06bd14d188e34 100644 --- a/src/test/run-pass/binding/pat-tuple-7.rs +++ b/src/test/run-pass/binding/pat-tuple-7.rs @@ -11,7 +11,8 @@ // run-pass fn main() { + #[allow(unused_parens)] match 0 { - pat => assert_eq!(pat, 0) + (pat) => assert_eq!(pat, 0) } }