Skip to content

Commit 885f97e

Browse files
borsbors
committed
Auto merge of rust-lang#11656 - y21:unnecessary_string_from_utf8, r=Jarcho
[`unnecessary_to_owned`]: catch `to_owned` on byte slice to create temporary `&str` Closes rust-lang#11648 Detects the pattern `&String::from_utf8(bytes.to_vec()).unwrap()` and suggests `core::str::from_utf8(bytes).unwrap()`, which avoids the unnecessary intermediate allocation. I decided to put this in the existing `unnecessary_to_owned` lint (rather than creating a new lint) for a few reasons: - we get to use some of its logic (for example, recognizing any of the functions in the `to_owned` family, e.g. `to_vec`) - the actual inefficient operation that can be avoided here is the call to `.to_vec()`, so this is in a way similar to the other cases caught by `unnecessary_to_owned`, just through a bunch of type conversions - we can make this more "generic" later and catch other cases, so imo it's best not to tie this lint specifically to the `String` type changelog: [`unnecessary_to_owned`]: catch `&String::from_utf8(bytes.to_vec()).unwrap()` and suggest `core::str::from_utf8(bytes).unwrap()`
2 parents 0aac16e + ed9ccf6 commit 885f97e

File tree

5 files changed

+154
-15
lines changed

5 files changed

+154
-15
lines changed

‎clippy_lints/src/methods/unnecessary_to_owned.rs

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
use super::implicit_clone::is_clone_like;
22
use super::unnecessary_iter_cloned::{self, is_into_iter};
33
use clippy_config::msrvs::{self, Msrv};
4-
use clippy_utils::diagnostics::span_lint_and_sugg;
5-
use clippy_utils::source::snippet_opt;
4+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
5+
use clippy_utils::source::{snippet, snippet_opt};
66
use clippy_utils::ty::{
77
get_iterator_item_ty, implements_trait, is_copy, is_type_diagnostic_item, is_type_lang_item, peel_mid_ty_refs,
88
};
99
use clippy_utils::visitors::find_all_ret_expressions;
10-
use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item, return_ty};
10+
use clippy_utils::{
11+
fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item, match_def_path, paths, return_ty,
12+
};
1113
use rustc_errors::Applicability;
1214
use rustc_hir::def::{DefKind, Res};
1315
use rustc_hir::def_id::DefId;
@@ -52,6 +54,9 @@ pub fn check<'tcx>(
5254
if check_into_iter_call_arg(cx, expr, method_name, receiver, msrv) {
5355
return;
5456
}
57+
if check_string_from_utf8(cx, expr, receiver) {
58+
return;
59+
}
5560
check_other_call_arg(cx, expr, method_name, receiver);
5661
}
5762
} else {
@@ -240,6 +245,65 @@ fn check_into_iter_call_arg(
240245
false
241246
}
242247

248+
/// Checks for `&String::from_utf8(bytes.{to_vec,to_owned,...}()).unwrap()` coercing to `&str`,
249+
/// which can be written as just `std::str::from_utf8(bytes).unwrap()`.
250+
fn check_string_from_utf8<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, receiver: &'tcx Expr<'tcx>) -> bool {
251+
if let Some((call, arg)) = skip_addr_of_ancestors(cx, expr)
252+
&& !arg.span.from_expansion()
253+
&& let ExprKind::Call(callee, _) = call.kind
254+
&& fn_def_id(cx, call).is_some_and(|did| match_def_path(cx, did, &paths::STRING_FROM_UTF8))
255+
&& let Some(unwrap_call) = get_parent_expr(cx, call)
256+
&& let ExprKind::MethodCall(unwrap_method_name, ..) = unwrap_call.kind
257+
&& matches!(unwrap_method_name.ident.name, sym::unwrap | sym::expect)
258+
&& let Some(ref_string) = get_parent_expr(cx, unwrap_call)
259+
&& let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, _) = ref_string.kind
260+
&& let adjusted_ty = cx.typeck_results().expr_ty_adjusted(ref_string)
261+
// `&...` creates a `&String`, so only actually lint if this coerces to a `&str`
262+
&& matches!(adjusted_ty.kind(), ty::Ref(_, ty, _) if ty.is_str())
263+
{
264+
span_lint_and_then(
265+
cx,
266+
UNNECESSARY_TO_OWNED,
267+
ref_string.span,
268+
"allocating a new `String` only to create a temporary `&str` from it",
269+
|diag| {
270+
let arg_suggestion = format!(
271+
"{borrow}{recv_snippet}",
272+
recv_snippet = snippet(cx, receiver.span.source_callsite(), ".."),
273+
borrow = if cx.typeck_results().expr_ty(receiver).is_ref() {
274+
""
275+
} else {
276+
// If not already a reference, prefix with a borrow so that it can coerce to one
277+
"&"
278+
}
279+
);
280+
281+
diag.multipart_suggestion(
282+
"convert from `&[u8]` to `&str` directly",
283+
vec![
284+
// `&String::from_utf8(bytes.to_vec()).unwrap()`
285+
// ^^^^^^^^^^^^^^^^^
286+
(callee.span, "core::str::from_utf8".into()),
287+
// `&String::from_utf8(bytes.to_vec()).unwrap()`
288+
// ^
289+
(
290+
ref_string.span.shrink_to_lo().to(unwrap_call.span.shrink_to_lo()),
291+
String::new(),
292+
),
293+
// `&String::from_utf8(bytes.to_vec()).unwrap()`
294+
// ^^^^^^^^^^^^^^
295+
(arg.span, arg_suggestion),
296+
],
297+
Applicability::MachineApplicable,
298+
);
299+
},
300+
);
301+
true
302+
} else {
303+
false
304+
}
305+
}
306+
243307
/// Checks whether `expr` is an argument in an `into_iter` call and, if so, determines whether its
244308
/// call of a `to_owned`-like function is unnecessary.
245309
fn check_split_call_arg(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: Symbol, receiver: &Expr<'_>) -> bool {

‎clippy_utils/src/paths.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ pub const SYMBOL_INTERN: [&str; 4] = ["rustc_span", "symbol", "Symbol", "intern"
8888
pub const SYMBOL_TO_IDENT_STRING: [&str; 4] = ["rustc_span", "symbol", "Symbol", "to_ident_string"];
8989
pub const SYM_MODULE: [&str; 3] = ["rustc_span", "symbol", "sym"];
9090
pub const SYNTAX_CONTEXT: [&str; 3] = ["rustc_span", "hygiene", "SyntaxContext"];
91+
pub const STRING_FROM_UTF8: [&str; 4] = ["alloc", "string", "String", "from_utf8"];
9192
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates
9293
pub const TOKIO_FILE_OPTIONS: [&str; 5] = ["tokio", "fs", "file", "File", "options"];
9394
#[expect(clippy::invalid_paths)] // internal lints do not know about all external crates

‎tests/ui/unnecessary_to_owned.fixed

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,25 @@ fn main() {
157157
require_path(&std::path::PathBuf::from("x"));
158158
require_str(&String::from("x"));
159159
require_slice(&[String::from("x")]);
160+
161+
let slice = [0u8; 1024];
162+
let _ref_str: &str = core::str::from_utf8(&slice).expect("not UTF-8");
163+
let _ref_str: &str = core::str::from_utf8(b"foo").unwrap();
164+
let _ref_str: &str = core::str::from_utf8(b"foo".as_slice()).unwrap();
165+
// Expression is of type `&String`, can't suggest `str::from_utf8` here
166+
let _ref_string = &String::from_utf8(b"foo".to_vec()).unwrap();
167+
macro_rules! arg_from_macro {
168+
() => {
169+
b"foo".to_vec()
170+
};
171+
}
172+
macro_rules! string_from_utf8_from_macro {
173+
() => {
174+
&String::from_utf8(b"foo".to_vec()).unwrap()
175+
};
176+
}
177+
let _ref_str: &str = &String::from_utf8(arg_from_macro!()).unwrap();
178+
let _ref_str: &str = string_from_utf8_from_macro!();
160179
}
161180

162181
fn require_c_str(_: &CStr) {}

‎tests/ui/unnecessary_to_owned.rs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,25 @@ fn main() {
157157
require_path(&std::path::PathBuf::from("x").to_path_buf());
158158
require_str(&String::from("x").to_string());
159159
require_slice(&[String::from("x")].to_owned());
160+
161+
let slice = [0u8; 1024];
162+
let _ref_str: &str = &String::from_utf8(slice.to_vec()).expect("not UTF-8");
163+
let _ref_str: &str = &String::from_utf8(b"foo".to_vec()).unwrap();
164+
let _ref_str: &str = &String::from_utf8(b"foo".as_slice().to_owned()).unwrap();
165+
// Expression is of type `&String`, can't suggest `str::from_utf8` here
166+
let _ref_string = &String::from_utf8(b"foo".to_vec()).unwrap();
167+
macro_rules! arg_from_macro {
168+
() => {
169+
b"foo".to_vec()
170+
};
171+
}
172+
macro_rules! string_from_utf8_from_macro {
173+
() => {
174+
&String::from_utf8(b"foo".to_vec()).unwrap()
175+
};
176+
}
177+
let _ref_str: &str = &String::from_utf8(arg_from_macro!()).unwrap();
178+
let _ref_str: &str = string_from_utf8_from_macro!();
160179
}
161180

162181
fn require_c_str(_: &CStr) {}

‎tests/ui/unnecessary_to_owned.stderr

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -477,8 +477,44 @@ error: unnecessary use of `to_owned`
477477
LL | let _ = IntoIterator::into_iter([std::path::PathBuf::new()][..].to_owned());
478478
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[std::path::PathBuf::new()][..].iter().cloned()`
479479

480+
error: allocating a new `String` only to create a temporary `&str` from it
481+
--> tests/ui/unnecessary_to_owned.rs:162:26
482+
|
483+
LL | let _ref_str: &str = &String::from_utf8(slice.to_vec()).expect("not UTF-8");
484+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
485+
|
486+
help: convert from `&[u8]` to `&str` directly
487+
|
488+
LL - let _ref_str: &str = &String::from_utf8(slice.to_vec()).expect("not UTF-8");
489+
LL + let _ref_str: &str = core::str::from_utf8(&slice).expect("not UTF-8");
490+
|
491+
492+
error: allocating a new `String` only to create a temporary `&str` from it
493+
--> tests/ui/unnecessary_to_owned.rs:163:26
494+
|
495+
LL | let _ref_str: &str = &String::from_utf8(b"foo".to_vec()).unwrap();
496+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
497+
|
498+
help: convert from `&[u8]` to `&str` directly
499+
|
500+
LL - let _ref_str: &str = &String::from_utf8(b"foo".to_vec()).unwrap();
501+
LL + let _ref_str: &str = core::str::from_utf8(b"foo").unwrap();
502+
|
503+
504+
error: allocating a new `String` only to create a temporary `&str` from it
505+
--> tests/ui/unnecessary_to_owned.rs:164:26
506+
|
507+
LL | let _ref_str: &str = &String::from_utf8(b"foo".as_slice().to_owned()).unwrap();
508+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
509+
|
510+
help: convert from `&[u8]` to `&str` directly
511+
|
512+
LL - let _ref_str: &str = &String::from_utf8(b"foo".as_slice().to_owned()).unwrap();
513+
LL + let _ref_str: &str = core::str::from_utf8(b"foo".as_slice()).unwrap();
514+
|
515+
480516
error: unnecessary use of `to_vec`
481-
--> tests/ui/unnecessary_to_owned.rs:202:14
517+
--> tests/ui/unnecessary_to_owned.rs:221:14
482518
|
483519
LL | for t in file_types.to_vec() {
484520
| ^^^^^^^^^^^^^^^^^^^
@@ -494,64 +530,64 @@ LL + let path = match get_file_path(t) {
494530
|
495531

496532
error: unnecessary use of `to_vec`
497-
--> tests/ui/unnecessary_to_owned.rs:225:14
533+
--> tests/ui/unnecessary_to_owned.rs:244:14
498534
|
499535
LL | let _ = &["x"][..].to_vec().into_iter();
500536
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().cloned()`
501537

502538
error: unnecessary use of `to_vec`
503-
--> tests/ui/unnecessary_to_owned.rs:230:14
539+
--> tests/ui/unnecessary_to_owned.rs:249:14
504540
|
505541
LL | let _ = &["x"][..].to_vec().into_iter();
506542
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `["x"][..].iter().copied()`
507543

508544
error: unnecessary use of `to_string`
509-
--> tests/ui/unnecessary_to_owned.rs:278:24
545+
--> tests/ui/unnecessary_to_owned.rs:297:24
510546
|
511547
LL | Box::new(build(y.to_string()))
512548
| ^^^^^^^^^^^^^ help: use: `y`
513549

514550
error: unnecessary use of `to_string`
515-
--> tests/ui/unnecessary_to_owned.rs:387:12
551+
--> tests/ui/unnecessary_to_owned.rs:406:12
516552
|
517553
LL | id("abc".to_string())
518554
| ^^^^^^^^^^^^^^^^^ help: use: `"abc"`
519555

520556
error: unnecessary use of `to_vec`
521-
--> tests/ui/unnecessary_to_owned.rs:530:37
557+
--> tests/ui/unnecessary_to_owned.rs:549:37
522558
|
523559
LL | IntoFuture::into_future(foo([].to_vec(), &0));
524560
| ^^^^^^^^^^^ help: use: `[]`
525561

526562
error: unnecessary use of `to_vec`
527-
--> tests/ui/unnecessary_to_owned.rs:540:18
563+
--> tests/ui/unnecessary_to_owned.rs:559:18
528564
|
529565
LL | s.remove(&a.to_vec());
530566
| ^^^^^^^^^^^ help: replace it with: `a`
531567

532568
error: unnecessary use of `to_owned`
533-
--> tests/ui/unnecessary_to_owned.rs:544:14
569+
--> tests/ui/unnecessary_to_owned.rs:563:14
534570
|
535571
LL | s.remove(&"b".to_owned());
536572
| ^^^^^^^^^^^^^^^ help: replace it with: `"b"`
537573

538574
error: unnecessary use of `to_string`
539-
--> tests/ui/unnecessary_to_owned.rs:545:14
575+
--> tests/ui/unnecessary_to_owned.rs:564:14
540576
|
541577
LL | s.remove(&"b".to_string());
542578
| ^^^^^^^^^^^^^^^^ help: replace it with: `"b"`
543579

544580
error: unnecessary use of `to_vec`
545-
--> tests/ui/unnecessary_to_owned.rs:550:14
581+
--> tests/ui/unnecessary_to_owned.rs:569:14
546582
|
547583
LL | s.remove(&["b"].to_vec());
548584
| ^^^^^^^^^^^^^^^ help: replace it with: `["b"].as_slice()`
549585

550586
error: unnecessary use of `to_vec`
551-
--> tests/ui/unnecessary_to_owned.rs:551:14
587+
--> tests/ui/unnecessary_to_owned.rs:570:14
552588
|
553589
LL | s.remove(&(&["b"]).to_vec());
554590
| ^^^^^^^^^^^^^^^^^^ help: replace it with: `(&["b"]).as_slice()`
555591

556-
error: aborting due to 85 previous errors
592+
error: aborting due to 88 previous errors
557593

0 commit comments

Comments
 (0)