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.

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

Add unneeded-wildcard-pattern lint #4537

Merged
merged 7 commits into from Sep 23, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,7 @@ Released 2018-09-13
[`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
[`unnecessary_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap
[`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern
[`unneeded_wildcard_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_wildcard_pattern
[`unreadable_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal
[`unsafe_removed_from_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_removed_from_name
[`unsafe_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_vector_initialization
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 313 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 314 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/eval_order_dependence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ struct DivergenceVisitor<'a, 'tcx> {
impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> {
fn maybe_walk_expr(&mut self, e: &'tcx Expr) {
match e.node {
ExprKind::Closure(.., _) => {},
ExprKind::Closure(..) => {},
ExprKind::Match(ref e, ref arms, _) => {
self.visit_expr(e);
for arm in arms {
Expand Down
2 changes: 2 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
misc_early::REDUNDANT_CLOSURE_CALL,
misc_early::REDUNDANT_PATTERN,
misc_early::UNNEEDED_FIELD_PATTERN,
misc_early::UNNEEDED_WILDCARD_PATTERN,
misc_early::ZERO_PREFIXED_LITERAL,
mut_reference::UNNECESSARY_MUT_PASSED,
mutex_atomic::MUTEX_ATOMIC,
Expand Down Expand Up @@ -1044,6 +1045,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con
methods::USELESS_ASREF,
misc::SHORT_CIRCUIT_STATEMENT,
misc_early::REDUNDANT_CLOSURE_CALL,
misc_early::UNNEEDED_WILDCARD_PATTERN,
misc_early::ZERO_PREFIXED_LITERAL,
needless_bool::BOOL_COMPARISON,
needless_bool::NEEDLESS_BOOL,
Expand Down
90 changes: 89 additions & 1 deletion clippy_lints/src/misc_early.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,40 @@ declare_clippy_lint! {
"using `name @ _` in a pattern"
}

declare_clippy_lint! {
/// **What it does:** Checks for tuple patterns with a wildcard
/// pattern (`_`) is next to a rest pattern (`..`).
///
This conversation was marked as resolved.
Show resolved Hide resolved
/// **Why is this bad?** The wildcard pattern is unneeded as the rest pattern
/// can match that element as well.
///
/// **Known problems:** None.
///
/// **Example:**
/// ```rust
/// # struct TupleStruct(u32, u32, u32);
/// # let t = TupleStruct(1, 2, 3);
///
/// match t {
/// TupleStruct(0, .., _) => (),
/// _ => (),
/// }
/// ```
/// can be written as
/// ```rust
/// # struct TupleStruct(u32, u32, u32);
/// # let t = TupleStruct(1, 2, 3);
///
/// match t {
/// TupleStruct(0, ..) => (),
/// _ => (),
/// }
/// ```
pub UNNEEDED_WILDCARD_PATTERN,
complexity,
"tuple patterns with a wildcard pattern (`_`) is next to a rest pattern (`..`)"
}

declare_lint_pass!(MiscEarlyLints => [
UNNEEDED_FIELD_PATTERN,
DUPLICATE_UNDERSCORE_ARGUMENT,
Expand All @@ -204,7 +238,8 @@ declare_lint_pass!(MiscEarlyLints => [
UNSEPARATED_LITERAL_SUFFIX,
ZERO_PREFIXED_LITERAL,
BUILTIN_TYPE_SHADOW,
REDUNDANT_PATTERN
REDUNDANT_PATTERN,
UNNEEDED_WILDCARD_PATTERN,
]);

// Used to find `return` statements or equivalents e.g., `?`
Expand Down Expand Up @@ -326,6 +361,8 @@ impl EarlyLintPass for MiscEarlyLints {
);
}
}

check_unneeded_wildcard_pattern(cx, pat);
}

fn check_fn(&mut self, cx: &EarlyContext<'_>, _: FnKind<'_>, decl: &FnDecl, _: Span, _: NodeId) {
Expand Down Expand Up @@ -520,3 +557,54 @@ impl MiscEarlyLints {
}
}
}

fn check_unneeded_wildcard_pattern(cx: &EarlyContext<'_>, pat: &Pat) {
if let PatKind::TupleStruct(_, ref patterns) | PatKind::Tuple(ref patterns) = pat.node {
fn span_lint(cx: &EarlyContext<'_>, span: Span, only_one: bool) {
span_lint_and_sugg(
cx,
UNNEEDED_WILDCARD_PATTERN,
span,
if only_one {
"this pattern is unneeded as the `..` pattern can match that element"
} else {
"these patterns are unneeded as the `..` pattern can match those elements"
},
if only_one { "remove it" } else { "remove them" },
"".to_string(),
Applicability::MachineApplicable,
);
}

#[allow(clippy::trivially_copy_pass_by_ref)]
fn is_wild<P: std::ops::Deref<Target = Pat>>(pat: &&P) -> bool {
if let PatKind::Wild = pat.node {
true
} else {
false
}
}

if let Some(rest_index) = patterns.iter().position(|pat| pat.is_rest()) {
if let Some((left_index, left_pat)) = patterns[..rest_index]
.iter()
.rev()
.take_while(is_wild)
.enumerate()
.last()
{
span_lint(cx, left_pat.span.until(patterns[rest_index].span), left_index == 0);
}

if let Some((right_index, right_pat)) =
patterns[rest_index + 1..].iter().take_while(is_wild).enumerate().last()
{
span_lint(
cx,
patterns[rest_index].span.shrink_to_hi().to(right_pat.span),
right_index == 0,
);
}
}
}
}
2 changes: 1 addition & 1 deletion clippy_lints/src/no_effect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
return false;
}
match expr.node {
ExprKind::Lit(..) | ExprKind::Closure(.., _) => true,
ExprKind::Lit(..) | ExprKind::Closure(..) => true,
ExprKind::Path(..) => !has_drop(cx, cx.tables.expr_ty(expr)),
ExprKind::Index(ref a, ref b) | ExprKind::Binary(_, ref a, ref b) => {
has_no_effect(cx, a) && has_no_effect(cx, b)
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/utils/sugg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ impl<'a> Sugg<'a> {
match expr.node {
hir::ExprKind::AddrOf(..)
| hir::ExprKind::Box(..)
| hir::ExprKind::Closure(.., _)
| hir::ExprKind::Closure(..)
| hir::ExprKind::Unary(..)
| hir::ExprKind::Match(..) => Sugg::MaybeParen(snippet),
hir::ExprKind::Continue(..)
Expand Down
9 changes: 8 additions & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;

// begin lint list, do not remove this comment, it’s used in `update_lints`
pub const ALL_LINTS: [Lint; 313] = [
pub const ALL_LINTS: [Lint; 314] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
Expand Down Expand Up @@ -1974,6 +1974,13 @@ pub const ALL_LINTS: [Lint; 313] = [
deprecation: None,
module: "misc_early",
},
Lint {
name: "unneeded_wildcard_pattern",
group: "complexity",
desc: "tuple patterns with a wildcard pattern (`_`) is next to a rest pattern (`..`) pattern",
deprecation: None,
module: "misc_early",
},
Lint {
name: "unreadable_literal",
group: "style",
Expand Down
41 changes: 41 additions & 0 deletions tests/ui/unneeded_wildcard_pattern.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// run-rustfix
#![feature(stmt_expr_attributes)]
#![deny(clippy::unneeded_wildcard_pattern)]

fn main() {
let t = (0, 1, 2, 3);

if let (0, ..) = t {};
if let (0, ..) = t {};
if let (0, ..) = t {};
if let (0, ..) = t {};
if let (_, 0, ..) = t {};
if let (.., 0, _) = t {};
if let (0, _, _, _) = t {};
if let (0, ..) = t {};
if let (.., 0) = t {};

#[rustfmt::skip]
{
if let (0, ..,) = t {};
}

struct S(usize, usize, usize, usize);

let s = S(0, 1, 2, 3);

if let S(0, ..) = s {};
if let S(0, ..) = s {};
if let S(0, ..) = s {};
if let S(0, ..) = s {};
if let S(_, 0, ..) = s {};
if let S(.., 0, _) = s {};
if let S(0, _, _, _) = s {};
if let S(0, ..) = s {};
if let S(.., 0) = s {};

#[rustfmt::skip]
{
if let S(0, ..,) = s {};
}
}
41 changes: 41 additions & 0 deletions tests/ui/unneeded_wildcard_pattern.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// run-rustfix
#![feature(stmt_expr_attributes)]
#![deny(clippy::unneeded_wildcard_pattern)]

fn main() {
let t = (0, 1, 2, 3);

if let (0, .., _) = t {};
if let (0, _, ..) = t {};
if let (0, _, _, ..) = t {};
if let (0, .., _, _) = t {};
if let (_, 0, ..) = t {};
if let (.., 0, _) = t {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if let (_, .., 2, 3) = t {} doesn't get linted, does it? Can you add a test for it? This could actually lead to refactoring errors, because with the _, .. it means, there is at least one element before the 2.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It absolutely will. This is the purpose of this lint.
What errors could this lead to and how is this different from the case (0, .., _, _)(0, ..)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember what I had in mind. And I also cannot come up with something. Can you add a test for it though?

if let (0, _, _, _) = t {};
if let (0, ..) = t {};
if let (.., 0) = t {};

#[rustfmt::skip]
{
if let (0, .., _, _,) = t {};
}

struct S(usize, usize, usize, usize);

let s = S(0, 1, 2, 3);

if let S(0, .., _) = s {};
if let S(0, _, ..) = s {};
if let S(0, _, _, ..) = s {};
if let S(0, .., _, _) = s {};
if let S(_, 0, ..) = s {};
if let S(.., 0, _) = s {};
if let S(0, _, _, _) = s {};
if let S(0, ..) = s {};
if let S(.., 0) = s {};

#[rustfmt::skip]
{
if let S(0, .., _, _,) = s {};
}
}
68 changes: 68 additions & 0 deletions tests/ui/unneeded_wildcard_pattern.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
error: this pattern is unneeded as the `..` pattern can match that element
--> $DIR/unneeded_wildcard_pattern.rs:8:18
|
LL | if let (0, .., _) = t {};
| ^^^ help: remove it
|
note: lint level defined here
--> $DIR/unneeded_wildcard_pattern.rs:3:9
|
LL | #![deny(clippy::unneeded_wildcard_pattern)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: this pattern is unneeded as the `..` pattern can match that element
--> $DIR/unneeded_wildcard_pattern.rs:9:16
|
LL | if let (0, _, ..) = t {};
| ^^^ help: remove it

error: these patterns are unneeded as the `..` pattern can match those elements
--> $DIR/unneeded_wildcard_pattern.rs:10:16
|
LL | if let (0, _, _, ..) = t {};
| ^^^^^^ help: remove them

error: these patterns are unneeded as the `..` pattern can match those elements
--> $DIR/unneeded_wildcard_pattern.rs:11:18
|
LL | if let (0, .., _, _) = t {};
| ^^^^^^ help: remove them

error: these patterns are unneeded as the `..` pattern can match those elements
--> $DIR/unneeded_wildcard_pattern.rs:20:22
|
LL | if let (0, .., _, _,) = t {};
| ^^^^^^ help: remove them

error: this pattern is unneeded as the `..` pattern can match that element
--> $DIR/unneeded_wildcard_pattern.rs:27:19
|
LL | if let S(0, .., _) = s {};
| ^^^ help: remove it

error: this pattern is unneeded as the `..` pattern can match that element
--> $DIR/unneeded_wildcard_pattern.rs:28:17
|
LL | if let S(0, _, ..) = s {};
| ^^^ help: remove it

error: these patterns are unneeded as the `..` pattern can match those elements
--> $DIR/unneeded_wildcard_pattern.rs:29:17
|
LL | if let S(0, _, _, ..) = s {};
| ^^^^^^ help: remove them

error: these patterns are unneeded as the `..` pattern can match those elements
--> $DIR/unneeded_wildcard_pattern.rs:30:19
|
LL | if let S(0, .., _, _) = s {};
| ^^^^^^ help: remove them

error: these patterns are unneeded as the `..` pattern can match those elements
--> $DIR/unneeded_wildcard_pattern.rs:39:23
|
LL | if let S(0, .., _, _,) = s {};
| ^^^^^^ help: remove them

error: aborting due to 10 previous errors