Skip to content
Merged
Changes from all 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
2 changes: 2 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -85,6 +85,8 @@ matrix:
allow_failures:
- os: windows
env: CARGO_INCREMENTAL=0 BASE_TESTS=true OS_WINDOWS=true
- os: osx # run base tests on both platforms
env: BASE_TESTS=true
# prevent these jobs with default env vars
exclude:
- os: linux
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -46,7 +46,7 @@ rustc_tools_util = { version = "0.2.0", path = "rustc_tools_util"}

[dev-dependencies]
cargo_metadata = "0.8.0"
compiletest_rs = { version = "0.3.22", features = ["tmp"] }
compiletest_rs = { version = "0.3.23", features = ["tmp"] }
lazy_static = "1.0"
clippy-mini-macro-test = { version = "0.2", path = "mini-macro" }
serde = { version = "1.0", features = ["derive"] }
3 changes: 1 addition & 2 deletions clippy_lints/src/cognitive_complexity.rs
Original file line number Diff line number Diff line change
@@ -112,8 +112,7 @@ impl<'tcx> Visitor<'tcx> for CCHelper {
walk_expr(self, e);
match e.node {
ExprKind::Match(_, ref arms, _) => {
let arms_n: u64 = arms.iter().map(|arm| arm.pats.len() as u64).sum();
if arms_n > 1 {
if arms.len() > 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading the intent of this code you might want to recurse and count each or-pattern as cc += 1 but I think the change you made makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah i debated this but i think we might need a more careful CC accounting for pats here

self.cc += 1;
}
self.cc += arms.iter().filter(|arm| arm.guard.is_some()).count() as u64;
39 changes: 17 additions & 22 deletions clippy_lints/src/copies.rs
Original file line number Diff line number Diff line change
@@ -193,7 +193,7 @@ fn lint_match_arms<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr) {
(min_index..=max_index).all(|index| arms[index].guard.is_none()) &&
SpanlessEq::new(cx).eq_expr(&lhs.body, &rhs.body) &&
// all patterns should have the same bindings
same_bindings(cx, &bindings(cx, &lhs.pats[0]), &bindings(cx, &rhs.pats[0]))
same_bindings(cx, &bindings(cx, &lhs.pat), &bindings(cx, &rhs.pat))
};

let indexed_arms: Vec<(usize, &Arm)> = arms.iter().enumerate().collect();
@@ -213,27 +213,22 @@ fn lint_match_arms<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &Expr) {
// span for the whole pattern, the suggestion is only shown when there is only
// one pattern. The user should know about `|` if they are already using it…

if i.pats.len() == 1 && j.pats.len() == 1 {
let lhs = snippet(cx, i.pats[0].span, "<pat1>");
let rhs = snippet(cx, j.pats[0].span, "<pat2>");

if let PatKind::Wild = j.pats[0].node {
// if the last arm is _, then i could be integrated into _
// note that i.pats[0] cannot be _, because that would mean that we're
// hiding all the subsequent arms, and rust won't compile
db.span_note(
i.body.span,
&format!(
"`{}` has the same arm body as the `_` wildcard, consider removing it`",
lhs
),
);
} else {
db.span_help(
i.pats[0].span,
&format!("consider refactoring into `{} | {}`", lhs, rhs),
);
}
let lhs = snippet(cx, i.pat.span, "<pat1>");
let rhs = snippet(cx, j.pat.span, "<pat2>");

if let PatKind::Wild = j.pat.node {
// if the last arm is _, then i could be integrated into _
// note that i.pat cannot be _, because that would mean that we're
// hiding all the subsequent arms, and rust won't compile
db.span_note(
i.body.span,
&format!(
"`{}` has the same arm body as the `_` wildcard, consider removing it`",
lhs
),
);
} else {
db.span_help(i.pat.span, &format!("consider refactoring into `{} | {}`", lhs, rhs));
Copy link
Contributor

Choose a reason for hiding this comment

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

Aside: In the spirit of this lint, in the future you may want a lint that suggests C(p0) | C(p1) => C(p0 | p1).

}
},
);
3 changes: 1 addition & 2 deletions clippy_lints/src/format.rs
Original file line number Diff line number Diff line change
@@ -84,9 +84,8 @@ fn on_argumentv1_new<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, arm
if let ExprKind::Path(ref qpath) = args[1].node;
if let Some(did) = resolve_node(cx, qpath, args[1].hir_id).opt_def_id();
if match_def_path(cx, did, &paths::DISPLAY_FMT_METHOD);
if arms[0].pats.len() == 1;
// check `(arg0,)` in match block
if let PatKind::Tuple(ref pats, None) = arms[0].pats[0].node;
if let PatKind::Tuple(ref pats, None) = arms[0].pat.node;
if pats.len() == 1;
then {
let ty = walk_ptrs_ty(cx.tables.pat_ty(&pats[0]));
4 changes: 2 additions & 2 deletions clippy_lints/src/infallible_destructuring_match.rs
Original file line number Diff line number Diff line change
@@ -47,8 +47,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InfallibleDestructingMatch {
if_chain! {
if let Some(ref expr) = local.init;
if let ExprKind::Match(ref target, ref arms, MatchSource::Normal) = expr.node;
if arms.len() == 1 && arms[0].pats.len() == 1 && arms[0].guard.is_none();
if let PatKind::TupleStruct(QPath::Resolved(None, ref variant_name), ref args, _) = arms[0].pats[0].node;
if arms.len() == 1 && arms[0].guard.is_none();
if let PatKind::TupleStruct(QPath::Resolved(None, ref variant_name), ref args, _) = arms[0].pat.node;
if args.len() == 1;
if let Some(arg) = get_arg_name(&args[0]);
let body = remove_blocks(&arms[0].body);
6 changes: 2 additions & 4 deletions clippy_lints/src/loops.rs
Original file line number Diff line number Diff line change
@@ -517,9 +517,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops {
match *source {
MatchSource::Normal | MatchSource::IfLetDesugar { .. } => {
if arms.len() == 2
&& arms[0].pats.len() == 1
&& arms[0].guard.is_none()
&& arms[1].pats.len() == 1
&& arms[1].guard.is_none()
&& is_simple_break_expr(&arms[1].body)
{
@@ -541,7 +539,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops {
"try",
format!(
"while let {} = {} {{ .. }}",
snippet_with_applicability(cx, arms[0].pats[0].span, "..", &mut applicability),
snippet_with_applicability(cx, arms[0].pat.span, "..", &mut applicability),
snippet_with_applicability(cx, matchexpr.span, "..", &mut applicability),
),
applicability,
@@ -554,7 +552,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Loops {
}
}
if let ExprKind::Match(ref match_expr, ref arms, MatchSource::WhileLetDesugar) = expr.node {
let pat = &arms[0].pats[0].node;
let pat = &arms[0].pat.node;
if let (
&PatKind::TupleStruct(ref qpath, ref pat_args, _),
&ExprKind::MethodCall(ref method_path, _, ref method_args),
88 changes: 37 additions & 51 deletions clippy_lints/src/matches.rs
Original file line number Diff line number Diff line change
@@ -14,7 +14,6 @@ use rustc::{declare_lint_pass, declare_tool_lint};
use rustc_errors::Applicability;
use std::cmp::Ordering;
use std::collections::Bound;
use std::ops::Deref;
use syntax::ast::LitKind;
use syntax::source_map::Span;

@@ -255,9 +254,12 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Matches {

#[rustfmt::skip]
fn check_single_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm], expr: &Expr) {
if arms.len() == 2 &&
arms[0].pats.len() == 1 && arms[0].guard.is_none() &&
arms[1].pats.len() == 1 && arms[1].guard.is_none() {
if arms.len() == 2 && arms[0].guard.is_none() && arms[1].guard.is_none() {
if let PatKind::Or(..) = arms[0].pat.node {
// don't lint for or patterns for now, this makes
// the lint noisy in unnecessary situations
return;
}
let els = remove_blocks(&arms[1].body);
let els = if is_unit_expr(els) {
None
@@ -283,7 +285,7 @@ fn check_single_match_single_pattern(
expr: &Expr,
els: Option<&Expr>,
) {
if is_wild(&arms[1].pats[0]) {
if is_wild(&arms[1].pat) {
report_single_match_single_pattern(cx, ex, arms, expr, els);
}
}
@@ -308,7 +310,7 @@ fn report_single_match_single_pattern(
"try this",
format!(
"if let {} = {} {}{}",
snippet(cx, arms[0].pats[0].span, ".."),
snippet(cx, arms[0].pat.span, ".."),
snippet(cx, ex.span, ".."),
expr_block(cx, &arms[0].body, None, ".."),
els_str,
@@ -336,7 +338,7 @@ fn check_single_match_opt_like(
(&paths::RESULT, "Ok"),
];

let path = match arms[1].pats[0].node {
let path = match arms[1].pat.node {
PatKind::TupleStruct(ref path, ref inner, _) => {
// Contains any non wildcard patterns (e.g., `Err(err)`)?
if !inner.iter().all(is_wild) {
@@ -365,9 +367,9 @@ fn check_match_bool(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm], expr: &Ex
expr.span,
"you seem to be trying to match on a boolean expression",
move |db| {
if arms.len() == 2 && arms[0].pats.len() == 1 {
if arms.len() == 2 {
// no guards
let exprs = if let PatKind::Lit(ref arm_bool) = arms[0].pats[0].node {
let exprs = if let PatKind::Lit(ref arm_bool) = arms[0].pat.node {
if let ExprKind::Lit(ref lit) = arm_bool.node {
match lit.node {
LitKind::Bool(true) => Some((&*arms[0].body, &*arms[1].body)),
@@ -446,7 +448,7 @@ fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) {
let ex_ty = walk_ptrs_ty(cx.tables.expr_ty(ex));
if match_type(cx, ex_ty, &paths::RESULT) {
for arm in arms {
if let PatKind::TupleStruct(ref path, ref inner, _) = arm.pats[0].node {
if let PatKind::TupleStruct(ref path, ref inner, _) = arm.pat.node {
let path_str = print::to_string(print::NO_ANN, |s| s.print_qpath(path, false));
if_chain! {
if path_str == "Err";
@@ -457,9 +459,9 @@ fn check_wild_err_arm(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) {
// `Err(_)` arm with `panic!` found
span_note_and_lint(cx,
MATCH_WILD_ERR_ARM,
arm.pats[0].span,
arm.pat.span,
"Err(_) will match all errors, maybe not a good idea",
arm.pats[0].span,
arm.pat.span,
"to remove this warning, match each error separately \
or use unreachable macro");
}
@@ -482,13 +484,11 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) {
let mut wildcard_span = None;
let mut wildcard_ident = None;
for arm in arms {
for pat in &arm.pats {
if let PatKind::Wild = pat.node {
wildcard_span = Some(pat.span);
} else if let PatKind::Binding(_, _, ident, None) = pat.node {
wildcard_span = Some(pat.span);
wildcard_ident = Some(ident);
}
if let PatKind::Wild = arm.pat.node {
wildcard_span = Some(arm.pat.span);
} else if let PatKind::Binding(_, _, ident, None) = arm.pat.node {
wildcard_span = Some(arm.pat.span);
wildcard_ident = Some(ident);
}
}

@@ -510,15 +510,13 @@ fn check_wild_enum_match(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm]) {
// covered by the set of guards that cover it, but that's really hard to do.
continue;
}
for pat in &arm.pats {
if let PatKind::Path(ref path) = pat.deref().node {
if let QPath::Resolved(_, p) = path {
missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
}
} else if let PatKind::TupleStruct(ref path, ..) = pat.deref().node {
if let QPath::Resolved(_, p) = path {
missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
}
if let PatKind::Path(ref path) = arm.pat.node {
if let QPath::Resolved(_, p) = path {
missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
}
} else if let PatKind::TupleStruct(ref path, ..) = arm.pat.node {
if let QPath::Resolved(_, p) = path {
missing_variants.retain(|e| e.ctor_def_id != Some(p.res.def_id()));
}
}
}
@@ -588,9 +586,9 @@ fn check_match_ref_pats(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm], expr:
)
};

suggs.extend(arms.iter().flat_map(|a| &a.pats).filter_map(|p| {
if let PatKind::Ref(ref refp, _) = p.node {
Some((p.span, snippet(cx, refp.span, "..").to_string()))
suggs.extend(arms.iter().filter_map(|a| {
if let PatKind::Ref(ref refp, _) = a.pat.node {
Some((a.pat.span, snippet(cx, refp.span, "..").to_string()))
} else {
None
}
@@ -605,12 +603,7 @@ fn check_match_ref_pats(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm], expr:
}

fn check_match_as_ref(cx: &LateContext<'_, '_>, ex: &Expr, arms: &[Arm], expr: &Expr) {
if arms.len() == 2
&& arms[0].pats.len() == 1
&& arms[0].guard.is_none()
&& arms[1].pats.len() == 1
&& arms[1].guard.is_none()
{
if arms.len() == 2 && arms[0].guard.is_none() && arms[1].guard.is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that I've seen this pattern a few times now btw... maybe refactor in a follow up PR.

let arm_ref: Option<BindingAnnotation> = if is_none_arm(&arms[0]) {
is_ref_some_arm(&arms[1])
} else if is_none_arm(&arms[1]) {
@@ -666,14 +659,9 @@ fn all_ranges<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arms: &'tcx [Arm]) -> Vec<Sp
arms.iter()
.flat_map(|arm| {
if let Arm {
ref pats, guard: None, ..
ref pat, guard: None, ..
} = *arm
{
pats.iter()
} else {
[].iter()
}
.filter_map(|pat| {
if let PatKind::Range(ref lhs, ref rhs, ref range_end) = pat.node {
let lhs = constant(cx, cx.tables, lhs)?.0;
let rhs = constant(cx, cx.tables, rhs)?.0;
@@ -694,9 +682,8 @@ fn all_ranges<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, arms: &'tcx [Arm]) -> Vec<Sp
node: (value.clone(), Bound::Included(value)),
});
}

None
})
}
None
})
.collect()
}
@@ -743,7 +730,7 @@ fn is_unit_expr(expr: &Expr) -> bool {

// Checks if arm has the form `None => None`
fn is_none_arm(arm: &Arm) -> bool {
match arm.pats[0].node {
match arm.pat.node {
PatKind::Path(ref path) if match_qpath(path, &paths::OPTION_NONE) => true,
_ => false,
}
@@ -752,7 +739,7 @@ fn is_none_arm(arm: &Arm) -> bool {
// Checks if arm has the form `Some(ref v) => Some(v)` (checks for `ref` and `ref mut`)
fn is_ref_some_arm(arm: &Arm) -> Option<BindingAnnotation> {
if_chain! {
if let PatKind::TupleStruct(ref path, ref pats, _) = arm.pats[0].node;
if let PatKind::TupleStruct(ref path, ref pats, _) = arm.pat.node;
if pats.len() == 1 && match_qpath(path, &paths::OPTION_SOME);
if let PatKind::Binding(rb, .., ident, _) = pats[0].node;
if rb == BindingAnnotation::Ref || rb == BindingAnnotation::RefMut;
@@ -772,9 +759,8 @@ fn is_ref_some_arm(arm: &Arm) -> Option<BindingAnnotation> {
fn has_only_ref_pats(arms: &[Arm]) -> bool {
let mapped = arms
.iter()
.flat_map(|a| &a.pats)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just inline into arm.p.node here.

.map(|p| {
match p.node {
.map(|a| {
match a.pat.node {
PatKind::Ref(..) => Some(true), // &-patterns
PatKind::Wild => Some(false), // an "anything" wildcard is also fine
_ => None, // any other pattern is not fine
2 changes: 1 addition & 1 deletion clippy_lints/src/ok_if_let.rs
Original file line number Diff line number Diff line change
@@ -42,7 +42,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OkIfLet {
if let ExprKind::Match(ref op, ref body, ref source) = expr.node; //test if expr is a match
if let MatchSource::IfLetDesugar { .. } = *source; //test if it is an If Let
if let ExprKind::MethodCall(_, _, ref result_types) = op.node; //check is expr.ok() has type Result<T,E>.ok()
if let PatKind::TupleStruct(QPath::Resolved(_, ref x), ref y, _) = body[0].pats[0].node; //get operation
if let PatKind::TupleStruct(QPath::Resolved(_, ref x), ref y, _) = body[0].pat.node; //get operation
if method_chain_args(op, &["ok"]).is_some(); //test to see if using ok() methoduse std::marker::Sized;

then {
Loading