forked from rust-lang/rust-clippy
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Find offending expressions from Issue rust-lang#6410
- Loading branch information
1 parent
02399f4
commit 2f4c16a
Showing
4 changed files
with
227 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| #![cfg_attr(debug_assertions, allow(dead_code, unused_imports, unused_variables))] | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir::def::*; | ||
| use rustc_hir::intravisit::FnKind; | ||
| use rustc_hir::*; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||
| use rustc_span::Span; | ||
|
|
||
| use rustc_span::sym; | ||
|
|
||
| use crate::utils; | ||
| use if_chain::if_chain; | ||
|
|
||
| use std::iter; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// **What it does:** | ||
| /// Suggest | ||
| /// ```rust,ignore | ||
| /// option.and_then(|value| value.inner_option) | ||
| /// ``` | ||
| /// instead of | ||
| /// ```rust,ignore | ||
| /// option.and_then(|value| Some(value.inner_option?)) | ||
| /// ``` | ||
| /// | ||
| /// **Why is this bad?** There's no reason to take an option, wrap it in another option, and use ? to short-circuit. and_then will work fine without it, and it's needlessly difficult to read. | ||
| /// | ||
| /// **Known problems:** None. | ||
| /// | ||
| /// **Example:** | ||
| /// | ||
| /// ```rust | ||
| /// // struct Outer { | ||
| /// // inner: Option<String> | ||
| /// // } | ||
| /// // | ||
| /// // let a = Some(Outer { inner: Some("hello".into()) }); | ||
| /// // a.and_then(|a| Some(a.inner?)) | ||
| /// ``` | ||
| /// Use instead: | ||
| /// ```rust | ||
| /// // struct Outer { | ||
| /// // inner: Option<String> | ||
| /// // } | ||
| /// // | ||
| /// // let a = Some(Outer { inner: Some("hello".into()) }); | ||
| /// // a.and_then(|a| a.inner) | ||
| /// ``` | ||
| pub NEEDLESS_QUESTION_MARK, | ||
| complexity, | ||
| "Suggest option.and_then(|value| value.inner_option) instead of option.and_then(|value| Some(value.inner_option?)). The same goes for Result<T, E>" | ||
| } | ||
|
|
||
| declare_lint_pass!(NeedlessQuestionMark => [NEEDLESS_QUESTION_MARK]); | ||
|
|
||
| impl LateLintPass<'_> for NeedlessQuestionMark { | ||
| fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) { | ||
| check_body_free(cx, body); | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| enum SomeOkCall<'a> { | ||
| SomeCall(&'a Expr<'a>, &'a Expr<'a>), | ||
| OkCall(&'a Expr<'a>, &'a Expr<'a>), | ||
| } | ||
|
|
||
| fn check_body_free<'a>(cx: &'a LateContext<'_>, body: &'a Body<'_>) /* -> Vec<SomeOkCall<'a>> */ | ||
| { | ||
| /* | ||
| * The question mark operator is compatible with both Result<T, E> and Option<T>, | ||
| * from Rust 1.13 and 1.22 respectively. | ||
| * | ||
| * This can be constrained like this: | ||
| * https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md | ||
| */ | ||
|
|
||
| /* | ||
| * What do we match: | ||
| * Expressions that look like this: | ||
| * Some(option?), Ok(result?) | ||
| * | ||
| * Where do we match: | ||
| * Last expression of a body or a return statement | ||
| */ | ||
|
|
||
| // Get last expression and expression in return statements in bodies | ||
| // TODO: what about one liners, like in closures? | ||
| let candidate_exprs: Vec<&Expr<'_>> = match body.value.kind { | ||
| ExprKind::Block(Block { stmts, expr, .. }, _) => { | ||
| let exprs_from_stmts = stmts.into_iter().filter_map(|stmt| stmt.extract_expr()); | ||
| let mut return_expr = expr.and_then(|expr| expr.extract_expr()).into_iter(); | ||
|
|
||
| exprs_from_stmts.chain(&mut return_expr).collect() | ||
| }, | ||
|
|
||
| _ => iter::empty().collect(), | ||
| }; | ||
|
|
||
| // We need to match Some(some_instance?) or Ok(result_instance?) | ||
| let candidate_exprs: Vec<SomeOkCall<'_>> = candidate_exprs | ||
| .into_iter() | ||
| .filter_map(|expr| is_some_or_ok_call(cx, expr)) | ||
| .collect(); | ||
|
|
||
| // candidate_exprs.iter().map(|expr|) | ||
|
|
||
| for expr in candidate_exprs.iter() { | ||
| let (entire_expr, inner_expr) = match expr { | ||
| SomeOkCall::OkCall(outer, inner) | SomeOkCall::SomeCall(outer, inner) => (outer, inner), | ||
| }; | ||
|
|
||
| let help_msg = match expr { | ||
| SomeOkCall::OkCall(_, _) => "Remove the Ok call and the question mark", | ||
| SomeOkCall::SomeCall(_, _) => "Remove the Some call and the question mark", | ||
| }; | ||
|
|
||
| // utils::span_lint_and_sugg( | ||
| // cx, NEEDLESS_QUESTION_MARK, entire_expr.span, "Question mark operator is useless | ||
| // here", help_msg, format!("{}", inner_expr.), Applicability::MachineApplicable | ||
| // ); | ||
| } | ||
| } | ||
|
|
||
| fn is_some_or_ok_call<'a>(cx: &'a LateContext<'_>, expr: &'a Expr<'_>) -> Option<SomeOkCall<'a>> { | ||
| if_chain! { | ||
| if let ExprKind::Call(path, args) = &expr.kind; | ||
| if let ExprKind::Path(qpath) = &path.kind; | ||
| if let QPath::Resolved(_, Path { span, segments, .. }) = qpath; | ||
| if segments.len() == 1 && args.len() == 1; | ||
|
|
||
| if utils::is_try(&args[0]).is_some(); | ||
| if let ExprKind::Match(inner_expr, _, _) = &args[0].kind; | ||
| then { | ||
| // Extract inner expr type from match argument generated by | ||
| // question mark operator | ||
| let inner_ty = cx.typeck_results().expr_ty(inner_expr); | ||
| let outer_ty = cx.typeck_results().expr_ty(expr); | ||
|
|
||
| // Check if outer and inner type are Option | ||
| let outer_is_some = utils::is_type_diagnostic_item(cx, outer_ty, sym::option_type); | ||
| let inner_is_some = utils::is_type_diagnostic_item(cx, inner_ty, sym::option_type); | ||
|
|
||
| if outer_is_some && inner_is_some { | ||
| return Some(SomeOkCall::SomeCall(expr, inner_expr)); | ||
| } | ||
|
|
||
| // Check if outer and inner type are Result | ||
| let outer_is_result = utils::is_type_diagnostic_item(cx, outer_ty, sym::result_type); | ||
| let inner_is_result = utils::is_type_diagnostic_item(cx, inner_ty, sym::result_type); | ||
|
|
||
| // TODO: Is there a better way of matching the contructor's name? | ||
| // sym! delivers "Ok", not Ok | ||
| if outer_is_result && inner_is_result && segments[0].ident.name.to_ident_string() == "Ok" { | ||
| return Some(SomeOkCall::OkCall(expr, inner_expr)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return Option::None; | ||
| } | ||
|
|
||
| trait ExtractExpr { | ||
| fn extract_expr(self: &Self) -> Option<&Expr<'_>>; | ||
| } | ||
|
|
||
| impl ExtractExpr for Expr<'_> { | ||
| fn extract_expr(self: &Self) -> Option<&Expr<'_>> { | ||
| // NOTE: | ||
| return match self.kind { | ||
| // We only concern ourself with return statements | ||
| ExprKind::Ret(e) if e.map(|e| utils::contains_return(e)).unwrap_or(false) => e, | ||
|
|
||
| // Lookup body from BodyId.hir_id | ||
| // ExprKind::Closure(_, _, BodyId { hir_id }, ..) => { | ||
| // let {} utils::get_enclosing_block(cx, hir_id).and_then(|block| { | ||
| // block. | ||
| // }) | ||
| // }, | ||
| _ => None, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| impl ExtractExpr for Stmt<'_> { | ||
| fn extract_expr(self: &Self) -> Option<&Expr<'_>> { | ||
| return match self.kind { | ||
| StmtKind::Semi(e) => e.extract_expr(), | ||
| _ => None, | ||
| }; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| #![warn(clippy::needless_question_mark)] | ||
| #![allow(clippy::needless_return)] | ||
| struct TO { | ||
| magic: Option<usize>, | ||
| } | ||
|
|
||
| struct TR { | ||
| magic: Result<usize, bool> | ||
| } | ||
|
|
||
| // fn bad1(to: TO) -> Option<usize> { | ||
| // Some(to.magic?) | ||
| // } | ||
|
|
||
| // fn bad2(tr: Result<TR, bool>) -> Result<usize, bool> { | ||
| // return tr.and_then(|value| { | ||
| // Ok(value.magic?) | ||
| // }); | ||
| // } | ||
|
|
||
| fn bad3(tr: TR) -> Result<usize, bool> { | ||
| return Ok(tr.magic?); | ||
| } | ||
|
|
||
| fn main() { | ||
|
|
||
| } |