From 9cde201ba1c4ca23fb3d1776969b58b2e8d8997e Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 20 Feb 2024 17:53:06 +0100 Subject: [PATCH 1/3] Extend `unnecessary_to_owned` to handle `Borrow` trait in map types --- .../src/methods/unnecessary_to_owned.rs | 98 ++++++++++++++++++- 1 file changed, 96 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs index 64fcd9f8f459..c234e4f9b110 100644 --- a/clippy_lints/src/methods/unnecessary_to_owned.rs +++ b/clippy_lints/src/methods/unnecessary_to_owned.rs @@ -3,7 +3,9 @@ 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::ty::{get_iterator_item_ty, implements_trait, is_copy, is_type_lang_item, peel_mid_ty_refs}; +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 rustc_errors::Applicability; @@ -16,7 +18,8 @@ use rustc_lint::LateContext; use rustc_middle::mir::Mutability; use rustc_middle::ty::adjustment::{Adjust, Adjustment, OverloadedDeref}; use rustc_middle::ty::{ - self, ClauseKind, GenericArg, GenericArgKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty, + self, ClauseKind, GenericArg, GenericArgKind, GenericArgsRef, ImplPolarity, ParamTy, ProjectionPredicate, + TraitPredicate, Ty, }; use rustc_span::{sym, Symbol}; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _; @@ -53,6 +56,8 @@ pub fn check<'tcx>( } check_other_call_arg(cx, expr, method_name, receiver); } + } else { + check_borrow_predicate(cx, expr); } } @@ -590,3 +595,92 @@ fn is_to_string_on_string_like<'a>( false } } + +fn is_a_std_map_type(cx: &LateContext<'_>, ty: Ty<'_>) -> bool { + is_type_diagnostic_item(cx, ty, sym::HashSet) + || is_type_diagnostic_item(cx, ty, sym::HashMap) + || is_type_diagnostic_item(cx, ty, sym::BTreeMap) + || is_type_diagnostic_item(cx, ty, sym::BTreeSet) +} + +fn is_str_and_string(cx: &LateContext<'_>, arg_ty: Ty<'_>, original_arg_ty: Ty<'_>) -> bool { + original_arg_ty.is_str() && is_type_lang_item(cx, arg_ty, LangItem::String) +} + +fn is_slice_and_vec(cx: &LateContext<'_>, arg_ty: Ty<'_>, original_arg_ty: Ty<'_>) -> bool { + (original_arg_ty.is_slice() || original_arg_ty.is_array() || original_arg_ty.is_array_slice()) + && is_type_diagnostic_item(cx, arg_ty, sym::Vec) +} + +// This function will check the following: +// 1. The argument is a non-mutable reference. +// 2. It calls `to_owned()`, `to_string()` or `to_vec()`. +// 3. That the method is called on `String` or on `Vec` (only types supported for the moment). +fn check_if_applicable_to_argument<'tcx>(cx: &LateContext<'tcx>, arg: &Expr<'tcx>) { + if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Not, expr) = arg.kind + && let ExprKind::MethodCall(method_path, caller, &[], _) = expr.kind + && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && let method_name = method_path.ident.name.as_str() + && match method_name { + "to_owned" => cx.tcx.is_diagnostic_item(sym::to_owned_method, method_def_id), + "to_string" => cx.tcx.is_diagnostic_item(sym::to_string_method, method_def_id), + "to_vec" => cx + .tcx + .impl_of_method(method_def_id) + .filter(|&impl_did| cx.tcx.type_of(impl_did).instantiate_identity().is_slice()) + .is_some(), + _ => false, + } + && let original_arg_ty = cx.typeck_results().node_type(caller.hir_id).peel_refs() + && let arg_ty = cx.typeck_results().expr_ty(arg) + && let ty::Ref(_, arg_ty, Mutability::Not) = arg_ty.kind() + // FIXME: try to fix `can_change_type` to make it work in this case. + // && can_change_type(cx, caller, *arg_ty) + && let arg_ty = arg_ty.peel_refs() + // For now we limit this lint to `String` and `Vec`. + && (is_str_and_string(cx, arg_ty, original_arg_ty) || is_slice_and_vec(cx, arg_ty, original_arg_ty)) + && let Some(snippet) = snippet_opt(cx, caller.span) + { + span_lint_and_sugg( + cx, + UNNECESSARY_TO_OWNED, + arg.span, + &format!("unnecessary use of `{method_name}`"), + "replace it with", + if original_arg_ty.is_array() { + format!("{snippet}.as_slice()") + } else { + snippet + }, + Applicability::MaybeIncorrect, + ); + } +} + +// In std "map types", the getters all expect a `Borrow` generic argument. So in here, we +// check that: +// 1. This is a method with only one argument that doesn't come from a trait. +// 2. That it has `Borrow` in its generic predicates. +// 3. `Self` is a std "map type" (ie `HashSet`, `HashMap`, BTreeSet`, `BTreeMap`). +fn check_borrow_predicate<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let ExprKind::MethodCall(_, caller, &[arg], _) = expr.kind + && let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) + && cx.tcx.trait_of_item(method_def_id).is_none() + && let Some(borrow_id) = cx.tcx.get_diagnostic_item(sym::Borrow) + && cx.tcx.predicates_of(method_def_id).predicates.iter().any(|(pred, _)| { + if let ClauseKind::Trait(trait_pred) = pred.kind().skip_binder() + && trait_pred.polarity == ImplPolarity::Positive + && trait_pred.trait_ref.def_id == borrow_id + { + true + } else { + false + } + }) + && let caller_ty = cx.typeck_results().expr_ty(caller) + // For now we limit it to "map types". + && is_a_std_map_type(cx, caller_ty) + { + check_if_applicable_to_argument(cx, &arg); + } +} From d28146133cfc11d9dfe6a082881c7242061275dc Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 20 Feb 2024 17:53:19 +0100 Subject: [PATCH 2/3] Add more ui tests for `unnecessary_to_owned` --- tests/ui/unnecessary_to_owned.fixed | 36 ++++++++++++++++++++++++++++ tests/ui/unnecessary_to_owned.rs | 36 ++++++++++++++++++++++++++++ tests/ui/unnecessary_to_owned.stderr | 32 ++++++++++++++++++++++++- 3 files changed, 103 insertions(+), 1 deletion(-) diff --git a/tests/ui/unnecessary_to_owned.fixed b/tests/ui/unnecessary_to_owned.fixed index 7f01c981a938..1afa5ab54c46 100644 --- a/tests/ui/unnecessary_to_owned.fixed +++ b/tests/ui/unnecessary_to_owned.fixed @@ -530,3 +530,39 @@ mod issue_11952 { IntoFuture::into_future(foo([], &0)); } } + +fn borrow_checks() { + use std::borrow::Borrow; + use std::collections::HashSet; + + fn inner(a: &[&str]) { + let mut s = HashSet::from([vec!["a"]]); + s.remove(a); //~ ERROR: unnecessary use of `to_vec` + } + + let mut s = HashSet::from(["a".to_string()]); + s.remove("b"); //~ ERROR: unnecessary use of `to_owned` + s.remove("b"); //~ ERROR: unnecessary use of `to_string` + // Should not warn. + s.remove("b"); + + let mut s = HashSet::from([vec!["a"]]); + s.remove(["b"].as_slice()); //~ ERROR: unnecessary use of `to_vec` + s.remove((&["b"]).as_slice()); //~ ERROR: unnecessary use of `to_vec` + + // Should not warn. + s.remove(&["b"].to_vec().clone()); + s.remove(["a"].as_slice()); + + trait SetExt { + fn foo>(&self, _: &String); + } + + impl SetExt for HashSet { + fn foo>(&self, _: &String) {} + } + + // Should not lint! + HashSet::::new().foo::<&str>(&"".to_owned()); + HashSet::::new().get(&1.to_string()); +} diff --git a/tests/ui/unnecessary_to_owned.rs b/tests/ui/unnecessary_to_owned.rs index a270ed1e1c21..aa88dde43bf6 100644 --- a/tests/ui/unnecessary_to_owned.rs +++ b/tests/ui/unnecessary_to_owned.rs @@ -530,3 +530,39 @@ mod issue_11952 { IntoFuture::into_future(foo([].to_vec(), &0)); } } + +fn borrow_checks() { + use std::borrow::Borrow; + use std::collections::HashSet; + + fn inner(a: &[&str]) { + let mut s = HashSet::from([vec!["a"]]); + s.remove(&a.to_vec()); //~ ERROR: unnecessary use of `to_vec` + } + + let mut s = HashSet::from(["a".to_string()]); + s.remove(&"b".to_owned()); //~ ERROR: unnecessary use of `to_owned` + s.remove(&"b".to_string()); //~ ERROR: unnecessary use of `to_string` + // Should not warn. + s.remove("b"); + + let mut s = HashSet::from([vec!["a"]]); + s.remove(&["b"].to_vec()); //~ ERROR: unnecessary use of `to_vec` + s.remove(&(&["b"]).to_vec()); //~ ERROR: unnecessary use of `to_vec` + + // Should not warn. + s.remove(&["b"].to_vec().clone()); + s.remove(["a"].as_slice()); + + trait SetExt { + fn foo>(&self, _: &String); + } + + impl SetExt for HashSet { + fn foo>(&self, _: &String) {} + } + + // Should not lint! + HashSet::::new().foo::<&str>(&"".to_owned()); + HashSet::::new().get(&1.to_string()); +} diff --git a/tests/ui/unnecessary_to_owned.stderr b/tests/ui/unnecessary_to_owned.stderr index d4199f8a30a7..5475df9c7b93 100644 --- a/tests/ui/unnecessary_to_owned.stderr +++ b/tests/ui/unnecessary_to_owned.stderr @@ -523,5 +523,35 @@ error: unnecessary use of `to_vec` LL | IntoFuture::into_future(foo([].to_vec(), &0)); | ^^^^^^^^^^^ help: use: `[]` -error: aborting due to 80 previous errors +error: unnecessary use of `to_vec` + --> tests/ui/unnecessary_to_owned.rs:540: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 + | +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 + | +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 + | +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 + | +LL | s.remove(&(&["b"]).to_vec()); + | ^^^^^^^^^^^^^^^^^^ help: replace it with: `(&["b"]).as_slice()` + +error: aborting due to 85 previous errors From 635acb6804ac5e944ca86375c05c262c582163e7 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 20 Feb 2024 17:58:23 +0100 Subject: [PATCH 3/3] Fix newly detected lint issues --- clippy_config/src/conf.rs | 2 +- clippy_lints/src/min_ident_chars.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 6a1d7cb852a2..b781259ad969 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -648,7 +648,7 @@ fn deserialize(file: &SourceFile) -> TryConf { extend_vec_if_indicator_present(&mut conf.conf.doc_valid_idents, DEFAULT_DOC_VALID_IDENTS); extend_vec_if_indicator_present(&mut conf.conf.disallowed_names, DEFAULT_DISALLOWED_NAMES); // TODO: THIS SHOULD BE TESTED, this comment will be gone soon - if conf.conf.allowed_idents_below_min_chars.contains(&"..".to_owned()) { + if conf.conf.allowed_idents_below_min_chars.contains("..") { conf.conf .allowed_idents_below_min_chars .extend(DEFAULT_ALLOWED_IDENTS_BELOW_MIN_CHARS.iter().map(ToString::to_string)); diff --git a/clippy_lints/src/min_ident_chars.rs b/clippy_lints/src/min_ident_chars.rs index ec07f283c468..31bc59fe9409 100644 --- a/clippy_lints/src/min_ident_chars.rs +++ b/clippy_lints/src/min_ident_chars.rs @@ -53,7 +53,7 @@ impl MinIdentChars { && str.len() <= self.min_ident_chars_threshold as usize && !str.starts_with('_') && !str.is_empty() - && self.allowed_idents_below_min_chars.get(&str.to_owned()).is_none() + && !self.allowed_idents_below_min_chars.contains(str) } }