|
| 1 | +use rustc::lint::*; |
| 2 | + |
| 3 | +use crate::utils::{in_macro, match_type, paths, span_lint_and_then, usage::is_potentially_mutated}; |
| 4 | +use rustc::hir::intravisit::*; |
| 5 | +use rustc::hir::*; |
| 6 | +use syntax::ast::NodeId; |
| 7 | +use syntax::codemap::Span; |
| 8 | + |
| 9 | +/// **What it does:** Checks for calls of unwrap[_err]() that cannot fail. |
| 10 | +/// |
| 11 | +/// **Why is this bad?** Using `if let` or `match` is more idiomatic. |
| 12 | +/// |
| 13 | +/// **Known problems:** Limitations of the borrow checker might make unwrap() necessary sometimes? |
| 14 | +/// |
| 15 | +/// **Example:** |
| 16 | +/// ```rust |
| 17 | +/// if option.is_some() { |
| 18 | +/// do_something_with(option.unwrap()) |
| 19 | +/// } |
| 20 | +/// ``` |
| 21 | +/// |
| 22 | +/// Could be written: |
| 23 | +/// |
| 24 | +/// ```rust |
| 25 | +/// if let Some(value) = option { |
| 26 | +/// do_something_with(value) |
| 27 | +/// } |
| 28 | +/// ``` |
| 29 | +declare_clippy_lint! { |
| 30 | + pub UNNECESSARY_UNWRAP, |
| 31 | + nursery, |
| 32 | + "checks for calls of unwrap[_err]() that cannot fail" |
| 33 | +} |
| 34 | + |
| 35 | +pub struct Pass; |
| 36 | + |
| 37 | +/// Visitor that keeps track of which variables are unwrappable. |
| 38 | +struct UnwrappableVariablesVisitor<'a, 'tcx: 'a> { |
| 39 | + unwrappables: Vec<UnwrapInfo<'tcx>>, |
| 40 | + cx: &'a LateContext<'a, 'tcx>, |
| 41 | +} |
| 42 | +/// Contains information about whether a variable can be unwrapped. |
| 43 | +#[derive(Copy, Clone, Debug)] |
| 44 | +struct UnwrapInfo<'tcx> { |
| 45 | + /// The variable that is checked |
| 46 | + ident: &'tcx Path, |
| 47 | + /// The check, like `x.is_ok()` |
| 48 | + check: &'tcx Expr, |
| 49 | + /// Whether `is_some()` or `is_ok()` was called (as opposed to `is_err()` or `is_none()`). |
| 50 | + safe_to_unwrap: bool, |
| 51 | +} |
| 52 | + |
| 53 | +/// Collects the information about unwrappable variables from an if condition |
| 54 | +/// The `invert` argument tells us whether the condition is negated. |
| 55 | +fn collect_unwrap_info<'a, 'tcx: 'a>( |
| 56 | + cx: &'a LateContext<'a, 'tcx>, |
| 57 | + expr: &'tcx Expr, |
| 58 | + invert: bool, |
| 59 | +) -> Vec<UnwrapInfo<'tcx>> { |
| 60 | + if let Expr_::ExprBinary(op, left, right) = &expr.node { |
| 61 | + match (invert, op.node) { |
| 62 | + (false, BinOp_::BiAnd) | (false, BinOp_::BiBitAnd) | (true, BinOp_::BiOr) | (true, BinOp_::BiBitOr) => { |
| 63 | + let mut unwrap_info = collect_unwrap_info(cx, left, invert); |
| 64 | + unwrap_info.append(&mut collect_unwrap_info(cx, right, invert)); |
| 65 | + return unwrap_info; |
| 66 | + }, |
| 67 | + _ => (), |
| 68 | + } |
| 69 | + } else if let Expr_::ExprUnary(UnNot, expr) = &expr.node { |
| 70 | + return collect_unwrap_info(cx, expr, !invert); |
| 71 | + } else { |
| 72 | + if_chain! { |
| 73 | + if let Expr_::ExprMethodCall(method_name, _, args) = &expr.node; |
| 74 | + if let Expr_::ExprPath(QPath::Resolved(None, path)) = &args[0].node; |
| 75 | + let ty = cx.tables.expr_ty(&args[0]); |
| 76 | + if match_type(cx, ty, &paths::OPTION) || match_type(cx, ty, &paths::RESULT); |
| 77 | + let name = method_name.name.as_str(); |
| 78 | + if ["is_some", "is_none", "is_ok", "is_err"].contains(&&*name); |
| 79 | + then { |
| 80 | + assert!(args.len() == 1); |
| 81 | + let unwrappable = match name.as_ref() { |
| 82 | + "is_some" | "is_ok" => true, |
| 83 | + "is_err" | "is_none" => false, |
| 84 | + _ => unreachable!(), |
| 85 | + }; |
| 86 | + let safe_to_unwrap = unwrappable != invert; |
| 87 | + return vec![UnwrapInfo { ident: path, check: expr, safe_to_unwrap }]; |
| 88 | + } |
| 89 | + } |
| 90 | + } |
| 91 | + Vec::new() |
| 92 | +} |
| 93 | + |
| 94 | +impl<'a, 'tcx: 'a> UnwrappableVariablesVisitor<'a, 'tcx> { |
| 95 | + fn visit_branch(&mut self, cond: &'tcx Expr, branch: &'tcx Expr, else_branch: bool) { |
| 96 | + let prev_len = self.unwrappables.len(); |
| 97 | + for unwrap_info in collect_unwrap_info(self.cx, cond, else_branch) { |
| 98 | + if is_potentially_mutated(unwrap_info.ident, cond, self.cx) |
| 99 | + || is_potentially_mutated(unwrap_info.ident, branch, self.cx) |
| 100 | + { |
| 101 | + // if the variable is mutated, we don't know whether it can be unwrapped: |
| 102 | + continue; |
| 103 | + } |
| 104 | + self.unwrappables.push(unwrap_info); |
| 105 | + } |
| 106 | + walk_expr(self, branch); |
| 107 | + self.unwrappables.truncate(prev_len); |
| 108 | + } |
| 109 | +} |
| 110 | + |
| 111 | +impl<'a, 'tcx: 'a> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> { |
| 112 | + fn visit_expr(&mut self, expr: &'tcx Expr) { |
| 113 | + if let Expr_::ExprIf(cond, then, els) = &expr.node { |
| 114 | + walk_expr(self, cond); |
| 115 | + self.visit_branch(cond, then, false); |
| 116 | + if let Some(els) = els { |
| 117 | + self.visit_branch(cond, els, true); |
| 118 | + } |
| 119 | + } else { |
| 120 | + // find `unwrap[_err]()` calls: |
| 121 | + if_chain! { |
| 122 | + if let Expr_::ExprMethodCall(ref method_name, _, ref args) = expr.node; |
| 123 | + if let Expr_::ExprPath(QPath::Resolved(None, ref path)) = args[0].node; |
| 124 | + if ["unwrap", "unwrap_err"].contains(&&*method_name.name.as_str()); |
| 125 | + let call_to_unwrap = method_name.name == "unwrap"; |
| 126 | + if let Some(unwrappable) = self.unwrappables.iter() |
| 127 | + .find(|u| u.ident.def == path.def && call_to_unwrap == u.safe_to_unwrap); |
| 128 | + then { |
| 129 | + span_lint_and_then( |
| 130 | + self.cx, |
| 131 | + UNNECESSARY_UNWRAP, |
| 132 | + expr.span, |
| 133 | + &format!("You checked before that `{}()` cannot fail. \ |
| 134 | + Instead of checking and unwrapping, it's better to use `if let` or `match`.", |
| 135 | + method_name.name), |
| 136 | + |db| { db.span_label(unwrappable.check.span, "the check is happening here"); }, |
| 137 | + ); |
| 138 | + } |
| 139 | + } |
| 140 | + walk_expr(self, expr); |
| 141 | + } |
| 142 | + } |
| 143 | + |
| 144 | + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { |
| 145 | + NestedVisitorMap::OnlyBodies(&self.cx.tcx.hir) |
| 146 | + } |
| 147 | +} |
| 148 | + |
| 149 | +impl<'a> LintPass for Pass { |
| 150 | + fn get_lints(&self) -> LintArray { |
| 151 | + lint_array!(UNNECESSARY_UNWRAP) |
| 152 | + } |
| 153 | +} |
| 154 | + |
| 155 | +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass { |
| 156 | + fn check_fn( |
| 157 | + &mut self, |
| 158 | + cx: &LateContext<'a, 'tcx>, |
| 159 | + kind: FnKind<'tcx>, |
| 160 | + decl: &'tcx FnDecl, |
| 161 | + body: &'tcx Body, |
| 162 | + span: Span, |
| 163 | + fn_id: NodeId, |
| 164 | + ) { |
| 165 | + if in_macro(span) { |
| 166 | + return; |
| 167 | + } |
| 168 | + |
| 169 | + let mut v = UnwrappableVariablesVisitor { |
| 170 | + cx, |
| 171 | + unwrappables: Vec::new(), |
| 172 | + }; |
| 173 | + |
| 174 | + walk_fn(&mut v, kind, decl, body.id(), span, fn_id); |
| 175 | + } |
| 176 | +} |
0 commit comments