-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #5301 - JarredAllen:option_if_let_else, r=flip1995
Suggest `Option::map_or`(_else) for `if let Some { y } else { x }`
Fixes #5203
There are two issues with this code that I have noticed:
- Use of `Option::map_or` causes it to always evaluate the code in the else block. If that block is computationally expensive or if it updates some state (such as getting the next value from an iterator), then this change would cause the code to behave differently. In either of those circumstances, it should suggest Option::map_or_else, which takes both cases as a closure and runs one. However, I don't know how to check if the expression would change some state, so I left the lint's applicability as MaybeIncorrect.
- There are lints which can trigger on specific sub-cases of this lint (`if_let_some_result`, `question_mark`, and `while_let_loop`) and suggest different changes (usually better ones because they're more specific). Is this acceptable for clippy to give multiple suggestions, or should I have the code check if those other lints trigger and then not trigger this lint if they do?- Loading branch information
Showing
7 changed files
with
381 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,202 @@ | ||
| use crate::utils::sugg::Sugg; | ||
| use crate::utils::{match_type, paths, span_lint_and_sugg}; | ||
| use if_chain::if_chain; | ||
|
|
||
| use rustc_errors::Applicability; | ||
| use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; | ||
| use rustc_hir::*; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_middle::hir::map::Map; | ||
| use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||
|
|
||
| use std::marker::PhantomData; | ||
|
|
||
| declare_clippy_lint! { | ||
| /// **What it does:** | ||
| /// Lints usage of `if let Some(v) = ... { y } else { x }` which is more | ||
| /// idiomatically done with `Option::map_or` (if the else bit is a simple | ||
| /// expression) or `Option::map_or_else` (if the else bit is a longer | ||
| /// block). | ||
| /// | ||
| /// **Why is this bad?** | ||
| /// Using the dedicated functions of the Option type is clearer and | ||
| /// more concise than an if let expression. | ||
| /// | ||
| /// **Known problems:** | ||
| /// This lint uses whether the block is just an expression or if it has | ||
| /// more statements to decide whether to use `Option::map_or` or | ||
| /// `Option::map_or_else`. If you have a single expression which calls | ||
| /// an expensive function, then it would be more efficient to use | ||
| /// `Option::map_or_else`, but this lint would suggest `Option::map_or`. | ||
| /// | ||
| /// Also, this lint uses a deliberately conservative metric for checking | ||
| /// if the inside of either body contains breaks or continues which will | ||
| /// cause it to not suggest a fix if either block contains a loop with | ||
| /// continues or breaks contained within the loop. | ||
| /// | ||
| /// **Example:** | ||
| /// | ||
| /// ```rust | ||
| /// # let optional: Option<u32> = Some(0); | ||
| /// let _ = if let Some(foo) = optional { | ||
| /// foo | ||
| /// } else { | ||
| /// 5 | ||
| /// }; | ||
| /// let _ = if let Some(foo) = optional { | ||
| /// foo | ||
| /// } else { | ||
| /// let y = do_complicated_function(); | ||
| /// y*y | ||
| /// }; | ||
| /// ``` | ||
| /// | ||
| /// should be | ||
| /// | ||
| /// ```rust | ||
| /// # let optional: Option<u32> = Some(0); | ||
| /// let _ = optional.map_or(5, |foo| foo); | ||
| /// let _ = optional.map_or_else(||{ | ||
| /// let y = do_complicated_function; | ||
| /// y*y | ||
| /// }, |foo| foo); | ||
| /// ``` | ||
| pub OPTION_IF_LET_ELSE, | ||
| style, | ||
| "reimplementation of Option::map_or" | ||
| } | ||
|
|
||
| declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]); | ||
|
|
||
| /// Returns true iff the given expression is the result of calling Result::ok | ||
| fn is_result_ok(cx: &LateContext<'_, '_>, expr: &'_ Expr<'_>) -> bool { | ||
| if_chain! { | ||
| if let ExprKind::MethodCall(ref path, _, &[ref receiver]) = &expr.kind; | ||
| if path.ident.name.to_ident_string() == "ok"; | ||
| if match_type(cx, &cx.tables.expr_ty(&receiver), &paths::RESULT); | ||
| then { | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A struct containing information about occurences of the | ||
| /// `if let Some(..) = .. else` construct that this lint detects. | ||
| struct OptionIfLetElseOccurence { | ||
| option: String, | ||
| method_sugg: String, | ||
| some_expr: String, | ||
| none_expr: String, | ||
| } | ||
|
|
||
| struct ReturnBreakContinueVisitor<'tcx> { | ||
| seen_return_break_continue: bool, | ||
| phantom_data: PhantomData<&'tcx bool>, | ||
| } | ||
| impl<'tcx> ReturnBreakContinueVisitor<'tcx> { | ||
| fn new() -> ReturnBreakContinueVisitor<'tcx> { | ||
| ReturnBreakContinueVisitor { | ||
| seen_return_break_continue: false, | ||
| phantom_data: PhantomData, | ||
| } | ||
| } | ||
| } | ||
| impl<'tcx> Visitor<'tcx> for ReturnBreakContinueVisitor<'tcx> { | ||
| type Map = Map<'tcx>; | ||
| fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> { | ||
| NestedVisitorMap::None | ||
| } | ||
|
|
||
| fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { | ||
| if self.seen_return_break_continue { | ||
| // No need to look farther if we've already seen one of them | ||
| return; | ||
| } | ||
| match &ex.kind { | ||
| ExprKind::Ret(..) | ExprKind::Break(..) | ExprKind::Continue(..) => { | ||
| self.seen_return_break_continue = true; | ||
| }, | ||
| // Something special could be done here to handle while or for loop | ||
| // desugaring, as this will detect a break if there's a while loop | ||
| // or a for loop inside the expression. | ||
| _ => { | ||
| rustc_hir::intravisit::walk_expr(self, ex); | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool { | ||
| let mut recursive_visitor: ReturnBreakContinueVisitor<'tcx> = ReturnBreakContinueVisitor::new(); | ||
| recursive_visitor.visit_expr(expression); | ||
| recursive_visitor.seen_return_break_continue | ||
| } | ||
|
|
||
| /// If this expression is the option if let/else construct we're detecting, then | ||
| /// this function returns an OptionIfLetElseOccurence struct with details if | ||
| /// this construct is found, or None if this construct is not found. | ||
| fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -> Option<OptionIfLetElseOccurence> { | ||
| //(String, String, String, String)> { | ||
| if_chain! { | ||
| if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind; | ||
| if arms.len() == 2; | ||
| if match_type(cx, &cx.tables.expr_ty(let_body), &paths::OPTION); | ||
| if !is_result_ok(cx, let_body); // Don't lint on Result::ok because a different lint does it already | ||
| if let PatKind::TupleStruct(_, &[inner_pat], _) = &arms[0].pat.kind; | ||
| if let PatKind::Binding(_, _, id, _) = &inner_pat.kind; | ||
| if !contains_return_break_continue(arms[0].body); | ||
| if !contains_return_break_continue(arms[1].body); | ||
| then { | ||
| let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _) | ||
| = &arms[0].body.kind { | ||
| if let &[] = &statements { | ||
| expr | ||
| } else { | ||
| &arms[0].body | ||
| } | ||
| } else { | ||
| return None; | ||
| }; | ||
| let (none_body, method_sugg) = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _) | ||
| = &arms[1].body.kind { | ||
| if let &[] = &statements { | ||
| (expr, "map_or") | ||
| } else { | ||
| (&arms[1].body, "map_or_else") | ||
| } | ||
| } else { | ||
| return None; | ||
| }; | ||
| let capture_name = id.name.to_ident_string(); | ||
| Some(OptionIfLetElseOccurence { | ||
| option: format!("{}", Sugg::hir(cx, let_body, "..")), | ||
| method_sugg: format!("{}", method_sugg), | ||
| some_expr: format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")), | ||
| none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")) | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse { | ||
| fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { | ||
| if let Some(detection) = detect_option_if_let_else(cx, expr) { | ||
| span_lint_and_sugg( | ||
| cx, | ||
| OPTION_IF_LET_ELSE, | ||
| expr.span, | ||
| format!("use Option::{} instead of an if let/else", detection.method_sugg).as_str(), | ||
| "try", | ||
| format!( | ||
| "{}.{}({}, {})", | ||
| detection.option, detection.method_sugg, detection.none_expr, detection.some_expr | ||
| ), | ||
| Applicability::MachineApplicable, | ||
| ); | ||
| } | ||
| } | ||
| } |
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,48 @@ | ||
| // run-rustfix | ||
| #![warn(clippy::option_if_let_else)] | ||
|
|
||
| fn bad1(string: Option<&str>) -> (bool, &str) { | ||
| string.map_or((false, "hello"), |x| (true, x)) | ||
| } | ||
|
|
||
| fn longer_body(arg: Option<u32>) -> u32 { | ||
| arg.map_or(13, |x| { | ||
| let y = x * x; | ||
| y * y | ||
| }) | ||
| } | ||
|
|
||
| fn test_map_or_else(arg: Option<u32>) { | ||
| let _ = arg.map_or_else(|| { | ||
| let mut y = 1; | ||
| y = (y + 2 / y) / 2; | ||
| y = (y + 2 / y) / 2; | ||
| y | ||
| }, |x| x * x * x * x); | ||
| } | ||
|
|
||
| fn negative_tests(arg: Option<u32>) -> u32 { | ||
| let _ = if let Some(13) = arg { "unlucky" } else { "lucky" }; | ||
| for _ in 0..10 { | ||
| let _ = if let Some(x) = arg { | ||
| x | ||
| } else { | ||
| continue; | ||
| }; | ||
| } | ||
| let _ = if let Some(x) = arg { | ||
| return x; | ||
| } else { | ||
| 5 | ||
| }; | ||
| 7 | ||
| } | ||
|
|
||
| fn main() { | ||
| let optional = Some(5); | ||
| let _ = optional.map_or(5, |x| x + 2); | ||
| let _ = bad1(None); | ||
| let _ = longer_body(None); | ||
| test_map_or_else(None); | ||
| let _ = negative_tests(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,56 @@ | ||
| // run-rustfix | ||
| #![warn(clippy::option_if_let_else)] | ||
|
|
||
| fn bad1(string: Option<&str>) -> (bool, &str) { | ||
| if let Some(x) = string { | ||
| (true, x) | ||
| } else { | ||
| (false, "hello") | ||
| } | ||
| } | ||
|
|
||
| fn longer_body(arg: Option<u32>) -> u32 { | ||
| if let Some(x) = arg { | ||
| let y = x * x; | ||
| y * y | ||
| } else { | ||
| 13 | ||
| } | ||
| } | ||
|
|
||
| fn test_map_or_else(arg: Option<u32>) { | ||
| let _ = if let Some(x) = arg { | ||
| x * x * x * x | ||
| } else { | ||
| let mut y = 1; | ||
| y = (y + 2 / y) / 2; | ||
| y = (y + 2 / y) / 2; | ||
| y | ||
| }; | ||
| } | ||
|
|
||
| fn negative_tests(arg: Option<u32>) -> u32 { | ||
| let _ = if let Some(13) = arg { "unlucky" } else { "lucky" }; | ||
| for _ in 0..10 { | ||
| let _ = if let Some(x) = arg { | ||
| x | ||
| } else { | ||
| continue; | ||
| }; | ||
| } | ||
| let _ = if let Some(x) = arg { | ||
| return x; | ||
| } else { | ||
| 5 | ||
| }; | ||
| 7 | ||
| } | ||
|
|
||
| fn main() { | ||
| let optional = Some(5); | ||
| let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ||
| let _ = bad1(None); | ||
| let _ = longer_body(None); | ||
| test_map_or_else(None); | ||
| let _ = negative_tests(None); | ||
| } |
Oops, something went wrong.