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

New lint: unnested_or_patterns #5378

Merged
merged 5 commits into from
Jun 8, 2020
Merged
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,7 @@ Released 2018-09-13
[`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
[`unnested_or_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns
[`unreachable`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreachable
[`unreadable_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal
[`unsafe_derive_deserialize`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_derive_deserialize
Expand Down
19 changes: 7 additions & 12 deletions clippy_lints/src/await_holding_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,18 +54,13 @@ declare_lint_pass!(AwaitHoldingLock => [AWAIT_HOLDING_LOCK]);
impl LateLintPass<'_, '_> for AwaitHoldingLock {
fn check_body(&mut self, cx: &LateContext<'_, '_>, body: &'_ Body<'_>) {
use AsyncGeneratorKind::{Block, Closure, Fn};
match body.generator_kind {
Some(GeneratorKind::Async(Block))
| Some(GeneratorKind::Async(Closure))
| Some(GeneratorKind::Async(Fn)) => {
let body_id = BodyId {
hir_id: body.value.hir_id,
};
let def_id = cx.tcx.hir().body_owner_def_id(body_id);
let tables = cx.tcx.typeck_tables_of(def_id);
check_interior_types(cx, &tables.generator_interior_types, body.value.span);
},
_ => {},
if let Some(GeneratorKind::Async(Block | Closure | Fn)) = body.generator_kind {
let body_id = BodyId {
hir_id: body.value.hir_id,
};
let def_id = cx.tcx.hir().body_owner_def_id(body_id);
let tables = cx.tcx.typeck_tables_of(def_id);
check_interior_types(cx, &tables.generator_interior_types, body.value.span);
}
}
}
Expand Down
8 changes: 2 additions & 6 deletions clippy_lints/src/formatting.rs
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
7 changes: 7 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// error-pattern:cargo-clippy

#![feature(bindings_after_at)]
#![feature(box_syntax)]
#![feature(box_patterns)]
#![feature(or_patterns)]
Expand All @@ -12,6 +13,7 @@
#![cfg_attr(feature = "deny-warnings", deny(warnings))]
#![feature(crate_visibility_modifier)]
#![feature(concat_idents)]
#![feature(drain_filter)]

// FIXME: switch to something more ergonomic here, once available.
// (Currently there is no way to opt into sysroot crates without `extern crate`.)
Expand Down Expand Up @@ -319,6 +321,7 @@ mod types;
mod unicode;
mod unnamed_address;
mod unnecessary_sort_by;
mod unnested_or_patterns;
mod unsafe_removed_from_name;
mod unused_io_amount;
mod unused_self;
Expand Down Expand Up @@ -836,6 +839,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&unnamed_address::FN_ADDRESS_COMPARISONS,
&unnamed_address::VTABLE_ADDRESS_COMPARISONS,
&unnecessary_sort_by::UNNECESSARY_SORT_BY,
&unnested_or_patterns::UNNESTED_OR_PATTERNS,
&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME,
&unused_io_amount::UNUSED_IO_AMOUNT,
&unused_self::UNUSED_SELF,
Expand Down Expand Up @@ -1073,6 +1077,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_early_pass(move || box non_expressive_names::NonExpressiveNames {
single_char_binding_names_threshold,
});
store.register_early_pass(|| box unnested_or_patterns::UnnestedOrPatterns);

store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
Expand Down Expand Up @@ -1433,6 +1438,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&unnamed_address::FN_ADDRESS_COMPARISONS),
LintId::of(&unnamed_address::VTABLE_ADDRESS_COMPARISONS),
LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY),
LintId::of(&unnested_or_patterns::UNNESTED_OR_PATTERNS),
LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
LintId::of(&unwrap::PANICKING_UNWRAP),
Expand Down Expand Up @@ -1616,6 +1622,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&types::UNNECESSARY_CAST),
LintId::of(&types::VEC_BOX),
LintId::of(&unnecessary_sort_by::UNNECESSARY_SORT_BY),
LintId::of(&unnested_or_patterns::UNNESTED_OR_PATTERNS),
LintId::of(&unwrap::UNNECESSARY_UNWRAP),
LintId::of(&useless_conversion::USELESS_CONVERSION),
LintId::of(&zero_div_zero::ZERO_DIVIDED_BY_ZERO),
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/methods/manual_saturating_arithmetic.rs
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
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

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

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
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
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
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
Loading