Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

unused_parens: account for or-patterns and &(mut x) #64128

Merged
merged 1 commit into from
Sep 5, 2019
Merged

unused_parens: account for or-patterns and &(mut x) #64128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 64 additions & 17 deletions src/librustc_lint/unused.rs
View file
Open in desktop
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
Open in desktop
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