mathieu/granite-rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Auto merge of #5034 - ThibsG:MatchWildErrArmImprove5024, r=flip1995

Browse source 
Match wild err arm improvements

This lint should trigger on other identifiers which have `_` prefix (such as `_e`) and only if they are unused in the panic block.

_Note_: the `is_unused` function is greatly inspired from `pat_is_wild` function in [loops lints](43ac9416d9/clippy_lints/src/loops.rs (L1689)).
I've been considering doing some refactoring, maybe in utils. Maybe this PR or a new one. What do you think ?

fixes #5024

changelog: none
This commit is contained in:
bors 2020-01-15 21:17:21 +00:00
commit be09bb47db
6 changed files with 176 additions and 98 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

78
clippy_lints/src/loops.rs
View file

@ -1,36 +1,34 @@
use crate::reexport::*;
use if_chain::if_chain;
use itertools::Itertools;
use rustc::lint::in_external_macro;
use rustc::middle::region;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id;
use rustc_hir::intravisit::{walk_block, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
use rustc_hir::*;
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_session::{declare_lint_pass, declare_tool_lint};
// use rustc::middle::region::CodeExtent;
use crate::consts::{constant, Constant};
use crate::utils::usage::mutated_variables;
use crate::utils::{is_type_diagnostic_item, qpath_res, same_tys, sext, sugg};
use rustc::hir::map::Map;
use rustc::ty::{self, Ty};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::Applicability;
use rustc_span::source_map::Span;
use rustc_span::{BytePos, Symbol};
use rustc_typeck::expr_use_visitor::*;
use std::iter::{once, Iterator};
use std::mem;
use syntax::ast;
use crate::reexport::*;
use crate::utils::paths;
use crate::utils::usage::{is_unused, mutated_variables};
use crate::utils::{
get_enclosing_block, get_parent_expr, get_trait_def_id, has_iter_method, higher, implements_trait,
is_integer_const, is_refutable, last_path_segment, match_trait_method, match_type, match_var, multispan_sugg,
snippet, snippet_opt, snippet_with_applicability, span_help_and_lint, span_lint, span_lint_and_sugg,
span_lint_and_then, SpanlessEq,
};
use crate::utils::{is_type_diagnostic_item, qpath_res, same_tys, sext, sugg};
use if_chain::if_chain;
use itertools::Itertools;
use rustc::hir::map::Map;
use rustc::lint::in_external_macro;
use rustc::middle::region;
use rustc::ty::{self, Ty};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::Applicability;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id;
use rustc_hir::intravisit::{walk_block, walk_expr, walk_pat, walk_stmt, NestedVisitorMap, Visitor};
use rustc_hir::*;
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Span;
use rustc_span::{BytePos, Symbol};
use rustc_typeck::expr_use_visitor::*;
use std::iter::{once, Iterator};
use std::mem;
use syntax::ast;
declare_clippy_lint! {
/// **What it does:** Checks for for-loops that manually copy items between
@ -1689,39 +1687,11 @@ fn check_for_mutation(
fn pat_is_wild<'tcx>(pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool {
match *pat {
PatKind::Wild => true,
PatKind::Binding(.., ident, None) if ident.as_str().starts_with('_') => {
let mut visitor = UsedVisitor {
var: ident.name,
used: false,
};
walk_expr(&mut visitor, body);
!visitor.used
},
PatKind::Binding(.., ident, None) if ident.as_str().starts_with('_') => is_unused(&ident, body),
_ => false,
}
}
struct UsedVisitor {
var: ast::Name, // var to look for
used: bool, // has the var been used otherwise?
}
impl<'tcx> Visitor<'tcx> for UsedVisitor {
type Map = Map<'tcx>;
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
if match_var(expr, self.var) {
self.used = true;
} else {
walk_expr(self, expr);
}
}
fn nested_visit_map(&mut self) -> NestedVisitorMap<'_, Self::Map> {
NestedVisitorMap::None
}
}
struct LocalUsedVisitor<'a, 'tcx> {
cx: &'a LateContext<'a, 'tcx>,
local: HirId,

52
clippy_lints/src/matches.rs
View file

@ -1,8 +1,9 @@
use crate::consts::{constant, miri_to_const, Constant};
use crate::utils::paths;
use crate::utils::sugg::Sugg;
use crate::utils::usage::is_unused;
use crate::utils::{
expr_block, is_allowed, is_expn_of, match_qpath, match_type, multispan_sugg, remove_blocks, snippet,
expr_block, is_allowed, is_expn_of, is_wild, match_qpath, match_type, multispan_sugg, remove_blocks, snippet,
snippet_with_applicability, span_help_and_lint, span_lint_and_sugg, span_lint_and_then, span_note_and_lint,
walk_ptrs_ty,
};
@ -461,33 +462,40 @@ fn check_overlapping_arms<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ex: &'tcx Expr<'
}
}
fn is_wild<'tcx>(pat: &impl std::ops::Deref<Target = Pat<'tcx>>) -> bool {
match pat.kind {
PatKind::Wild => true,
_ => false,
}
}
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.pat.kind {
let path_str = print::to_string(print::NO_ANN, |s| s.print_qpath(path, false));
if_chain! {
if path_str == "Err";
if inner.iter().any(is_wild);
if let ExprKind::Block(ref block, _) = arm.body.kind;
if is_panic_block(block);
then {
// `Err(_)` arm with `panic!` found
span_note_and_lint(cx,
MATCH_WILD_ERR_ARM,
arm.pat.span,
"`Err(_)` will match all errors, maybe not a good idea",
arm.pat.span,
"to remove this warning, match each error separately \
or use `unreachable!` macro");
if path_str == "Err" {
let mut matching_wild = inner.iter().any(is_wild);
let mut ident_bind_name = String::from("_");
if !matching_wild {
// Looking for unused bindings (i.e.: `_e`)
inner.iter().for_each(|pat| {
if let PatKind::Binding(.., ident, None) = &pat.kind {
if ident.as_str().starts_with('_') && is_unused(ident, arm.body) {
ident_bind_name = (&ident.name.as_str()).to_string();
matching_wild = true;
}
}
});
}
if_chain! {
if matching_wild;
if let ExprKind::Block(ref block, _) = arm.body.kind;
if is_panic_block(block);
then {
// `Err(_)` or `Err(_e)` arm with `panic!` found
span_note_and_lint(cx,
MATCH_WILD_ERR_ARM,
arm.pat.span,
&format!("`Err({})` matches all errors", &ident_bind_name),
arm.pat.span,
"match each error separately or use the error output",
);
}
}
}
}

8
clippy_lints/src/utils/mod.rs
View file

@ -127,6 +127,14 @@ pub fn is_present_in_source<T: LintContext>(cx: &T, span: Span) -> bool {
true
}
/// Checks if given pattern is a wildcard (`_`)
pub fn is_wild<'tcx>(pat: &impl std::ops::Deref<Target = Pat<'tcx>>) -> bool {
match pat.kind {
PatKind::Wild => true,
_ => false,
}
}
/// Checks if type is struct, enum or union type with the given def path.
pub fn match_type(cx: &LateContext<'_, '_>, ty: Ty<'_>, path: &[&str]) -> bool {
match ty.kind {

35
clippy_lints/src/utils/usage.rs
View file

@ -1,9 +1,14 @@
use crate::utils::match_var;
use rustc::hir::map::Map;
use rustc::ty;
use rustc_data_structures::fx::FxHashSet;
use rustc_hir::def::Res;
use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
use rustc_hir::*;
use rustc_lint::LateContext;
use rustc_span::symbol::Ident;
use rustc_typeck::expr_use_visitor::*;
use syntax::ast;
/// Returns a set of mutated local variable IDs, or `None` if mutations could not be determined.
pub fn mutated_variables<'a, 'tcx>(expr: &'tcx Expr<'_>, cx: &'a LateContext<'a, 'tcx>) -> Option<FxHashSet<HirId>> {
@ -70,3 +75,33 @@ impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
self.update(&cmt)
}
}
pub struct UsedVisitor {
pub var: ast::Name, // var to look for
pub used: bool, // has the var been used otherwise?
}
impl<'tcx> Visitor<'tcx> for UsedVisitor {
type Map = Map<'tcx>;
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
if match_var(expr, self.var) {
self.used = true;
} else {
walk_expr(self, expr);
}
}
fn nested_visit_map(&mut self) -> NestedVisitorMap<'_, Self::Map> {
NestedVisitorMap::None
}
}
pub fn is_unused<'tcx>(ident: &'tcx Ident, body: &'tcx Expr<'_>) -> bool {
let mut visitor = UsedVisitor {
var: ident.name,
used: false,
};
walk_expr(&mut visitor, body);
!visitor.used
}

13
tests/ui/matches.rs
View file

@ -28,6 +28,19 @@ fn match_wild_err_arm() {
},
}
match x {
Ok(3) => println!("ok"),
Ok(_) => println!("ok"),
Err(_e) => panic!(),
}
// Allowed when used in `panic!`.
match x {
Ok(3) => println!("ok"),
Ok(_) => println!("ok"),
Err(_e) => panic!("{}", _e),
}
// Allowed when not with `panic!` block.
match x {
Ok(3) => println!("ok"),

88
tests/ui/matches.stderr
View file

@ -1,11 +1,11 @@
error: `Err(_)` will match all errors, maybe not a good idea
error: `Err(_)` matches all errors
--> $DIR/matches.rs:14:9
|
LL | Err(_) => panic!("err"),
| ^^^^^^
|
= note: `-D clippy::match-wild-err-arm` implied by `-D warnings`
= note: to remove this warning, match each error separately or use `unreachable!` macro
= note: match each error separately or use the error output
error: this `match` has identical arm bodies
--> $DIR/matches.rs:13:18
@ -26,13 +26,13 @@ LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: `Err(_)` will match all errors, maybe not a good idea
error: `Err(_)` matches all errors
--> $DIR/matches.rs:20:9
|
LL | Err(_) => panic!(),
| ^^^^^^
|
= note: to remove this warning, match each error separately or use `unreachable!` macro
= note: match each error separately or use the error output
error: this `match` has identical arm bodies
--> $DIR/matches.rs:19:18
@ -52,13 +52,13 @@ LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: `Err(_)` will match all errors, maybe not a good idea
error: `Err(_)` matches all errors
--> $DIR/matches.rs:26:9
|
LL | Err(_) => {
| ^^^^^^
|
= note: to remove this warning, match each error separately or use `unreachable!` macro
= note: match each error separately or use the error output
error: this `match` has identical arm bodies
--> $DIR/matches.rs:25:18
@ -78,37 +78,45 @@ LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: `Err(_e)` matches all errors
--> $DIR/matches.rs:34:9
|
LL | Err(_e) => panic!(),
| ^^^^^^^
|
= note: match each error separately or use the error output
error: this `match` has identical arm bodies
--> $DIR/matches.rs:34:18
--> $DIR/matches.rs:33:18
|
LL | Ok(_) => println!("ok"),
| ^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/matches.rs:33:18
--> $DIR/matches.rs:32:18
|
LL | Ok(3) => println!("ok"),
| ^^^^^^^^^^^^^^
help: consider refactoring into `Ok(3) | Ok(_)`
--> $DIR/matches.rs:33:9
--> $DIR/matches.rs:32:9
|
LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: this `match` has identical arm bodies
--> $DIR/matches.rs:41:18
--> $DIR/matches.rs:40:18
|
LL | Ok(_) => println!("ok"),
| ^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/matches.rs:40:18
--> $DIR/matches.rs:39:18
|
LL | Ok(3) => println!("ok"),
| ^^^^^^^^^^^^^^
help: consider refactoring into `Ok(3) | Ok(_)`
--> $DIR/matches.rs:40:9
--> $DIR/matches.rs:39:9
|
LL | Ok(3) => println!("ok"),
| ^^^^^
@ -133,58 +141,94 @@ LL | Ok(3) => println!("ok"),
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: this `match` has identical arm bodies
--> $DIR/matches.rs:53:18
--> $DIR/matches.rs:54:18
|
LL | Ok(_) => println!("ok"),
| ^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/matches.rs:52:18
--> $DIR/matches.rs:53:18
|
LL | Ok(3) => println!("ok"),
| ^^^^^^^^^^^^^^
help: consider refactoring into `Ok(3) | Ok(_)`
--> $DIR/matches.rs:52:9
--> $DIR/matches.rs:53:9
|
LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: this `match` has identical arm bodies
--> $DIR/matches.rs:76:29
--> $DIR/matches.rs:60:18
|
LL | Ok(_) => println!("ok"),
| ^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/matches.rs:59:18
|
LL | Ok(3) => println!("ok"),
| ^^^^^^^^^^^^^^
help: consider refactoring into `Ok(3) | Ok(_)`
--> $DIR/matches.rs:59:9
|
LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: this `match` has identical arm bodies
--> $DIR/matches.rs:66:18
|
LL | Ok(_) => println!("ok"),
| ^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/matches.rs:65:18
|
LL | Ok(3) => println!("ok"),
| ^^^^^^^^^^^^^^
help: consider refactoring into `Ok(3) | Ok(_)`
--> $DIR/matches.rs:65:9
|
LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: this `match` has identical arm bodies
--> $DIR/matches.rs:89:29
|
LL | (Ok(_), Some(x)) => println!("ok {}", x),
| ^^^^^^^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/matches.rs:75:29
--> $DIR/matches.rs:88:29
|
LL | (Ok(x), Some(_)) => println!("ok {}", x),
| ^^^^^^^^^^^^^^^^^^^^
help: consider refactoring into `(Ok(x), Some(_)) | (Ok(_), Some(x))`
--> $DIR/matches.rs:75:9
--> $DIR/matches.rs:88:9
|
LL | (Ok(x), Some(_)) => println!("ok {}", x),
| ^^^^^^^^^^^^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: this `match` has identical arm bodies
--> $DIR/matches.rs:91:18
--> $DIR/matches.rs:104:18
|
LL | Ok(_) => println!("ok"),
| ^^^^^^^^^^^^^^
|
note: same as this
--> $DIR/matches.rs:90:18
--> $DIR/matches.rs:103:18
|
LL | Ok(3) => println!("ok"),
| ^^^^^^^^^^^^^^
help: consider refactoring into `Ok(3) | Ok(_)`
--> $DIR/matches.rs:90:9
--> $DIR/matches.rs:103:9
|
LL | Ok(3) => println!("ok"),
| ^^^^^
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: aborting due to 12 previous errors
error: aborting due to 15 previous errors