Skip to content

Commit

Permalink
Rollup merge of #64128 - Centril:unused-parens-pat, r=davidtwco
Browse files Browse the repository at this point in the history 
unused_parens: account for or-patterns and `&(mut x)`

Fixes #55342.
Fixes #64106.

cc #54883
cc #64111

r? @oli-obk
  • Loading branch information
@Centril
Centril committed Sep 5, 2019
2 parents c195145 + e85b181 commit 57ffc83
Show file tree
Hide file tree
Showing 3 changed files with 256 additions and 64 deletions.
81 changes: 64 additions & 17 deletions src/librustc_lint/unused.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,18 +398,37 @@ impl UnusedParens {
}
}

fn check_unused_parens_pat(&self,
cx: &EarlyContext<'_>,
value: &ast::Pat,
msg: &str) {
if let ast::PatKind::Paren(_) = value.node {
fn check_unused_parens_pat(
&self,
cx: &EarlyContext<'_>,
value: &ast::Pat,
avoid_or: bool,
avoid_mut: bool,
) {
use ast::{PatKind, BindingMode::ByValue, Mutability::Mutable};

if let PatKind::Paren(inner) = &value.node {
match inner.node {
// 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 unnecessary parens they serve a purpose of readability.
PatKind::Range(..) => return,
// Avoid `p0 | .. | pn` if we should.
PatKind::Or(..) if avoid_or => return,
// Avoid `mut x` and `mut x @ p` if we should:
PatKind::Ident(ByValue(Mutable), ..) if avoid_mut => return,
// Otherwise proceed with linting.
_ => {}
}

let pattern_text = if let Ok(snippet) = cx.sess().source_map()
.span_to_snippet(value.span) {
snippet
} else {
pprust::pat_to_string(value)
};
Self::remove_outer_parens(cx, value.span, &pattern_text, msg, (false, false));
Self::remove_outer_parens(cx, value.span, &pattern_text, "pattern", (false, false));
}
}

Expand Down Expand Up @@ -474,6 +493,13 @@ impl EarlyLintPass for UnusedParens {
fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &ast::Expr) {
use syntax::ast::ExprKind::*;
let (value, msg, followed_by_block, left_pos, right_pos) = match e.node {
Let(ref pats, ..) => {
for p in pats {
self.check_unused_parens_pat(cx, p, false, false);
}
return;
}

If(ref cond, ref block, ..) => {
let left = e.span.lo() + syntax_pos::BytePos(2);
let right = block.span.lo();
Expand All @@ -486,7 +512,8 @@ impl EarlyLintPass for UnusedParens {
(cond, "`while` condition", true, Some(left), Some(right))
},

ForLoop(_, ref cond, ref block, ..) => {
ForLoop(ref pat, ref cond, ref block, ..) => {
self.check_unused_parens_pat(cx, pat, false, false);
(cond, "`for` head expression", true, None, Some(block.span.lo()))
}

Expand Down Expand Up @@ -531,26 +558,46 @@ 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
// unnecessary parens they serve a purpose of readability.
if let Paren(ref pat) = p.node {
match pat.node {
Range(..) => {}
_ => self.check_unused_parens_pat(cx, &p, "pattern")
}
use ast::{PatKind::*, Mutability};
match &p.node {
// Do not lint on `(..)` as that will result in the other arms being useless.
Paren(_)
// The other cases do not contain sub-patterns.
| Wild | Rest | Lit(..) | Mac(..) | Range(..) | Ident(.., None) | Path(..) => return,
// These are list-like patterns; parens can always be removed.
TupleStruct(_, ps) | Tuple(ps) | Slice(ps) | Or(ps) => for p in ps {
self.check_unused_parens_pat(cx, p, false, false);
},
Struct(_, fps, _) => for f in fps {
self.check_unused_parens_pat(cx, &f.pat, false, false);
},
// Avoid linting on `i @ (p0 | .. | pn)` and `box (p0 | .. | pn)`, #64106.
Ident(.., Some(p)) | Box(p) => self.check_unused_parens_pat(cx, p, true, false),
// Avoid linting on `&(mut x)` as `&mut x` has a different meaning, #55342.
// Also avoid linting on `& mut? (p0 | .. | pn)`, #64106.
Ref(p, m) => self.check_unused_parens_pat(cx, p, true, *m == Mutability::Immutable),
}
}

fn check_stmt(&mut self, cx: &EarlyContext<'_>, s: &ast::Stmt) {
if let ast::StmtKind::Local(ref local) = s.node {
self.check_unused_parens_pat(cx, &local.pat, false, false);

if let Some(ref value) = local.init {
self.check_unused_parens_expr(cx, &value, "assigned value", false, None, None);
}
}
}

fn check_param(&mut self, cx: &EarlyContext<'_>, param: &ast::Param) {
self.check_unused_parens_pat(cx, &param.pat, true, false);
}

fn check_arm(&mut self, cx: &EarlyContext<'_>, arm: &ast::Arm) {
for p in &arm.pats {
self.check_unused_parens_pat(cx, p, false, false);
}
}
}

declare_lint! {
Expand Down
90 changes: 70 additions & 20 deletions src/test/ui/lint/issue-54538-unused-parens-lint.rs
View file
Original file line number Diff line number Diff line change
@@ -1,25 +1,75 @@
// build-pass (FIXME(62277): could be check-pass?)
#![feature(box_patterns)]

#![feature(or_patterns)]
//~^ WARN the feature `or_patterns` is incomplete

#![allow(ellipsis_inclusive_range_patterns)]
#![allow(unreachable_patterns)]
#![allow(unused_variables)]
#![warn(unused_parens)]
#![deny(unused_parens)]

fn lint_on_top_level() {
let (a) = 0; //~ ERROR unnecessary parentheses around pattern
for (a) in 0..1 {} //~ ERROR unnecessary parentheses around pattern
if let (a) = 0 {} //~ ERROR unnecessary parentheses around pattern
while let (a) = 0 {} //~ ERROR unnecessary parentheses around pattern
fn foo((a): u8) {} //~ ERROR unnecessary parentheses around pattern
let _ = |(a): u8| 0; //~ ERROR unnecessary parentheses around pattern
}

// Don't lint in these cases (#64106).
fn or_patterns_no_lint() {
match Box::new(0) {
box (0 | 1) => {} // Should not lint as `box 0 | 1` binds as `(box 0) | 1`.
_ => {}
}

match 0 {
x @ (0 | 1) => {} // Should not lint as `x @ 0 | 1` binds as `(x @ 0) | 1`.
_ => {}
}

if let &(0 | 1) = &0 {} // Should also not lint.
if let &mut (0 | 1) = &mut 0 {} // Same.

fn foo((Ok(a) | Err(a)): Result<u8, u8>) {} // Doesn't parse if we remove parens for now.
//~^ ERROR identifier `a` is bound more than once

let _ = |(Ok(a) | Err(a)): Result<u8, u8>| 1; // `|Ok(a) | Err(a)| 1` parses as bit-or.
//~^ ERROR identifier `a` is bound more than once
}

fn or_patterns_will_lint() {
if let (0 | 1) = 0 {} //~ ERROR unnecessary parentheses around pattern
if let ((0 | 1),) = (0,) {} //~ ERROR unnecessary parentheses around pattern
if let [(0 | 1)] = [0] {} //~ ERROR unnecessary parentheses around pattern
if let 0 | (1 | 2) = 0 {} //~ ERROR unnecessary parentheses around pattern
struct TS(u8);
if let TS((0 | 1)) = TS(0) {} //~ ERROR unnecessary parentheses around pattern
struct NS { f: u8 }
if let NS { f: (0 | 1) } = (NS { f: 0 }) {} //~ ERROR unnecessary parentheses around pattern
}

// Don't lint on `&(mut x)` because `&mut x` means something else (#55342).
fn deref_mut_binding_no_lint() {
let &(mut x) = &0;
}

fn main() {
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
(_) => {} //~ ERROR unnecessary parentheses around pattern
(y) => {} //~ ERROR unnecessary parentheses around pattern
(ref r) => {} //~ ERROR unnecessary parentheses around pattern
(e @ 1...2) => {} //~ ERROR unnecessary parentheses around 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
(e @ &(1...2)) => {} //~ ERROR unnecessary parentheses around pattern
&(_) => {} //~ ERROR unnecessary parentheses around pattern
e @ &(1...2) => {} // Ambiguous range pattern should not warn
&(1...2) => {} // Ambiguous range pattern should not warn
}

match &1 {
Expand All @@ -28,19 +78,19 @@ fn main() {
}

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
(_) => {} //~ ERROR unnecessary parentheses around pattern
(y) => {} //~ ERROR unnecessary parentheses around pattern
(ref r) => {} //~ ERROR unnecessary parentheses around pattern
(e @ 1..=2) => {} //~ ERROR unnecessary parentheses around 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
(e @ &(1..=2)) => {} //~ ERROR unnecessary parentheses around pattern
&(_) => {} //~ ERROR unnecessary parentheses around pattern
e @ &(1..=2) => {} // Ambiguous range pattern should not warn
&(1..=2) => {} // Ambiguous range pattern should not warn
}

match &1 {
Expand Down
Loading

0 comments on commit 57ffc83

Please sign in to comment.