From ed9ccf66e9b61406176d57499e91c3bf773e95da Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Thu, 20 Jun 2024 00:09:31 +0200 Subject: [PATCH] [`unnecessary_to_owned`]: catch to_owned from byte slice to string --- .../src/methods/unnecessary_to_owned.rs | 70 ++++++++++++++++++- clippy_utils/src/paths.rs | 1 + tests/ui/unnecessary_to_owned.fixed | 19 +++++ tests/ui/unnecessary_to_owned.rs | 19 +++++ tests/ui/unnecessary_to_owned.stderr | 60 ++++++++++++---- 5 files changed, 154 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index ae9aa83efd68..5d899415d772 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -1,13 +1,15 @@ use super::implicit_clone::is_clone_like; use super::unnecessary_iter_cloned::{self, is_into_iter}; use clippy_config::msrvs::{self, Msrv}; -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_opt; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::source::{snippet, snippet_opt}; use clippy_utils::ty::{ get_iterator_item_ty, implements_trait, is_copy, is_type_diagnostic_item, is_type_lang_item, peel_mid_ty_refs, }; use clippy_utils::visitors::find_all_ret_expressions; -use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item, return_ty}; +use clippy_utils::{ + fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item, match_def_path, paths, return_ty, +}; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::DefId; @@ -52,6 +54,9 @@ pub fn check<'tcx>( if check_into_iter_call_arg(cx, expr, method_name, receiver, msrv) { return; } + if check_string_from_utf8(cx, expr, receiver) { + return; + } check_other_call_arg(cx, expr, method_name, receiver); } } else { @@ -240,6 +245,65 @@ fn check_into_iter_call_arg( false } +/// Checks for `&String::from_utf8(bytes.{to_vec,to_owned,...}()).unwrap()` coercing to `&str`, +/// which can be written as just `std::str::from_utf8(bytes).unwrap()`. +fn check_string_from_utf8<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, receiver: &'tcx Expr<'tcx>) -> bool { + if let Some((call, arg)) = skip_addr_of_ancestors(cx, expr) + && !arg.span.from_expansion() + && let ExprKind::Call(callee, _) = call.kind + && fn_def_id(cx, call).is_some_and(|did| match_def_path(cx, did, &paths::STRING_FROM_UTF8)) + && let Some(unwrap_call) = get_parent_expr(cx, call) + && let ExprKind::MethodCall(unwrap_method_name, ..) = unwrap_call.kind + && matches!(unwrap_method_name.ident.name, sym::unwrap | sym::expect) + && let Some(ref_string) = get_parent_expr(cx, unwrap_call) + && let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) = ref_string.kind + && let adjusted_ty = cx.typeck_results().expr_ty_adjusted(ref_string) + // `&...` creates a `&String`, so only actually lint if this coerces to a `&str` + && matches!(adjusted_ty.kind(), ty::Ref(_, ty, _) if ty.is_str()) + { + span_lint_and_then( + cx, + UNNECESSARY_TO_OWNED, + ref_string.span, + "allocating a new `String` only to create a temporary `&str` from it", + |diag| { + let arg_suggestion = format!( + "{borrow}{recv_snippet}", + recv_snippet = snippet(cx, receiver.span.source_callsite(), ".."), + borrow = if cx.typeck_results().expr_ty(receiver).is_ref() { + "" + } else { + // If not already a reference, prefix with a borrow so that it can coerce to one + "&" + } + ); + + diag.multipart_suggestion( + "convert from `&[u8]` to `&str` directly", + vec![ + // `&String::from_utf8(bytes.to_vec()).unwrap()` + // ^^^^^^^^^^^^^^^^^ + (callee.span, "core::str::from_utf8".into()), + // `&String::from_utf8(bytes.to_vec()).unwrap()` + // ^ + ( + ref_string.span.shrink_to_lo().to(unwrap_call.span.shrink_to_lo()), + String::new(), + ), + // `&String::from_utf8(bytes.to_vec()).unwrap()` + // ^^^^^^^^^^^^^^ + (arg.span, arg_suggestion), + ], + Applicability::MachineApplicable, + ); + }, + ); + true + } else { + false + } +} + /// Checks whether `expr` is an argument in an `into_iter` call and, if so, determines whether its /// call of a `to_owned`-like function is unnecessary. fn check_split_call_arg(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: Symbol, receiver: &Expr<'_>) -> bool { diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 9bf068ee3cde..bf72194f370c 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -88,6 +88,7 @@ pub const SYMBOL_INTERN: [&str; 4] = ["rustc_span", "symbol", "Symbol", "intern" pub const SYMBOL_TO_IDENT_STRING: [&str; 4] = ["rustc_span", "symbol", "Symbol", "to_ident_string"]; pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"]; pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"]; +pub const STRING_FROM_UTF8: [&str; 4] = ["alloc", "string", "String", "from_utf8"]; #[expect(clippy::invalid_paths)] // internal lints do not know about all external crates pub const TOKIO_FILE_OPTIONS: [&str; 5] = ["tokio", "fs", "file", "File", "options"]; #[expect(clippy::invalid_paths)] // internal lints do not know about all external crates diff --git a/tests/ui/unnecessary_to_owned.fixed b/tests/ui/unnecessary_to_owned.fixed index 1afa5ab54c46..fdcac8fb08dc 100644 --- a/tests/ui/unnecessary_to_owned.fixed +++ b/tests/ui/unnecessary_to_owned.fixed @@ -157,6 +157,25 @@ fn main() { require_path(&std::path::PathBuf::from("x")); require_str(&String::from("x")); require_slice(&[String::from("x")]); + + let slice = [0u8; 1024]; + let _ref_str: &str = core::str::from_utf8(&slice).expect("not UTF-8"); + let _ref_str: &str = core::str::from_utf8(b"foo").unwrap(); + let _ref_str: &str = core::str::from_utf8(b"foo".as_slice()).unwrap(); + // Expression is of type `&String`, can't suggest `str::from_utf8` here + let _ref_string = &String::from_utf8(b"foo".to_vec()).unwrap(); + macro_rules! arg_from_macro { + () => { + b"foo".to_vec() + }; + } + macro_rules! string_from_utf8_from_macro { + () => { + &String::from_utf8(b"foo".to_vec()).unwrap() + }; + } + let _ref_str: &str = &String::from_utf8(arg_from_macro!()).unwrap(); + let _ref_str: &str = string_from_utf8_from_macro!(); } fn require_c_str(_: &CStr) {} diff --git a/tests/ui/unnecessary_to_owned.rs b/tests/ui/unnecessary_to_owned.rs index aa88dde43bf6..10a9727a9a79 100644 --- a/tests/ui/unnecessary_to_owned.rs +++ b/tests/ui/unnecessary_to_owned.rs @@ -157,6 +157,25 @@ fn main() { require_path(&std::path::PathBuf::from("x").to_path_buf()); require_str(&String::from("x").to_string()); require_slice(&[String::from("x")].to_owned()); + + let slice = [0u8; 1024]; + let _ref_str: &str = &String::from_utf8(slice.to_vec()).expect("not UTF-8"); + let _ref_str: &str = &String::from_utf8(b"foo".to_vec()).unwrap(); + let _ref_str: &str = &String::from_utf8(b"foo".as_slice().to_owned()).unwrap(); + // Expression is of type `&String`, can't suggest `str::from_utf8` here + let _ref_string = &String::from_utf8(b"foo".to_vec()).unwrap(); + macro_rules! arg_from_macro { + () => { + b"foo".to_vec() + }; + } + macro_rules! string_from_utf8_from_macro { + () => { + &String::from_utf8(b"foo".to_vec()).unwrap() + }; + } + let _ref_str: &str = &String::from_utf8(arg_from_macro!()).unwrap(); + let _ref_str: &str = string_from_utf8_from_macro!(); } fn require_c_str(_: &CStr) {} diff --git a/tests/ui/unnecessary_to_owned.stderr b/tests/ui/unnecessary_to_owned.stderr index 2829f3cd6e98..511b4ae119f8 100644 --- a/tests/ui/unnecessary_to_owned.stderr +++ b/tests/ui/unnecessary_to_owned.stderr @@ -477,8 +477,44 @@ error: unnecessary use of `to_owned` LL | let _ = IntoIterator::into_iter([std::path::PathBuf::new()][..].to_owned()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[std::path::PathBuf::new()][..].iter().cloned()` +error: allocating a new `String` only to create a temporary `&str` from it + --> tests/ui/unnecessary_to_owned.rs:162:26 + | +LL | let _ref_str: &str = &String::from_utf8(slice.to_vec()).expect("not UTF-8"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: convert from `&[u8]` to `&str` directly + | +LL - let _ref_str: &str = &String::from_utf8(slice.to_vec()).expect("not UTF-8"); +LL + let _ref_str: &str = core::str::from_utf8(&slice).expect("not UTF-8"); + | + +error: allocating a new `String` only to create a temporary `&str` from it + --> tests/ui/unnecessary_to_owned.rs:163:26 + | +LL | let _ref_str: &str = &String::from_utf8(b"foo".to_vec()).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: convert from `&[u8]` to `&str` directly + | +LL - let _ref_str: &str = &String::from_utf8(b"foo".to_vec()).unwrap(); +LL + let _ref_str: &str = core::str::from_utf8(b"foo").unwrap(); + | + +error: allocating a new `String` only to create a temporary `&str` from it + --> tests/ui/unnecessary_to_owned.rs:164:26 + | +LL | let _ref_str: &str = &String::from_utf8(b"foo".as_slice().to_owned()).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: convert from `&[u8]` to `&str` directly + | +LL - let _ref_str: &str = &String::from_utf8(b"foo".as_slice().to_owned()).unwrap(); +LL + let _ref_str: &str = core::str::from_utf8(b"foo".as_slice()).unwrap(); + | + error: unnecessary use of `to_vec` - --> tests/ui/unnecessary_to_owned.rs:202:14 + --> tests/ui/unnecessary_to_owned.rs:221:14 | LL | for t in file_types.to_vec() { | ^^^^^^^^^^^^^^^^^^^ @@ -494,64 +530,64 @@ LL + let path = match get_file_path(t) { | error: unnecessary use of `to_vec` - --> tests/ui/unnecessary_to_owned.rs:225:14 + --> tests/ui/unnecessary_to_owned.rs:244:14 | LL | let _ = &["x"][..].to_vec().into_iter(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().cloned()` error: unnecessary use of `to_vec` - --> tests/ui/unnecessary_to_owned.rs:230:14 + --> tests/ui/unnecessary_to_owned.rs:249:14 | LL | let _ = &["x"][..].to_vec().into_iter(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().copied()` error: unnecessary use of `to_string` - --> tests/ui/unnecessary_to_owned.rs:278:24 + --> tests/ui/unnecessary_to_owned.rs:297:24 | LL | Box::new(build(y.to_string())) | ^^^^^^^^^^^^^ help: use: `y` error: unnecessary use of `to_string` - --> tests/ui/unnecessary_to_owned.rs:387:12 + --> tests/ui/unnecessary_to_owned.rs:406:12 | LL | id("abc".to_string()) | ^^^^^^^^^^^^^^^^^ help: use: `"abc"` error: unnecessary use of `to_vec` - --> tests/ui/unnecessary_to_owned.rs:530:37 + --> tests/ui/unnecessary_to_owned.rs:549:37 | LL | IntoFuture::into_future(foo([].to_vec(), &0)); | ^^^^^^^^^^^ help: use: `[]` error: unnecessary use of `to_vec` - --> tests/ui/unnecessary_to_owned.rs:540:18 + --> tests/ui/unnecessary_to_owned.rs:559:18 | LL | s.remove(&a.to_vec()); | ^^^^^^^^^^^ help: replace it with: `a` error: unnecessary use of `to_owned` - --> tests/ui/unnecessary_to_owned.rs:544:14 + --> tests/ui/unnecessary_to_owned.rs:563:14 | LL | s.remove(&"b".to_owned()); | ^^^^^^^^^^^^^^^ help: replace it with: `"b"` error: unnecessary use of `to_string` - --> tests/ui/unnecessary_to_owned.rs:545:14 + --> tests/ui/unnecessary_to_owned.rs:564:14 | LL | s.remove(&"b".to_string()); | ^^^^^^^^^^^^^^^^ help: replace it with: `"b"` error: unnecessary use of `to_vec` - --> tests/ui/unnecessary_to_owned.rs:550:14 + --> tests/ui/unnecessary_to_owned.rs:569:14 | LL | s.remove(&["b"].to_vec()); | ^^^^^^^^^^^^^^^ help: replace it with: `["b"].as_slice()` error: unnecessary use of `to_vec` - --> tests/ui/unnecessary_to_owned.rs:551:14 + --> tests/ui/unnecessary_to_owned.rs:570:14 | LL | s.remove(&(&["b"]).to_vec()); | ^^^^^^^^^^^^^^^^^^ help: replace it with: `(&["b"]).as_slice()` -error: aborting due to 85 previous errors +error: aborting due to 88 previous errors