diff --git a/CHANGELOG.md b/CHANGELOG.md index 37d46d349667..28e8ffee16b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6643,6 +6643,7 @@ Released 2018-09-13 [`option_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_map_unwrap_or_else [`option_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_option [`option_unwrap_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_unwrap_used +[`or_else_then_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_else_then_unwrap [`or_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call [`or_then_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#or_then_unwrap [`out_of_bounds_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#out_of_bounds_indexing diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 375d179681da..5889bb490580 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -442,6 +442,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::OPTION_AS_REF_DEREF_INFO, crate::methods::OPTION_FILTER_MAP_INFO, crate::methods::OPTION_MAP_OR_NONE_INFO, + crate::methods::OR_ELSE_THEN_UNWRAP_INFO, crate::methods::OR_FUN_CALL_INFO, crate::methods::OR_THEN_UNWRAP_INFO, crate::methods::PATH_BUF_PUSH_OVERWRITE_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c9066be51c44..bfaece2fa88b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -89,6 +89,7 @@ mod option_as_ref_cloned; mod option_as_ref_deref; mod option_map_or_none; mod option_map_unwrap_or; +mod or_else_then_unwrap; mod or_fun_call; mod or_then_unwrap; mod path_buf_push_overwrite; @@ -918,6 +919,42 @@ declare_clippy_lint! { "using `.chars().next()` to check if a string starts with a char" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `.or_else(…).unwrap()` calls to Options and Results. + /// + /// ### Why is this bad? + /// You should use `.unwrap_or_else(…)` instead for clarity. + /// + /// ### Example + /// ```no_run + /// # let fallback = "fallback"; + /// // Result + /// # type Error = &'static str; + /// # let result: Result, Error> = Err("error"); + /// let value = result.or_else(|_| Ok::, Error>(vec![fallback])).unwrap(); + /// + /// // Option + /// # let option: Option> = None; + /// let value = option.or_else(|| Some(vec![fallback])).unwrap(); + /// ``` + /// Use instead: + /// ```no_run + /// # let fallback = "fallback"; + /// // Result + /// # let result: Result, &str> = Err("error"); + /// let value = result.unwrap_or_else(|_| vec![fallback]); + /// + /// // Option + /// # let option: Option> = None; + /// let value = option.unwrap_or_else(|| vec![fallback]); + /// ``` + #[clippy::version = "1.90.0"] + pub OR_ELSE_THEN_UNWRAP, + complexity, + "checks for `.or_else(…).unwrap()` calls to Options and Results." +} + declare_clippy_lint! { /// ### What it does /// Checks for calls to `.or(foo(..))`, `.unwrap_or(foo(..))`, @@ -4706,6 +4743,7 @@ impl_lint_pass!(Methods => [ RESULT_MAP_OR_INTO_OPTION, OPTION_MAP_OR_NONE, BIND_INSTEAD_OF_MAP, + OR_ELSE_THEN_UNWRAP, OR_FUN_CALL, OR_THEN_UNWRAP, EXPECT_FUN_CALL, @@ -5500,6 +5538,9 @@ impl Methods { Some((sym::or, recv, [or_arg], or_span, _)) => { or_then_unwrap::check(cx, expr, recv, or_arg, or_span); }, + Some((sym::or_else, recv, [or_arg], or_span, _)) => { + or_else_then_unwrap::check(cx, expr, recv, or_arg, or_span); + }, _ => {}, } unnecessary_literal_unwrap::check(cx, expr, recv, name, args); diff --git a/clippy_lints/src/methods/or_else_then_unwrap.rs b/clippy_lints/src/methods/or_else_then_unwrap.rs new file mode 100644 index 000000000000..fa6327dbcf3d --- /dev/null +++ b/clippy_lints/src/methods/or_else_then_unwrap.rs @@ -0,0 +1,66 @@ +use crate::clippy_utils::res::{MaybeDef, MaybeQPath}; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet_with_applicability; +use rustc_errors::Applicability; +use rustc_hir::lang_items::LangItem; +use rustc_hir::{Body, Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::AdtDef; +use rustc_span::{Span, sym}; + +use super::OR_ELSE_THEN_UNWRAP; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + unwrap_expr: &Expr<'_>, + recv: &'tcx Expr<'tcx>, + or_else_arg: &'tcx Expr<'_>, + or_span: Span, +) { + let ty = cx.typeck_results().expr_ty(recv); // get type of x (we later check if it's Option or Result) + let (title, or_else_arg_content) = match ty + .ty_adt_def() + .map(AdtDef::did) + .and_then(|did| cx.tcx.get_diagnostic_name(did)) + { + Some(sym::Option) + if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::OptionSome) => + { + ("found `.or_else(|| Some(…)).unwrap()`", content) + }, + Some(sym::Result) + if let Some(content) = get_content_if_ctor_matches_in_closure(cx, or_else_arg, LangItem::ResultOk) => + { + ("found `.or_else(|| Ok(…)).unwrap()`", content) + }, + // Someone has implemented a struct with .or(...).unwrap() chaining, + // but it's not an Option or a Result, so bail + _ => return, + }; + + let mut applicability = Applicability::MachineApplicable; + let suggestion = format!( + "unwrap_or_else(|| {})", + snippet_with_applicability(cx, or_else_arg_content, "..", &mut applicability) + ); + + let span = unwrap_expr.span.with_lo(or_span.lo()); + span_lint_and_then(cx, OR_ELSE_THEN_UNWRAP, span, title, |diag| { + diag.span_suggestion_verbose(span, "try", suggestion, applicability); + }); +} + +fn get_content_if_ctor_matches_in_closure(cx: &LateContext<'_>, expr: &Expr<'_>, item: LangItem) -> Option { + if let ExprKind::Closure(closure) = expr.kind + && let Body { + params: [], + value: body, + } = cx.tcx.hir_body(closure.body) + && let ExprKind::Call(some_or_ok, [arg]) = body.kind + && some_or_ok.res(cx).ctor_parent(cx).is_lang_item(cx, item) + { + Some(arg.span.source_callsite()) + } else { + None + } +} diff --git a/tests/ui/or_else_then_unwrap.fixed b/tests/ui/or_else_then_unwrap.fixed new file mode 100644 index 000000000000..2218af23d7ce --- /dev/null +++ b/tests/ui/or_else_then_unwrap.fixed @@ -0,0 +1,74 @@ +//@aux-build:proc_macros.rs + +#![warn(clippy::or_then_unwrap)] +#![allow(clippy::map_identity, clippy::let_unit_value, clippy::unnecessary_literal_unwrap)] + +extern crate proc_macros; + +struct SomeStruct; +impl SomeStruct { + fn or_else Option>(self, _: F) -> Self { + self + } + fn unwrap(&self) {} +} + +struct SomeOtherStruct; +impl SomeOtherStruct { + fn or_else(self) -> Self { + self + } + fn unwrap(&self) {} +} + +struct Wrapper { + inner: &'static str, +} +impl Wrapper { + fn new(inner: &'static str) -> Self { + Self { inner } + } +} + +fn main() { + let option: Option = None; + let _ = option.unwrap_or_else(|| Wrapper::new("fallback")); //~ or_else_then_unwrap + + // as part of a method chain + let option: Option = None; + let _ = option + .map(|v| v) + .unwrap_or_else(|| Wrapper::new("fallback")) + .inner + .to_string() + .chars(); + + // Call with macro should preserve the macro call rather than expand it + let option: Option> = None; + let _ = option.unwrap_or_else(|| vec!["fallback"]); // should trigger lint + // + //~^^ or_else_then_unwrap + + // Not Option/Result + let instance = SomeStruct {}; + let _ = instance.or_else(|| Some(SomeStruct {})).unwrap(); // should not trigger lint + + // `or_else` takes no argument + let instance = SomeOtherStruct {}; + let _ = instance.or_else().unwrap(); // should not trigger lint and should not panic + + // None in or + let option: Option = None; + #[allow(clippy::unnecessary_lazy_evaluations)] + let _ = option.or_else(|| None).unwrap(); // should not trigger lint + + // other function between + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).map(|v| v).unwrap(); // should not trigger lint + + // We don't lint external macros + proc_macros::external! { + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); + }; +} diff --git a/tests/ui/or_else_then_unwrap.rs b/tests/ui/or_else_then_unwrap.rs new file mode 100644 index 000000000000..22d6d5807b93 --- /dev/null +++ b/tests/ui/or_else_then_unwrap.rs @@ -0,0 +1,77 @@ +//@aux-build:proc_macros.rs + +#![warn(clippy::or_then_unwrap)] +#![allow(clippy::map_identity, clippy::let_unit_value, clippy::unnecessary_literal_unwrap)] + +extern crate proc_macros; + +struct SomeStruct; +impl SomeStruct { + fn or_else Option>(self, _: F) -> Self { + self + } + fn unwrap(&self) {} +} + +struct SomeOtherStruct; +impl SomeOtherStruct { + fn or_else(self) -> Self { + self + } + fn unwrap(&self) {} +} + +struct Wrapper { + inner: &'static str, +} +impl Wrapper { + fn new(inner: &'static str) -> Self { + Self { inner } + } +} + +fn main() { + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); //~ or_else_then_unwrap + + // as part of a method chain + let option: Option = None; + let _ = option + .map(|v| v) + .or_else(|| Some(Wrapper::new("fallback"))) // should trigger lint + // + //~^^ or_else_then_unwrap + .unwrap() + .inner + .to_string() + .chars(); + + // Call with macro should preserve the macro call rather than expand it + let option: Option> = None; + let _ = option.or_else(|| Some(vec!["fallback"])).unwrap(); // should trigger lint + // + //~^^ or_else_then_unwrap + + // Not Option/Result + let instance = SomeStruct {}; + let _ = instance.or_else(|| Some(SomeStruct {})).unwrap(); // should not trigger lint + + // `or_else` takes no argument + let instance = SomeOtherStruct {}; + let _ = instance.or_else().unwrap(); // should not trigger lint and should not panic + + // None in or + let option: Option = None; + #[allow(clippy::unnecessary_lazy_evaluations)] + let _ = option.or_else(|| None).unwrap(); // should not trigger lint + + // other function between + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).map(|v| v).unwrap(); // should not trigger lint + + // We don't lint external macros + proc_macros::external! { + let option: Option = None; + let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); + }; +} diff --git a/tests/ui/or_else_then_unwrap.stderr b/tests/ui/or_else_then_unwrap.stderr new file mode 100644 index 000000000000..850e4214cb5c --- /dev/null +++ b/tests/ui/or_else_then_unwrap.stderr @@ -0,0 +1,46 @@ +error: found `.or_else(|| Some(…)).unwrap()` + --> tests/ui/or_else_then_unwrap.rs:35:20 + | +LL | let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::or-else-then-unwrap` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::or_else_then_unwrap)]` +help: try + | +LL - let _ = option.or_else(|| Some(Wrapper::new("fallback"))).unwrap(); +LL + let _ = option.unwrap_or_else(|| Wrapper::new("fallback")); + | + +error: found `.or_else(|| Some(…)).unwrap()` + --> tests/ui/or_else_then_unwrap.rs:41:10 + | +LL | .or_else(|| Some(Wrapper::new("fallback"))) // should trigger lint + | __________^ +... | +LL | | .unwrap() + | |_________________^ + | +help: try + | +LL - .or_else(|| Some(Wrapper::new("fallback"))) // should trigger lint +LL - // +LL - +LL - .unwrap() +LL + .unwrap_or_else(|| Wrapper::new("fallback")) + | + +error: found `.or_else(|| Some(…)).unwrap()` + --> tests/ui/or_else_then_unwrap.rs:51:20 + | +LL | let _ = option.or_else(|| Some(vec!["fallback"])).unwrap(); // should trigger lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: try + | +LL - let _ = option.or_else(|| Some(vec!["fallback"])).unwrap(); // should trigger lint +LL + let _ = option.unwrap_or_else(|| vec!["fallback"]); // should trigger lint + | + +error: aborting due to 3 previous errors +