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

New lint: unnested_or_patterns #5378

Merged
merged 5 commits into from
Jun 8, 2020
Merged

New lint: unnested_or_patterns #5378

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b6dc7b
add `unnested_or_patterns` lint
Centril Mar 19, 2020
78f158e
dogfood unnested_or_patterns
Centril Mar 28, 2020
d6136b9
split unnested_or_patterns test
Centril Mar 28, 2020
ecabed6
Apply self-review by Centril
flip1995 Jun 7, 2020
a9ca832
Fix rebase fallout
flip1995 Jun 7, 2020
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
8 changes: 2 additions & 6 deletions clippy_lints/src/formatting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,8 @@ declare_lint_pass!(Formatting => [
impl EarlyLintPass for Formatting {
fn check_block(&mut self, cx: &EarlyContext<'_>, block: &Block) {
for w in block.stmts.windows(2) {
match (&w[0].kind, &w[1].kind) {
(&StmtKind::Expr(ref first), &StmtKind::Expr(ref second))
| (&StmtKind::Expr(ref first), &StmtKind::Semi(ref second)) => {
check_missing_else(cx, first, second);
},
_ => (),
if let (StmtKind::Expr(first), StmtKind::Expr(second) | StmtKind::Semi(second)) = (&w[0].kind, &w[1].kind) {
check_missing_else(cx, first, second);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/manual_saturating_arithmetic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ pub fn lint(cx: &LateContext<'_, '_>, expr: &hir::Expr<'_>, args: &[&[hir::Expr<
);
} else {
match (mm, arith) {
(MinMax::Max, "add") | (MinMax::Max, "mul") | (MinMax::Min, "sub") => (),
(MinMax::Max, "add" | "mul") | (MinMax::Min, "sub") => (),
_ => return,
}

Expand Down
13 changes: 4 additions & 9 deletions clippy_lints/src/methods/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1403,9 +1403,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
lint_search_is_some(cx, expr, "rposition", arg_lists[1], arg_lists[0], method_spans[1])
},
["extend", ..] => lint_extend(cx, expr, arg_lists[0]),
["as_ptr", "unwrap"] | ["as_ptr", "expect"] => {
lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0])
},
["as_ptr", "unwrap" | "expect"] => lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]),
Copy link
Member

@phansch phansch Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For example here, is that really less complex than before? In my mind I first have to think about the precedence to figure out what's going on. Maybe it just takes some time to get used to it.

Copy link
Member

@flip1995 flip1995 Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it just takes some time to get used to it.

I think that's it. But especially this example is really nice IMO. I think it expresses more the thought of ""as_ptr" is the first part, followed by "unwrap" OR "expect"", instead of "it's either an "as_ptr", "unwrap" array OR an "as_ptr", "expect" array. I can see why one would prefer the later way of writing this though.

What I struggeled with on first reviewing this was a pattern like:

(A | B, C | D) => { ... }

Because this is combinatorially evaluated (A+C, A+D, B+C, B+D), which may not be directly obvious, if your not used to this kind of pattern.

["nth", "iter"] => lint_iter_nth(cx, expr, &arg_lists, false),
["nth", "iter_mut"] => lint_iter_nth(cx, expr, &arg_lists, true),
["nth", ..] => lint_iter_nth_zero(cx, expr, arg_lists[0]),
Expand All @@ -1418,12 +1416,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods {
["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]),
["count", "map"] => lint_suspicious_map(cx, expr),
["assume_init"] => lint_maybe_uninit(cx, &arg_lists[0][0], expr),
["unwrap_or", arith @ "checked_add"]
| ["unwrap_or", arith @ "checked_sub"]
| ["unwrap_or", arith @ "checked_mul"] => {
["unwrap_or", arith @ ("checked_add" | "checked_sub" | "checked_mul")] => {
manual_saturating_arithmetic::lint(cx, expr, &arg_lists, &arith["checked_".len()..])
},
["add"] | ["offset"] | ["sub"] | ["wrapping_offset"] | ["wrapping_add"] | ["wrapping_sub"] => {
["add" | "offset" | "sub" | "wrapping_offset" | "wrapping_add" | "wrapping_sub"] => {
check_pointer_offset(cx, expr, arg_lists[0])
},
["is_file", ..] => lint_filetype_is_file(cx, expr, arg_lists[0]),
Expand Down Expand Up @@ -1829,8 +1825,7 @@ fn lint_expect_fun_call(
hir::ExprKind::Call(fun, _) => {
if let hir::ExprKind::Path(ref p) = fun.kind {
match cx.tables.qpath_res(p, fun.hir_id) {
hir::def::Res::Def(hir::def::DefKind::Fn, def_id)
| hir::def::Res::Def(hir::def::DefKind::AssocFn, def_id) => matches!(
hir::def::Res::Def(hir::def::DefKind::Fn | hir::def::DefKind::AssocFn, def_id) => matches!(
cx.tcx.fn_sig(def_id).output().skip_binder().kind,
ty::Ref(ty::ReStatic, ..)
),
Expand Down
19 changes: 8 additions & 11 deletions clippy_lints/src/misc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,17 +275,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MiscLints {
return;
}
for arg in iter_input_pats(decl, body) {
match arg.pat.kind {
PatKind::Binding(BindingAnnotation::Ref, ..) | PatKind::Binding(BindingAnnotation::RefMut, ..) => {
span_lint(
cx,
TOPLEVEL_REF_ARG,
arg.pat.span,
"`ref` directly on a function argument is ignored. Consider using a reference type \
instead.",
);
},
_ => {},
if let PatKind::Binding(BindingAnnotation::Ref | BindingAnnotation::RefMut, ..) = arg.pat.kind {
span_lint(
cx,
TOPLEVEL_REF_ARG,
arg.pat.span,
"`ref` directly on a function argument is ignored. \
Consider using a reference type instead.",
);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/no_effect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ fn reduce_expression<'a>(cx: &LateContext<'_, '_>, expr: &'a Expr<'a>) -> Option
if let ExprKind::Path(ref qpath) = callee.kind {
let res = qpath_res(cx, qpath, callee.hir_id);
match res {
Res::Def(DefKind::Struct, ..) | Res::Def(DefKind::Variant, ..) | Res::Def(DefKind::Ctor(..), _)
Res::Def(DefKind::Struct | DefKind::Variant | DefKind::Ctor(..), ..)
if !has_drop(cx, cx.tables.expr_ty(expr)) =>
{
Some(args.iter().collect())
Expand Down
6 changes: 2 additions & 4 deletions clippy_lints/src/suspicious_trait_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for SuspiciousImpl {
if let hir::Node::Expr(e) = cx.tcx.hir().get(parent_expr) {
match e.kind {
hir::ExprKind::Binary(..)
| hir::ExprKind::Unary(hir::UnOp::UnNot, _)
| hir::ExprKind::Unary(hir::UnOp::UnNeg, _)
| hir::ExprKind::Unary(hir::UnOp::UnNot | hir::UnOp::UnNeg, _)
| hir::ExprKind::AssignOp(..) => return,
_ => {},
}
Expand Down Expand Up @@ -191,8 +190,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BinaryExprVisitor {
fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) {
match expr.kind {
hir::ExprKind::Binary(..)
| hir::ExprKind::Unary(hir::UnOp::UnNot, _)
| hir::ExprKind::Unary(hir::UnOp::UnNeg, _)
| hir::ExprKind::Unary(hir::UnOp::UnNot | hir::UnOp::UnNeg, _)
| hir::ExprKind::AssignOp(..) => self.in_binary_expr = true,
_ => {},
}
Expand Down
47 changes: 22 additions & 25 deletions clippy_lints/src/transmute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
e.span,
&format!("transmute from a type (`{}`) to itself", from_ty),
),
(&ty::Ref(_, rty, rty_mutbl), &ty::RawPtr(ptr_ty)) => span_lint_and_then(
(ty::Ref(_, rty, rty_mutbl), ty::RawPtr(ptr_ty)) => span_lint_and_then(
cx,
USELESS_TRANSMUTE,
e.span,
Expand All @@ -321,10 +321,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
if let Some(arg) = sugg::Sugg::hir_opt(cx, &args[0]) {
let rty_and_mut = ty::TypeAndMut {
ty: rty,
mutbl: rty_mutbl,
mutbl: *rty_mutbl,
};

let sugg = if ptr_ty == rty_and_mut {
let sugg = if *ptr_ty == rty_and_mut {
arg.as_ty(to_ty)
} else {
arg.as_ty(cx.tcx.mk_ptr(rty_and_mut)).as_ty(to_ty)
Expand All @@ -334,7 +334,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
}
},
),
(&ty::Int(_), &ty::RawPtr(_)) | (&ty::Uint(_), &ty::RawPtr(_)) => span_lint_and_then(
(ty::Int(_) | ty::Uint(_), ty::RawPtr(_)) => span_lint_and_then(
cx,
USELESS_TRANSMUTE,
e.span,
Expand All @@ -350,16 +350,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
}
},
),
(&ty::Float(_), &ty::Ref(..))
| (&ty::Float(_), &ty::RawPtr(_))
| (&ty::Char, &ty::Ref(..))
| (&ty::Char, &ty::RawPtr(_)) => span_lint(
(ty::Float(_) | ty::Char, ty::Ref(..) | ty::RawPtr(_)) => span_lint(
cx,
WRONG_TRANSMUTE,
e.span,
&format!("transmute from a `{}` to a pointer", from_ty),
),
(&ty::RawPtr(from_ptr), _) if from_ptr.ty == to_ty => span_lint(
(ty::RawPtr(from_ptr), _) if from_ptr.ty == to_ty => span_lint(
cx,
CROSSPOINTER_TRANSMUTE,
e.span,
Expand All @@ -368,7 +365,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
from_ty, to_ty
),
),
(_, &ty::RawPtr(to_ptr)) if to_ptr.ty == from_ty => span_lint(
(_, ty::RawPtr(to_ptr)) if to_ptr.ty == from_ty => span_lint(
cx,
CROSSPOINTER_TRANSMUTE,
e.span,
Expand All @@ -377,7 +374,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
from_ty, to_ty
),
),
(&ty::RawPtr(from_pty), &ty::Ref(_, to_ref_ty, mutbl)) => span_lint_and_then(
(ty::RawPtr(from_pty), ty::Ref(_, to_ref_ty, mutbl)) => span_lint_and_then(
cx,
TRANSMUTE_PTR_TO_REF,
e.span,
Expand All @@ -388,13 +385,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
),
|diag| {
let arg = sugg::Sugg::hir(cx, &args[0], "..");
let (deref, cast) = if mutbl == Mutability::Mut {
let (deref, cast) = if *mutbl == Mutability::Mut {
("&mut *", "*mut")
} else {
("&*", "*const")
};

let arg = if from_pty.ty == to_ref_ty {
let arg = if from_pty.ty == *to_ref_ty {
arg
} else {
arg.as_ty(&format!("{} {}", cast, get_type_snippet(cx, qpath, to_ref_ty)))
Expand All @@ -408,7 +405,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
);
},
),
(&ty::Int(ast::IntTy::I32), &ty::Char) | (&ty::Uint(ast::UintTy::U32), &ty::Char) => {
(ty::Int(ast::IntTy::I32) | ty::Uint(ast::UintTy::U32), &ty::Char) => {
span_lint_and_then(
cx,
TRANSMUTE_INT_TO_CHAR,
Expand All @@ -430,13 +427,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
},
)
},
(&ty::Ref(_, ty_from, from_mutbl), &ty::Ref(_, ty_to, to_mutbl)) => {
(ty::Ref(_, ty_from, from_mutbl), ty::Ref(_, ty_to, to_mutbl)) => {
if_chain! {
if let (&ty::Slice(slice_ty), &ty::Str) = (&ty_from.kind, &ty_to.kind);
if let ty::Uint(ast::UintTy::U8) = slice_ty.kind;
if from_mutbl == to_mutbl;
then {
let postfix = if from_mutbl == Mutability::Mut {
let postfix = if *from_mutbl == Mutability::Mut {
"_mut"
} else {
""
Expand Down Expand Up @@ -465,13 +462,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
|diag| if let Some(arg) = sugg::Sugg::hir_opt(cx, &args[0]) {
let ty_from_and_mut = ty::TypeAndMut {
ty: ty_from,
mutbl: from_mutbl
mutbl: *from_mutbl
};
let ty_to_and_mut = ty::TypeAndMut { ty: ty_to, mutbl: to_mutbl };
let ty_to_and_mut = ty::TypeAndMut { ty: ty_to, mutbl: *to_mutbl };
let sugg_paren = arg
.as_ty(cx.tcx.mk_ptr(ty_from_and_mut))
.as_ty(cx.tcx.mk_ptr(ty_to_and_mut));
let sugg = if to_mutbl == Mutability::Mut {
let sugg = if *to_mutbl == Mutability::Mut {
sugg_paren.mut_addr_deref()
} else {
sugg_paren.addr_deref()
Expand All @@ -488,19 +485,19 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
}
}
},
(&ty::RawPtr(_), &ty::RawPtr(to_ty)) => span_lint_and_then(
(ty::RawPtr(_), ty::RawPtr(to_ty)) => span_lint_and_then(
cx,
TRANSMUTE_PTR_TO_PTR,
e.span,
"transmute from a pointer to a pointer",
|diag| {
if let Some(arg) = sugg::Sugg::hir_opt(cx, &args[0]) {
let sugg = arg.as_ty(cx.tcx.mk_ptr(to_ty));
let sugg = arg.as_ty(cx.tcx.mk_ptr(*to_ty));
diag.span_suggestion(e.span, "try", sugg.to_string(), Applicability::Unspecified);
}
},
),
(&ty::Int(ast::IntTy::I8), &ty::Bool) | (&ty::Uint(ast::UintTy::U8), &ty::Bool) => {
(ty::Int(ast::IntTy::I8) | ty::Uint(ast::UintTy::U8), ty::Bool) => {
span_lint_and_then(
cx,
TRANSMUTE_INT_TO_BOOL,
Expand All @@ -518,7 +515,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
},
)
},
(&ty::Int(_), &ty::Float(_)) | (&ty::Uint(_), &ty::Float(_)) => span_lint_and_then(
(ty::Int(_) | ty::Uint(_), ty::Float(_)) => span_lint_and_then(
cx,
TRANSMUTE_INT_TO_FLOAT,
e.span,
Expand All @@ -541,7 +538,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
);
},
),
(&ty::Float(float_ty), &ty::Int(_)) | (&ty::Float(float_ty), &ty::Uint(_)) => span_lint_and_then(
(ty::Float(float_ty), ty::Int(_) | ty::Uint(_)) => span_lint_and_then(
cx,
TRANSMUTE_FLOAT_TO_INT,
e.span,
Expand Down Expand Up @@ -585,7 +582,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute {
);
},
),
(&ty::Adt(ref from_adt, ref from_substs), &ty::Adt(ref to_adt, ref to_substs)) => {
(ty::Adt(from_adt, from_substs), ty::Adt(to_adt, to_substs)) => {
if from_adt.did != to_adt.did ||
!COLLECTIONS.iter().any(|path| match_def_path(cx, to_adt.did, path)) {
return;
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/unwrap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ fn collect_unwrap_info<'a, 'tcx>(

if let ExprKind::Binary(op, left, right) = &expr.kind {
match (invert, op.node) {
(false, BinOpKind::And) | (false, BinOpKind::BitAnd) | (true, BinOpKind::Or) | (true, BinOpKind::BitOr) => {
(false, BinOpKind::And | BinOpKind::BitAnd) | (true, BinOpKind::Or | BinOpKind::BitOr) => {
let mut unwrap_info = collect_unwrap_info(cx, left, branch, invert);
unwrap_info.append(&mut collect_unwrap_info(cx, right, branch, invert));
return unwrap_info;
Expand Down
9 changes: 3 additions & 6 deletions clippy_lints/src/utils/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#[macro_use]
pub mod sym;

#[allow(clippy::module_name_repetitions)]
pub mod ast_utils;
pub mod attrs;
pub mod author;
Expand Down Expand Up @@ -73,7 +74,7 @@ pub fn in_constant(cx: &LateContext<'_, '_>, id: HirId) -> bool {
let parent_id = cx.tcx.hir().get_parent_item(id);
match cx.tcx.hir().get(parent_id) {
Node::Item(&Item {
kind: ItemKind::Const(..),
kind: ItemKind::Const(..) | ItemKind::Static(..),
..
})
| Node::TraitItem(&TraitItem {
Expand All @@ -84,11 +85,7 @@ pub fn in_constant(cx: &LateContext<'_, '_>, id: HirId) -> bool {
kind: ImplItemKind::Const(..),
..
})
| Node::AnonConst(_)
| Node::Item(&Item {
kind: ItemKind::Static(..),
..
}) => true,
| Node::AnonConst(_) => true,
Node::Item(&Item {
kind: ItemKind::Fn(ref sig, ..),
..
Expand Down