Skip to content

Commit 46b7cb6

Browse files
committed
Auto merge of #9424 - mikerite:fix-9351-4, r=xFrednet
Fix `unnecessary_to_owned` false positive Fixes #9351. Note that this commit reworks that fix for #9317. The change is to check that the type implements `AsRef<str>` before regarding `to_string` as an equivalent of `to_owned`. This was suggested by Jarcho in the #9317 issue comments. The benefit of this is that it moves some complexity out of `check_other_call_arg` and simplifies the module as a whole. changelog: FP: [`unnecessary_to_owned`]: No longer lints, if type change would cause errors in the caller function
2 parents 99ab5fe + 750a2d5 commit 46b7cb6

File tree

4 files changed

+262
-71
lines changed

4 files changed

+262
-71
lines changed

clippy_lints/src/methods/unnecessary_to_owned.rs

+135-70
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,25 @@
11
use super::implicit_clone::is_clone_like;
22
use super::unnecessary_iter_cloned::{self, is_into_iter};
3+
use crate::rustc_middle::ty::Subst;
34
use clippy_utils::diagnostics::span_lint_and_sugg;
45
use clippy_utils::source::snippet_opt;
5-
use clippy_utils::ty::{
6-
get_associated_type, get_iterator_item_ty, implements_trait, is_copy, is_type_diagnostic_item, peel_mid_ty_refs,
7-
};
8-
use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item};
6+
use clippy_utils::ty::{get_associated_type, get_iterator_item_ty, implements_trait, is_copy, peel_mid_ty_refs};
7+
use clippy_utils::visitors::find_all_ret_expressions;
8+
use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item, return_ty};
99
use clippy_utils::{meets_msrv, msrvs};
1010
use rustc_errors::Applicability;
11-
use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind};
11+
use rustc_hir::{def_id::DefId, BorrowKind, Expr, ExprKind, ItemKind, Node};
12+
use rustc_infer::infer::TyCtxtInferExt;
1213
use rustc_lint::LateContext;
1314
use rustc_middle::mir::Mutability;
1415
use rustc_middle::ty::adjustment::{Adjust, Adjustment, OverloadedDeref};
1516
use rustc_middle::ty::subst::{GenericArg, GenericArgKind, SubstsRef};
16-
use rustc_middle::ty::{self, PredicateKind, ProjectionPredicate, TraitPredicate, Ty};
17+
use rustc_middle::ty::EarlyBinder;
18+
use rustc_middle::ty::{self, ParamTy, PredicateKind, ProjectionPredicate, TraitPredicate, Ty};
1719
use rustc_semver::RustcVersion;
1820
use rustc_span::{sym, Symbol};
21+
use rustc_trait_selection::traits::{query::evaluate_obligation::InferCtxtExt as _, Obligation, ObligationCause};
22+
use rustc_typeck::check::{FnCtxt, Inherited};
1923
use std::cmp::max;
2024

2125
use super::UNNECESSARY_TO_OWNED;
@@ -33,7 +37,7 @@ pub fn check<'tcx>(
3337
then {
3438
if is_cloned_or_copied(cx, method_name, method_def_id) {
3539
unnecessary_iter_cloned::check(cx, expr, method_name, receiver);
36-
} else if is_to_owned_like(cx, method_name, method_def_id) {
40+
} else if is_to_owned_like(cx, expr, method_name, method_def_id) {
3741
// At this point, we know the call is of a `to_owned`-like function. The functions
3842
// `check_addr_of_expr` and `check_call_arg` determine whether the call is unnecessary
3943
// based on its context, that is, whether it is a referent in an `AddrOf` expression, an
@@ -245,65 +249,26 @@ fn check_other_call_arg<'tcx>(
245249
) -> bool {
246250
if_chain! {
247251
if let Some((maybe_call, maybe_arg)) = skip_addr_of_ancestors(cx, expr);
248-
if let Some((callee_def_id, call_substs, call_args)) = get_callee_substs_and_args(cx, maybe_call);
252+
if let Some((callee_def_id, _, call_args)) = get_callee_substs_and_args(cx, maybe_call);
249253
let fn_sig = cx.tcx.fn_sig(callee_def_id).skip_binder();
250254
if let Some(i) = call_args.iter().position(|arg| arg.hir_id == maybe_arg.hir_id);
251255
if let Some(input) = fn_sig.inputs().get(i);
252256
let (input, n_refs) = peel_mid_ty_refs(*input);
253-
if let (trait_predicates, projection_predicates) = get_input_traits_and_projections(cx, callee_def_id, input);
257+
if let (trait_predicates, _) = get_input_traits_and_projections(cx, callee_def_id, input);
254258
if let Some(sized_def_id) = cx.tcx.lang_items().sized_trait();
255259
if let [trait_predicate] = trait_predicates
256260
.iter()
257261
.filter(|trait_predicate| trait_predicate.def_id() != sized_def_id)
258262
.collect::<Vec<_>>()[..];
259263
if let Some(deref_trait_id) = cx.tcx.get_diagnostic_item(sym::Deref);
260264
if let Some(as_ref_trait_id) = cx.tcx.get_diagnostic_item(sym::AsRef);
265+
if trait_predicate.def_id() == deref_trait_id || trait_predicate.def_id() == as_ref_trait_id;
261266
let receiver_ty = cx.typeck_results().expr_ty(receiver);
262-
// If the callee has type parameters, they could appear in `projection_predicate.ty` or the
263-
// types of `trait_predicate.trait_ref.substs`.
264-
if if trait_predicate.def_id() == deref_trait_id {
265-
if let [projection_predicate] = projection_predicates[..] {
266-
let normalized_ty =
267-
cx.tcx
268-
.subst_and_normalize_erasing_regions(call_substs, cx.param_env, projection_predicate.term);
269-
implements_trait(cx, receiver_ty, deref_trait_id, &[])
270-
&& get_associated_type(cx, receiver_ty, deref_trait_id, "Target")
271-
.map_or(false, |ty| ty::Term::Ty(ty) == normalized_ty)
272-
} else {
273-
false
274-
}
275-
} else if trait_predicate.def_id() == as_ref_trait_id {
276-
let composed_substs = compose_substs(
277-
cx,
278-
&trait_predicate.trait_ref.substs.iter().skip(1).collect::<Vec<_>>()[..],
279-
call_substs,
280-
);
281-
// if `expr` is a `String` and generic target is [u8], skip
282-
// (https://github.com/rust-lang/rust-clippy/issues/9317).
283-
if let [subst] = composed_substs[..]
284-
&& let GenericArgKind::Type(arg_ty) = subst.unpack()
285-
&& arg_ty.is_slice()
286-
&& let inner_ty = arg_ty.builtin_index().unwrap()
287-
&& let ty::Uint(ty::UintTy::U8) = inner_ty.kind()
288-
&& let self_ty = cx.typeck_results().expr_ty(expr).peel_refs()
289-
&& is_type_diagnostic_item(cx, self_ty, sym::String) {
290-
false
291-
} else {
292-
implements_trait(cx, receiver_ty, as_ref_trait_id, &composed_substs)
293-
}
294-
} else {
295-
false
296-
};
267+
if can_change_type(cx, maybe_arg, receiver_ty);
297268
// We can't add an `&` when the trait is `Deref` because `Target = &T` won't match
298269
// `Target = T`.
299270
if n_refs > 0 || is_copy(cx, receiver_ty) || trait_predicate.def_id() != deref_trait_id;
300271
let n_refs = max(n_refs, if is_copy(cx, receiver_ty) { 0 } else { 1 });
301-
// If the trait is `AsRef` and the input type variable `T` occurs in the output type, then
302-
// `T` must not be instantiated with a reference
303-
// (https://github.com/rust-lang/rust-clippy/issues/8507).
304-
if (n_refs == 0 && !receiver_ty.is_ref())
305-
|| trait_predicate.def_id() != as_ref_trait_id
306-
|| !fn_sig.output().contains(input);
307272
if let Some(receiver_snippet) = snippet_opt(cx, receiver.span);
308273
then {
309274
span_lint_and_sugg(
@@ -389,22 +354,102 @@ fn get_input_traits_and_projections<'tcx>(
389354
(trait_predicates, projection_predicates)
390355
}
391356

392-
/// Composes two substitutions by applying the latter to the types of the former.
393-
fn compose_substs<'tcx>(
394-
cx: &LateContext<'tcx>,
395-
left: &[GenericArg<'tcx>],
396-
right: SubstsRef<'tcx>,
397-
) -> Vec<GenericArg<'tcx>> {
398-
left.iter()
399-
.map(|arg| {
400-
if let GenericArgKind::Type(arg_ty) = arg.unpack() {
401-
let normalized_ty = cx.tcx.subst_and_normalize_erasing_regions(right, cx.param_env, arg_ty);
402-
GenericArg::from(normalized_ty)
403-
} else {
404-
*arg
357+
fn can_change_type<'a>(cx: &LateContext<'a>, mut expr: &'a Expr<'a>, mut ty: Ty<'a>) -> bool {
358+
for (_, node) in cx.tcx.hir().parent_iter(expr.hir_id) {
359+
match node {
360+
Node::Stmt(_) => return true,
361+
Node::Block(..) => continue,
362+
Node::Item(item) => {
363+
if let ItemKind::Fn(_, _, body_id) = &item.kind
364+
&& let output_ty = return_ty(cx, item.hir_id())
365+
&& let local_def_id = cx.tcx.hir().local_def_id(item.hir_id())
366+
&& Inherited::build(cx.tcx, local_def_id).enter(|inherited| {
367+
let fn_ctxt = FnCtxt::new(&inherited, cx.param_env, item.hir_id());
368+
fn_ctxt.can_coerce(ty, output_ty)
369+
}) {
370+
if has_lifetime(output_ty) && has_lifetime(ty) {
371+
return false;
372+
}
373+
let body = cx.tcx.hir().body(*body_id);
374+
let body_expr = &body.value;
375+
let mut count = 0;
376+
return find_all_ret_expressions(cx, body_expr, |_| { count += 1; count <= 1 });
377+
}
405378
}
406-
})
407-
.collect()
379+
Node::Expr(parent_expr) => {
380+
if let Some((callee_def_id, call_substs, call_args)) = get_callee_substs_and_args(cx, parent_expr) {
381+
let fn_sig = cx.tcx.fn_sig(callee_def_id).skip_binder();
382+
if let Some(arg_index) = call_args.iter().position(|arg| arg.hir_id == expr.hir_id)
383+
&& let Some(param_ty) = fn_sig.inputs().get(arg_index)
384+
&& let ty::Param(ParamTy { index: param_index , ..}) = param_ty.kind()
385+
{
386+
if fn_sig
387+
.inputs()
388+
.iter()
389+
.enumerate()
390+
.filter(|(i, _)| *i != arg_index)
391+
.any(|(_, ty)| ty.contains(*param_ty))
392+
{
393+
return false;
394+
}
395+
396+
let mut trait_predicates = cx.tcx.param_env(callee_def_id)
397+
.caller_bounds().iter().filter(|predicate| {
398+
if let PredicateKind::Trait(trait_predicate) = predicate.kind().skip_binder()
399+
&& trait_predicate.trait_ref.self_ty() == *param_ty {
400+
true
401+
} else {
402+
false
403+
}
404+
});
405+
406+
let new_subst = cx.tcx.mk_substs(
407+
call_substs.iter()
408+
.enumerate()
409+
.map(|(i, t)|
410+
if i == (*param_index as usize) {
411+
GenericArg::from(ty)
412+
} else {
413+
t
414+
}));
415+
416+
if trait_predicates.any(|predicate| {
417+
let predicate = EarlyBinder(predicate).subst(cx.tcx, new_subst);
418+
let obligation = Obligation::new(ObligationCause::dummy(), cx.param_env, predicate);
419+
!cx.tcx
420+
.infer_ctxt()
421+
.enter(|infcx| infcx.predicate_must_hold_modulo_regions(&obligation))
422+
}) {
423+
return false;
424+
}
425+
426+
let output_ty = fn_sig.output();
427+
if output_ty.contains(*param_ty) {
428+
if let Ok(new_ty) = cx.tcx.try_subst_and_normalize_erasing_regions(
429+
new_subst, cx.param_env, output_ty) {
430+
expr = parent_expr;
431+
ty = new_ty;
432+
continue;
433+
}
434+
return false;
435+
}
436+
437+
return true;
438+
}
439+
} else if let ExprKind::Block(..) = parent_expr.kind {
440+
continue;
441+
}
442+
return false;
443+
},
444+
_ => return false,
445+
}
446+
}
447+
448+
false
449+
}
450+
451+
fn has_lifetime(ty: Ty<'_>) -> bool {
452+
ty.walk().any(|t| matches!(t.unpack(), GenericArgKind::Lifetime(_)))
408453
}
409454

410455
/// Returns true if the named method is `Iterator::cloned` or `Iterator::copied`.
@@ -415,18 +460,38 @@ fn is_cloned_or_copied(cx: &LateContext<'_>, method_name: Symbol, method_def_id:
415460

416461
/// Returns true if the named method can be used to convert the receiver to its "owned"
417462
/// representation.
418-
fn is_to_owned_like(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
463+
fn is_to_owned_like<'a>(cx: &LateContext<'a>, call_expr: &Expr<'a>, method_name: Symbol, method_def_id: DefId) -> bool {
419464
is_clone_like(cx, method_name.as_str(), method_def_id)
420465
|| is_cow_into_owned(cx, method_name, method_def_id)
421-
|| is_to_string(cx, method_name, method_def_id)
466+
|| is_to_string_on_string_like(cx, call_expr, method_name, method_def_id)
422467
}
423468

424469
/// Returns true if the named method is `Cow::into_owned`.
425470
fn is_cow_into_owned(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
426471
method_name.as_str() == "into_owned" && is_diag_item_method(cx, method_def_id, sym::Cow)
427472
}
428473

429-
/// Returns true if the named method is `ToString::to_string`.
430-
fn is_to_string(cx: &LateContext<'_>, method_name: Symbol, method_def_id: DefId) -> bool {
431-
method_name == sym::to_string && is_diag_trait_item(cx, method_def_id, sym::ToString)
474+
/// Returns true if the named method is `ToString::to_string` and it's called on a type that
475+
/// is string-like i.e. implements `AsRef<str>` or `Deref<str>`.
476+
fn is_to_string_on_string_like<'a>(
477+
cx: &LateContext<'_>,
478+
call_expr: &'a Expr<'a>,
479+
method_name: Symbol,
480+
method_def_id: DefId,
481+
) -> bool {
482+
if method_name != sym::to_string || !is_diag_trait_item(cx, method_def_id, sym::ToString) {
483+
return false;
484+
}
485+
486+
if let Some(substs) = cx.typeck_results().node_substs_opt(call_expr.hir_id)
487+
&& let [generic_arg] = substs.as_slice()
488+
&& let GenericArgKind::Type(ty) = generic_arg.unpack()
489+
&& let Some(deref_trait_id) = cx.tcx.get_diagnostic_item(sym::Deref)
490+
&& let Some(as_ref_trait_id) = cx.tcx.get_diagnostic_item(sym::AsRef)
491+
&& (implements_trait(cx, ty, deref_trait_id, &[cx.tcx.types.str_.into()]) ||
492+
implements_trait(cx, ty, as_ref_trait_id, &[cx.tcx.types.str_.into()])) {
493+
true
494+
} else {
495+
false
496+
}
432497
}

tests/ui/unnecessary_to_owned.fixed

+60
Original file line numberDiff line numberDiff line change
@@ -357,3 +357,63 @@ mod issue_9317 {
357357
consume(b.to_string());
358358
}
359359
}
360+
361+
mod issue_9351 {
362+
#![allow(dead_code)]
363+
364+
use std::ops::Deref;
365+
use std::path::{Path, PathBuf};
366+
367+
fn require_deref_path<T: Deref<Target = std::path::Path>>(x: T) -> T {
368+
x
369+
}
370+
371+
fn generic_arg_used_elsewhere<T: AsRef<Path>>(_x: T, _y: T) {}
372+
373+
fn id<T: AsRef<str>>(x: T) -> T {
374+
x
375+
}
376+
377+
fn predicates_are_satisfied(_x: impl std::fmt::Write) {}
378+
379+
// Should lint
380+
fn single_return() -> impl AsRef<str> {
381+
id("abc")
382+
}
383+
384+
// Should not lint
385+
fn multiple_returns(b: bool) -> impl AsRef<str> {
386+
if b {
387+
return String::new();
388+
}
389+
390+
id("abc".to_string())
391+
}
392+
393+
struct S1(String);
394+
395+
// Should not lint
396+
fn fields1() -> S1 {
397+
S1(id("abc".to_string()))
398+
}
399+
400+
struct S2 {
401+
s: String,
402+
}
403+
404+
// Should not lint
405+
fn fields2() {
406+
let mut s = S2 { s: "abc".into() };
407+
s.s = id("abc".to_string());
408+
}
409+
410+
pub fn main() {
411+
let path = std::path::Path::new("x");
412+
let path_buf = path.to_owned();
413+
414+
// Should not lint.
415+
let _x: PathBuf = require_deref_path(path.to_owned());
416+
generic_arg_used_elsewhere(path.to_owned(), path_buf);
417+
predicates_are_satisfied(id("abc".to_string()));
418+
}
419+
}

0 commit comments

Comments
 (0)