|
| 1 | +use rustc_errors::Applicability; |
| 2 | +use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; |
| 3 | +use rustc_hir::{Body, Expr, ExprKind, LangItem, MatchSource, QPath}; |
| 4 | +use rustc_lint::{LateContext, LateLintPass, LintContext}; |
| 5 | +use rustc_middle::ty::DefIdTree; |
| 6 | +use rustc_semver::RustcVersion; |
| 7 | +use rustc_session::{declare_tool_lint, impl_lint_pass}; |
| 8 | +use rustc_span::sym; |
| 9 | + |
| 10 | +use crate::utils; |
| 11 | +use if_chain::if_chain; |
| 12 | + |
| 13 | +declare_clippy_lint! { |
| 14 | + /// **What it does:** |
| 15 | + /// Suggests alternatives for useless applications of `?` in terminating expressions |
| 16 | + /// |
| 17 | + /// **Why is this bad?** There's no reason to use ? to short-circuit when execution of the body will end there anyway. |
| 18 | + /// |
| 19 | + /// **Known problems:** None. |
| 20 | + /// |
| 21 | + /// **Example:** |
| 22 | + /// |
| 23 | + /// ```rust |
| 24 | + /// struct TO { |
| 25 | + /// magic: Option<usize>, |
| 26 | + /// } |
| 27 | + /// |
| 28 | + /// fn f(to: TO) -> Option<usize> { |
| 29 | + /// Some(to.magic?) |
| 30 | + /// } |
| 31 | + /// |
| 32 | + /// struct TR { |
| 33 | + /// magic: Result<usize, bool>, |
| 34 | + /// } |
| 35 | + /// |
| 36 | + /// fn g(tr: Result<TR, bool>) -> Result<usize, bool> { |
| 37 | + /// tr.and_then(|t| Ok(t.magic?)) |
| 38 | + /// } |
| 39 | + /// |
| 40 | + /// ``` |
| 41 | + /// Use instead: |
| 42 | + /// ```rust |
| 43 | + /// struct TO { |
| 44 | + /// magic: Option<usize>, |
| 45 | + /// } |
| 46 | + /// |
| 47 | + /// fn f(to: TO) -> Option<usize> { |
| 48 | + /// to.magic |
| 49 | + /// } |
| 50 | + /// |
| 51 | + /// struct TR { |
| 52 | + /// magic: Result<usize, bool>, |
| 53 | + /// } |
| 54 | + /// |
| 55 | + /// fn g(tr: Result<TR, bool>) -> Result<usize, bool> { |
| 56 | + /// tr.and_then(|t| t.magic) |
| 57 | + /// } |
| 58 | + /// ``` |
| 59 | + pub NEEDLESS_QUESTION_MARK, |
| 60 | + complexity, |
| 61 | + "Suggest value.inner_option instead of Some(value.inner_option?). The same goes for Result<T, E>." |
| 62 | +} |
| 63 | + |
| 64 | +const NEEDLESS_QUESTION_MARK_RESULT_MSRV: RustcVersion = RustcVersion::new(1, 13, 0); |
| 65 | +const NEEDLESS_QUESTION_MARK_OPTION_MSRV: RustcVersion = RustcVersion::new(1, 22, 0); |
| 66 | + |
| 67 | +pub struct NeedlessQuestionMark { |
| 68 | + msrv: Option<RustcVersion>, |
| 69 | +} |
| 70 | + |
| 71 | +impl NeedlessQuestionMark { |
| 72 | + #[must_use] |
| 73 | + pub fn new(msrv: Option<RustcVersion>) -> Self { |
| 74 | + Self { msrv } |
| 75 | + } |
| 76 | +} |
| 77 | + |
| 78 | +impl_lint_pass!(NeedlessQuestionMark => [NEEDLESS_QUESTION_MARK]); |
| 79 | + |
| 80 | +#[derive(Debug)] |
| 81 | +enum SomeOkCall<'a> { |
| 82 | + SomeCall(&'a Expr<'a>, &'a Expr<'a>), |
| 83 | + OkCall(&'a Expr<'a>, &'a Expr<'a>), |
| 84 | +} |
| 85 | + |
| 86 | +impl LateLintPass<'_> for NeedlessQuestionMark { |
| 87 | + /* |
| 88 | + * The question mark operator is compatible with both Result<T, E> and Option<T>, |
| 89 | + * from Rust 1.13 and 1.22 respectively. |
| 90 | + */ |
| 91 | + |
| 92 | + /* |
| 93 | + * What do we match: |
| 94 | + * Expressions that look like this: |
| 95 | + * Some(option?), Ok(result?) |
| 96 | + * |
| 97 | + * Where do we match: |
| 98 | + * Last expression of a body |
| 99 | + * Return statement |
| 100 | + * A body's value (single line closure) |
| 101 | + * |
| 102 | + * What do we not match: |
| 103 | + * Implicit calls to `from(..)` on the error value |
| 104 | + */ |
| 105 | + |
| 106 | + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { |
| 107 | + let e = match &expr.kind { |
| 108 | + ExprKind::Ret(Some(e)) => e, |
| 109 | + _ => return, |
| 110 | + }; |
| 111 | + |
| 112 | + if let Some(ok_some_call) = is_some_or_ok_call(self, cx, e) { |
| 113 | + emit_lint(cx, &ok_some_call); |
| 114 | + } |
| 115 | + } |
| 116 | + |
| 117 | + fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) { |
| 118 | + // Function / Closure block |
| 119 | + let expr_opt = if let ExprKind::Block(block, _) = &body.value.kind { |
| 120 | + block.expr |
| 121 | + } else { |
| 122 | + // Single line closure |
| 123 | + Some(&body.value) |
| 124 | + }; |
| 125 | + |
| 126 | + if_chain! { |
| 127 | + if let Some(expr) = expr_opt; |
| 128 | + if let Some(ok_some_call) = is_some_or_ok_call(self, cx, expr); |
| 129 | + then { |
| 130 | + emit_lint(cx, &ok_some_call); |
| 131 | + } |
| 132 | + }; |
| 133 | + } |
| 134 | + |
| 135 | + extract_msrv_attr!(LateContext); |
| 136 | +} |
| 137 | + |
| 138 | +fn emit_lint(cx: &LateContext<'_>, expr: &SomeOkCall<'_>) { |
| 139 | + let (entire_expr, inner_expr) = match expr { |
| 140 | + SomeOkCall::OkCall(outer, inner) | SomeOkCall::SomeCall(outer, inner) => (outer, inner), |
| 141 | + }; |
| 142 | + |
| 143 | + utils::span_lint_and_sugg( |
| 144 | + cx, |
| 145 | + NEEDLESS_QUESTION_MARK, |
| 146 | + entire_expr.span, |
| 147 | + "Question mark operator is useless here", |
| 148 | + "try", |
| 149 | + format!("{}", utils::snippet(cx, inner_expr.span, r#""...""#)), |
| 150 | + Applicability::MachineApplicable, |
| 151 | + ); |
| 152 | +} |
| 153 | + |
| 154 | +fn is_some_or_ok_call<'a>( |
| 155 | + nqml: &NeedlessQuestionMark, |
| 156 | + cx: &'a LateContext<'_>, |
| 157 | + expr: &'a Expr<'_>, |
| 158 | +) -> Option<SomeOkCall<'a>> { |
| 159 | + if_chain! { |
| 160 | + // Check outer expression matches CALL_IDENT(ARGUMENT) format |
| 161 | + if let ExprKind::Call(path, args) = &expr.kind; |
| 162 | + if let ExprKind::Path(QPath::Resolved(None, path)) = &path.kind; |
| 163 | + if is_some_ctor(cx, path.res) || is_ok_ctor(cx, path.res); |
| 164 | + |
| 165 | + // Extract inner expression from ARGUMENT |
| 166 | + if let ExprKind::Match(inner_expr_with_q, _, MatchSource::TryDesugar) = &args[0].kind; |
| 167 | + if let ExprKind::Call(called, args) = &inner_expr_with_q.kind; |
| 168 | + if args.len() == 1; |
| 169 | + |
| 170 | + if let ExprKind::Path(QPath::LangItem(LangItem::TryIntoResult, _)) = &called.kind; |
| 171 | + then { |
| 172 | + // Extract inner expr type from match argument generated by |
| 173 | + // question mark operator |
| 174 | + let inner_expr = &args[0]; |
| 175 | + |
| 176 | + let inner_ty = cx.typeck_results().expr_ty(inner_expr); |
| 177 | + let outer_ty = cx.typeck_results().expr_ty(expr); |
| 178 | + |
| 179 | + // Check if outer and inner type are Option |
| 180 | + let outer_is_some = utils::is_type_diagnostic_item(cx, outer_ty, sym::option_type); |
| 181 | + let inner_is_some = utils::is_type_diagnostic_item(cx, inner_ty, sym::option_type); |
| 182 | + |
| 183 | + // Check for Option MSRV |
| 184 | + let meets_option_msrv = utils::meets_msrv(nqml.msrv.as_ref(), &NEEDLESS_QUESTION_MARK_OPTION_MSRV); |
| 185 | + if outer_is_some && inner_is_some && meets_option_msrv { |
| 186 | + return Some(SomeOkCall::SomeCall(expr, inner_expr)); |
| 187 | + } |
| 188 | + |
| 189 | + // Check if outer and inner type are Result |
| 190 | + let outer_is_result = utils::is_type_diagnostic_item(cx, outer_ty, sym::result_type); |
| 191 | + let inner_is_result = utils::is_type_diagnostic_item(cx, inner_ty, sym::result_type); |
| 192 | + |
| 193 | + // Additional check: if the error type of the Result can be converted |
| 194 | + // via the From trait, then don't match |
| 195 | + let does_not_call_from = !has_implicit_error_from(cx, expr, inner_expr); |
| 196 | + |
| 197 | + // Must meet Result MSRV |
| 198 | + let meets_result_msrv = utils::meets_msrv(nqml.msrv.as_ref(), &NEEDLESS_QUESTION_MARK_RESULT_MSRV); |
| 199 | + if outer_is_result && inner_is_result && does_not_call_from && meets_result_msrv { |
| 200 | + return Some(SomeOkCall::OkCall(expr, inner_expr)); |
| 201 | + } |
| 202 | + } |
| 203 | + } |
| 204 | + |
| 205 | + None |
| 206 | +} |
| 207 | + |
| 208 | +fn has_implicit_error_from(cx: &LateContext<'_>, entire_expr: &Expr<'_>, inner_result_expr: &Expr<'_>) -> bool { |
| 209 | + return cx.typeck_results().expr_ty(entire_expr) != cx.typeck_results().expr_ty(inner_result_expr); |
| 210 | +} |
| 211 | + |
| 212 | +fn is_ok_ctor(cx: &LateContext<'_>, res: Res) -> bool { |
| 213 | + if let Some(ok_id) = cx.tcx.lang_items().result_ok_variant() { |
| 214 | + if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res { |
| 215 | + if let Some(variant_id) = cx.tcx.parent(id) { |
| 216 | + return variant_id == ok_id; |
| 217 | + } |
| 218 | + } |
| 219 | + } |
| 220 | + false |
| 221 | +} |
| 222 | + |
| 223 | +fn is_some_ctor(cx: &LateContext<'_>, res: Res) -> bool { |
| 224 | + if let Some(some_id) = cx.tcx.lang_items().option_some_variant() { |
| 225 | + if let Res::Def(DefKind::Ctor(CtorOf::Variant, CtorKind::Fn), id) = res { |
| 226 | + if let Some(variant_id) = cx.tcx.parent(id) { |
| 227 | + return variant_id == some_id; |
| 228 | + } |
| 229 | + } |
| 230 | + } |
| 231 | + false |
| 232 | +} |
0 commit comments