From 5ab43a2d4f6cad52c52f5aaaecaa78a6d460397e Mon Sep 17 00:00:00 2001 From: sjwang05 <63834813+sjwang05@users.noreply.github.com> Date: Thu, 28 Sep 2023 20:30:59 -0700 Subject: [PATCH 1/2] Add manual_option_folding lint --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/manual_option_folding.rs | 98 +++++++++++++++++++++++ tests/ui/manual_option_folding.rs | 31 +++++++ tests/ui/manual_option_folding.stderr | 69 ++++++++++++++++ 6 files changed, 202 insertions(+) create mode 100644 clippy_lints/src/manual_option_folding.rs create mode 100644 tests/ui/manual_option_folding.rs create mode 100644 tests/ui/manual_option_folding.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index abe975fa42b2..21971e1fdfb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5238,6 +5238,7 @@ Released 2018-09-13 [`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or +[`manual_option_folding`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_option_folding [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains [`manual_range_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_patterns [`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5a109fcc2ccd..272045c257fd 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -292,6 +292,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::manual_let_else::MANUAL_LET_ELSE_INFO, crate::manual_main_separator_str::MANUAL_MAIN_SEPARATOR_STR_INFO, crate::manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE_INFO, + crate::manual_option_folding::MANUAL_OPTION_FOLDING_INFO, crate::manual_range_patterns::MANUAL_RANGE_PATTERNS_INFO, crate::manual_rem_euclid::MANUAL_REM_EUCLID_INFO, crate::manual_retain::MANUAL_RETAIN_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1c59b2df853b..cc29a48b3871 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -193,6 +193,7 @@ mod manual_is_ascii_check; mod manual_let_else; mod manual_main_separator_str; mod manual_non_exhaustive; +mod manual_option_folding; mod manual_range_patterns; mod manual_rem_euclid; mod manual_retain; @@ -1069,6 +1070,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter)); store.register_late_pass(|_| Box::new(iter_over_hash_type::IterOverHashType)); store.register_late_pass(|_| Box::new(impl_hash_with_borrow_str_and_bytes::ImplHashWithBorrowStrBytes)); + store.register_late_pass(|_| Box::new(manual_option_folding::ManualOptionFolding::new())); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/manual_option_folding.rs b/clippy_lints/src/manual_option_folding.rs new file mode 100644 index 000000000000..c725adda4633 --- /dev/null +++ b/clippy_lints/src/manual_option_folding.rs @@ -0,0 +1,98 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{sym, Symbol}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for cases where `unwrap_or_else` can be used in lieu of + /// `get_or_insert_with` followed by `unwrap`/`unwrap_unchecked`/`expect`. + /// + /// ### Why is this bad? + /// It is more concise to use `unwrap_or_else`, and using `unwrap_or_else` + /// instead of `unwrap_unchecked` eliminates the need for an `unsafe` + /// block. + /// + /// ### Example + /// ```rust + /// let mut opt: Option = None; + /// opt.get_or_insert(42); + /// let res = unsafe { opt.unwrap_unchecked() }; + /// ``` + /// Use instead: + /// ```rust + /// let opt: Option = None; + /// let res: i32 = opt.unwrap_or(42); + /// ``` + #[clippy::version = "1.74.0"] + pub MANUAL_OPTION_FOLDING, + style, + "manual implementation of `Option::unwrap_or_else`" +} + +impl_lint_pass!(ManualOptionFolding<'_> => [MANUAL_OPTION_FOLDING]); + +pub struct ManualOptionFolding<'tcx> { + get_call: Option<&'tcx hir::Expr<'tcx>>, + recv: Option<&'tcx hir::Expr<'tcx>>, + get_method_name: Option, +} + +impl<'tcx> ManualOptionFolding<'tcx> { + pub fn new() -> Self { + Self { + get_call: None, + recv: None, + get_method_name: None, + } + } +} + +impl<'tcx> LateLintPass<'tcx> for ManualOptionFolding<'tcx> { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) { + if !expr.span.from_expansion() + && let hir::ExprKind::MethodCall(path, recv, ..) = expr.kind + && let ty = cx.typeck_results().expr_ty(recv).peel_refs() + && is_type_diagnostic_item(cx, ty, sym::Option) + { + if path.ident.name == sym!(get_or_insert) + || path.ident.name == sym!(get_or_insert_with) + || path.ident.name == sym!(get_or_insert_default) + { + self.get_call = Some(expr); + self.recv = Some(recv); + self.get_method_name = Some(path.ident.name); + } else if let Some(get_call) = self.get_call + && let Some(get_call_recv) = self.recv + && let Some(get_method_name) = self.get_method_name + && (path.ident.name == sym::unwrap + || path.ident.name == sym!(unwrap_unchecked) + || path.ident.name == sym::expect) + && let hir::ExprKind::Path(hir::QPath::Resolved(_, recv_path)) = recv.kind + && let hir::ExprKind::Path(hir::QPath::Resolved(_, get_call_recv_path)) = get_call_recv.kind + && recv_path.res == get_call_recv_path.res + { + let sugg_method = if get_method_name == sym!(get_or_insert) { + "unwrap_or".to_string() + } else if get_method_name == sym!(get_or_insert_with) { + "unwrap_or_else".to_string() + } else { + "unwrap_or_default".to_string() + }; + + span_lint_and_then( + cx, + MANUAL_OPTION_FOLDING, + expr.span, + &format!("`{}` used after `{get_method_name}`", path.ident.name), + |diag| { + diag.span_note(get_call.span, format!("`{get_method_name}` used here")); + diag.help(format!("try using `{sugg_method}` instead")); + } + ); + } + } + } +} diff --git a/tests/ui/manual_option_folding.rs b/tests/ui/manual_option_folding.rs new file mode 100644 index 000000000000..307269540c52 --- /dev/null +++ b/tests/ui/manual_option_folding.rs @@ -0,0 +1,31 @@ +#![feature(option_get_or_insert_default)] +#![allow(dead_code, clippy::unnecessary_lazy_evaluations, clippy::unnecessary_literal_unwrap)] +#![warn(clippy::manual_option_folding)] + +fn main() { + let mut opt: Option = Some(42); + opt.get_or_insert_with(|| 21); + + let opt: Option = Some(42); + opt.unwrap(); + + let mut opt: Option = Some(42); + opt.get_or_insert_with(|| 21); + let _res: i32 = unsafe { opt.unwrap_unchecked() }; + + let mut opt: Option = Some(42); + opt.get_or_insert_with(|| 21); + let _res: i32 = opt.unwrap(); + + let mut opt: Option = Some(42); + opt.get_or_insert_with(|| 21); + let _res: i32 = opt.expect("msg"); + + let mut opt: Option = Some(42); + opt.get_or_insert(21); + let _res: i32 = opt.unwrap(); + + let mut opt: Option = Some(42); + opt.get_or_insert_default(); + let _res: i32 = opt.unwrap(); +} diff --git a/tests/ui/manual_option_folding.stderr b/tests/ui/manual_option_folding.stderr new file mode 100644 index 000000000000..73e9a995cbd1 --- /dev/null +++ b/tests/ui/manual_option_folding.stderr @@ -0,0 +1,69 @@ +error: `unwrap_unchecked` used after `get_or_insert_with` + --> $DIR/manual_option_folding.rs:14:30 + | +LL | let _res: i32 = unsafe { opt.unwrap_unchecked() }; + | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: `get_or_insert_with` used here + --> $DIR/manual_option_folding.rs:13:5 + | +LL | opt.get_or_insert_with(|| 21); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: try using `unwrap_or_else` instead + = note: `-D clippy::manual-option-folding` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::manual_option_folding)]` + +error: `unwrap` used after `get_or_insert_with` + --> $DIR/manual_option_folding.rs:18:21 + | +LL | let _res: i32 = opt.unwrap(); + | ^^^^^^^^^^^^ + | +note: `get_or_insert_with` used here + --> $DIR/manual_option_folding.rs:17:5 + | +LL | opt.get_or_insert_with(|| 21); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: try using `unwrap_or_else` instead + +error: `expect` used after `get_or_insert_with` + --> $DIR/manual_option_folding.rs:22:21 + | +LL | let _res: i32 = opt.expect("msg"); + | ^^^^^^^^^^^^^^^^^ + | +note: `get_or_insert_with` used here + --> $DIR/manual_option_folding.rs:21:5 + | +LL | opt.get_or_insert_with(|| 21); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: try using `unwrap_or_else` instead + +error: `unwrap` used after `get_or_insert` + --> $DIR/manual_option_folding.rs:26:21 + | +LL | let _res: i32 = opt.unwrap(); + | ^^^^^^^^^^^^ + | +note: `get_or_insert` used here + --> $DIR/manual_option_folding.rs:25:5 + | +LL | opt.get_or_insert(21); + | ^^^^^^^^^^^^^^^^^^^^^ + = help: try using `unwrap_or` instead + +error: `unwrap` used after `get_or_insert_default` + --> $DIR/manual_option_folding.rs:30:21 + | +LL | let _res: i32 = opt.unwrap(); + | ^^^^^^^^^^^^ + | +note: `get_or_insert_default` used here + --> $DIR/manual_option_folding.rs:29:5 + | +LL | opt.get_or_insert_default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: try using `unwrap_or_default` instead + +error: aborting due to 5 previous errors + From 486cac5bd087d5858891547654141856a6ee8305 Mon Sep 17 00:00:00 2001 From: sjwang05 <63834813+sjwang05@users.noreply.github.com> Date: Fri, 29 Sep 2023 12:32:30 -0700 Subject: [PATCH 2/2] Use `take` to access lint fields --- clippy_lints/src/manual_option_folding.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/manual_option_folding.rs b/clippy_lints/src/manual_option_folding.rs index c725adda4633..a78a72d24599 100644 --- a/clippy_lints/src/manual_option_folding.rs +++ b/clippy_lints/src/manual_option_folding.rs @@ -64,9 +64,9 @@ impl<'tcx> LateLintPass<'tcx> for ManualOptionFolding<'tcx> { self.get_call = Some(expr); self.recv = Some(recv); self.get_method_name = Some(path.ident.name); - } else if let Some(get_call) = self.get_call - && let Some(get_call_recv) = self.recv - && let Some(get_method_name) = self.get_method_name + } else if let Some(get_call) = self.get_call.take() + && let Some(get_call_recv) = self.recv.take() + && let Some(get_method_name) = self.get_method_name.take() && (path.ident.name == sym::unwrap || path.ident.name == sym!(unwrap_unchecked) || path.ident.name == sym::expect)