Skip to content

Commit

Permalink
Auto merge of #10027 - smoelius:fix-10021, r=dswij
Browse files Browse the repository at this point in the history
Fix 10021

This PR proposes a fix for #10021.

The problem is similar to the one that `@mikerite` described in #9505. The compiler is generating an empty substitution for a call, even though the type of `Self` seems to be needed for a predicate. In `@mikerite's` case, the call was to [`IntoFuture::into_future`](https://doc.rust-lang.org/std/future/trait.IntoFuture.html#tymethod.into_future). In this case, the call is to [`Try::branch`](https://doc.rust-lang.org/std/ops/trait.Try.html#tymethod.branch).

The proposed fix is to verify that the parameter whose type is changing has an index within the substitution. The strikes me as a reasonable approach, since if the check were to fail, the following code would be a no-op:

https://github.com/rust-lang/rust-clippy/blob/4c123a06ba3c2ec17d3a4dfa251dccdc5368b712/clippy_lints/src/methods/unnecessary_to_owned.rs#L420-L428

Like `@mikerite's` original solution, this solution turns ICEs into false negatives.

changelog: fix `unnecessary_to_owned` false positive involving `Try::branch`
  • Loading branch information
bors committed Dec 4, 2022
2 parents 4c123a0 + 2701a40 commit cb8df45
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 4 deletions.
6 changes: 2 additions & 4 deletions clippy_lints/src/methods/unnecessary_to_owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,14 +386,12 @@ fn can_change_type<'a>(cx: &LateContext<'a>, mut expr: &'a Expr<'a>, mut ty: Ty<
Node::Expr(parent_expr) => {
if let Some((callee_def_id, call_substs, recv, call_args)) = get_callee_substs_and_args(cx, parent_expr)
{
if Some(callee_def_id) == cx.tcx.lang_items().into_future_fn() {
return false;
}

let fn_sig = cx.tcx.fn_sig(callee_def_id).skip_binder();
if let Some(arg_index) = recv.into_iter().chain(call_args).position(|arg| arg.hir_id == expr.hir_id)
&& let Some(param_ty) = fn_sig.inputs().get(arg_index)
&& let ty::Param(ParamTy { index: param_index , ..}) = param_ty.kind()
// https://github.com/rust-lang/rust-clippy/issues/9504 and https://github.com/rust-lang/rust-clippy/issues/10021
&& (*param_index as usize) < call_substs.len()
{
if fn_sig
.inputs()
Expand Down
20 changes: 20 additions & 0 deletions tests/ui/unnecessary_to_owned.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -454,3 +454,23 @@ mod issue_9771b {
Key(v.to_vec())
}
}

// This is a watered down version of the code in: https://github.com/oxigraph/rio
// The ICE is triggered by the call to `to_owned` on this line:
// https://github.com/oxigraph/rio/blob/66635b9ff8e5423e58932353fa40d6e64e4820f7/testsuite/src/parser_evaluator.rs#L116
mod issue_10021 {
#![allow(unused)]

pub struct Iri<T>(T);

impl<T: AsRef<str>> Iri<T> {
pub fn parse(iri: T) -> Result<Self, ()> {
unimplemented!()
}
}

pub fn parse_w3c_rdf_test_file(url: &str) -> Result<(), ()> {
let base_iri = Iri::parse(url.to_owned())?;
Ok(())
}
}
20 changes: 20 additions & 0 deletions tests/ui/unnecessary_to_owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -454,3 +454,23 @@ mod issue_9771b {
Key(v.to_vec())
}
}

// This is a watered down version of the code in: https://github.com/oxigraph/rio
// The ICE is triggered by the call to `to_owned` on this line:
// https://github.com/oxigraph/rio/blob/66635b9ff8e5423e58932353fa40d6e64e4820f7/testsuite/src/parser_evaluator.rs#L116
mod issue_10021 {
#![allow(unused)]

pub struct Iri<T>(T);

impl<T: AsRef<str>> Iri<T> {
pub fn parse(iri: T) -> Result<Self, ()> {
unimplemented!()
}
}

pub fn parse_w3c_rdf_test_file(url: &str) -> Result<(), ()> {
let base_iri = Iri::parse(url.to_owned())?;
Ok(())
}
}

0 comments on commit cb8df45

Please sign in to comment.