diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f666caf306f..8b8b23877f0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7074,6 +7074,7 @@ Released 2018-09-13 [`unnecessary_struct_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_struct_initialization [`unnecessary_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned [`unnecessary_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap +[`unnecessary_unwrap_unchecked`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_unwrap_unchecked [`unnecessary_wraps`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps [`unneeded_field_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_field_pattern [`unneeded_struct_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#unneeded_struct_pattern diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 87d75234ebc0..5a91ac965427 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -493,6 +493,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::UNNECESSARY_RESULT_MAP_OR_ELSE_INFO, crate::methods::UNNECESSARY_SORT_BY_INFO, crate::methods::UNNECESSARY_TO_OWNED_INFO, + crate::methods::UNNECESSARY_UNWRAP_UNCHECKED_INFO, crate::methods::UNWRAP_OR_DEFAULT_INFO, crate::methods::UNWRAP_USED_INFO, crate::methods::USELESS_ASREF_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 48842c8739c0..bab3cf562a49 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -139,6 +139,7 @@ mod unnecessary_option_map_or_else; mod unnecessary_result_map_or_else; mod unnecessary_sort_by; mod unnecessary_to_owned; +mod unnecessary_unwrap_unchecked; mod unwrap_expect_used; mod useless_asref; mod useless_nonzero_new_unchecked; @@ -4749,6 +4750,29 @@ declare_clippy_lint! { "filtering `std::io::Lines` with `filter_map()`, `flat_map()`, or `flatten()` might cause an infinite loop" } +declare_clippy_lint! { + /// ### What it does + /// Checks for calls to `unwrap_unchecked` when an `_unchecked` variant of the function exists. + /// + /// ### Why is this bad? + /// Calling the non-unchecked variant most likely results in some checking, which is entirely + /// redundant if `unwrap_unchecked` is called directly afterwards, whereas the unchecked + /// variant most likely avoids performing the check completely. + /// + /// ### Example + /// ```rust + /// let s = unsafe { std::str::from_utf8(&[]).unwrap_unchecked() }; + /// ``` + /// Use instead: + /// ```rust + /// let s = unsafe { std::str::from_utf8_unchecked(&[]) }; + /// ``` + #[clippy::version = "1.94.0"] + pub UNNECESSARY_UNWRAP_UNCHECKED, + perf, + "calling `unwrap_unchecked` on a function which has an `_unchecked` variant" +} + #[expect(clippy::struct_excessive_bools)] pub struct Methods { avoid_breaking_exported_api: bool, @@ -4933,6 +4957,7 @@ impl_lint_pass!(Methods => [ REDUNDANT_ITER_CLONED, UNNECESSARY_OPTION_MAP_OR_ELSE, LINES_FILTER_MAP_OK, + UNNECESSARY_UNWRAP_UNCHECKED, ]); /// Extracts a method call name, args, and `Span` of the method name. @@ -5211,7 +5236,11 @@ impl Methods { } unnecessary_literal_unwrap::check(cx, expr, recv, name, args); }, - (sym::expect_err, [_]) | (sym::unwrap_err | sym::unwrap_unchecked | sym::unwrap_err_unchecked, []) => { + (sym::unwrap_unchecked, []) => { + unnecessary_unwrap_unchecked::check(cx, expr, recv, span); + unnecessary_literal_unwrap::check(cx, expr, recv, name, args); + }, + (sym::expect_err, [_]) | (sym::unwrap_err | sym::unwrap_err_unchecked, []) => { unnecessary_literal_unwrap::check(cx, expr, recv, name, args); }, (sym::extend, [arg]) => { diff --git a/clippy_lints/src/methods/unnecessary_unwrap_unchecked.rs b/clippy_lints/src/methods/unnecessary_unwrap_unchecked.rs new file mode 100644 index 000000000000..b84c65e7bd39 --- /dev/null +++ b/clippy_lints/src/methods/unnecessary_unwrap_unchecked.rs @@ -0,0 +1,175 @@ +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::res::MaybeQPath; +use clippy_utils::{is_from_proc_macro, last_path_segment}; +use rustc_hir::def::{DefKind, Namespace, Res}; +use rustc_hir::def_id::DefId; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty::{self, Ty}; +use rustc_span::Span; +use rustc_span::symbol::Ident; + +use super::UNNECESSARY_UNWRAP_UNCHECKED; + +#[derive(Clone, Copy, Debug)] +struct VariantAndIdent { + variant: Variant, + ident: Ident, +} + +impl<'tcx> VariantAndIdent { + fn new(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, recv: &Expr<'_>) -> Option { + let expected_ret_ty = cx.typeck_results().expr_ty(expr); + match recv.kind { + // Construct `Variant::Fn(_)`, if applicable. This is necessary for us to handle + // functions like `std::str::from_utf8_unchecked`. + ExprKind::Call(path, _) + if let ExprKind::Path(qpath) = path.kind + && let parent = cx.tcx.parent(path.res(cx).def_id()) + // Don't use `parent_module`. We only want to lint if its first parent is a `Mod` + && cx.tcx.def_kind(parent) == DefKind::Mod + && let children = parent.as_local().map_or_else( + || cx.tcx.module_children(parent), + // We must use a !query for local modules to prevent an ICE. + |parent| cx.tcx.module_children_local(parent), + ) + && !children.is_empty() + && let Some(unchecked_ident) = unchecked_ident(&last_path_segment(&qpath).ident) + && let Some(unchecked_def_id) = children.iter().find_map(|child| { + if child.ident == unchecked_ident + && let Res::Def(DefKind::Fn, def_id) = child.res + { + Some(def_id) + } else { + None + } + }) + && fn_ret_ty(cx, unchecked_def_id) == expected_ret_ty => + { + Some(Self { + variant: Variant::Fn, + ident: unchecked_ident, + }) + }, + // We unfortunately must handle `A::a(&a)` and `a.a()` separately, this handles the + // former + ExprKind::Call(path, _) + if let ExprKind::Path(qpath) = path.kind + && let parent = cx.tcx.parent(path.res(cx).def_id()) + // Don't use `parent_impl`. We only want to lint if its first parent is an `Impl` + && matches!(cx.tcx.def_kind(parent), DefKind::Impl { .. }) + && let Some(unchecked_ident) = unchecked_ident(&last_path_segment(&qpath).ident) + && let Some(unchecked) = cx.tcx.associated_items(parent).find_by_ident_and_namespace( + cx.tcx, + unchecked_ident, + Namespace::ValueNS, + parent, + ) + && let ty::AssocKind::Fn { has_self, .. } = unchecked.kind + && fn_ret_ty(cx, unchecked.def_id) == expected_ret_ty => + { + Some(Self { + variant: Variant::Assoc(AssocKind::new(has_self)), + ident: unchecked_ident, + }) + }, + // ... And now the latter ^^ + ExprKind::MethodCall(segment, _, _, _) + if let Some(def_id) = cx.typeck_results().type_dependent_def_id(recv.hir_id) + && let parent = cx.tcx.parent(def_id) + // Don't use `parent_impl`. We only want to lint if its first parent is an `Impl` + && matches!(cx.tcx.def_kind(parent), DefKind::Impl { .. }) + && let ident = segment.ident.to_string() + && let Some(unchecked_ident) = unchecked_ident(&ident) + && let Some(unchecked) = cx.tcx.associated_items(parent).find_by_ident_and_namespace( + cx.tcx, + unchecked_ident, + Namespace::ValueNS, + parent, + ) + && fn_ret_ty(cx, unchecked.def_id) == expected_ret_ty => + { + Some(Self { + variant: Variant::Assoc(AssocKind::Method), + ident: unchecked_ident, + }) + }, + _ => None, + } + } + + fn msg(self) -> &'static str { + // Don't use `format!` instead -- it won't be optimized out. + match self.variant { + Variant::Fn => "usage of `unwrap_unchecked` when an `_unchecked` variant of the function exists", + Variant::Assoc(AssocKind::Fn) => { + "usage of `unwrap_unchecked` when an `_unchecked` variant of the associated function exists" + }, + Variant::Assoc(AssocKind::Method) => { + "usage of `unwrap_unchecked` when an `_unchecked` variant of the method exists" + }, + } + } + + fn as_str(self) -> &'static str { + match self.variant { + Variant::Fn => "function", + Variant::Assoc(AssocKind::Fn) => "associated function", + Variant::Assoc(AssocKind::Method) => "method", + } + } +} + +#[derive(Clone, Copy, Debug)] +enum Variant { + /// Free `fn` in a module. `DefId` is the `_unchecked` function. + Fn, + /// Associated item from an `impl`. `DefId` is the `_unchecked` associated item. + Assoc(AssocKind), +} + +fn unchecked_ident(checked_ident: &impl ToString) -> Option { + let checked_ident = checked_ident.to_string(); + // Only add `_unchecked` if it doesn't already end with `_` + (!checked_ident.ends_with('_')).then(|| Ident::from_str(&(checked_ident + "_unchecked"))) +} + +fn fn_ret_ty<'tcx>(cx: &LateContext<'tcx>, def_id: DefId) -> Ty<'tcx> { + cx.tcx.fn_sig(def_id).skip_binder().output().skip_binder() +} + +/// This only exists so the help message shows `associated function` or `method`, depending on +/// whether it has a `self` parameter. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum AssocKind { + /// No `self`: `fn new() -> Self` + Fn, + /// Has `self`: `fn ty<'tcx>(&self) -> Ty<'tcx>` + Method, +} + +impl AssocKind { + fn new(fn_has_self_parameter: bool) -> Self { + if fn_has_self_parameter { Self::Method } else { Self::Fn } + } +} + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, recv: &Expr<'_>, span: Span) { + if !expr.span.from_expansion() + && let Some(variant) = VariantAndIdent::new(cx, expr, recv) + && !is_from_proc_macro(cx, expr) + { + span_lint_and_help( + cx, + UNNECESSARY_UNWRAP_UNCHECKED, + span, + variant.msg(), + None, + format!( + "call the {} `{}` instead, and remove the `unwrap_unchecked` call", + variant.as_str(), + variant.ident, + ), + ); + } +} diff --git a/tests/ui/auxiliary/unnecessary_unwrap_unchecked_helper.rs b/tests/ui/auxiliary/unnecessary_unwrap_unchecked_helper.rs new file mode 100644 index 000000000000..e0074651964e --- /dev/null +++ b/tests/ui/auxiliary/unnecessary_unwrap_unchecked_helper.rs @@ -0,0 +1,9 @@ +#![allow(unused)] + +pub fn lol() -> Option { + Some(0) +} + +pub fn lol_unchecked() -> u32 { + 0 +} diff --git a/tests/ui/unnecessary_unwrap_unchecked.rs b/tests/ui/unnecessary_unwrap_unchecked.rs new file mode 100644 index 000000000000..35c9018936e6 --- /dev/null +++ b/tests/ui/unnecessary_unwrap_unchecked.rs @@ -0,0 +1,75 @@ +//@aux-build:unnecessary_unwrap_unchecked_helper.rs +//@aux-build:proc_macros.rs +#![warn(clippy::unnecessary_unwrap_unchecked)] + +use proc_macros::{external, with_span}; + +use unnecessary_unwrap_unchecked_helper::lol; + +mod b { + pub fn test_fn() -> Option { + Some(0) + } + + pub fn test_fn_unchecked() -> u32 { + 0 + } +} + +fn test_fn() -> Option { + Some(0) +} + +fn test_fn_unchecked() -> u32 { + 0 +} + +struct A; + +impl A { + fn a(&self) -> Option { + Some(0) + } + + fn a_unchecked(&self) -> u32 { + 0 + } + + fn an_assoc_fn() -> Option { + Some(0) + } + + fn an_assoc_fn_unchecked() -> u32 { + 0 + } +} + +fn main() { + let string_slice = unsafe { std::str::from_utf8(&[]).unwrap_unchecked() }; + let a = unsafe { A::a(&A).unwrap_unchecked() }; + //~^ unnecessary_unwrap_unchecked + let a = unsafe { + let a = A; + a.a().unwrap_unchecked() + }; + //~^^ unnecessary_unwrap_unchecked + let an_assoc_fn = unsafe { A::an_assoc_fn().unwrap_unchecked() }; + //~^ unnecessary_unwrap_unchecked + let extern_fn = unsafe { lol().unwrap_unchecked() }; + //~^ unnecessary_unwrap_unchecked + let local_fn = unsafe { b::test_fn().unwrap_unchecked() }; + //~^ unnecessary_unwrap_unchecked + macro_rules! local { + () => {{ + unsafe { ::std::str::from_utf8(&[]).unwrap_unchecked() }; + }}; + } + local!(); + external! { + unsafe { std::str::from_utf8(&[]).unwrap_unchecked() }; + } + with_span! { + span + unsafe { std::str::from_utf8(&[]).unwrap_unchecked() }; + } +} diff --git a/tests/ui/unnecessary_unwrap_unchecked.stderr b/tests/ui/unnecessary_unwrap_unchecked.stderr new file mode 100644 index 000000000000..9abf78144111 --- /dev/null +++ b/tests/ui/unnecessary_unwrap_unchecked.stderr @@ -0,0 +1,44 @@ +error: usage of `unwrap_unchecked` when an `_unchecked` variant of the method exists + --> tests/ui/unnecessary_unwrap_unchecked.rs:49:31 + | +LL | let a = unsafe { A::a(&A).unwrap_unchecked() }; + | ^^^^^^^^^^^^^^^^ + | + = help: call the method `a_unchecked` instead, and remove the `unwrap_unchecked` call + = note: `-D clippy::unnecessary-unwrap-unchecked` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_unwrap_unchecked)]` + +error: usage of `unwrap_unchecked` when an `_unchecked` variant of the method exists + --> tests/ui/unnecessary_unwrap_unchecked.rs:53:15 + | +LL | a.a().unwrap_unchecked() + | ^^^^^^^^^^^^^^^^ + | + = help: call the method `a_unchecked` instead, and remove the `unwrap_unchecked` call + +error: usage of `unwrap_unchecked` when an `_unchecked` variant of the associated function exists + --> tests/ui/unnecessary_unwrap_unchecked.rs:56:49 + | +LL | let an_assoc_fn = unsafe { A::an_assoc_fn().unwrap_unchecked() }; + | ^^^^^^^^^^^^^^^^ + | + = help: call the associated function `an_assoc_fn_unchecked` instead, and remove the `unwrap_unchecked` call + +error: usage of `unwrap_unchecked` when an `_unchecked` variant of the function exists + --> tests/ui/unnecessary_unwrap_unchecked.rs:58:36 + | +LL | let extern_fn = unsafe { lol().unwrap_unchecked() }; + | ^^^^^^^^^^^^^^^^ + | + = help: call the function `lol_unchecked` instead, and remove the `unwrap_unchecked` call + +error: usage of `unwrap_unchecked` when an `_unchecked` variant of the function exists + --> tests/ui/unnecessary_unwrap_unchecked.rs:60:42 + | +LL | let local_fn = unsafe { b::test_fn().unwrap_unchecked() }; + | ^^^^^^^^^^^^^^^^ + | + = help: call the function `test_fn_unchecked` instead, and remove the `unwrap_unchecked` call + +error: aborting due to 5 previous errors +