New lint: deref_method_call_chain

CHANGELOG.md

@@ -6845,6 +6845,7 @@ Released 2018-09-13
 [`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
 [`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof
 [`deref_by_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_by_slicing
+[`deref_method_call_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_method_call_chain
 [`derivable_impls`]: https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
 [`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
 [`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord

clippy_lints/src/declared_lints.rs

@@ -376,6 +376,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::methods::CLONED_INSTEAD_OF_COPIED_INFO,
     crate::methods::COLLAPSIBLE_STR_REPLACE_INFO,
     crate::methods::CONST_IS_EMPTY_INFO,
+    crate::methods::DEREF_METHOD_CALL_CHAIN_INFO,
     crate::methods::DOUBLE_ENDED_ITERATOR_LAST_INFO,
     crate::methods::DRAIN_COLLECT_INFO,
     crate::methods::ERR_EXPECT_INFO,

clippy_lints/src/methods/deref_method_call_chain.rs

@@ -0,0 +1,128 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::sym;
+use clippy_utils::ty::implements_trait;
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+use rustc_span::{Span, Symbol};
+
+use super::{DEREF_METHOD_CALL_CHAIN, method_call};
+
+/// Checks whether the receiver of `outer_name` is a redundant deref-like conversion call
+/// (e.g. `Vec::as_slice`, `String::as_str`) which can be removed because method resolution
+/// would find the same method on the original value through deref coercion.
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    outer_name: Symbol,
+    outer_span: Span,
+    recv: &'tcx Expr<'tcx>,
+) {
+    let Some((inner_name, inner_recv, [], inner_span, _)) = method_call(recv) else {
+        return;
+    };
+    if !matches!(
+        inner_name,
+        sym::as_c_str
+            | sym::as_mut_slice
+            | sym::as_mut_str
+            | sym::as_os_str
+            | sym::as_path
+            | sym::as_slice
+            | sym::as_str
+    ) {
+        return;
+    }
+
+    let Some(inner_did) = cx.typeck_results().type_dependent_def_id(recv.hir_id) else {
+        return;
+    };
+    let inner_diag = cx.tcx.get_diagnostic_name(inner_did);
+    let is_std_conversion = matches!(
+        inner_diag,
+        Some(
+            sym::cstring_as_c_str
+                | sym::os_string_as_os_str
+                | sym::pathbuf_as_path
+                | sym::string_as_mut_str
+                | sym::string_as_str
+                | sym::vec_as_mut_slice
+                | sym::vec_as_slice
+        )
+    );
+
+    // `String::as_str()` followed by these methods is covered by `REDUNDANT_AS_STR`
+    if inner_diag == Some(sym::string_as_str) && matches!(outer_name, sym::as_bytes | sym::is_empty) {
+        return;
+    }
+
+    // The conversion must be deref-equivalent: it returns a reference to exactly
+    // `<RecvTy as Deref>::Target`, so removing it lets auto-deref do the same work
+    let recv_base_ty = cx.typeck_results().expr_ty(inner_recv).peel_refs();
+    let ty::Ref(_, target_ty, mutability) = *cx.typeck_results().expr_ty(recv).kind() else {
+        return;
+    };
+    let Some(deref_trait_id) = cx.tcx.get_diagnostic_item(sym::Deref) else {
+        return;
+    };
+    if !implements_trait(cx, recv_base_ty, deref_trait_id, &[])
+        || cx.get_associated_type(recv_base_ty, deref_trait_id, sym::Target) != Some(target_ty)
+    {
+        return;
+    }
+    // `as_mut_*`-style conversions return `&mut Target`, which means the outer method needs
+    // `&mut self`. Reaching that mutable reference via auto-deref also requires `DerefMut`,
+    // even though the redundant conversion itself only needed `Deref`
+    if mutability.is_mut()
+        && !cx
+            .tcx
+            .lang_items()
+            .deref_mut_trait()
+            .is_some_and(|deref_mut_trait_id| implements_trait(cx, recv_base_ty, deref_mut_trait_id, &[]))
+    {
+        return;
+    }
+
+    // The following method must be an inherent method of the deref target. Trait methods
+    // could resolve to a different impl on the original receiver (e.g. `IntoIterator` on
+    // `Vec<T>` vs `&[T]`), changing semantics
+    let Some(outer_did) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else {
+        return;
+    };
+    let Some(impl_id) = cx.tcx.inherent_impl_of_assoc(outer_did) else {
+        return;
+    };
+    let outer_args = cx.typeck_results().node_args(expr.hir_id);
+    let impl_args = outer_args.truncate_to(cx.tcx, cx.tcx.generics_of(impl_id));
+    if cx.tcx.type_of(impl_id).instantiate(cx.tcx, impl_args).skip_norm_wip() != target_ty {
+        return;
+    }
+
+    // For user-defined conversions, a same-named inherent method on the receiver type would
+    // shadow the target's method. Std receivers (`Vec`, `String`, ...) only shadow with
+    // delegating methods (`Vec::len`, ...), which are exactly the idiomatic suggestion
+    if !is_std_conversion
+        && let Some(adt) = recv_base_ty.ty_adt_def()
+        && cx
+            .tcx
+            .inherent_impls(adt.did())
+            .iter()
+            .flat_map(|&impl_id| cx.tcx.associated_items(impl_id).filter_by_name_unhygienic(outer_name))
+            .any(ty::AssocItem::is_method)
+    {
+        return;
+    }
+
+    let mut applicability = Applicability::MachineApplicable;
+    span_lint_and_sugg(
+        cx,
+        DEREF_METHOD_CALL_CHAIN,
+        inner_span.to(outer_span),
+        format!("this `{inner_name}` call is redundant as `{outer_name}` can be called on the receiver directly"),
+        "call the method directly",
+        snippet_with_applicability(cx, outer_span, "..", &mut applicability).into_owned(),
+        applicability,
+    );
+}

clippy_lints/src/methods/mod.rs

@@ -16,6 +16,7 @@ mod clone_on_copy;
 mod clone_on_ref_ptr;
 mod cloned_instead_of_copied;
 mod collapsible_str_replace;
+mod deref_method_call_chain;
 mod double_ended_iterator_last;
 mod drain_collect;
 mod err_expect;
@@ -540,6 +541,39 @@ declare_clippy_lint! {
     "is_empty() called on strings known at compile time"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for method calls whose receiver is a redundant deref-like conversion call
+    /// such as `as_slice`, `as_str`, or `as_path`, when the method could be called on the
+    /// original value directly through deref coercion.
+    ///
+    /// ### Why is this bad?
+    /// The conversion call adds noise without changing behavior: method resolution
+    /// automatically dereferences the receiver, so the same method is called either way.
+    ///
+    /// ### Known problems
+    /// Receivers behind more than one level of deref (e.g. `Box<Vec<T>>`) are not detected.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let vec = vec![1, 2, 3];
+    /// let string = String::from("hello world");
+    /// let _ = vec.as_slice().first();
+    /// let _ = string.as_str().find('l');
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let vec = vec![1, 2, 3];
+    /// let string = String::from("hello world");
+    /// let _ = vec.first();
+    /// let _ = string.find('l');
+    /// ```
+    #[clippy::version = "1.99.0"]
+    pub DEREF_METHOD_CALL_CHAIN,
+    complexity,
+    "redundant deref-conversion call preceding a method call"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     ///
@@ -4936,6 +4970,7 @@ impl_lint_pass!(Methods => [
     CLONE_ON_REF_PTR,
     COLLAPSIBLE_STR_REPLACE,
     CONST_IS_EMPTY,
+    DEREF_METHOD_CALL_CHAIN,
     DOUBLE_ENDED_ITERATOR_LAST,
     DRAIN_COLLECT,
     ERR_EXPECT,
@@ -5268,6 +5303,7 @@ impl Methods {
     fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         // Handle method calls whose receiver and arguments may not come from expansion
         if let Some((name, recv, args, span, call_span)) = method_call(expr) {
+            deref_method_call_chain::check(cx, expr, name, span, recv);
             match (name, args) {
                 (sym::add | sym::sub | sym::wrapping_add | sym::wrapping_sub, [_arg]) => {
                     zst_offset::check(cx, expr, recv);

clippy_utils/src/sym.rs

@@ -135,11 +135,16 @@ generate! {
     applicability,
     arg,
     as_bytes,
+    as_c_str,
     as_deref,
     as_deref_mut,
     as_mut,
+    as_mut_slice,
+    as_mut_str,
+    as_os_str,
     as_path,
     as_ptr,
+    as_slice,
     as_str,
     assert_failed,
     author,

lintcheck/src/main.rs

@@ -472,13 +472,12 @@ fn get_perf_data_filename(source_path: &Path) -> String {
             .filter_map(Result::ok)
             .filter(|path| {
                 path.file_name()
-                    .as_os_str()
                     .to_string_lossy() // We don't care about data loss, as we're checking for equality
                     .starts_with("perf.data")
             })
             .for_each(|path| {
                 let file_name = path.file_name();
-                let file_name = file_name.as_os_str().to_str().unwrap().split('.').next_back().unwrap();
+                let file_name = file_name.to_str().unwrap().split('.').next_back().unwrap();
                 if let Ok(parsed_file_name) = file_name.parse::<usize>()
                     && parsed_file_name >= max_number
                 {

tests/ui/deref_method_call_chain.fixed

@@ -0,0 +1,148 @@
+#![warn(clippy::deref_method_call_chain)]
+#![allow(clippy::needless_borrow, clippy::redundant_as_str, clippy::into_iter_on_ref)]
+
+mod issue2094 {
+    use std::ffi::{CString, OsString};
+    use std::ops::Deref;
+    use std::path::{Path, PathBuf};
+
+    struct DerefWrapper(Vec<u8>);
+
+    impl Deref for DerefWrapper {
+        type Target = [u8];
+        fn deref(&self) -> &[u8] {
+            &self.0
+        }
+    }
+
+    impl DerefWrapper {
+        fn as_slice(&self) -> &[u8] {
+            &self.0
+        }
+    }
+
+    struct Shadowing(Vec<u8>);
+
+    impl Deref for Shadowing {
+        type Target = [u8];
+        fn deref(&self) -> &[u8] {
+            &self.0
+        }
+    }
+
+    impl Shadowing {
+        fn as_slice(&self) -> &[u8] {
+            &self.0
+        }
+
+        fn first(&self) -> u32 {
+            42
+        }
+    }
+
+    struct NotDeref(String);
+
+    impl NotDeref {
+        fn as_str(&self) -> &str {
+            &self.0
+        }
+    }
+
+    struct DerefOnly(Vec<u8>);
+
+    impl Deref for DerefOnly {
+        type Target = [u8];
+        fn deref(&self) -> &[u8] {
+            &self.0
+        }
+    }
+
+    impl DerefOnly {
+        fn as_mut_slice(&mut self) -> &mut [u8] {
+            &mut self.0
+        }
+    }
+
+    macro_rules! in_macro {
+        ($v:expr) => {
+            $v.as_slice().first()
+        };
+    }
+
+    fn check() {
+        let mut vec = vec![1, 2, 3];
+        let mut string = String::from("hello world");
+        let pathbuf = PathBuf::from("path");
+        let os_string = OsString::from("hello");
+        let c_string = CString::new("hello").unwrap();
+
+        // The conversion is redundant: the method exists on the deref target and
+        // is found on the original receiver through deref coercion
+        let _ = vec.first();
+        //~^ deref_method_call_chain
+        let _ = vec.len();
+        //~^ deref_method_call_chain
+        vec.sort_unstable();
+        //~^ deref_method_call_chain
+        let _ = string.find('l');
+        //~^ deref_method_call_chain
+        let _ = string.parse::<i32>();
+        //~^ deref_method_call_chain
+        string.make_ascii_uppercase();
+        //~^ deref_method_call_chain
+        let _ = pathbuf.exists();
+        //~^ deref_method_call_chain
+        let _ = os_string.len();
+        //~^ deref_method_call_chain
+        let _ = c_string.to_bytes();
+        //~^ deref_method_call_chain
+        let _ = (&vec).first();
+        //~^ deref_method_call_chain
+        let vec_ref: &Vec<i32> = &vec;
+        let _ = vec_ref.first();
+        //~^ deref_method_call_chain
+        let _ = vec.first().copied();
+        //~^ deref_method_call_chain
+
+        // User types with a deref-equivalent conversion are detected too
+        let wrapper = DerefWrapper(vec![1, 2, 3]);
+        let _ = wrapper.first();
+        //~^ deref_method_call_chain
+
+        // Trait methods could resolve to a different impl on the original receiver
+        let _ = vec.as_slice().into_iter();
+        let _ = string.as_str().to_owned();
+        let _ = string.as_str().to_string();
+
+        // Covered by `redundant_as_str`
+        let _ = string.as_str().is_empty();
+        let _ = string.as_str().as_bytes();
+
+        // Arrays don't implement `Deref`
+        let _ = [1, 2, 3].as_slice().first();
+
+        // `Path` doesn't implement `Deref`
+        let path: &Path = pathbuf.as_path();
+        let _ = path.as_os_str().len();
+
+        // Not a deref-equivalent conversion (receiver doesn't implement `Deref`)
+        let not_deref = NotDeref(String::from("hi"));
+        let _ = not_deref.as_str().len();
+
+        // `as_mut_slice` needs `DerefMut` to be removable, but this type only implements `Deref`
+        let mut deref_only = DerefOnly(vec![1, 2, 3]);
+        deref_only.as_mut_slice().sort_unstable();
+
+        // The receiver type has its own `first`, which would shadow `<[u8]>::first`
+        let shadowing = Shadowing(vec![1, 2, 3]);
+        let _ = shadowing.as_slice().first();
+
+        // No method follows the conversion
+        let _ = vec.as_slice();
+
+        // Not linted inside macro expansions
+        let _ = in_macro!(vec);
+    }
+}
+
+fn main() {}