Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-11 07:34:18 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #2811 from fanzier/checked_unwrap

Browse source 
Implement lint that checks for unidiomatic unwrap() (closes #1770)
This commit is contained in:
Oliver Schneider 2018-06-08 07:15:02 +02:00 committed by GitHub
commit d68b8cea15
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 510 additions and 66 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
clippy_lints/src/lib.rs
View file

@ -200,6 +200,7 @@ pub mod unicode;
pub mod unsafe_removed_from_name;
pub mod unused_io_amount;
pub mod unused_label;
pub mod unwrap;
pub mod use_self;
pub mod vec;
pub mod write;
@ -421,6 +422,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
reg.register_late_lint_pass(box infallible_destructuring_match::Pass);
reg.register_late_lint_pass(box inherent_impl::Pass::default());
reg.register_late_lint_pass(box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd);
reg.register_late_lint_pass(box unwrap::Pass);
reg.register_lint_group("clippy_restriction", vec![
arithmetic::FLOAT_ARITHMETIC,
@ -837,6 +840,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
types::UNIT_ARG,
types::UNNECESSARY_CAST,
unused_label::UNUSED_LABEL,
unwrap::UNNECESSARY_UNWRAP,
zero_div_zero::ZERO_DIVIDED_BY_ZERO,
]);

83
clippy_lints/src/loops.rs
View file

@ -18,6 +18,7 @@ use std::iter::{once, Iterator};
use syntax::ast;
use syntax::codemap::Span;
use crate::utils::{sugg, sext};
use crate::utils::usage::mutated_variables;
use crate::consts::{constant, Constant};
use crate::utils::{get_enclosing_block, get_parent_expr, higher, in_external_macro, is_integer_literal, is_refutable,
@ -504,8 +505,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
}
// check for while loops which conditions never change
if let ExprWhile(ref cond, ref block, _) = expr.node {
check_infinite_loop(cx, cond, block, expr);
if let ExprWhile(ref cond, _, _) = expr.node {
check_infinite_loop(cx, cond, expr);
}
}
@ -2145,35 +2146,30 @@ fn path_name(e: &Expr) -> Option<Name> {
None
}
fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, block: &'tcx Block, expr: &'tcx Expr) {
fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, expr: &'tcx Expr) {
if constant(cx, cx.tables, cond).is_some() {
// A pure constant condition (e.g. while false) is not linted.
return;
}
let mut mut_var_visitor = VarCollectorVisitor {
let mut var_visitor = VarCollectorVisitor {
cx,
ids: HashMap::new(),
ids: HashSet::new(),
def_ids: HashMap::new(),
skip: false,
};
mut_var_visitor.visit_expr(cond);
if mut_var_visitor.skip {
var_visitor.visit_expr(cond);
if var_visitor.skip {
return;
}
let mut delegate = MutVarsDelegate {
used_mutably: mut_var_visitor.ids,
skip: false,
let used_in_condition = &var_visitor.ids;
let no_cond_variable_mutated = if let Some(used_mutably) = mutated_variables(expr, cx) {
used_in_condition.is_disjoint(&used_mutably)
} else {
return
};
let def_id = def_id::DefId::local(block.hir_id.owner);
let region_scope_tree = &cx.tcx.region_scope_tree(def_id);
ExprUseVisitor::new(&mut delegate, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).walk_expr(expr);
if delegate.skip {
return;
}
if !(delegate.used_mutably.iter().any(|(_, v)| *v) || mut_var_visitor.def_ids.iter().any(|(_, v)| *v)) {
let mutable_static_in_cond = var_visitor.def_ids.iter().any(|(_, v)| *v);
if no_cond_variable_mutated && !mutable_static_in_cond {
span_lint(
cx,
WHILE_IMMUTABLE_CONDITION,
@ -2189,7 +2185,7 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, b
/// All variables definition IDs are collected
struct VarCollectorVisitor<'a, 'tcx: 'a> {
cx: &'a LateContext<'a, 'tcx>,
ids: HashMap<NodeId, bool>,
ids: HashSet<NodeId>,
def_ids: HashMap<def_id::DefId, bool>,
skip: bool,
}
@ -2203,7 +2199,7 @@ impl<'a, 'tcx> VarCollectorVisitor<'a, 'tcx> {
then {
match def {
Def::Local(node_id) | Def::Upvar(node_id, ..) => {
self.ids.insert(node_id, false);
self.ids.insert(node_id);
},
Def::Static(def_id, mutable) => {
self.def_ids.insert(def_id, mutable);
@ -2230,48 +2226,3 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> {
NestedVisitorMap::None
}
}
struct MutVarsDelegate {
used_mutably: HashMap<NodeId, bool>,
skip: bool,
}
impl<'tcx> MutVarsDelegate {
fn update(&mut self, cat: &'tcx Categorization) {
match *cat {
Categorization::Local(id) =>
if let Some(used) = self.used_mutably.get_mut(&id) {
*used = true;
},
Categorization::Upvar(_) => {
//FIXME: This causes false negatives. We can't get the `NodeId` from
//`Categorization::Upvar(_)`. So we search for any `Upvar`s in the
//`while`-body, not just the ones in the condition.
self.skip = true
},
Categorization::Deref(ref cmt, _) | Categorization::Interior(ref cmt, _) => self.update(&cmt.cat),
_ => {}
}
}
}
impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
fn consume(&mut self, _: NodeId, _: Span, _: &cmt_<'tcx>, _: ConsumeMode) {}
fn matched_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: MatchMode) {}
fn consume_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: ConsumeMode) {}
fn borrow(&mut self, _: NodeId, _: Span, cmt: &cmt_<'tcx>, _: ty::Region, bk: ty::BorrowKind, _: LoanCause) {
if let ty::BorrowKind::MutBorrow = bk {
self.update(&cmt.cat)
}
}
fn mutate(&mut self, _: NodeId, _: Span, cmt: &cmt_<'tcx>, _: MutateMode) {
self.update(&cmt.cat)
}
fn decl_without_init(&mut self, _: NodeId, _: Span) {}
}

176
clippy_lints/src/unwrap.rs Normal file
View file

@ -0,0 +1,176 @@
use rustc::lint::*;
use crate::utils::{in_macro, match_type, paths, span_lint_and_then, usage::is_potentially_mutated};
use rustc::hir::intravisit::*;
use rustc::hir::*;
use syntax::ast::NodeId;
use syntax::codemap::Span;
/// **What it does:** Checks for calls of unwrap[_err]() that cannot fail.
///
/// **Why is this bad?** Using `if let` or `match` is more idiomatic.
///
/// **Known problems:** Limitations of the borrow checker might make unwrap() necessary sometimes?
///
/// **Example:**
/// ```rust
/// if option.is_some() {
/// do_something_with(option.unwrap())
/// }
/// ```
///
/// Could be written:
///
/// ```rust
/// if let Some(value) = option {
/// do_something_with(value)
/// }
/// ```
declare_clippy_lint! {
pub UNNECESSARY_UNWRAP,
nursery,
"checks for calls of unwrap[_err]() that cannot fail"
}
pub struct Pass;
/// Visitor that keeps track of which variables are unwrappable.
struct UnwrappableVariablesVisitor<'a, 'tcx: 'a> {
unwrappables: Vec<UnwrapInfo<'tcx>>,
cx: &'a LateContext<'a, 'tcx>,
}
/// Contains information about whether a variable can be unwrapped.
#[derive(Copy, Clone, Debug)]
struct UnwrapInfo<'tcx> {
/// The variable that is checked
ident: &'tcx Path,
/// The check, like `x.is_ok()`
check: &'tcx Expr,
/// Whether `is_some()` or `is_ok()` was called (as opposed to `is_err()` or `is_none()`).
safe_to_unwrap: bool,
}
/// Collects the information about unwrappable variables from an if condition
/// The `invert` argument tells us whether the condition is negated.
fn collect_unwrap_info<'a, 'tcx: 'a>(
cx: &'a LateContext<'a, 'tcx>,
expr: &'tcx Expr,
invert: bool,
) -> Vec<UnwrapInfo<'tcx>> {
if let Expr_::ExprBinary(op, left, right) = &expr.node {
match (invert, op.node) {
(false, BinOp_::BiAnd) | (false, BinOp_::BiBitAnd) | (true, BinOp_::BiOr) | (true, BinOp_::BiBitOr) => {
let mut unwrap_info = collect_unwrap_info(cx, left, invert);
unwrap_info.append(&mut collect_unwrap_info(cx, right, invert));
return unwrap_info;
},
_ => (),
}
} else if let Expr_::ExprUnary(UnNot, expr) = &expr.node {
return collect_unwrap_info(cx, expr, !invert);
} else {
if_chain! {
if let Expr_::ExprMethodCall(method_name, _, args) = &expr.node;
if let Expr_::ExprPath(QPath::Resolved(None, path)) = &args[0].node;
let ty = cx.tables.expr_ty(&args[0]);
if match_type(cx, ty, &paths::OPTION) || match_type(cx, ty, &paths::RESULT);
let name = method_name.name.as_str();
if ["is_some", "is_none", "is_ok", "is_err"].contains(&&*name);
then {
assert!(args.len() == 1);
let unwrappable = match name.as_ref() {
"is_some" | "is_ok" => true,
"is_err" | "is_none" => false,
_ => unreachable!(),
};
let safe_to_unwrap = unwrappable != invert;
return vec![UnwrapInfo { ident: path, check: expr, safe_to_unwrap }];
}
}
}
Vec::new()
}
impl<'a, 'tcx: 'a> UnwrappableVariablesVisitor<'a, 'tcx> {
fn visit_branch(&mut self, cond: &'tcx Expr, branch: &'tcx Expr, else_branch: bool) {
let prev_len = self.unwrappables.len();
for unwrap_info in collect_unwrap_info(self.cx, cond, else_branch) {
if is_potentially_mutated(unwrap_info.ident, cond, self.cx)
|| is_potentially_mutated(unwrap_info.ident, branch, self.cx)
{
// if the variable is mutated, we don't know whether it can be unwrapped:
continue;
}
self.unwrappables.push(unwrap_info);
}
walk_expr(self, branch);
self.unwrappables.truncate(prev_len);
}
}
impl<'a, 'tcx: 'a> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> {
fn visit_expr(&mut self, expr: &'tcx Expr) {
if let Expr_::ExprIf(cond, then, els) = &expr.node {
walk_expr(self, cond);
self.visit_branch(cond, then, false);
if let Some(els) = els {
self.visit_branch(cond, els, true);
}
} else {
// find `unwrap[_err]()` calls:
if_chain! {
if let Expr_::ExprMethodCall(ref method_name, _, ref args) = expr.node;
if let Expr_::ExprPath(QPath::Resolved(None, ref path)) = args[0].node;
if ["unwrap", "unwrap_err"].contains(&&*method_name.name.as_str());
let call_to_unwrap = method_name.name == "unwrap";
if let Some(unwrappable) = self.unwrappables.iter()
.find(|u| u.ident.def == path.def && call_to_unwrap == u.safe_to_unwrap);
then {
span_lint_and_then(
self.cx,
UNNECESSARY_UNWRAP,
expr.span,
&format!("You checked before that `{}()` cannot fail. \
Instead of checking and unwrapping, it's better to use `if let` or `match`.",
method_name.name),
|db| { db.span_label(unwrappable.check.span, "the check is happening here"); },
);
}
}
walk_expr(self, expr);
}
}
fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
NestedVisitorMap::OnlyBodies(&self.cx.tcx.hir)
}
}
impl<'a> LintPass for Pass {
fn get_lints(&self) -> LintArray {
lint_array!(UNNECESSARY_UNWRAP)
}
}
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
fn check_fn(
&mut self,
cx: &LateContext<'a, 'tcx>,
kind: FnKind<'tcx>,
decl: &'tcx FnDecl,
body: &'tcx Body,
span: Span,
fn_id: NodeId,
) {
if in_macro(span) {
return;
}
let mut v = UnwrappableVariablesVisitor {
cx,
unwrappables: Vec::new(),
};
walk_fn(&mut v, kind, decl, body.id(), span, fn_id);
}
}

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

@ -32,6 +32,7 @@ pub mod inspector;
pub mod internal_lints;
pub mod author;
pub mod ptr;
pub mod usage;
pub use self::hir_utils::{SpanlessEq, SpanlessHash};
pub type MethodArgs = HirVec<P<Expr>>;

82
clippy_lints/src/utils/usage.rs Normal file
View file

@ -0,0 +1,82 @@
use rustc::lint::*;
use rustc::hir::def::Def;
use rustc::hir::*;
use rustc::middle::expr_use_visitor::*;
use rustc::middle::mem_categorization::cmt_;
use rustc::middle::mem_categorization::Categorization;
use rustc::ty;
use std::collections::HashSet;
use syntax::ast::NodeId;
use syntax::codemap::Span;
/// Returns a set of mutated local variable ids or None if mutations could not be determined.
pub fn mutated_variables<'a, 'tcx: 'a>(expr: &'tcx Expr, cx: &'a LateContext<'a, 'tcx>) -> Option<HashSet<NodeId>> {
let mut delegate = MutVarsDelegate {
used_mutably: HashSet::new(),
skip: false,
};
let def_id = def_id::DefId::local(expr.hir_id.owner);
let region_scope_tree = &cx.tcx.region_scope_tree(def_id);
ExprUseVisitor::new(&mut delegate, cx.tcx, cx.param_env, region_scope_tree, cx.tables, None).walk_expr(expr);
if delegate.skip {
return None;
}
Some(delegate.used_mutably)
}
pub fn is_potentially_mutated<'a, 'tcx: 'a>(
variable: &'tcx Path,
expr: &'tcx Expr,
cx: &'a LateContext<'a, 'tcx>,
) -> bool {
let id = match variable.def {
Def::Local(id) | Def::Upvar(id, ..) => id,
_ => return true,
};
mutated_variables(expr, cx).map_or(true, |mutated| mutated.contains(&id))
}
struct MutVarsDelegate {
used_mutably: HashSet<NodeId>,
skip: bool,
}
impl<'tcx> MutVarsDelegate {
fn update(&mut self, cat: &'tcx Categorization) {
match *cat {
Categorization::Local(id) => {
self.used_mutably.insert(id);
},
Categorization::Upvar(_) => {
//FIXME: This causes false negatives. We can't get the `NodeId` from
//`Categorization::Upvar(_)`. So we search for any `Upvar`s in the
//`while`-body, not just the ones in the condition.
self.skip = true
},
Categorization::Deref(ref cmt, _) | Categorization::Interior(ref cmt, _) => self.update(&cmt.cat),
_ => {},
}
}
}
impl<'tcx> Delegate<'tcx> for MutVarsDelegate {
fn consume(&mut self, _: NodeId, _: Span, _: &cmt_<'tcx>, _: ConsumeMode) {}
fn matched_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: MatchMode) {}
fn consume_pat(&mut self, _: &Pat, _: &cmt_<'tcx>, _: ConsumeMode) {}
fn borrow(&mut self, _: NodeId, _: Span, cmt: &cmt_<'tcx>, _: ty::Region, bk: ty::BorrowKind, _: LoanCause) {
if let ty::BorrowKind::MutBorrow = bk {
self.update(&cmt.cat)
}
}
fn mutate(&mut self, _: NodeId, _: Span, cmt: &cmt_<'tcx>, _: MutateMode) {
self.update(&cmt.cat)
}
fn decl_without_init(&mut self, _: NodeId, _: Span) {}
}

75
tests/ui/checked_unwrap.rs Normal file
View file

@ -0,0 +1,75 @@
#![deny(unnecessary_unwrap)]
fn main() {
let x = Some(());
if x.is_some() {
x.unwrap();
}
if x.is_none() {
// nothing to do here
} else {
x.unwrap();
}
let mut x: Result<(), ()> = Ok(());
if x.is_ok() {
x.unwrap();
} else {
x.unwrap_err();
}
if x.is_err() {
x.unwrap_err();
} else {
x.unwrap();
}
if x.is_ok() {
x = Err(());
x.unwrap();
} else {
x = Ok(());
x.unwrap_err();
}
}
fn test_complex_conditions() {
let x: Result<(), ()> = Ok(());
let y: Result<(), ()> = Ok(());
if x.is_ok() && y.is_err() {
x.unwrap();
y.unwrap_err();
} else {
// not clear whether unwrappable:
x.unwrap_err();
y.unwrap();
}
if x.is_ok() || y.is_ok() {
// not clear whether unwrappable:
x.unwrap();
y.unwrap();
} else {
x.unwrap_err();
y.unwrap_err();
}
let z: Result<(), ()> = Ok(());
if x.is_ok() && !(y.is_ok() || z.is_err()) {
x.unwrap();
y.unwrap_err();
z.unwrap();
}
if x.is_ok() || !(y.is_ok() && z.is_err()) {
// not clear what's unwrappable
} else {
x.unwrap_err();
y.unwrap();
z.unwrap_err();
}
}
fn test_nested() {
fn nested() {
let x = Some(());
if x.is_some() {
x.unwrap();
}
}
}

155
tests/ui/checked_unwrap.stderr Normal file
View file

@ -0,0 +1,155 @@
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:6:9
|
5 | if x.is_some() {
| ----------- the check is happening here
6 | x.unwrap();
| ^^^^^^^^^^
|
note: lint level defined here
--> $DIR/checked_unwrap.rs:1:9
|
1 | #![deny(unnecessary_unwrap)]
| ^^^^^^^^^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:11:9
|
8 | if x.is_none() {
| ----------- the check is happening here
...
11 | x.unwrap();
| ^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:15:9
|
14 | if x.is_ok() {
| --------- the check is happening here
15 | x.unwrap();
| ^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:17:9
|
14 | if x.is_ok() {
| --------- the check is happening here
...
17 | x.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:20:9
|
19 | if x.is_err() {
| ---------- the check is happening here
20 | x.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:22:9
|
19 | if x.is_err() {
| ---------- the check is happening here
...
22 | x.unwrap();
| ^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:37:9
|
36 | if x.is_ok() && y.is_err() {
| --------- the check is happening here
37 | x.unwrap();
| ^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:38:9
|
36 | if x.is_ok() && y.is_err() {
| ---------- the check is happening here
37 | x.unwrap();
38 | y.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:50:9
|
45 | if x.is_ok() || y.is_ok() {
| --------- the check is happening here
...
50 | x.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:51:9
|
45 | if x.is_ok() || y.is_ok() {
| --------- the check is happening here
...
51 | y.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:55:9
|
54 | if x.is_ok() && !(y.is_ok() || z.is_err()) {
| --------- the check is happening here
55 | x.unwrap();
| ^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:56:9
|
54 | if x.is_ok() && !(y.is_ok() || z.is_err()) {
| --------- the check is happening here
55 | x.unwrap();
56 | y.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:57:9
|
54 | if x.is_ok() && !(y.is_ok() || z.is_err()) {
| ---------- the check is happening here
...
57 | z.unwrap();
| ^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:62:9
|
59 | if x.is_ok() || !(y.is_ok() && z.is_err()) {
| --------- the check is happening here
...
62 | x.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:63:9
|
59 | if x.is_ok() || !(y.is_ok() && z.is_err()) {
| --------- the check is happening here
...
63 | y.unwrap();
| ^^^^^^^^^^
error: You checked before that `unwrap_err()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:64:9
|
59 | if x.is_ok() || !(y.is_ok() && z.is_err()) {
| ---------- the check is happening here
...
64 | z.unwrap_err();
| ^^^^^^^^^^^^^^
error: You checked before that `unwrap()` cannot fail. Instead of checking and unwrapping, it's better to use `if let` or `match`.
--> $DIR/checked_unwrap.rs:72:13
|
71 | if x.is_some() {
| ----------- the check is happening here
72 | x.unwrap();
| ^^^^^^^^^^
error: aborting due to 17 previous errors