|
| 1 | +use crate::utils::{ |
| 2 | + is_ctor_or_promotable_const_function, match_def_path, match_type, paths, snippet_with_applicability, |
| 3 | + span_lint_and_then, |
| 4 | +}; |
| 5 | +use if_chain::if_chain; |
| 6 | +use rustc::hir::intravisit::Visitor; |
| 7 | +use rustc::hir::{self, *}; |
| 8 | +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; |
| 9 | +use rustc::ty::Ty; |
| 10 | +use rustc::{declare_lint_pass, declare_tool_lint}; |
| 11 | +use rustc_errors::Applicability; |
| 12 | +use syntax_pos::Span; |
| 13 | + |
| 14 | +declare_clippy_lint! { |
| 15 | + /// **What it does:** Checks for usages of `option.ok_or_else(|| <..>::from(..))?` or |
| 16 | + /// `result.map_err(|x| <..>::from(..))`. |
| 17 | + /// |
| 18 | + /// **Why is this bad?** The `?` operator will call `from` in the `Err` case, |
| 19 | + /// so calling it manually is redundant. |
| 20 | + /// |
| 21 | + /// **Known problems:** The suggested fix does not correct any explicitly provided |
| 22 | + /// type arguments in `ok_or_else` or `map_err`. |
| 23 | + /// |
| 24 | + /// **Example:** |
| 25 | + /// ```rust |
| 26 | + /// fn bar() -> Result<i32, String> { |
| 27 | + /// let x = Some(52.3).ok_or_else(|| String::from("foo"))?; |
| 28 | + /// Ok(42) |
| 29 | + /// } |
| 30 | + /// ``` |
| 31 | + /// Could be written: |
| 32 | + /// |
| 33 | + /// ```rust |
| 34 | + /// fn bar() -> Result<i32, String> { |
| 35 | + /// let x = Some(52.3).ok_or("foo")?; |
| 36 | + /// Ok(42) |
| 37 | + /// } |
| 38 | + /// ``` |
| 39 | + pub UNNEEDED_TRY_CONVERT, |
| 40 | + complexity, |
| 41 | + "unneeded conversion inside `?`" |
| 42 | +} |
| 43 | + |
| 44 | +declare_lint_pass!(UnneededTryConvert => [UNNEEDED_TRY_CONVERT]); |
| 45 | + |
| 46 | +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnneededTryConvert { |
| 47 | + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { |
| 48 | + if_chain! { |
| 49 | + if let ExprKind::Match(match_arg, arms, MatchSource::TryDesugar) = &expr.kind; |
| 50 | + if let ExprKind::Call(_, args) = &match_arg.kind; |
| 51 | + if let [try_arg] = &**args; |
| 52 | + if let Some(fn_error_ty) = get_try_err_ty(cx, arms); |
| 53 | + then { |
| 54 | + check_option_ok_or_else(cx, fn_error_ty, try_arg); |
| 55 | + check_result_map_err(cx, fn_error_ty, try_arg); |
| 56 | + } |
| 57 | + } |
| 58 | + } |
| 59 | +} |
| 60 | + |
| 61 | +/// Given the arms of a `match` expr from desugaring a `?`, return the error type of the `Try` type |
| 62 | +fn get_try_err_ty<'tcx>(cx: &LateContext<'_, 'tcx>, match_arms: &[Arm]) -> Option<Ty<'tcx>> { |
| 63 | + if_chain! { |
| 64 | + if let [err_arm, _] = match_arms; |
| 65 | + if let ExprKind::Ret(Some(ret_expr)) = &err_arm.body.kind; |
| 66 | + if let ExprKind::Call(_, try_from_error_args) = &ret_expr.kind; |
| 67 | + if let [try_from_error_arg] = &**try_from_error_args; |
| 68 | + then { |
| 69 | + return Some(cx.tables.expr_ty(try_from_error_arg)); |
| 70 | + } |
| 71 | + } |
| 72 | + None |
| 73 | +} |
| 74 | + |
| 75 | +fn check_option_ok_or_else<'tcx>(cx: &LateContext<'_, 'tcx>, fn_error_ty: Ty<'tcx>, expr: &Expr) { |
| 76 | + if_chain! { |
| 77 | + if let ExprKind::MethodCall(call_path, _, call_args) = &expr.kind; |
| 78 | + if call_path.ident.as_str() == "ok_or_else"; |
| 79 | + if let [receiver, closure_expr] = &**call_args; |
| 80 | + if match_type(cx, cx.tables.expr_ty(receiver), &paths::OPTION); |
| 81 | + if let Some((closure_body_span, conv_arg)) = check_closure_expr(cx, fn_error_ty, closure_expr); |
| 82 | + then { |
| 83 | + let mut applicability = Applicability::MachineApplicable; |
| 84 | + let conv_arg_snip = snippet_with_applicability(cx, conv_arg.span, "..", &mut applicability); |
| 85 | + let (sugg_span, sugg_snip) = if is_trivial_expr(cx, conv_arg) { |
| 86 | + // suggest inlining the closure and using `ok_or` |
| 87 | + let receiver_snip = snippet_with_applicability(cx, receiver.span, "..", &mut applicability); |
| 88 | + (expr.span, format!("{}.ok_or({})", receiver_snip, conv_arg_snip)) |
| 89 | + } else { |
| 90 | + // suggest removing the conversion in the closure |
| 91 | + (closure_body_span, conv_arg_snip.into_owned()) |
| 92 | + }; |
| 93 | + emit_lint(cx, closure_body_span, sugg_span, sugg_snip, applicability); |
| 94 | + } |
| 95 | + } |
| 96 | +} |
| 97 | + |
| 98 | +fn check_result_map_err<'tcx>(cx: &LateContext<'_, 'tcx>, fn_error_ty: Ty<'tcx>, expr: &Expr) { |
| 99 | + if_chain! { |
| 100 | + if let ExprKind::MethodCall(call_path, _, call_args) = &expr.kind; |
| 101 | + if call_path.ident.as_str() == "map_err"; |
| 102 | + if let [receiver, mapper_expr] = &**call_args; |
| 103 | + let receiver_ty = cx.tables.expr_ty(receiver); |
| 104 | + if match_type(cx, receiver_ty, &paths::RESULT); |
| 105 | + then { |
| 106 | + if let Some((closure_body_span, conv_arg)) = check_closure_expr(cx, fn_error_ty, mapper_expr) { |
| 107 | + // suggest removing just the conversion in the closure |
| 108 | + let mut applicability = Applicability::MachineApplicable; |
| 109 | + let conv_arg_snip = snippet_with_applicability(cx, conv_arg.span, "..", &mut applicability); |
| 110 | + emit_lint( |
| 111 | + cx, |
| 112 | + closure_body_span, |
| 113 | + closure_body_span, |
| 114 | + conv_arg_snip.into_owned(), |
| 115 | + applicability, |
| 116 | + ); |
| 117 | + return; |
| 118 | + } |
| 119 | + if_chain! { |
| 120 | + if let ExprKind::Path(qpath) = &mapper_expr.kind; |
| 121 | + if let def::Res::Def(_, def_id) = cx.tables.qpath_res(qpath, mapper_expr.hir_id); |
| 122 | + if match_def_path(cx, def_id, &paths::FROM_FROM) |
| 123 | + || match_def_path(cx, def_id, &paths::INTO_INTO); |
| 124 | + if *cx.tables.expr_ty(mapper_expr).fn_sig(cx.tcx).output().skip_binder() == fn_error_ty; |
| 125 | + then { |
| 126 | + // suggest removing the entire `map_err(..)` call |
| 127 | + let mut applicability = Applicability::MachineApplicable; |
| 128 | + let receiver_snip = snippet_with_applicability(cx, receiver.span, "..", &mut applicability); |
| 129 | + emit_lint( |
| 130 | + cx, |
| 131 | + mapper_expr.span, |
| 132 | + expr.span, |
| 133 | + receiver_snip.into_owned(), |
| 134 | + applicability, |
| 135 | + ); |
| 136 | + } |
| 137 | + } |
| 138 | + } |
| 139 | + } |
| 140 | +} |
| 141 | + |
| 142 | +fn emit_lint(cx: &LateContext<'_, '_>, lint_span: Span, sugg_span: Span, sugg: String, applicability: Applicability) { |
| 143 | + span_lint_and_then( |
| 144 | + cx, |
| 145 | + UNNEEDED_TRY_CONVERT, |
| 146 | + lint_span, |
| 147 | + "unneeded conversion inside `?`", |
| 148 | + move |db| { |
| 149 | + db.note("the `?` operator will automatically call `from` in the `Err` case"); |
| 150 | + db.span_suggestion(sugg_span, "remove the conversion", sugg, applicability); |
| 151 | + }, |
| 152 | + ); |
| 153 | +} |
| 154 | + |
| 155 | +/// If `closure_expr` is a closure whose body is a conversion to `fn_error_ty`, |
| 156 | +/// return (the span of the conversion call, the argument of the conversion call) |
| 157 | +fn check_closure_expr<'tcx>( |
| 158 | + cx: &LateContext<'_, 'tcx>, |
| 159 | + fn_error_ty: Ty<'tcx>, |
| 160 | + closure_expr: &Expr, |
| 161 | +) -> Option<(Span, &'tcx Expr)> { |
| 162 | + if_chain! { |
| 163 | + if let ExprKind::Closure(_, _, body_id, _, _) = closure_expr.kind; |
| 164 | + let closure_body = &cx.tcx.hir().body(body_id).value; |
| 165 | + if let Some(conv_arg) = conversion_subject(cx, closure_body); |
| 166 | + if cx.tables.expr_ty(closure_body) == fn_error_ty; |
| 167 | + then { |
| 168 | + return Some((closure_body.span, conv_arg)); |
| 169 | + } |
| 170 | + } |
| 171 | + None |
| 172 | +} |
| 173 | + |
| 174 | +/// If `expr` is `From::from(<inner>)` or `(<inner>).into()`, returns `<inner>`. |
| 175 | +fn conversion_subject<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &'tcx Expr) -> Option<&'tcx Expr> { |
| 176 | + if_chain! { |
| 177 | + if let ExprKind::Call(fn_expr, from_args) = &expr.kind; |
| 178 | + if let ExprKind::Path(fn_qpath) = &fn_expr.kind; |
| 179 | + if let def::Res::Def(def::DefKind::Method, fn_did) = cx.tables.qpath_res(fn_qpath, fn_expr.hir_id); |
| 180 | + if match_def_path(cx, fn_did, &paths::FROM_FROM); |
| 181 | + if let [from_arg] = &**from_args; |
| 182 | + then { |
| 183 | + return Some(from_arg); |
| 184 | + } |
| 185 | + } |
| 186 | + if_chain! { |
| 187 | + if let ExprKind::MethodCall(_, _, args) = &expr.kind; |
| 188 | + if let Some(call_did) = cx.tables.type_dependent_def_id(expr.hir_id); |
| 189 | + if match_def_path(cx, call_did, &paths::INTO_INTO); |
| 190 | + if let [receiver] = &**args; |
| 191 | + then { |
| 192 | + return Some(receiver); |
| 193 | + } |
| 194 | + } |
| 195 | + None |
| 196 | +} |
| 197 | + |
| 198 | +/// Is this expression "trivial" such that a closure containing it could be inlined? |
| 199 | +/// (currently very conservative) |
| 200 | +fn is_trivial_expr<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &'tcx Expr) -> bool { |
| 201 | + struct TrivialVisitor<'a, 'tcx> { |
| 202 | + cx: &'a LateContext<'a, 'tcx>, |
| 203 | + trivial: bool, |
| 204 | + } |
| 205 | + |
| 206 | + impl<'a, 'tcx> intravisit::Visitor<'tcx> for TrivialVisitor<'a, 'tcx> { |
| 207 | + fn visit_expr(&mut self, expr: &'tcx hir::Expr) { |
| 208 | + // whitelist of definitely trivial expressions |
| 209 | + self.trivial &= match &expr.kind { |
| 210 | + hir::ExprKind::Call(..) => is_ctor_or_promotable_const_function(self.cx, expr), |
| 211 | + hir::ExprKind::Tup(..) |
| 212 | + | hir::ExprKind::Lit(..) |
| 213 | + | hir::ExprKind::Cast(..) |
| 214 | + | hir::ExprKind::Field(..) |
| 215 | + | hir::ExprKind::Index(..) |
| 216 | + | hir::ExprKind::Path(..) |
| 217 | + | hir::ExprKind::AddrOf(..) |
| 218 | + | hir::ExprKind::Struct(..) => true, |
| 219 | + _ => false, |
| 220 | + }; |
| 221 | + |
| 222 | + if self.trivial { |
| 223 | + intravisit::walk_expr(self, expr); |
| 224 | + } |
| 225 | + } |
| 226 | + |
| 227 | + fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> { |
| 228 | + intravisit::NestedVisitorMap::None |
| 229 | + } |
| 230 | + } |
| 231 | + |
| 232 | + let mut visitor = TrivialVisitor { cx, trivial: true }; |
| 233 | + visitor.visit_expr(expr); |
| 234 | + visitor.trivial |
| 235 | +} |
0 commit comments