From bf48a2d50d82cccac58d7c4c73700eaf66926aee Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Thu, 5 Mar 2020 10:35:05 -0800 Subject: [PATCH 01/14] Lint for if let Some(x) = ... instead of Option::map_or --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 5 + clippy_lints/src/option_if_let_else.rs | 202 +++++++++++++++++++++++++ src/lintlist/mod.rs | 7 + tests/ui/option_if_let_else.fixed | 48 ++++++ tests/ui/option_if_let_else.rs | 56 +++++++ tests/ui/option_if_let_else.stderr | 62 ++++++++ 7 files changed, 381 insertions(+) create mode 100644 clippy_lints/src/option_if_let_else.rs create mode 100644 tests/ui/option_if_let_else.fixed create mode 100644 tests/ui/option_if_let_else.rs create mode 100644 tests/ui/option_if_let_else.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index ed8f16e65bb9a..1a081bb85feab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1577,6 +1577,7 @@ Released 2018-09-13 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref [`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref [`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap +[`option_if_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else [`option_map_or_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_or_none [`option_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unit_fn [`option_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1d5be893ffba2..cd91e7ceb32a8 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -264,6 +264,7 @@ mod non_copy_const; mod non_expressive_names; mod open_options; mod option_env_unwrap; +mod option_if_let_else; mod overflow_check_conditional; mod panic_unimplemented; mod partialeq_ne_impl; @@ -734,6 +735,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &non_expressive_names::SIMILAR_NAMES, &open_options::NONSENSICAL_OPEN_OPTIONS, &option_env_unwrap::OPTION_ENV_UNWRAP, + &option_if_let_else::OPTION_IF_LET_ELSE, &overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL, &panic_unimplemented::PANIC, &panic_unimplemented::PANIC_PARAMS, @@ -1052,6 +1054,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box redundant_pub_crate::RedundantPubCrate::default()); store.register_late_pass(|| box unnamed_address::UnnamedAddress); store.register_late_pass(|| box dereference::Dereferencing); + store.register_late_pass(|| box option_if_let_else::OptionIfLetElse); store.register_late_pass(|| box future_not_send::FutureNotSend); store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls); store.register_late_pass(|| box if_let_mutex::IfLetMutex); @@ -1369,6 +1372,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP), + LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(&panic_unimplemented::PANIC_PARAMS), LintId::of(&partialeq_ne_impl::PARTIALEQ_NE_IMPL), @@ -1517,6 +1521,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&new_without_default::NEW_WITHOUT_DEFAULT), LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), + LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(&panic_unimplemented::PANIC_PARAMS), LintId::of(&ptr::CMP_NULL), LintId::of(&ptr::PTR_ARG), diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs new file mode 100644 index 0000000000000..f092f1297c1bb --- /dev/null +++ b/clippy_lints/src/option_if_let_else.rs @@ -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 = 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 = 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 { + 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 { + //(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, + ); + } + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 6402efc252121..b499d565fa7f9 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1620,6 +1620,13 @@ pub static ref ALL_LINTS: Vec = vec![ deprecation: None, module: "option_env_unwrap", }, + Lint { + name: "option_if_let_else", + group: "style", + desc: "reimplementation of Option::map_or", + deprecation: None, + module: "option_if_let_else", + }, Lint { name: "option_map_or_none", group: "style", diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed new file mode 100644 index 0000000000000..3aa895120d196 --- /dev/null +++ b/tests/ui/option_if_let_else.fixed @@ -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 { + arg.map_or(13, |x| { + let y = x * x; + y * y + }) +} + +fn test_map_or_else(arg: Option) { + 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 { + 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); +} diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs new file mode 100644 index 0000000000000..7d029b0bcf48c --- /dev/null +++ b/tests/ui/option_if_let_else.rs @@ -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 { + if let Some(x) = arg { + let y = x * x; + y * y + } else { + 13 + } +} + +fn test_map_or_else(arg: Option) { + 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 { + 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); +} diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr new file mode 100644 index 0000000000000..d6cf083673341 --- /dev/null +++ b/tests/ui/option_if_let_else.stderr @@ -0,0 +1,62 @@ +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:5:5 + | +LL | / if let Some(x) = string { +LL | | (true, x) +LL | | } else { +LL | | (false, "hello") +LL | | } + | |_____^ help: try: `string.map_or((false, "hello"), |x| (true, x))` + | + = note: `-D clippy::option-if-let-else` implied by `-D warnings` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:13:5 + | +LL | / if let Some(x) = arg { +LL | | let y = x * x; +LL | | y * y +LL | | } else { +LL | | 13 +LL | | } + | |_____^ + | +help: try + | +LL | arg.map_or(13, |x| { +LL | let y = x * x; +LL | y * y +LL | }) + | + +error: use Option::map_or_else instead of an if let/else + --> $DIR/option_if_let_else.rs:22:13 + | +LL | let _ = if let Some(x) = arg { + | _____________^ +LL | | x * x * x * x +LL | | } else { +LL | | let mut y = 1; +... | +LL | | y +LL | | }; + | |_____^ + | +help: try + | +LL | let _ = arg.map_or_else(|| { +LL | let mut y = 1; +LL | y = (y + 2 / y) / 2; +LL | y = (y + 2 / y) / 2; +LL | y +LL | }, |x| x * x * x * x); + | + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:51:13 + | +LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` + +error: aborting due to 4 previous errors + From 82f8d4d6f1645dd08b107c3ead9155412637739b Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sat, 25 Apr 2020 08:32:33 -0700 Subject: [PATCH 02/14] Stop linting on macros and correctly use braces for constructs --- clippy_lints/src/option_if_let_else.rs | 23 ++++++++++++++++++++--- tests/ui/option_if_let_else.fixed | 7 +++++++ tests/ui/option_if_let_else.rs | 11 +++++++++++ tests/ui/option_if_let_else.stderr | 19 +++++++++++++++---- 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index f092f1297c1bb..1edec1cad6ec6 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -1,3 +1,4 @@ +use crate::utils; use crate::utils::sugg::Sugg; use crate::utils::{match_type, paths, span_lint_and_sugg}; use if_chain::if_chain; @@ -89,6 +90,7 @@ struct OptionIfLetElseOccurence { method_sugg: String, some_expr: String, none_expr: String, + wrap_braces: bool, } struct ReturnBreakContinueVisitor<'tcx> { @@ -140,6 +142,7 @@ fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool { fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) -> Option { //(String, String, String, String)> { if_chain! { + // if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly 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); @@ -170,11 +173,23 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - return None; }; let capture_name = id.name.to_ident_string(); + let wrap_braces = utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| { + if_chain! { + if let Some(Expr { kind: ExprKind::Match(condition, arms, MatchSource::IfDesugar{contains_else_clause: true}|MatchSource::IfLetDesugar{contains_else_clause: true}), .. } ) = parent.expr; + if expr.hir_id == arms[1].body.hir_id; + then { + true + } else { + false + } + } + }); 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, "..")) + none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")), + wrap_braces, }) } else { None @@ -192,8 +207,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse { 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 + "{}{}.{}({}, {}){}", + if detection.wrap_braces { "{ " } else { "" }, + detection.option, detection.method_sugg, detection.none_expr, detection.some_expr, + if detection.wrap_braces { " }" } else { "" }, ), Applicability::MachineApplicable, ); diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 3aa895120d196..343e099b2b77d 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -5,6 +5,12 @@ fn bad1(string: Option<&str>) -> (bool, &str) { string.map_or((false, "hello"), |x| (true, x)) } +fn bad2(string: Option<&str>) -> Option<(bool, &str)> { + if string.is_none() { + None + } else { string.map_or(Some((false, "")), |x| Some((true, x))) } +} + fn longer_body(arg: Option) -> u32 { arg.map_or(13, |x| { let y = x * x; @@ -42,6 +48,7 @@ fn main() { let optional = Some(5); let _ = optional.map_or(5, |x| x + 2); let _ = bad1(None); + let _ = bad2(None); let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index 7d029b0bcf48c..b0c203f06375c 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -9,6 +9,16 @@ fn bad1(string: Option<&str>) -> (bool, &str) { } } +fn bad2(string: Option<&str>) -> Option<(bool, &str)> { + if string.is_none() { + None + } else if let Some(x) = string { + Some((true, x)) + } else { + Some((false, "")) + } +} + fn longer_body(arg: Option) -> u32 { if let Some(x) = arg { let y = x * x; @@ -50,6 +60,7 @@ fn main() { let optional = Some(5); let _ = if let Some(x) = optional { x + 2 } else { 5 }; let _ = bad1(None); + let _ = bad2(None); let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index d6cf083673341..656cfb2f62ace 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -11,7 +11,18 @@ LL | | } = note: `-D clippy::option-if-let-else` implied by `-D warnings` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:13:5 + --> $DIR/option_if_let_else.rs:15:12 + | +LL | } else if let Some(x) = string { + | ____________^ +LL | | Some((true, x)) +LL | | } else { +LL | | Some((false, "")) +LL | | } + | |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:23:5 | LL | / if let Some(x) = arg { LL | | let y = x * x; @@ -30,7 +41,7 @@ LL | }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:22:13 + --> $DIR/option_if_let_else.rs:32:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -53,10 +64,10 @@ LL | }, |x| x * x * x * x); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:51:13 + --> $DIR/option_if_let_else.rs:61:13 | LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors From b85796fe3613e20a4af21933783a3d993bb8d7ad Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sat, 25 Apr 2020 09:08:23 -0700 Subject: [PATCH 03/14] Properly parenthesize to avoid operator precedence errors --- clippy_lints/src/option_if_let_else.rs | 25 ++++++++++++++++++++++--- tests/ui/option_if_let_else.fixed | 9 +++++++-- tests/ui/option_if_let_else.rs | 13 +++++++++++-- tests/ui/option_if_let_else.stderr | 16 +++++++++++++--- 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 1edec1cad6ec6..66971ee02620d 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -175,7 +175,12 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - let capture_name = id.name.to_ident_string(); let wrap_braces = utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| { if_chain! { - if let Some(Expr { kind: ExprKind::Match(condition, arms, MatchSource::IfDesugar{contains_else_clause: true}|MatchSource::IfLetDesugar{contains_else_clause: true}), .. } ) = parent.expr; + if let Some(Expr { kind: ExprKind::Match( + _, + arms, + MatchSource::IfDesugar{contains_else_clause: true} + | MatchSource::IfLetDesugar{contains_else_clause: true}), + .. } ) = parent.expr; if expr.hir_id == arms[1].body.hir_id; then { true @@ -184,8 +189,19 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - } } }); + let parens_around_option = match &let_body.kind { + ExprKind::Call(..) + | ExprKind::MethodCall(..) + | ExprKind::Loop(..) + | ExprKind::Match(..) + | ExprKind::Block(..) + | ExprKind::Field(..) + | ExprKind::Path(_) + => false, + _ => true, + }; Some(OptionIfLetElseOccurence { - option: format!("{}", Sugg::hir(cx, let_body, "..")), + option: format!("{}{}{}", if parens_around_option { "(" } else { "" }, Sugg::hir(cx, let_body, ".."), if parens_around_option { ")" } else { "" }), 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, "..")), @@ -209,7 +225,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse { format!( "{}{}.{}({}, {}){}", if detection.wrap_braces { "{ " } else { "" }, - detection.option, detection.method_sugg, detection.none_expr, detection.some_expr, + detection.option, + detection.method_sugg, + detection.none_expr, + detection.some_expr, if detection.wrap_braces { " }" } else { "" }, ), Applicability::MachineApplicable, diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 343e099b2b77d..80b162714acaa 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -5,12 +5,16 @@ fn bad1(string: Option<&str>) -> (bool, &str) { string.map_or((false, "hello"), |x| (true, x)) } -fn bad2(string: Option<&str>) -> Option<(bool, &str)> { +fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> { if string.is_none() { None } else { string.map_or(Some((false, "")), |x| Some((true, x))) } } +fn unop_bad(string: &Option<&str>) -> usize { + (*string).map_or(0, |s| s.len()) +} + fn longer_body(arg: Option) -> u32 { arg.map_or(13, |x| { let y = x * x; @@ -48,7 +52,8 @@ fn main() { let optional = Some(5); let _ = optional.map_or(5, |x| x + 2); let _ = bad1(None); - let _ = bad2(None); + let _ = else_if_option(None); + let _ = unop_bad(&None); let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index b0c203f06375c..7c43fbea373f3 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -9,7 +9,7 @@ fn bad1(string: Option<&str>) -> (bool, &str) { } } -fn bad2(string: Option<&str>) -> Option<(bool, &str)> { +fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> { if string.is_none() { None } else if let Some(x) = string { @@ -19,6 +19,14 @@ fn bad2(string: Option<&str>) -> Option<(bool, &str)> { } } +fn unop_bad(string: &Option<&str>) -> usize { + if let Some(s) = *string { + s.len() + } else { + 0 + } +} + fn longer_body(arg: Option) -> u32 { if let Some(x) = arg { let y = x * x; @@ -60,7 +68,8 @@ fn main() { let optional = Some(5); let _ = if let Some(x) = optional { x + 2 } else { 5 }; let _ = bad1(None); - let _ = bad2(None); + let _ = else_if_option(None); + let _ = unop_bad(&None); let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index 656cfb2f62ace..b932fe5975900 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -24,6 +24,16 @@ LL | | } error: use Option::map_or instead of an if let/else --> $DIR/option_if_let_else.rs:23:5 | +LL | / if let Some(s) = *string { +LL | | s.len() +LL | | } else { +LL | | 0 +LL | | } + | |_____^ help: try: `(*string).map_or(0, |s| s.len())` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:31:5 + | LL | / if let Some(x) = arg { LL | | let y = x * x; LL | | y * y @@ -41,7 +51,7 @@ LL | }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:32:13 + --> $DIR/option_if_let_else.rs:40:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -64,10 +74,10 @@ LL | }, |x| x * x * x * x); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:61:13 + --> $DIR/option_if_let_else.rs:69:13 | LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` -error: aborting due to 5 previous errors +error: aborting due to 6 previous errors From 88c8afdddff07adeff4c87431cbe8bc630a36d68 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sat, 9 May 2020 20:20:57 -0700 Subject: [PATCH 04/14] Handle ref, mut, &, and &mut on the option --- clippy_lints/src/option_if_let_else.rs | 28 +++++--- tests/ui/option_if_let_else.fixed | 20 +++++- tests/ui/option_if_let_else.rs | 36 ++++++++-- tests/ui/option_if_let_else.stderr | 99 +++++++++++++++++++++++--- 4 files changed, 158 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 66971ee02620d..4e501f4ca02ac 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -140,18 +140,24 @@ fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool { /// 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 { - //(String, String, String, String)> { if_chain! { - // if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly + if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly 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 type_is_option(cx, &cx.tables.expr_ty(let_body).kind); 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 let PatKind::TupleStruct(struct_qpath, &[inner_pat], _) = &arms[0].pat.kind; + if utils::match_qpath(struct_qpath, &paths::OPTION_SOME); + if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind; if !contains_return_break_continue(arms[0].body); if !contains_return_break_continue(arms[1].body); then { + let (capture_mut, capture_ref, capture_ref_mut) = match bind_annotation { + BindingAnnotation::Unannotated => (false, false, false), + BindingAnnotation::Mutable => (true, false, false), + BindingAnnotation::Ref => (false, true, false), + BindingAnnotation::RefMut => (false, false, true), + }; let some_body = if let ExprKind::Block(Block { stmts: statements, expr: Some(expr), .. }, _) = &arms[0].body.kind { if let &[] = &statements { @@ -189,7 +195,7 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - } } }); - let parens_around_option = match &let_body.kind { + let (parens_around_option, as_ref, as_mut, let_body) = match &let_body.kind { ExprKind::Call(..) | ExprKind::MethodCall(..) | ExprKind::Loop(..) @@ -197,13 +203,15 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - | ExprKind::Block(..) | ExprKind::Field(..) | ExprKind::Path(_) - => false, - _ => true, + => (false, capture_ref, capture_ref_mut, let_body), + ExprKind::Unary(UnOp::UnDeref, expr) => (false, capture_ref, capture_ref_mut, expr), + ExprKind::AddrOf(_, mutability, expr) => (false, mutability == &Mutability::Not, mutability == &Mutability::Mut, expr), + _ => (true, capture_ref, capture_ref_mut, let_body), }; Some(OptionIfLetElseOccurence { - option: format!("{}{}{}", if parens_around_option { "(" } else { "" }, Sugg::hir(cx, let_body, ".."), if parens_around_option { ")" } else { "" }), + option: format!("{}{}{}{}", if parens_around_option { "(" } else { "" }, Sugg::hir(cx, let_body, ".."), if parens_around_option { ")" } else { "" }, if as_mut { ".as_mut()" } else if as_ref { ".as_ref()" } else { "" }), method_sugg: format!("{}", method_sugg), - some_expr: format!("|{}| {}", capture_name, Sugg::hir(cx, some_body, "..")), + some_expr: format!("|{}{}{}| {}", if false { "ref " } else { "" }, if capture_mut { "mut " } else { "" }, capture_name, Sugg::hir(cx, some_body, "..")), none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")), wrap_braces, }) diff --git a/tests/ui/option_if_let_else.fixed b/tests/ui/option_if_let_else.fixed index 80b162714acaa..695a460cc4edf 100644 --- a/tests/ui/option_if_let_else.fixed +++ b/tests/ui/option_if_let_else.fixed @@ -11,8 +11,22 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> { } else { string.map_or(Some((false, "")), |x| Some((true, x))) } } -fn unop_bad(string: &Option<&str>) -> usize { - (*string).map_or(0, |s| s.len()) +fn unop_bad(string: &Option<&str>, mut num: Option) { + let _ = string.map_or(0, |s| s.len()); + let _ = num.as_ref().map_or(&0, |s| s); + let _ = num.as_mut().map_or(&mut 0, |s| { + *s += 1; + s + }); + let _ = num.as_ref().map_or(&0, |s| s); + let _ = num.map_or(0, |mut s| { + s += 1; + s + }); + let _ = num.as_mut().map_or(&mut 0, |s| { + *s += 1; + s + }); } fn longer_body(arg: Option) -> u32 { @@ -53,7 +67,7 @@ fn main() { let _ = optional.map_or(5, |x| x + 2); let _ = bad1(None); let _ = else_if_option(None); - let _ = unop_bad(&None); + unop_bad(&None, None); let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index 7c43fbea373f3..6f9d506d3470b 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -19,12 +19,40 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> { } } -fn unop_bad(string: &Option<&str>) -> usize { - if let Some(s) = *string { +fn unop_bad(string: &Option<&str>, mut num: Option) { + let _ = if let Some(s) = *string { s.len() } else { 0 - } + }; + let _ = if let Some(s) = &num { + s + } else { + &0 + }; + let _ = if let Some(s) = &mut num { + *s += 1; + s + } else { + &mut 0 + }; + let _ = if let Some(ref s) = num { + s + } else { + &0 + }; + let _ = if let Some(mut s) = num { + s += 1; + s + } else { + 0 + }; + let _ = if let Some(ref mut s) = num { + *s += 1; + s + } else { + &mut 0 + }; } fn longer_body(arg: Option) -> u32 { @@ -69,7 +97,7 @@ fn main() { let _ = if let Some(x) = optional { x + 2 } else { 5 }; let _ = bad1(None); let _ = else_if_option(None); - let _ = unop_bad(&None); + unop_bad(&None, None); let _ = longer_body(None); test_map_or_else(None); let _ = negative_tests(None); diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index b932fe5975900..9240d3edb27e9 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -22,17 +22,100 @@ LL | | } | |_____^ help: try: `{ string.map_or(Some((false, "")), |x| Some((true, x))) }` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:23:5 + --> $DIR/option_if_let_else.rs:23:13 | -LL | / if let Some(s) = *string { +LL | let _ = if let Some(s) = *string { + | _____________^ LL | | s.len() LL | | } else { LL | | 0 -LL | | } - | |_____^ help: try: `(*string).map_or(0, |s| s.len())` +LL | | }; + | |_____^ help: try: `string.map_or(0, |s| s.len())` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:28:13 + | +LL | let _ = if let Some(s) = &num { + | _____________^ +LL | | s +LL | | } else { +LL | | &0 +LL | | }; + | |_____^ help: try: `num.as_ref().map_or(&0, |s| s)` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:33:13 + | +LL | let _ = if let Some(s) = &mut num { + | _____________^ +LL | | *s += 1; +LL | | s +LL | | } else { +LL | | &mut 0 +LL | | }; + | |_____^ + | +help: try + | +LL | let _ = num.as_mut().map_or(&mut 0, |s| { +LL | *s += 1; +LL | s +LL | }); + | + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:39:13 + | +LL | let _ = if let Some(ref s) = num { + | _____________^ +LL | | s +LL | | } else { +LL | | &0 +LL | | }; + | |_____^ help: try: `num.as_ref().map_or(&0, |s| s)` + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:44:13 + | +LL | let _ = if let Some(mut s) = num { + | _____________^ +LL | | s += 1; +LL | | s +LL | | } else { +LL | | 0 +LL | | }; + | |_____^ + | +help: try + | +LL | let _ = num.map_or(0, |mut s| { +LL | s += 1; +LL | s +LL | }); + | + +error: use Option::map_or instead of an if let/else + --> $DIR/option_if_let_else.rs:50:13 + | +LL | let _ = if let Some(ref mut s) = num { + | _____________^ +LL | | *s += 1; +LL | | s +LL | | } else { +LL | | &mut 0 +LL | | }; + | |_____^ + | +help: try + | +LL | let _ = num.as_mut().map_or(&mut 0, |s| { +LL | *s += 1; +LL | s +LL | }); + | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:31:5 + --> $DIR/option_if_let_else.rs:59:5 | LL | / if let Some(x) = arg { LL | | let y = x * x; @@ -51,7 +134,7 @@ LL | }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:40:13 + --> $DIR/option_if_let_else.rs:68:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -74,10 +157,10 @@ LL | }, |x| x * x * x * x); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:69:13 + --> $DIR/option_if_let_else.rs:97:13 | LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` -error: aborting due to 6 previous errors +error: aborting due to 11 previous errors From f73b455b99694fbc5ddec38317f705f546729db2 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sat, 9 May 2020 20:44:56 -0700 Subject: [PATCH 05/14] Refactoring --- clippy_lints/src/option_if_let_else.rs | 170 +++++++++++++++---------- tests/ui/option_if_let_else.rs | 18 +-- tests/ui/option_if_let_else.stderr | 43 ++----- 3 files changed, 123 insertions(+), 108 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 4e501f4ca02ac..208aa550765a4 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -5,7 +5,7 @@ use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::intravisit::{NestedVisitorMap, Visitor}; -use rustc_hir::*; +use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, MatchSource, Mutability, PatKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -69,17 +69,12 @@ declare_clippy_lint! { declare_lint_pass!(OptionIfLetElse => [OPTION_IF_LET_ELSE]); -/// Returns true iff the given expression is the result of calling Result::ok +/// 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 - } + if let ExprKind::MethodCall(ref path, _, &[ref receiver]) = &expr.kind { + path.ident.name.to_ident_string() == "ok" && match_type(cx, &cx.tables.expr_ty(&receiver), &paths::RESULT) + } else { + false } } @@ -136,66 +131,108 @@ fn contains_return_break_continue<'tcx>(expression: &'tcx Expr<'tcx>) -> bool { recursive_visitor.seen_return_break_continue } +/// Extracts the body of a given arm. If the arm contains only an expression, +/// then it returns the expression. Otherwise, it returns the entire block +fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> { + if let ExprKind::Block( + Block { + stmts: statements, + expr: Some(expr), + .. + }, + _, + ) = &arm.body.kind + { + if let [] = statements { + Some(&expr) + } else { + Some(&arm.body) + } + } else { + None + } +} + +/// If this is the else body of an if/else expression, then we need to wrap +/// it in curcly braces. Otherwise, we don't. +fn should_wrap_in_braces(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool { + utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| { + if let Some(Expr { + kind: + ExprKind::Match( + _, + arms, + MatchSource::IfDesugar { + contains_else_clause: true, + } + | MatchSource::IfLetDesugar { + contains_else_clause: true, + }, + ), + .. + }) = parent.expr + { + expr.hir_id == arms[1].body.hir_id + } else { + false + } + }) +} + +fn format_option_in_sugg( + cx: &LateContext<'_, '_>, + cond_expr: &Expr<'_>, + parens_around_option: bool, + as_ref: bool, + as_mut: bool, +) -> String { + format!( + "{}{}{}{}", + if parens_around_option { "(" } else { "" }, + Sugg::hir(cx, cond_expr, ".."), + if parens_around_option { ")" } else { "" }, + if as_mut { + ".as_mut()" + } else if as_ref { + ".as_ref()" + } else { + "" + } + ) +} + /// If this expression is the option if let/else construct we're detecting, then -/// this function returns an OptionIfLetElseOccurence struct with details if +/// 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 { if_chain! { if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly - if let ExprKind::Match(let_body, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind; + if let ExprKind::Match(cond_expr, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind; if arms.len() == 2; - // if type_is_option(cx, &cx.tables.expr_ty(let_body).kind); - if !is_result_ok(cx, let_body); // Don't lint on Result::ok because a different lint does it already + if !is_result_ok(cx, cond_expr); // Don't lint on Result::ok because a different lint does it already if let PatKind::TupleStruct(struct_qpath, &[inner_pat], _) = &arms[0].pat.kind; if utils::match_qpath(struct_qpath, &paths::OPTION_SOME); if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind; if !contains_return_break_continue(arms[0].body); if !contains_return_break_continue(arms[1].body); then { - let (capture_mut, capture_ref, capture_ref_mut) = match bind_annotation { - BindingAnnotation::Unannotated => (false, false, false), - BindingAnnotation::Mutable => (true, false, false), - BindingAnnotation::Ref => (false, true, false), - BindingAnnotation::RefMut => (false, false, true), - }; - 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_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" }; + let some_body = extract_body_from_arm(&arms[0])?; + let none_body = extract_body_from_arm(&arms[1])?; + let method_sugg = match &none_body.kind { + ExprKind::Block(..) => "map_or_else", + _ => "map_or", }; let capture_name = id.name.to_ident_string(); - let wrap_braces = utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| { - if_chain! { - if let Some(Expr { kind: ExprKind::Match( - _, - arms, - MatchSource::IfDesugar{contains_else_clause: true} - | MatchSource::IfLetDesugar{contains_else_clause: true}), - .. } ) = parent.expr; - if expr.hir_id == arms[1].body.hir_id; - then { - true - } else { - false - } - } - }); - let (parens_around_option, as_ref, as_mut, let_body) = match &let_body.kind { + let wrap_braces = should_wrap_in_braces(cx, expr); + let (as_ref, as_mut) = match &cond_expr.kind { + ExprKind::AddrOf(_, Mutability::Not, _) => (true, false), + ExprKind::AddrOf(_, Mutability::Mut, _) => (false, true), + _ => (bind_annotation == &BindingAnnotation::Ref, bind_annotation == &BindingAnnotation::RefMut), + }; + let parens_around_option = match &cond_expr.kind { + // Put parens around the option expression if not doing so might + // mess up the order of operations. ExprKind::Call(..) | ExprKind::MethodCall(..) | ExprKind::Loop(..) @@ -203,15 +240,20 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - | ExprKind::Block(..) | ExprKind::Field(..) | ExprKind::Path(_) - => (false, capture_ref, capture_ref_mut, let_body), - ExprKind::Unary(UnOp::UnDeref, expr) => (false, capture_ref, capture_ref_mut, expr), - ExprKind::AddrOf(_, mutability, expr) => (false, mutability == &Mutability::Not, mutability == &Mutability::Mut, expr), - _ => (true, capture_ref, capture_ref_mut, let_body), + | ExprKind::Unary(UnOp::UnDeref, _) + | ExprKind::AddrOf(..) + => false, + _ => true, + }; + let cond_expr = match &cond_expr.kind { + // Pointer dereferencing happens automatically, so we can omit it in the suggestion + ExprKind::Unary(UnOp::UnDeref, expr)|ExprKind::AddrOf(_, _, expr) => expr, + _ => cond_expr, }; Some(OptionIfLetElseOccurence { - option: format!("{}{}{}{}", if parens_around_option { "(" } else { "" }, Sugg::hir(cx, let_body, ".."), if parens_around_option { ")" } else { "" }, if as_mut { ".as_mut()" } else if as_ref { ".as_ref()" } else { "" }), - method_sugg: format!("{}", method_sugg), - some_expr: format!("|{}{}{}| {}", if false { "ref " } else { "" }, if capture_mut { "mut " } else { "" }, capture_name, Sugg::hir(cx, some_body, "..")), + option: format_option_in_sugg(cx, cond_expr, parens_around_option, as_ref, as_mut), + method_sugg: method_sugg.to_string(), + some_expr: format!("|{}{}| {}", capture_mut, capture_name, Sugg::hir(cx, some_body, "..")), none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")), wrap_braces, }) diff --git a/tests/ui/option_if_let_else.rs b/tests/ui/option_if_let_else.rs index 6f9d506d3470b..dee80d26bd976 100644 --- a/tests/ui/option_if_let_else.rs +++ b/tests/ui/option_if_let_else.rs @@ -20,27 +20,15 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> { } fn unop_bad(string: &Option<&str>, mut num: Option) { - let _ = if let Some(s) = *string { - s.len() - } else { - 0 - }; - let _ = if let Some(s) = &num { - s - } else { - &0 - }; + let _ = if let Some(s) = *string { s.len() } else { 0 }; + let _ = if let Some(s) = &num { s } else { &0 }; let _ = if let Some(s) = &mut num { *s += 1; s } else { &mut 0 }; - let _ = if let Some(ref s) = num { - s - } else { - &0 - }; + let _ = if let Some(ref s) = num { s } else { &0 }; let _ = if let Some(mut s) = num { s += 1; s diff --git a/tests/ui/option_if_let_else.stderr b/tests/ui/option_if_let_else.stderr index 9240d3edb27e9..7005850efaf83 100644 --- a/tests/ui/option_if_let_else.stderr +++ b/tests/ui/option_if_let_else.stderr @@ -24,27 +24,17 @@ LL | | } error: use Option::map_or instead of an if let/else --> $DIR/option_if_let_else.rs:23:13 | -LL | let _ = if let Some(s) = *string { - | _____________^ -LL | | s.len() -LL | | } else { -LL | | 0 -LL | | }; - | |_____^ help: try: `string.map_or(0, |s| s.len())` +LL | let _ = if let Some(s) = *string { s.len() } else { 0 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `string.map_or(0, |s| s.len())` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:28:13 + --> $DIR/option_if_let_else.rs:24:13 | -LL | let _ = if let Some(s) = &num { - | _____________^ -LL | | s -LL | | } else { -LL | | &0 -LL | | }; - | |_____^ help: try: `num.as_ref().map_or(&0, |s| s)` +LL | let _ = if let Some(s) = &num { s } else { &0 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:33:13 + --> $DIR/option_if_let_else.rs:25:13 | LL | let _ = if let Some(s) = &mut num { | _____________^ @@ -64,18 +54,13 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:39:13 + --> $DIR/option_if_let_else.rs:31:13 | -LL | let _ = if let Some(ref s) = num { - | _____________^ -LL | | s -LL | | } else { -LL | | &0 -LL | | }; - | |_____^ help: try: `num.as_ref().map_or(&0, |s| s)` +LL | let _ = if let Some(ref s) = num { s } else { &0 }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `num.as_ref().map_or(&0, |s| s)` error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:44:13 + --> $DIR/option_if_let_else.rs:32:13 | LL | let _ = if let Some(mut s) = num { | _____________^ @@ -95,7 +80,7 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:50:13 + --> $DIR/option_if_let_else.rs:38:13 | LL | let _ = if let Some(ref mut s) = num { | _____________^ @@ -115,7 +100,7 @@ LL | }); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:59:5 + --> $DIR/option_if_let_else.rs:47:5 | LL | / if let Some(x) = arg { LL | | let y = x * x; @@ -134,7 +119,7 @@ LL | }) | error: use Option::map_or_else instead of an if let/else - --> $DIR/option_if_let_else.rs:68:13 + --> $DIR/option_if_let_else.rs:56:13 | LL | let _ = if let Some(x) = arg { | _____________^ @@ -157,7 +142,7 @@ LL | }, |x| x * x * x * x); | error: use Option::map_or instead of an if let/else - --> $DIR/option_if_let_else.rs:97:13 + --> $DIR/option_if_let_else.rs:85:13 | LL | let _ = if let Some(x) = optional { x + 2 } else { 5 }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `optional.map_or(5, |x| x + 2)` From 7c4de9d3dee3a7e16118df5c1cd2080af7350d98 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Mon, 11 May 2020 17:52:47 -0700 Subject: [PATCH 06/14] Refactoring pt. 2 --- clippy_lints/src/option_if_let_else.rs | 33 ++++---------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 208aa550765a4..6e8d7515671fb 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -179,18 +179,10 @@ fn should_wrap_in_braces(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool { }) } -fn format_option_in_sugg( - cx: &LateContext<'_, '_>, - cond_expr: &Expr<'_>, - parens_around_option: bool, - as_ref: bool, - as_mut: bool, -) -> String { +fn format_option_in_sugg(cx: &LateContext<'_, '_>, cond_expr: &Expr<'_>, as_ref: bool, as_mut: bool) -> String { format!( - "{}{}{}{}", - if parens_around_option { "(" } else { "" }, - Sugg::hir(cx, cond_expr, ".."), - if parens_around_option { ")" } else { "" }, + "{}{}", + Sugg::hir(cx, cond_expr, "..").maybe_par(), if as_mut { ".as_mut()" } else if as_ref { @@ -230,28 +222,13 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - ExprKind::AddrOf(_, Mutability::Mut, _) => (false, true), _ => (bind_annotation == &BindingAnnotation::Ref, bind_annotation == &BindingAnnotation::RefMut), }; - let parens_around_option = match &cond_expr.kind { - // Put parens around the option expression if not doing so might - // mess up the order of operations. - ExprKind::Call(..) - | ExprKind::MethodCall(..) - | ExprKind::Loop(..) - | ExprKind::Match(..) - | ExprKind::Block(..) - | ExprKind::Field(..) - | ExprKind::Path(_) - | ExprKind::Unary(UnOp::UnDeref, _) - | ExprKind::AddrOf(..) - => false, - _ => true, - }; let cond_expr = match &cond_expr.kind { // Pointer dereferencing happens automatically, so we can omit it in the suggestion - ExprKind::Unary(UnOp::UnDeref, expr)|ExprKind::AddrOf(_, _, expr) => expr, + ExprKind::Unary(UnOp::UnDeref, expr) | ExprKind::AddrOf(_, _, expr) => expr, _ => cond_expr, }; Some(OptionIfLetElseOccurence { - option: format_option_in_sugg(cx, cond_expr, parens_around_option, as_ref, as_mut), + option: format_option_in_sugg(cx, cond_expr, as_ref, as_mut), method_sugg: method_sugg.to_string(), some_expr: format!("|{}{}| {}", capture_mut, capture_name, Sugg::hir(cx, some_body, "..")), none_expr: format!("{}{}", if method_sugg == "map_or" { "" } else { "|| " }, Sugg::hir(cx, none_body, "..")), From 5e20475e47d3db1d32a7649a7c3a107caba32a14 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sun, 31 May 2020 15:34:10 -0700 Subject: [PATCH 07/14] Don't lint if contains a macro --- clippy_lints/src/option_if_let_else.rs | 28 +++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 6e8d7515671fb..be0c44cae343f 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -10,8 +10,6 @@ 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 @@ -88,19 +86,17 @@ struct OptionIfLetElseOccurence { wrap_braces: bool, } -struct ReturnBreakContinueVisitor<'tcx> { +struct ReturnBreakContinueMacroVisitor { seen_return_break_continue: bool, - phantom_data: PhantomData<&'tcx bool>, } -impl<'tcx> ReturnBreakContinueVisitor<'tcx> { - fn new() -> ReturnBreakContinueVisitor<'tcx> { - ReturnBreakContinueVisitor { +impl ReturnBreakContinueMacroVisitor { + fn new() -> ReturnBreakContinueMacroVisitor { + ReturnBreakContinueMacroVisitor { seen_return_break_continue: false, - phantom_data: PhantomData, } } } -impl<'tcx> Visitor<'tcx> for ReturnBreakContinueVisitor<'tcx> { +impl<'tcx> Visitor<'tcx> for ReturnBreakContinueMacroVisitor { type Map = Map<'tcx>; fn nested_visit_map(&mut self) -> NestedVisitorMap { NestedVisitorMap::None @@ -119,14 +115,18 @@ impl<'tcx> Visitor<'tcx> for ReturnBreakContinueVisitor<'tcx> { // 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); + if utils::in_macro(ex.span) { + self.seen_return_break_continue = true; + } else { + 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(); +fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool { + let mut recursive_visitor = ReturnBreakContinueMacroVisitor::new(); recursive_visitor.visit_expr(expression); recursive_visitor.seen_return_break_continue } @@ -205,8 +205,8 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - if let PatKind::TupleStruct(struct_qpath, &[inner_pat], _) = &arms[0].pat.kind; if utils::match_qpath(struct_qpath, &paths::OPTION_SOME); if let PatKind::Binding(bind_annotation, _, id, _) = &inner_pat.kind; - if !contains_return_break_continue(arms[0].body); - if !contains_return_break_continue(arms[1].body); + if !contains_return_break_continue_macro(arms[0].body); + if !contains_return_break_continue_macro(arms[1].body); then { let capture_mut = if bind_annotation == &BindingAnnotation::Mutable { "mut " } else { "" }; let some_body = extract_body_from_arm(&arms[0])?; From 5150277a4f765bb113e163adc7eb495dcbb57129 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sun, 31 May 2020 15:37:21 -0700 Subject: [PATCH 08/14] Used clippy to clean itself --- clippy_lints/src/attrs.rs | 14 ++----- clippy_lints/src/if_let_mutex.rs | 8 +--- clippy_lints/src/len_zero.rs | 8 +--- clippy_lints/src/literal_representation.rs | 7 ++-- clippy_lints/src/loops.rs | 26 +++--------- clippy_lints/src/methods/mod.rs | 6 +-- .../src/methods/unnecessary_filter_map.rs | 6 +-- clippy_lints/src/minmax.rs | 21 +++++----- clippy_lints/src/misc.rs | 8 +--- clippy_lints/src/option_if_let_else.rs | 4 +- clippy_lints/src/returns.rs | 9 +---- clippy_lints/src/shadow.rs | 10 ++--- clippy_lints/src/types.rs | 12 +++--- clippy_lints/src/use_self.rs | 8 +--- clippy_lints/src/utils/attrs.rs | 14 +++---- clippy_lints/src/utils/mod.rs | 40 ++++--------------- clippy_lints/src/utils/numeric_literal.rs | 6 +-- clippy_lints/src/utils/sugg.rs | 8 +--- clippy_lints/src/write.rs | 6 +-- 19 files changed, 67 insertions(+), 154 deletions(-) diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index 2d0855f689554..cfad9d79f2b75 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -481,15 +481,11 @@ fn is_relevant_trait(cx: &LateContext<'_>, item: &TraitItem<'_>) -> bool { } fn is_relevant_block(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, block: &Block<'_>) -> bool { - if let Some(stmt) = block.stmts.first() { - match &stmt.kind { + block.stmts.first().map_or(block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e)), |stmt| match &stmt.kind { StmtKind::Local(_) => true, StmtKind::Expr(expr) | StmtKind::Semi(expr) => is_relevant_expr(cx, tables, expr), _ => false, - } - } else { - block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e)) - } + }) } fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: &Expr<'_>) -> bool { @@ -499,11 +495,7 @@ fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: & ExprKind::Ret(None) | ExprKind::Break(_, None) => false, ExprKind::Call(path_expr, _) => { if let ExprKind::Path(qpath) = &path_expr.kind { - if let Some(fun_id) = tables.qpath_res(qpath, path_expr.hir_id).opt_def_id() { - !match_def_path(cx, fun_id, &paths::BEGIN_PANIC) - } else { - true - } + tables.qpath_res(qpath, path_expr.hir_id).opt_def_id().map_or(true, |fun_id| !match_def_path(cx, fun_id, &paths::BEGIN_PANIC)) } else { true } diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs index f911cb68ea579..7e44618e90eb1 100644 --- a/clippy_lints/src/if_let_mutex.rs +++ b/clippy_lints/src/if_let_mutex.rs @@ -135,13 +135,9 @@ impl<'tcx> Visitor<'tcx> for ArmVisitor<'_, 'tcx> { } } -impl<'tcx> ArmVisitor<'_, 'tcx> { +impl<'tcx, 'l> ArmVisitor<'tcx, 'l> { fn same_mutex(&self, cx: &LateContext<'_>, op_mutex: &Expr<'_>) -> bool { - if let Some(arm_mutex) = self.found_mutex { - SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex) - } else { - false - } + self.found_mutex.map_or(false, |arm_mutex| SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex)) } } diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 26d96428771d6..f57fa830adcdd 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -303,14 +303,10 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { let ty = &walk_ptrs_ty(cx.tables().expr_ty(expr)); match ty.kind { ty::Dynamic(ref tt, ..) => { - if let Some(principal) = tt.principal() { - cx.tcx + tt.principal().map_or(false, |principal| cx.tcx .associated_items(principal.def_id()) .in_definition_order() - .any(|item| is_is_empty(cx, &item)) - } else { - false - } + .any(|item| is_is_empty(cx, &item))) }, ty::Projection(ref proj) => has_is_empty_impl(cx, proj.item_def_id), ty::Adt(id, _) => has_is_empty_impl(cx, id.did), diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 7ba43562d7d44..ea2e23bd3a19e 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -264,10 +264,11 @@ impl LiteralDigitGrouping { let (part, mistyped_suffixes, missing_char) = if let Some((_, exponent)) = &mut num_lit.exponent { (exponent, &["32", "64"][..], 'f') - } else if let Some(fraction) = &mut num_lit.fraction { - (fraction, &["32", "64"][..], 'f') } else { - (&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i') + num_lit.fraction.as_mut().map_or( + (&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i'), + |fraction| (fraction, &["32", "64"][..], 'f') + ) }; let mut split = part.rsplit('_'); diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index d821b5134841e..8d48f39a0454a 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -687,11 +687,7 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult { } }, ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => { - if let Some(ref e) = *e { - combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak) - } else { - NeverLoopResult::AlwaysBreak - } + e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)) }, ExprKind::InlineAsm(ref asm) => asm .operands @@ -1882,11 +1878,7 @@ fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool { // IntoIterator is currently only implemented for array sizes <= 32 in rustc match ty.kind { ty::Array(_, n) => { - if let Some(val) = n.try_eval_usize(cx.tcx, cx.param_env) { - (0..=32).contains(&val) - } else { - false - } + n.try_eval_usize(cx.tcx, cx.param_env).map_or(false, |val| (0..=32).contains(&val)) }, _ => false, } @@ -1899,11 +1891,7 @@ fn extract_expr_from_first_stmt<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr< return None; } if let StmtKind::Local(ref local) = block.stmts[0].kind { - if let Some(expr) = local.init { - Some(expr) - } else { - None - } + local.init.map(|expr| expr) } else { None } @@ -2023,15 +2011,11 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { if let PatKind::Binding(.., ident, _) = local.pat.kind { self.name = Some(ident.name); - self.state = if let Some(ref init) = local.init { - if is_integer_const(&self.cx, init, 0) { + self.state = local.init.as_ref().map_or(VarState::Declared, |init| if is_integer_const(&self.cx, init, 0) { VarState::Warn } else { VarState::Declared - } - } else { - VarState::Declared - } + }) } } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 160304865c560..ddad16e163e44 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2461,11 +2461,7 @@ fn derefs_to_slice<'tcx>( ty::Adt(def, _) if def.is_box() => may_slice(cx, ty.boxed_ty()), ty::Adt(..) => is_type_diagnostic_item(cx, ty, sym!(vec_type)), ty::Array(_, size) => { - if let Some(size) = size.try_eval_usize(cx.tcx, cx.param_env) { - size < 32 - } else { - false - } + size.try_eval_usize(cx.tcx, cx.param_env).map_or(false, |size| size < 32) }, ty::Ref(_, inner, _) => may_slice(cx, inner), _ => false, diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index fdcba11054288..97909c97fc728 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -78,11 +78,7 @@ fn check_expression<'tcx>(cx: &LateContext<'tcx>, arg_id: hir::HirId, expr: &'tc (true, true) }, hir::ExprKind::Block(ref block, _) => { - if let Some(expr) = &block.expr { - check_expression(cx, arg_id, &expr) - } else { - (false, false) - } + block.expr.as_ref().map_or((false, false), |expr| check_expression(cx, arg_id, &expr)) }, hir::ExprKind::Match(_, arms, _) => { let mut found_mapping = false; diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs index 0a2d577396a5f..a3d1a500aa7c9 100644 --- a/clippy_lints/src/minmax.rs +++ b/clippy_lints/src/minmax.rs @@ -86,16 +86,19 @@ fn fetch_const<'a>(cx: &LateContext<'_>, args: &'a [Expr<'a>], m: MinMax) -> Opt if args.len() != 2 { return None; } - if let Some(c) = constant_simple(cx, cx.tables(), &args[0]) { - if constant_simple(cx, cx.tables(), &args[1]).is_none() { - // otherwise ignore - Some((m, c, &args[1])) + constant_simple(cx, cx.tables, &args[0]).map_or_else( + || if let Some(c) = constant_simple(cx, cx.tables(), &args[1]) { + Some((m, c, &args[0])) } else { None + }, + |c| { + if constant_simple(cx, cx.tables, &args[1]).is_none() { + // otherwise ignore + Some((c, &args[1])) + } else { + None + } } - } else if let Some(c) = constant_simple(cx, cx.tables(), &args[1]) { - Some((m, c, &args[0])) - } else { - None - } + ).map(|(c, arg)| (m, c, arg)) } diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 6a256627bd18e..5b0f9d6e3ec38 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -682,16 +682,12 @@ fn check_to_owned(cx: &LateContext<'_>, expr: &Expr<'_>, other: &Expr<'_>, left: /// `unused_variables`'s idea /// of what it means for an expression to be "used". fn is_used(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if let Some(parent) = get_parent_expr(cx, expr) { - match parent.kind { + get_parent_expr(cx, expr).map_or(true, |parent| match parent.kind { ExprKind::Assign(_, ref rhs, _) | ExprKind::AssignOp(_, _, ref rhs) => { SpanlessEq::new(cx).eq_expr(rhs, expr) }, _ => is_used(cx, parent), - } - } else { - true - } + }) } /// Tests whether an expression is in a macro expansion (e.g., something diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index be0c44cae343f..b6d08b8ae17a5 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -37,6 +37,7 @@ declare_clippy_lint! { /// /// ```rust /// # let optional: Option = Some(0); + /// # fn do_complicated_function() -> u32 { 5 }; /// let _ = if let Some(foo) = optional { /// foo /// } else { @@ -54,9 +55,10 @@ declare_clippy_lint! { /// /// ```rust /// # let optional: Option = Some(0); + /// # fn do_complicated_function() -> u32 { 5 }; /// let _ = optional.map_or(5, |foo| foo); /// let _ = optional.map_or_else(||{ - /// let y = do_complicated_function; + /// let y = do_complicated_function(); /// y*y /// }, |foo| foo); /// ``` diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 3c93974417356..4d54e3117faf4 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -259,15 +259,10 @@ fn is_unit_expr(expr: &ast::Expr) -> bool { fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { - if let Some(rpos) = fn_source.rfind("->") { - #[allow(clippy::cast_possible_truncation)] - ( + fn_source.rfind("->").map_or((ty.span, Applicability::MaybeIncorrect), |rpos| ( ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), Applicability::MachineApplicable, - ) - } else { - (ty.span, Applicability::MaybeIncorrect) - } + )) } else { (ty.span, Applicability::MaybeIncorrect) }; diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index 7da47ee4ff94b..de94fb8714725 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -164,15 +164,11 @@ fn check_local<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>, bindings: & } fn is_binding(cx: &LateContext<'_>, pat_id: HirId) -> bool { - let var_ty = cx.tables().node_type_opt(pat_id); - if let Some(var_ty) = var_ty { - match var_ty.kind { + let var_ty = cx.tables.node_type_opt(pat_id); + var_ty.map_or(false, |var_ty| match var_ty.kind { ty::Adt(..) => false, _ => true, - } - } else { - false - } + }) } fn check_pat<'tcx>( diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index b1345f0de5e4b..df87c1b980254 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1205,16 +1205,14 @@ fn span_lossless_lint(cx: &LateContext<'_>, expr: &Expr<'_>, op: &Expr<'_>, cast // has parens on the outside, they are no longer needed. let mut applicability = Applicability::MachineApplicable; let opt = snippet_opt(cx, op.span); - let sugg = if let Some(ref snip) = opt { - if should_strip_parens(op, snip) { + let sugg = opt.as_ref().map_or_else(|| { + applicability = Applicability::HasPlaceholders; + ".." + }, |snip| if should_strip_parens(op, snip) { &snip[1..snip.len() - 1] } else { snip.as_str() - } - } else { - applicability = Applicability::HasPlaceholders; - ".." - }; + }); span_lint_and_sugg( cx, diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index f85db1e2006e8..eac7ae2358e8c 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -167,14 +167,10 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { if let TyKind::Path(QPath::Resolved(_, ref item_path)) = item_type.kind; then { let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args; - let should_check = if let Some(ref params) = *parameters { - !params.parenthesized && !params.args.iter().any(|arg| match arg { + let should_check = parameters.as_ref().map_or(true, |params| !params.parenthesized && !params.args.iter().any(|arg| match arg { GenericArg::Lifetime(_) => true, _ => false, - }) - } else { - true - }; + })); if should_check { let visitor = &mut UseSelfVisitor { diff --git a/clippy_lints/src/utils/attrs.rs b/clippy_lints/src/utils/attrs.rs index 4dcf6c105ec63..2d72f9c3fe172 100644 --- a/clippy_lints/src/utils/attrs.rs +++ b/clippy_lints/src/utils/attrs.rs @@ -65,8 +65,7 @@ pub fn get_attr<'a>( }; let attr_segments = &attr.path.segments; if attr_segments.len() == 2 && attr_segments[0].ident.to_string() == "clippy" { - if let Some(deprecation_status) = - BUILTIN_ATTRIBUTES + BUILTIN_ATTRIBUTES .iter() .find_map(|(builtin_name, deprecation_status)| { if *builtin_name == attr_segments[1].ident.to_string() { @@ -74,8 +73,10 @@ pub fn get_attr<'a>( } else { None } - }) - { + }).map_or_else(|| { + sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute"); + false + }, |deprecation_status| { let mut diag = sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute"); match *deprecation_status { DeprecationStatus::Deprecated => { @@ -97,10 +98,7 @@ pub fn get_attr<'a>( attr_segments[1].ident.to_string() == name }, } - } else { - sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute"); - false - } + }) } else { false } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 99ba7d64331cd..6f23e96800684 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -153,11 +153,7 @@ pub fn is_type_diagnostic_item(cx: &LateContext<'_>, ty: Ty<'_>, diag_item: Symb pub fn match_trait_method(cx: &LateContext<'_>, expr: &Expr<'_>, path: &[&str]) -> bool { let def_id = cx.tables().type_dependent_def_id(expr.hir_id).unwrap(); let trt_id = cx.tcx.trait_of_item(def_id); - if let Some(trt_id) = trt_id { - match_def_path(cx, trt_id, path) - } else { - false - } + trt_id.map_or(false, |trt_id| match_def_path(cx, trt_id, path)) } /// Checks if an expression references a variable of the given name. @@ -600,21 +596,13 @@ pub fn snippet_block_with_applicability<'a, T: LintContext>( /// // ^^^^^^^^^^ /// ``` pub fn first_line_of_span(cx: &T, span: Span) -> Span { - if let Some(first_char_pos) = first_char_in_first_line(cx, span) { - span.with_lo(first_char_pos) - } else { - span - } + first_char_in_first_line(cx, span).map_or(span, |first_char_pos| span.with_lo(first_char_pos)) } fn first_char_in_first_line(cx: &T, span: Span) -> Option { let line_span = line_span(cx, span); - if let Some(snip) = snippet_opt(cx, line_span) { - snip.find(|c: char| !c.is_whitespace()) - .map(|pos| line_span.lo() + BytePos::from_usize(pos)) - } else { - None - } + snippet_opt(cx, line_span).and_then(|snip| snip.find(|c: char| !c.is_whitespace()) + .map(|pos| line_span.lo() + BytePos::from_usize(pos))) } /// Returns the indentation of the line of a span @@ -626,11 +614,7 @@ fn first_char_in_first_line(cx: &T, span: Span) -> Option(cx: &T, span: Span) -> Option { - if let Some(snip) = snippet_opt(cx, line_span(cx, span)) { - snip.find(|c: char| !c.is_whitespace()) - } else { - None - } + snippet_opt(cx, line_span(cx, span)).and_then(|snip| snip.find(|c: char| !c.is_whitespace())) } /// Extends the span to the beginning of the spans line, incl. whitespaces. @@ -738,8 +722,7 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio let enclosing_node = map .get_enclosing_scope(hir_id) .and_then(|enclosing_id| map.find(enclosing_id)); - if let Some(node) = enclosing_node { - match node { + enclosing_node.and_then(|node| match node { Node::Block(block) => Some(block), Node::Item(&Item { kind: ItemKind::Fn(_, _, eid), @@ -753,10 +736,7 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio _ => None, }, _ => None, - } - } else { - None - } + }) } /// Returns the base type for HIR references and pointers. @@ -1328,11 +1308,7 @@ pub fn is_must_use_func_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { _ => None, }; - if let Some(did) = did { - must_use_attr(&cx.tcx.get_attrs(did)).is_some() - } else { - false - } + did.map_or(false, |did| must_use_attr(&cx.tcx.get_attrs(did)).is_some()) } pub fn is_no_std_crate(krate: &Crate<'_>) -> bool { diff --git a/clippy_lints/src/utils/numeric_literal.rs b/clippy_lints/src/utils/numeric_literal.rs index 99413153d49bb..7a79741b30bd0 100644 --- a/clippy_lints/src/utils/numeric_literal.rs +++ b/clippy_lints/src/utils/numeric_literal.rs @@ -200,12 +200,10 @@ impl<'a> NumericLiteral<'a> { fn split_suffix<'a>(src: &'a str, lit_kind: &LitKind) -> (&'a str, Option<&'a str>) { debug_assert!(lit_kind.is_numeric()); - if let Some(suffix_length) = lit_suffix_length(lit_kind) { + lit_suffix_length(lit_kind).map_or((src, None), |suffix_length| { let (unsuffixed, suffix) = src.split_at(src.len() - suffix_length); (unsuffixed, Some(suffix)) - } else { - (src, None) - } + }) } fn lit_suffix_length(lit_kind: &LitKind) -> Option { diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index 8fc97f2fd64ae..20bea3cbabe64 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -492,8 +492,7 @@ fn astbinop2assignop(op: ast::BinOp) -> AssocOp { /// before it on its line. fn indentation(cx: &T, span: Span) -> Option { let lo = cx.sess().source_map().lookup_char_pos(span.lo()); - if let Some(line) = lo.file.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) { - if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') { + lo.file.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */).and_then(|line| if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') { // We can mix char and byte positions here because we only consider `[ \t]`. if lo.col == CharPos(pos) { Some(line[..pos].into()) @@ -502,10 +501,7 @@ fn indentation(cx: &T, span: Span) -> Option { } } else { None - } - } else { - None - } + }) } /// Convenience extension trait for `DiagnosticBuilder`. diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 732f4b28e06e3..3b10b7b82a089 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -297,12 +297,10 @@ impl EarlyLintPass for Write { if let (Some(fmt_str), expr) = self.check_tts(cx, &mac.args.inner_tokens(), true) { if fmt_str.symbol == Symbol::intern("") { let mut applicability = Applicability::MachineApplicable; - let suggestion = if let Some(e) = expr { - snippet_with_applicability(cx, e.span, "v", &mut applicability) - } else { + let suggestion = expr.map_or_else(|| { applicability = Applicability::HasPlaceholders; Cow::Borrowed("v") - }; + }, |e| snippet_with_applicability(cx, e.span, "v", &mut Applicability::MachineApplicable)); span_lint_and_sugg( cx, From 93f0f5d37b45c648aebf87fe7e7379599ba3e726 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Fri, 12 Jun 2020 08:32:38 -0700 Subject: [PATCH 09/14] Last few tweaks --- CHANGELOG.md | 1 - clippy_lints/src/lib.rs | 3 +-- clippy_lints/src/minmax.rs | 3 +-- clippy_lints/src/option_if_let_else.rs | 2 +- clippy_lints/src/returns.rs | 9 +++++++-- src/lintlist/mod.rs | 2 +- 6 files changed, 11 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a081bb85feab..01c0e4b0302a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1491,7 +1491,6 @@ Released 2018-09-13 [`large_stack_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_arrays [`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty [`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero -[`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return [`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock [`let_underscore_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_must_use [`let_unit_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cd91e7ceb32a8..fe34e4390d659 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1161,6 +1161,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&needless_continue::NEEDLESS_CONTINUE), LintId::of(&needless_pass_by_value::NEEDLESS_PASS_BY_VALUE), LintId::of(&non_expressive_names::SIMILAR_NAMES), + LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(&ranges::RANGE_PLUS_ONE), LintId::of(&shadow::SHADOW_UNRELATED), LintId::of(&strings::STRING_ADD_ASSIGN), @@ -1372,7 +1373,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), LintId::of(&open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(&option_env_unwrap::OPTION_ENV_UNWRAP), - LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(&overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(&panic_unimplemented::PANIC_PARAMS), LintId::of(&partialeq_ne_impl::PARTIALEQ_NE_IMPL), @@ -1521,7 +1521,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&new_without_default::NEW_WITHOUT_DEFAULT), LintId::of(&non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(&non_expressive_names::MANY_SINGLE_CHAR_NAMES), - LintId::of(&option_if_let_else::OPTION_IF_LET_ELSE), LintId::of(&panic_unimplemented::PANIC_PARAMS), LintId::of(&ptr::CMP_NULL), LintId::of(&ptr::PTR_ARG), diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs index a3d1a500aa7c9..6ec7f8cae5d75 100644 --- a/clippy_lints/src/minmax.rs +++ b/clippy_lints/src/minmax.rs @@ -99,6 +99,5 @@ fn fetch_const<'a>(cx: &LateContext<'_>, args: &'a [Expr<'a>], m: MinMax) -> Opt } else { None } - } - ).map(|(c, arg)| (m, c, arg)) + }) } diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index b6d08b8ae17a5..c877cc6fa8b8a 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -63,7 +63,7 @@ declare_clippy_lint! { /// }, |foo| foo); /// ``` pub OPTION_IF_LET_ELSE, - style, + pedantic, "reimplementation of Option::map_or" } diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 4d54e3117faf4..3c93974417356 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -259,10 +259,15 @@ fn is_unit_expr(expr: &ast::Expr) -> bool { fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { - fn_source.rfind("->").map_or((ty.span, Applicability::MaybeIncorrect), |rpos| ( + if let Some(rpos) = fn_source.rfind("->") { + #[allow(clippy::cast_possible_truncation)] + ( ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), Applicability::MachineApplicable, - )) + ) + } else { + (ty.span, Applicability::MaybeIncorrect) + } } else { (ty.span, Applicability::MaybeIncorrect) }; diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index b499d565fa7f9..e681f47f949df 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1622,7 +1622,7 @@ pub static ref ALL_LINTS: Vec = vec![ }, Lint { name: "option_if_let_else", - group: "style", + group: "pedantic", desc: "reimplementation of Option::map_or", deprecation: None, module: "option_if_let_else", From ccb999851aaa4d3e9cd97110bd6523a6c3df46fd Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Thu, 25 Jun 2020 11:39:58 -0700 Subject: [PATCH 10/14] Fix compile error from library change --- CHANGELOG.md | 1 + clippy_lints/src/option_if_let_else.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01c0e4b0302a4..1a081bb85feab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1491,6 +1491,7 @@ Released 2018-09-13 [`large_stack_arrays`]: https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_arrays [`len_without_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_without_is_empty [`len_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#len_zero +[`let_and_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return [`let_underscore_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_lock [`let_underscore_must_use`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_must_use [`let_unit_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#let_unit_value diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index c877cc6fa8b8a..7e299dd5fd99f 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -71,7 +71,7 @@ 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 let ExprKind::MethodCall(ref path, _, &[ref receiver]) = &expr.kind { + if let ExprKind::MethodCall(ref path, _, &[ref receiver], _) = &expr.kind { path.ident.name.to_ident_string() == "ok" && match_type(cx, &cx.tables.expr_ty(&receiver), &paths::RESULT) } else { false From 6ce981225b73d6c3514b0abb759a5282521a17d9 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Thu, 25 Jun 2020 11:58:47 -0700 Subject: [PATCH 11/14] Clean existing lint code to match new lint --- clippy_lints/src/attrs.rs | 14 ++-- clippy_lints/src/if_let_mutex.rs | 5 +- clippy_lints/src/len_zero.rs | 12 ++-- clippy_lints/src/literal_representation.rs | 10 +-- clippy_lints/src/loops.rs | 20 +++--- clippy_lints/src/methods/mod.rs | 6 +- .../src/methods/unnecessary_filter_map.rs | 7 +- clippy_lints/src/minmax.rs | 2 +- clippy_lints/src/misc.rs | 8 +-- clippy_lints/src/option_if_let_else.rs | 2 +- clippy_lints/src/returns.rs | 18 ++--- clippy_lints/src/shadow.rs | 6 +- clippy_lints/src/types.rs | 21 +++--- clippy_lints/src/use_self.rs | 7 +- clippy_lints/src/utils/attrs.rs | 65 ++++++++++--------- clippy_lints/src/utils/mod.rs | 32 ++++----- clippy_lints/src/utils/sugg.rs | 16 +++-- clippy_lints/src/write.rs | 11 ++-- 18 files changed, 147 insertions(+), 115 deletions(-) diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index cfad9d79f2b75..c4397560d7db1 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -480,12 +480,15 @@ fn is_relevant_trait(cx: &LateContext<'_>, item: &TraitItem<'_>) -> bool { } } -fn is_relevant_block(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, block: &Block<'_>) -> bool { - block.stmts.first().map_or(block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e)), |stmt| match &stmt.kind { +fn is_relevant_block(cx: &LateContext<'_, '_>, tables: &ty::TypeckTables<'_>, block: &Block<'_>) -> bool { + block.stmts.first().map_or( + block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e)), + |stmt| match &stmt.kind { StmtKind::Local(_) => true, StmtKind::Expr(expr) | StmtKind::Semi(expr) => is_relevant_expr(cx, tables, expr), _ => false, - }) + }, + ) } fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: &Expr<'_>) -> bool { @@ -495,7 +498,10 @@ fn is_relevant_expr(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, expr: & ExprKind::Ret(None) | ExprKind::Break(_, None) => false, ExprKind::Call(path_expr, _) => { if let ExprKind::Path(qpath) = &path_expr.kind { - tables.qpath_res(qpath, path_expr.hir_id).opt_def_id().map_or(true, |fun_id| !match_def_path(cx, fun_id, &paths::BEGIN_PANIC)) + tables + .qpath_res(qpath, path_expr.hir_id) + .opt_def_id() + .map_or(true, |fun_id| !match_def_path(cx, fun_id, &paths::BEGIN_PANIC)) } else { true } diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs index 7e44618e90eb1..5426e14ead573 100644 --- a/clippy_lints/src/if_let_mutex.rs +++ b/clippy_lints/src/if_let_mutex.rs @@ -136,8 +136,9 @@ impl<'tcx> Visitor<'tcx> for ArmVisitor<'_, 'tcx> { } impl<'tcx, 'l> ArmVisitor<'tcx, 'l> { - fn same_mutex(&self, cx: &LateContext<'_>, op_mutex: &Expr<'_>) -> bool { - self.found_mutex.map_or(false, |arm_mutex| SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex)) + fn same_mutex(&self, cx: &LateContext<'_, '_>, op_mutex: &Expr<'_>) -> bool { + self.found_mutex + .map_or(false, |arm_mutex| SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex)) } } diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index f57fa830adcdd..1b09328ceabb0 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -302,12 +302,12 @@ fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { let ty = &walk_ptrs_ty(cx.tables().expr_ty(expr)); match ty.kind { - ty::Dynamic(ref tt, ..) => { - tt.principal().map_or(false, |principal| cx.tcx - .associated_items(principal.def_id()) - .in_definition_order() - .any(|item| is_is_empty(cx, &item))) - }, + ty::Dynamic(ref tt, ..) => tt.principal().map_or(false, |principal| { + cx.tcx + .associated_items(principal.def_id()) + .in_definition_order() + .any(|item| is_is_empty(cx, &item)) + }), ty::Projection(ref proj) => has_is_empty_impl(cx, proj.item_def_id), ty::Adt(id, _) => has_is_empty_impl(cx, id.did), ty::Array(..) | ty::Slice(..) | ty::Str => true, diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index ea2e23bd3a19e..a36fdca5d5de6 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -265,10 +265,12 @@ impl LiteralDigitGrouping { let (part, mistyped_suffixes, missing_char) = if let Some((_, exponent)) = &mut num_lit.exponent { (exponent, &["32", "64"][..], 'f') } else { - num_lit.fraction.as_mut().map_or( - (&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i'), - |fraction| (fraction, &["32", "64"][..], 'f') - ) + num_lit + .fraction + .as_mut() + .map_or((&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i'), |fraction| { + (fraction, &["32", "64"][..], 'f') + }) }; let mut split = part.rsplit('_'); diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 8d48f39a0454a..b803d753b6d04 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -686,9 +686,9 @@ fn never_loop_expr(expr: &Expr<'_>, main_loop_id: HirId) -> NeverLoopResult { NeverLoopResult::AlwaysBreak } }, - ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => { - e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak)) - }, + ExprKind::Break(_, ref e) | ExprKind::Ret(ref e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| { + combine_seq(never_loop_expr(e, main_loop_id), NeverLoopResult::AlwaysBreak) + }), ExprKind::InlineAsm(ref asm) => asm .operands .iter() @@ -1877,9 +1877,9 @@ fn is_ref_iterable_type(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { fn is_iterable_array<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool { // IntoIterator is currently only implemented for array sizes <= 32 in rustc match ty.kind { - ty::Array(_, n) => { - n.try_eval_usize(cx.tcx, cx.param_env).map_or(false, |val| (0..=32).contains(&val)) - }, + ty::Array(_, n) => n + .try_eval_usize(cx.tcx, cx.param_env) + .map_or(false, |val| (0..=32).contains(&val)), _ => false, } } @@ -1891,7 +1891,7 @@ fn extract_expr_from_first_stmt<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr< return None; } if let StmtKind::Local(ref local) = block.stmts[0].kind { - local.init.map(|expr| expr) + local.init //.map(|expr| expr) } else { None } @@ -2011,11 +2011,13 @@ impl<'a, 'tcx> Visitor<'tcx> for InitializeVisitor<'a, 'tcx> { if let PatKind::Binding(.., ident, _) = local.pat.kind { self.name = Some(ident.name); - self.state = local.init.as_ref().map_or(VarState::Declared, |init| if is_integer_const(&self.cx, init, 0) { + self.state = local.init.as_ref().map_or(VarState::Declared, |init| { + if is_integer_const(&self.cx, init, 0) { VarState::Warn } else { VarState::Declared - }) + } + }) } } } diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ddad16e163e44..f1c8894c0ee29 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2460,9 +2460,9 @@ fn derefs_to_slice<'tcx>( ty::Slice(_) => true, ty::Adt(def, _) if def.is_box() => may_slice(cx, ty.boxed_ty()), ty::Adt(..) => is_type_diagnostic_item(cx, ty, sym!(vec_type)), - ty::Array(_, size) => { - size.try_eval_usize(cx.tcx, cx.param_env).map_or(false, |size| size < 32) - }, + ty::Array(_, size) => size + .try_eval_usize(cx.tcx, cx.param_env) + .map_or(false, |size| size < 32), ty::Ref(_, inner, _) => may_slice(cx, inner), _ => false, } diff --git a/clippy_lints/src/methods/unnecessary_filter_map.rs b/clippy_lints/src/methods/unnecessary_filter_map.rs index 97909c97fc728..75e123eb5939d 100644 --- a/clippy_lints/src/methods/unnecessary_filter_map.rs +++ b/clippy_lints/src/methods/unnecessary_filter_map.rs @@ -77,9 +77,10 @@ fn check_expression<'tcx>(cx: &LateContext<'tcx>, arg_id: hir::HirId, expr: &'tc } (true, true) }, - hir::ExprKind::Block(ref block, _) => { - block.expr.as_ref().map_or((false, false), |expr| check_expression(cx, arg_id, &expr)) - }, + hir::ExprKind::Block(ref block, _) => block + .expr + .as_ref() + .map_or((false, false), |expr| check_expression(cx, arg_id, &expr)), hir::ExprKind::Match(_, arms, _) => { let mut found_mapping = false; let mut found_filtering = false; diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs index 6ec7f8cae5d75..5eb8398d68e51 100644 --- a/clippy_lints/src/minmax.rs +++ b/clippy_lints/src/minmax.rs @@ -53,7 +53,7 @@ impl<'tcx> LateLintPass<'tcx> for MinMaxPass { } } -#[derive(PartialEq, Eq, Debug)] +#[derive(PartialEq, Eq, Debug, Clone, Copy)] enum MinMax { Min, Max, diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 5b0f9d6e3ec38..3d4225f36a7d0 100644 --- a/clippy_lints/src/misc.rs +++ b/clippy_lints/src/misc.rs @@ -683,11 +683,9 @@ fn check_to_owned(cx: &LateContext<'_>, expr: &Expr<'_>, other: &Expr<'_>, left: /// of what it means for an expression to be "used". fn is_used(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { get_parent_expr(cx, expr).map_or(true, |parent| match parent.kind { - ExprKind::Assign(_, ref rhs, _) | ExprKind::AssignOp(_, _, ref rhs) => { - SpanlessEq::new(cx).eq_expr(rhs, expr) - }, - _ => is_used(cx, parent), - }) + ExprKind::Assign(_, ref rhs, _) | ExprKind::AssignOp(_, _, ref rhs) => SpanlessEq::new(cx).eq_expr(rhs, expr), + _ => is_used(cx, parent), + }) } /// Tests whether an expression is in a macro expansion (e.g., something diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index 7e299dd5fd99f..ab9ea76a83896 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -260,7 +260,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse { detection.some_expr, if detection.wrap_braces { " }" } else { "" }, ), - Applicability::MachineApplicable, + Applicability::MaybeIncorrect, ); } } diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs index 3c93974417356..faef7e724dd05 100644 --- a/clippy_lints/src/returns.rs +++ b/clippy_lints/src/returns.rs @@ -259,15 +259,15 @@ fn is_unit_expr(expr: &ast::Expr) -> bool { fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { - if let Some(rpos) = fn_source.rfind("->") { - #[allow(clippy::cast_possible_truncation)] - ( - ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), - Applicability::MachineApplicable, - ) - } else { - (ty.span, Applicability::MaybeIncorrect) - } + fn_source + .rfind("->") + .map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { + ( + #[allow(clippy::cast_possible_truncation)] + ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), + Applicability::MachineApplicable, + ) + }) } else { (ty.span, Applicability::MaybeIncorrect) }; diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index de94fb8714725..f16db2df3a927 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -166,9 +166,9 @@ fn check_local<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>, bindings: & fn is_binding(cx: &LateContext<'_>, pat_id: HirId) -> bool { let var_ty = cx.tables.node_type_opt(pat_id); var_ty.map_or(false, |var_ty| match var_ty.kind { - ty::Adt(..) => false, - _ => true, - }) + ty::Adt(..) => false, + _ => true, + }) } fn check_pat<'tcx>( diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index df87c1b980254..d6f31a99bb36a 100644 --- a/clippy_lints/src/types.rs +++ b/clippy_lints/src/types.rs @@ -1205,14 +1205,19 @@ fn span_lossless_lint(cx: &LateContext<'_>, expr: &Expr<'_>, op: &Expr<'_>, cast // has parens on the outside, they are no longer needed. let mut applicability = Applicability::MachineApplicable; let opt = snippet_opt(cx, op.span); - let sugg = opt.as_ref().map_or_else(|| { - applicability = Applicability::HasPlaceholders; - ".." - }, |snip| if should_strip_parens(op, snip) { - &snip[1..snip.len() - 1] - } else { - snip.as_str() - }); + let sugg = opt.as_ref().map_or_else( + || { + applicability = Applicability::HasPlaceholders; + ".." + }, + |snip| { + if should_strip_parens(op, snip) { + &snip[1..snip.len() - 1] + } else { + snip.as_str() + } + }, + ); span_lint_and_sugg( cx, diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index eac7ae2358e8c..39a8c02087284 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -167,10 +167,13 @@ impl<'tcx> LateLintPass<'tcx> for UseSelf { if let TyKind::Path(QPath::Resolved(_, ref item_path)) = item_type.kind; then { let parameters = &item_path.segments.last().expect(SEGMENTS_MSG).args; - let should_check = parameters.as_ref().map_or(true, |params| !params.parenthesized && !params.args.iter().any(|arg| match arg { + let should_check = parameters.as_ref().map_or( + true, + |params| !params.parenthesized && !params.args.iter().any(|arg| match arg { GenericArg::Lifetime(_) => true, _ => false, - })); + }) + ); if should_check { let visitor = &mut UseSelfVisitor { diff --git a/clippy_lints/src/utils/attrs.rs b/clippy_lints/src/utils/attrs.rs index 2d72f9c3fe172..4bb4b087c5566 100644 --- a/clippy_lints/src/utils/attrs.rs +++ b/clippy_lints/src/utils/attrs.rs @@ -66,39 +66,44 @@ pub fn get_attr<'a>( let attr_segments = &attr.path.segments; if attr_segments.len() == 2 && attr_segments[0].ident.to_string() == "clippy" { BUILTIN_ATTRIBUTES - .iter() - .find_map(|(builtin_name, deprecation_status)| { - if *builtin_name == attr_segments[1].ident.to_string() { - Some(deprecation_status) - } else { - None - } - }).map_or_else(|| { - sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute"); - false - }, |deprecation_status| { - let mut diag = sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute"); - match *deprecation_status { - DeprecationStatus::Deprecated => { - diag.emit(); - false - }, - DeprecationStatus::Replaced(new_name) => { - diag.span_suggestion( - attr_segments[1].ident.span, - "consider using", - new_name.to_string(), - Applicability::MachineApplicable, - ); - diag.emit(); + .iter() + .find_map(|(builtin_name, deprecation_status)| { + if *builtin_name == attr_segments[1].ident.to_string() { + Some(deprecation_status) + } else { + None + } + }) + .map_or_else( + || { + sess.span_err(attr_segments[1].ident.span, "Usage of unknown attribute"); false }, - DeprecationStatus::None => { - diag.cancel(); - attr_segments[1].ident.to_string() == name + |deprecation_status| { + let mut diag = + sess.struct_span_err(attr_segments[1].ident.span, "Usage of deprecated attribute"); + match *deprecation_status { + DeprecationStatus::Deprecated => { + diag.emit(); + false + }, + DeprecationStatus::Replaced(new_name) => { + diag.span_suggestion( + attr_segments[1].ident.span, + "consider using", + new_name.to_string(), + Applicability::MachineApplicable, + ); + diag.emit(); + false + }, + DeprecationStatus::None => { + diag.cancel(); + attr_segments[1].ident.to_string() == name + }, + } }, - } - }) + ) } else { false } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 6f23e96800684..3a3b79925ff9a 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -601,8 +601,10 @@ pub fn first_line_of_span(cx: &T, span: Span) -> Span { fn first_char_in_first_line(cx: &T, span: Span) -> Option { let line_span = line_span(cx, span); - snippet_opt(cx, line_span).and_then(|snip| snip.find(|c: char| !c.is_whitespace()) - .map(|pos| line_span.lo() + BytePos::from_usize(pos))) + snippet_opt(cx, line_span).and_then(|snip| { + snip.find(|c: char| !c.is_whitespace()) + .map(|pos| line_span.lo() + BytePos::from_usize(pos)) + }) } /// Returns the indentation of the line of a span @@ -723,20 +725,20 @@ pub fn get_enclosing_block<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio .get_enclosing_scope(hir_id) .and_then(|enclosing_id| map.find(enclosing_id)); enclosing_node.and_then(|node| match node { - Node::Block(block) => Some(block), - Node::Item(&Item { - kind: ItemKind::Fn(_, _, eid), - .. - }) - | Node::ImplItem(&ImplItem { - kind: ImplItemKind::Fn(_, eid), - .. - }) => match cx.tcx.hir().body(eid).value.kind { - ExprKind::Block(ref block, _) => Some(block), - _ => None, - }, - _ => None, + Node::Block(block) => Some(block), + Node::Item(&Item { + kind: ItemKind::Fn(_, _, eid), + .. }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Fn(_, eid), + .. + }) => match cx.tcx.hir().body(eid).value.kind { + ExprKind::Block(ref block, _) => Some(block), + _ => None, + }, + _ => None, + }) } /// Returns the base type for HIR references and pointers. diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index 20bea3cbabe64..0ac7714fbeb79 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -492,15 +492,19 @@ fn astbinop2assignop(op: ast::BinOp) -> AssocOp { /// before it on its line. fn indentation(cx: &T, span: Span) -> Option { let lo = cx.sess().source_map().lookup_char_pos(span.lo()); - lo.file.get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */).and_then(|line| if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') { - // We can mix char and byte positions here because we only consider `[ \t]`. - if lo.col == CharPos(pos) { - Some(line[..pos].into()) + lo.file + .get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) + .and_then(|line| { + if let Some((pos, _)) = line.char_indices().find(|&(_, c)| c != ' ' && c != '\t') { + // We can mix char and byte positions here because we only consider `[ \t]`. + if lo.col == CharPos(pos) { + Some(line[..pos].into()) + } else { + None + } } else { None } - } else { - None }) } diff --git a/clippy_lints/src/write.rs b/clippy_lints/src/write.rs index 3b10b7b82a089..063f94582b9d1 100644 --- a/clippy_lints/src/write.rs +++ b/clippy_lints/src/write.rs @@ -297,10 +297,13 @@ impl EarlyLintPass for Write { if let (Some(fmt_str), expr) = self.check_tts(cx, &mac.args.inner_tokens(), true) { if fmt_str.symbol == Symbol::intern("") { let mut applicability = Applicability::MachineApplicable; - let suggestion = expr.map_or_else(|| { - applicability = Applicability::HasPlaceholders; - Cow::Borrowed("v") - }, |e| snippet_with_applicability(cx, e.span, "v", &mut Applicability::MachineApplicable)); + let suggestion = expr.map_or_else( + || { + applicability = Applicability::HasPlaceholders; + Cow::Borrowed("v") + }, + |e| snippet_with_applicability(cx, e.span, "v", &mut Applicability::MachineApplicable), + ); span_lint_and_sugg( cx, From 6e2d55c8db04a80f9c217f2089066b29302f62a9 Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Sat, 27 Jun 2020 16:55:47 -0700 Subject: [PATCH 12/14] Update compile-test to follow new lint --- tests/compile-test.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 99505fc6b29b2..eb6d495acbe20 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -12,19 +12,11 @@ use std::path::{Path, PathBuf}; mod cargo; fn host_lib() -> PathBuf { - if let Some(path) = option_env!("HOST_LIBS") { - PathBuf::from(path) - } else { - cargo::CARGO_TARGET_DIR.join(env!("PROFILE")) - } + option_env!("HOST_LIBS").map_or(cargo::CARGO_TARGET_DIR.join(env!("PROFILE")), PathBuf::from) } fn clippy_driver_path() -> PathBuf { - if let Some(path) = option_env!("CLIPPY_DRIVER_PATH") { - PathBuf::from(path) - } else { - cargo::TARGET_LIB.join("clippy-driver") - } + option_env!("CLIPPY_DRIVER_PATH").map_or(cargo::TARGET_LIB.join("clippy-driver"), PathBuf::from) } // When we'll want to use `extern crate ..` for a dependency that is used From 1c32263176d95ae47928d1955e44a4315ffcea2d Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Wed, 1 Jul 2020 11:41:11 -0700 Subject: [PATCH 13/14] Formatted updates to lints --- clippy_lints/src/minmax.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs index 5eb8398d68e51..2e5f5f10f4b9e 100644 --- a/clippy_lints/src/minmax.rs +++ b/clippy_lints/src/minmax.rs @@ -87,10 +87,12 @@ fn fetch_const<'a>(cx: &LateContext<'_>, args: &'a [Expr<'a>], m: MinMax) -> Opt return None; } constant_simple(cx, cx.tables, &args[0]).map_or_else( - || if let Some(c) = constant_simple(cx, cx.tables(), &args[1]) { - Some((m, c, &args[0])) - } else { - None + || { + if let Some(c) = constant_simple(cx, cx.tables(), &args[1]) { + Some((m, c, &args[0])) + } else { + None + } }, |c| { if constant_simple(cx, cx.tables, &args[1]).is_none() { @@ -99,5 +101,6 @@ fn fetch_const<'a>(cx: &LateContext<'_>, args: &'a [Expr<'a>], m: MinMax) -> Opt } else { None } - }) + }, + ) } From c8f700ea697f74ef8f86891b050c859cf457e3ab Mon Sep 17 00:00:00 2001 From: JarredAllen Date: Fri, 3 Jul 2020 20:28:40 -0700 Subject: [PATCH 14/14] Fixed compile errors --- clippy_lints/src/attrs.rs | 2 +- clippy_lints/src/if_let_mutex.rs | 2 +- clippy_lints/src/minmax.rs | 14 ++++---------- clippy_lints/src/option_if_let_else.rs | 14 +++++++------- clippy_lints/src/shadow.rs | 2 +- 5 files changed, 14 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/attrs.rs b/clippy_lints/src/attrs.rs index c4397560d7db1..d68d0d8ccf58d 100644 --- a/clippy_lints/src/attrs.rs +++ b/clippy_lints/src/attrs.rs @@ -480,7 +480,7 @@ fn is_relevant_trait(cx: &LateContext<'_>, item: &TraitItem<'_>) -> bool { } } -fn is_relevant_block(cx: &LateContext<'_, '_>, tables: &ty::TypeckTables<'_>, block: &Block<'_>) -> bool { +fn is_relevant_block(cx: &LateContext<'_>, tables: &ty::TypeckTables<'_>, block: &Block<'_>) -> bool { block.stmts.first().map_or( block.expr.as_ref().map_or(false, |e| is_relevant_expr(cx, tables, e)), |stmt| match &stmt.kind { diff --git a/clippy_lints/src/if_let_mutex.rs b/clippy_lints/src/if_let_mutex.rs index 5426e14ead573..fbd2eeacc6ef5 100644 --- a/clippy_lints/src/if_let_mutex.rs +++ b/clippy_lints/src/if_let_mutex.rs @@ -136,7 +136,7 @@ impl<'tcx> Visitor<'tcx> for ArmVisitor<'_, 'tcx> { } impl<'tcx, 'l> ArmVisitor<'tcx, 'l> { - fn same_mutex(&self, cx: &LateContext<'_, '_>, op_mutex: &Expr<'_>) -> bool { + fn same_mutex(&self, cx: &LateContext<'_>, op_mutex: &Expr<'_>) -> bool { self.found_mutex .map_or(false, |arm_mutex| SpanlessEq::new(cx).eq_expr(op_mutex, arm_mutex)) } diff --git a/clippy_lints/src/minmax.rs b/clippy_lints/src/minmax.rs index 2e5f5f10f4b9e..c8aa98d348927 100644 --- a/clippy_lints/src/minmax.rs +++ b/clippy_lints/src/minmax.rs @@ -86,18 +86,12 @@ fn fetch_const<'a>(cx: &LateContext<'_>, args: &'a [Expr<'a>], m: MinMax) -> Opt if args.len() != 2 { return None; } - constant_simple(cx, cx.tables, &args[0]).map_or_else( - || { - if let Some(c) = constant_simple(cx, cx.tables(), &args[1]) { - Some((m, c, &args[0])) - } else { - None - } - }, + constant_simple(cx, cx.tables(), &args[0]).map_or_else( + || constant_simple(cx, cx.tables(), &args[1]).map(|c| (m, c, &args[0])), |c| { - if constant_simple(cx, cx.tables, &args[1]).is_none() { + if constant_simple(cx, cx.tables(), &args[1]).is_none() { // otherwise ignore - Some((c, &args[1])) + Some((m, c, &args[1])) } else { None } diff --git a/clippy_lints/src/option_if_let_else.rs b/clippy_lints/src/option_if_let_else.rs index ab9ea76a83896..8dbe58763bfb2 100644 --- a/clippy_lints/src/option_if_let_else.rs +++ b/clippy_lints/src/option_if_let_else.rs @@ -70,9 +70,9 @@ declare_clippy_lint! { 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 { +fn is_result_ok(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool { if let ExprKind::MethodCall(ref path, _, &[ref receiver], _) = &expr.kind { - path.ident.name.to_ident_string() == "ok" && match_type(cx, &cx.tables.expr_ty(&receiver), &paths::RESULT) + path.ident.name.to_ident_string() == "ok" && match_type(cx, &cx.tables().expr_ty(&receiver), &paths::RESULT) } else { false } @@ -157,7 +157,7 @@ fn extract_body_from_arm<'a>(arm: &'a Arm<'a>) -> Option<&'a Expr<'a>> { /// If this is the else body of an if/else expression, then we need to wrap /// it in curcly braces. Otherwise, we don't. -fn should_wrap_in_braces(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool { +fn should_wrap_in_braces(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { utils::get_enclosing_block(cx, expr.hir_id).map_or(false, |parent| { if let Some(Expr { kind: @@ -181,7 +181,7 @@ fn should_wrap_in_braces(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool { }) } -fn format_option_in_sugg(cx: &LateContext<'_, '_>, cond_expr: &Expr<'_>, as_ref: bool, as_mut: bool) -> String { +fn format_option_in_sugg(cx: &LateContext<'_>, cond_expr: &Expr<'_>, as_ref: bool, as_mut: bool) -> String { format!( "{}{}", Sugg::hir(cx, cond_expr, "..").maybe_par(), @@ -198,7 +198,7 @@ fn format_option_in_sugg(cx: &LateContext<'_, '_>, cond_expr: &Expr<'_>, as_ref: /// 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 { +fn detect_option_if_let_else(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { if_chain! { if !utils::in_macro(expr.span); // Don't lint macros, because it behaves weirdly if let ExprKind::Match(cond_expr, arms, MatchSource::IfLetDesugar{contains_else_clause: true}) = &expr.kind; @@ -242,8 +242,8 @@ fn detect_option_if_let_else<'a>(cx: &LateContext<'_, 'a>, expr: &'a Expr<'a>) - } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OptionIfLetElse { - fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { +impl<'a> LateLintPass<'a> for OptionIfLetElse { + fn check_expr(&mut self, cx: &LateContext<'a>, expr: &Expr<'_>) { if let Some(detection) = detect_option_if_let_else(cx, expr) { span_lint_and_sugg( cx, diff --git a/clippy_lints/src/shadow.rs b/clippy_lints/src/shadow.rs index f16db2df3a927..4cdff63f1180a 100644 --- a/clippy_lints/src/shadow.rs +++ b/clippy_lints/src/shadow.rs @@ -164,7 +164,7 @@ fn check_local<'tcx>(cx: &LateContext<'tcx>, local: &'tcx Local<'_>, bindings: & } fn is_binding(cx: &LateContext<'_>, pat_id: HirId) -> bool { - let var_ty = cx.tables.node_type_opt(pat_id); + let var_ty = cx.tables().node_type_opt(pat_id); var_ty.map_or(false, |var_ty| match var_ty.kind { ty::Adt(..) => false, _ => true,