diff --git a/CHANGELOG.md b/CHANGELOG.md index 858547c84c18..d4096ca89df8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3473,6 +3473,7 @@ Released 2018-09-13 [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes [`needless_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_match [`needless_option_as_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_as_deref +[`needless_option_take`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_take [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark [`needless_range_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_range_loop diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 52c440de9dd4..84e3176c0030 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -183,6 +183,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(methods::MAP_FLATTEN), LintId::of(methods::MAP_IDENTITY), LintId::of(methods::NEEDLESS_OPTION_AS_DEREF), + LintId::of(methods::NEEDLESS_OPTION_TAKE), LintId::of(methods::NEEDLESS_SPLITN), LintId::of(methods::NEW_RET_NO_SELF), LintId::of(methods::OK_EXPECT), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index 10369a855ae6..2f6ebd445ebd 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -45,6 +45,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(methods::MAP_FLATTEN), LintId::of(methods::MAP_IDENTITY), LintId::of(methods::NEEDLESS_OPTION_AS_DEREF), + LintId::of(methods::NEEDLESS_OPTION_TAKE), LintId::of(methods::NEEDLESS_SPLITN), LintId::of(methods::OPTION_AS_REF_DEREF), LintId::of(methods::OPTION_FILTER_MAP), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index cfaa6871b50b..aa9435f3b59a 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -322,6 +322,7 @@ store.register_lints(&[ methods::MAP_IDENTITY, methods::MAP_UNWRAP_OR, methods::NEEDLESS_OPTION_AS_DEREF, + methods::NEEDLESS_OPTION_TAKE, methods::NEEDLESS_SPLITN, methods::NEW_RET_NO_SELF, methods::OK_EXPECT, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 82f4769a6997..48fa30ba9295 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -43,6 +43,7 @@ mod map_flatten; mod map_identity; mod map_unwrap_or; mod needless_option_as_deref; +mod needless_option_take; mod ok_expect; mod option_as_ref_deref; mod option_map_or_none; @@ -2162,6 +2163,26 @@ declare_clippy_lint! { "use of `char::is_digit(..)` with literal radix of 10 or 16" } +declare_clippy_lint! { + /// + /// ### Why is this bad? + /// + /// ### Example + /// ```rust + /// let x = Some(3); + /// x.as_ref().take(); + /// ``` + /// Use instead: + /// ```rust + /// let x = Some(3); + /// x.as_ref(); + /// ``` + #[clippy::version = "1.61.0"] + pub NEEDLESS_OPTION_TAKE, + complexity, + "using `.as_ref().take()` on a temporary value" +} + pub struct Methods { avoid_breaking_exported_api: bool, msrv: Option, @@ -2251,6 +2272,7 @@ impl_lint_pass!(Methods => [ ERR_EXPECT, NEEDLESS_OPTION_AS_DEREF, IS_DIGIT_ASCII_RADIX, + NEEDLESS_OPTION_TAKE, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -2623,6 +2645,7 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio } } }, + ("take", []) => needless_option_take::check(cx, expr, recv), ("to_os_string" | "to_owned" | "to_path_buf" | "to_vec", []) => { implicit_clone::check(cx, name, expr, recv); }, diff --git a/clippy_lints/src/methods/needless_option_take.rs b/clippy_lints/src/methods/needless_option_take.rs new file mode 100644 index 000000000000..829c118d2916 --- /dev/null +++ b/clippy_lints/src/methods/needless_option_take.rs @@ -0,0 +1,41 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::match_def_path; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_errors::Applicability; +use rustc_hir::Expr; +use rustc_lint::LateContext; +use rustc_span::sym; + +use super::NEEDLESS_OPTION_TAKE; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, recv: &'tcx Expr<'_>) { + // Checks if expression type is equal to sym::Option and if the expr is not a syntactic place + if !recv.is_syntactic_place_expr() && is_expr_option(cx, recv) && has_expr_as_ref_path(cx, recv) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + NEEDLESS_OPTION_TAKE, + expr.span, + "called `Option::take()` on a temporary value", + "try", + format!( + "{}", + snippet_with_applicability(cx, recv.span, "..", &mut applicability) + ), + applicability, + ); + } +} + +fn is_expr_option(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + let expr_type = cx.typeck_results().expr_ty(expr); + is_type_diagnostic_item(cx, expr_type, sym::Option) +} + +fn has_expr_as_ref_path(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if let Some(ref_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) { + return match_def_path(cx, ref_id, &["core", "option", "Option", "as_ref"]); + } + false +} diff --git a/tests/ui/needless_option_take.fixed b/tests/ui/needless_option_take.fixed new file mode 100644 index 000000000000..29691e81666f --- /dev/null +++ b/tests/ui/needless_option_take.fixed @@ -0,0 +1,15 @@ +// run-rustfix + +fn main() { + println!("Testing non erroneous option_take_on_temporary"); + let mut option = Some(1); + let _ = Box::new(move || option.take().unwrap()); + + println!("Testing non erroneous option_take_on_temporary"); + let x = Some(3); + x.as_ref(); + + println!("Testing erroneous option_take_on_temporary"); + let x = Some(3); + x.as_ref(); +} diff --git a/tests/ui/needless_option_take.rs b/tests/ui/needless_option_take.rs new file mode 100644 index 000000000000..9f4109eb4635 --- /dev/null +++ b/tests/ui/needless_option_take.rs @@ -0,0 +1,15 @@ +// run-rustfix + +fn main() { + println!("Testing non erroneous option_take_on_temporary"); + let mut option = Some(1); + let _ = Box::new(move || option.take().unwrap()); + + println!("Testing non erroneous option_take_on_temporary"); + let x = Some(3); + x.as_ref(); + + println!("Testing erroneous option_take_on_temporary"); + let x = Some(3); + x.as_ref().take(); +} diff --git a/tests/ui/needless_option_take.stderr b/tests/ui/needless_option_take.stderr new file mode 100644 index 000000000000..cb3bf015b369 --- /dev/null +++ b/tests/ui/needless_option_take.stderr @@ -0,0 +1,10 @@ +error: called `Option::take()` on a temporary value + --> $DIR/needless_option_take.rs:14:5 + | +LL | x.as_ref().take(); + | ^^^^^^^^^^^^^^^^^ help: try: `x.as_ref()` + | + = note: `-D clippy::needless-option-take` implied by `-D warnings` + +error: aborting due to previous error + diff --git a/tests/ui/option_take_on_temporary.fixed b/tests/ui/option_take_on_temporary.fixed new file mode 100644 index 000000000000..29691e81666f --- /dev/null +++ b/tests/ui/option_take_on_temporary.fixed @@ -0,0 +1,15 @@ +// run-rustfix + +fn main() { + println!("Testing non erroneous option_take_on_temporary"); + let mut option = Some(1); + let _ = Box::new(move || option.take().unwrap()); + + println!("Testing non erroneous option_take_on_temporary"); + let x = Some(3); + x.as_ref(); + + println!("Testing erroneous option_take_on_temporary"); + let x = Some(3); + x.as_ref(); +}