Skip to content

Commit

Permalink
Auto merge of #11301 - y21:issue11300, r=dswij
Browse files Browse the repository at this point in the history
[`useless_conversion`]: don't lint if type parameter has unsatisfiable bounds for `.into_iter()` receiver

Fixes #11300.

Before this PR, clippy assumed that if it sees a `f(x.into_iter())` call and the type at that argument position is generic over any `IntoIterator`, then the `.into_iter()` call must be useless because `x` already implements `IntoIterator`, *however* this assumption is not right if the generic parameter has more than just the `IntoIterator` bound (because other traits can be implemented for the IntoIterator target type but not the IntoIterator implementor, as can be seen in the linked issue: `<[i32; 3] as IntoIterator>::IntoIter` satisfies `ExactSizeIterator`, but `[i32; 3]` does not).

So, this PR makes it check that the type parameter only has a single `IntoIterator` bound. It *might* be possible to check if the type of `x` in `f(x.into_iter())` satisfies all the bounds on the generic type parameter as defined on the function (which would allow removing the `.into_iter()` call even with multiple bounds), however I'm not sure how to do that, and the current fix should always work.

**Edit:** This PR has been changed to check if any of the bounds don't hold for the type of the `.into_iter()` receiver, so we can still lint in some cases.

changelog: [`useless_conversion`]: don't lint `.into_iter()` if type parameter has multiple bounds
  • Loading branch information
bors committed Sep 16, 2023
2 parents daadab5 + 18f3689 commit 59636a2
Show file tree
Hide file tree
Showing 4 changed files with 331 additions and 31 deletions.
110 changes: 90 additions & 20 deletions clippy_lints/src/useless_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@ use rustc_errors::Applicability;
use rustc_hir::def::DefKind;
use rustc_hir::def_id::DefId;
use rustc_hir::{BindingAnnotation, Expr, ExprKind, HirId, MatchSource, Node, PatKind};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_infer::traits::Obligation;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_middle::traits::ObligationCause;
use rustc_middle::ty::{self, EarlyBinder, GenericArg, GenericArgsRef, Ty, TypeVisitableExt};
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::{sym, Span};
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;

declare_clippy_lint! {
/// ### What it does
Expand Down Expand Up @@ -61,22 +65,69 @@ impl MethodOrFunction {
}
}

/// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did`
fn into_iter_bound(cx: &LateContext<'_>, fn_did: DefId, into_iter_did: DefId, param_index: u32) -> Option<Span> {
cx.tcx
.predicates_of(fn_did)
.predicates
.iter()
.find_map(|&(ref pred, span)| {
if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder()
&& tr.def_id() == into_iter_did
&& tr.self_ty().is_param(param_index)
{
Some(span)
} else {
None
/// Returns the span of the `IntoIterator` trait bound in the function pointed to by `fn_did`,
/// iff all of the bounds also hold for the type of the `.into_iter()` receiver.
/// ```ignore
/// pub fn foo<I>(i: I)
/// where I: IntoIterator<Item=i32> + ExactSizeIterator
/// ^^^^^^^^^^^^^^^^^ this extra bound stops us from suggesting to remove `.into_iter()` ...
/// {
/// assert_eq!(i.len(), 3);
/// }
///
/// pub fn bar() {
/// foo([1, 2, 3].into_iter());
/// ^^^^^^^^^^^^ ... here, because `[i32; 3]` is not `ExactSizeIterator`
/// }
/// ```
fn into_iter_bound<'tcx>(
cx: &LateContext<'tcx>,
fn_did: DefId,
into_iter_did: DefId,
into_iter_receiver: Ty<'tcx>,
param_index: u32,
node_args: GenericArgsRef<'tcx>,
) -> Option<Span> {
let param_env = cx.tcx.param_env(fn_did);
let mut into_iter_span = None;

for (pred, span) in cx.tcx.explicit_predicates_of(fn_did).predicates {
if let ty::ClauseKind::Trait(tr) = pred.kind().skip_binder() {
if tr.self_ty().is_param(param_index) {
if tr.def_id() == into_iter_did {
into_iter_span = Some(*span);
} else {
let tr = cx.tcx.erase_regions(tr);
if tr.has_escaping_bound_vars() {
return None;
}

// Substitute generics in the predicate and replace the IntoIterator type parameter with the
// `.into_iter()` receiver to see if the bound also holds for that type.
let args = cx.tcx.mk_args_from_iter(node_args.iter().enumerate().map(|(i, arg)| {
if i == param_index as usize {
GenericArg::from(into_iter_receiver)
} else {
arg
}
}));

let predicate = EarlyBinder::bind(tr).instantiate(cx.tcx, args);
let obligation = Obligation::new(cx.tcx, ObligationCause::dummy(), param_env, predicate);
if !cx
.tcx
.infer_ctxt()
.build()
.predicate_must_hold_modulo_regions(&obligation)
{
return None;
}
}
}
})
}
}

into_iter_span
}

/// Extracts the receiver of a `.into_iter()` method call.
Expand Down Expand Up @@ -160,22 +211,41 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
// `fn_sig` does not ICE. (see #11065)
&& cx.tcx.opt_def_kind(did).is_some_and(DefKind::is_fn_like) =>
{
Some((did, args, MethodOrFunction::Function))
Some((
did,
args,
cx.typeck_results().node_args(recv.hir_id),
MethodOrFunction::Function
))
}
ExprKind::MethodCall(.., args, _) => {
cx.typeck_results().type_dependent_def_id(parent.hir_id)
.map(|did| (did, args, MethodOrFunction::Method))
.map(|did| {
return (
did,
args,
cx.typeck_results().node_args(parent.hir_id),
MethodOrFunction::Method
);
})
}
_ => None,
};

if let Some((parent_fn_did, args, kind)) = parent_fn
if let Some((parent_fn_did, args, node_args, kind)) = parent_fn
&& let Some(into_iter_did) = cx.tcx.get_diagnostic_item(sym::IntoIterator)
&& let sig = cx.tcx.fn_sig(parent_fn_did).skip_binder().skip_binder()
&& let Some(arg_pos) = args.iter().position(|x| x.hir_id == e.hir_id)
&& let Some(&into_iter_param) = sig.inputs().get(kind.param_pos(arg_pos))
&& let ty::Param(param) = into_iter_param.kind()
&& let Some(span) = into_iter_bound(cx, parent_fn_did, into_iter_did, param.index)
&& let Some(span) = into_iter_bound(
cx,
parent_fn_did,
into_iter_did,
cx.typeck_results().expr_ty(into_iter_recv),
param.index,
node_args
)
&& self.expn_depth == 0
{
// Get the "innermost" `.into_iter()` call, e.g. given this expression:
Expand Down
91 changes: 91 additions & 0 deletions tests/ui/useless_conversion.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ fn main() {
let _ = s3;
let s4: Foo<'a'> = Foo;
let _ = vec![s4, s4, s4].into_iter();

issue11300::bar();
}

#[allow(dead_code)]
Expand Down Expand Up @@ -196,6 +198,95 @@ fn explicit_into_iter_fn_arg() {
b(macro_generated!());
}

mod issue11300 {
pub fn foo<I>(i: I)
where
I: IntoIterator<Item = i32> + ExactSizeIterator,
{
assert_eq!(i.len(), 3);
}

trait Helper<T: ?Sized> {}
impl Helper<i32> for [i32; 3] {}
impl Helper<i32> for std::array::IntoIter<i32, 3> {}
impl Helper<()> for std::array::IntoIter<i32, 3> {}

fn foo2<X: ?Sized, I>(_: I)
where
I: IntoIterator<Item = i32> + Helper<X>,
{
}

trait Helper2<T> {}
impl Helper2<std::array::IntoIter<i32, 3>> for i32 {}
impl Helper2<[i32; 3]> for i32 {}
fn foo3<I>(_: I)
where
I: IntoIterator<Item = i32>,
i32: Helper2<I>,
{
}

pub fn bar() {
// This should not trigger the lint:
// `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be
// removed and is not useless.
foo([1, 2, 3].into_iter());

// This should trigger the lint, receiver type [i32; 3] also implements `Helper`
foo2::<i32, _>([1, 2, 3]);

// This again should *not* lint, since X = () and I = std::array::IntoIter<i32, 3>,
// and `[i32; 3]: Helper<()>` is not true (only `std::array::IntoIter<i32, 3>: Helper<()>` is).
foo2::<(), _>([1, 2, 3].into_iter());

// This should lint. Removing the `.into_iter()` means that `I` gets substituted with `[i32; 3]`,
// and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary.
foo3([1, 2, 3]);
}

fn ice() {
struct S1;
impl S1 {
pub fn foo<I: IntoIterator>(&self, _: I) {}
}

S1.foo([1, 2]);

// ICE that occured in itertools
trait Itertools {
fn interleave_shortest<J>(self, other: J)
where
J: IntoIterator,
Self: Sized;
}
impl<I: Iterator> Itertools for I {
fn interleave_shortest<J>(self, other: J)
where
J: IntoIterator,
Self: Sized,
{
}
}
let v0: Vec<i32> = vec![0, 2, 4];
let v1: Vec<i32> = vec![1, 3, 5, 7];
v0.into_iter().interleave_shortest(v1);

trait TraitWithLifetime<'a> {}
impl<'a> TraitWithLifetime<'a> for std::array::IntoIter<&'a i32, 2> {}

struct Helper;
impl<'a> Helper {
fn with_lt<I>(&self, _: I)
where
I: IntoIterator + TraitWithLifetime<'a>,
{
}
}
Helper.with_lt([&1, &2].into_iter());
}
}

#[derive(Copy, Clone)]
struct Foo<const C: char>;

Expand Down
91 changes: 91 additions & 0 deletions tests/ui/useless_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ fn main() {
let _ = Foo::<'a'>::from(s3);
let s4: Foo<'a'> = Foo;
let _ = vec![s4, s4, s4].into_iter().into_iter();

issue11300::bar();
}

#[allow(dead_code)]
Expand Down Expand Up @@ -196,6 +198,95 @@ fn explicit_into_iter_fn_arg() {
b(macro_generated!());
}

mod issue11300 {
pub fn foo<I>(i: I)
where
I: IntoIterator<Item = i32> + ExactSizeIterator,
{
assert_eq!(i.len(), 3);
}

trait Helper<T: ?Sized> {}
impl Helper<i32> for [i32; 3] {}
impl Helper<i32> for std::array::IntoIter<i32, 3> {}
impl Helper<()> for std::array::IntoIter<i32, 3> {}

fn foo2<X: ?Sized, I>(_: I)
where
I: IntoIterator<Item = i32> + Helper<X>,
{
}

trait Helper2<T> {}
impl Helper2<std::array::IntoIter<i32, 3>> for i32 {}
impl Helper2<[i32; 3]> for i32 {}
fn foo3<I>(_: I)
where
I: IntoIterator<Item = i32>,
i32: Helper2<I>,
{
}

pub fn bar() {
// This should not trigger the lint:
// `[i32, 3]` does not satisfy the `ExactSizeIterator` bound, so the into_iter call cannot be
// removed and is not useless.
foo([1, 2, 3].into_iter());

// This should trigger the lint, receiver type [i32; 3] also implements `Helper`
foo2::<i32, _>([1, 2, 3].into_iter());

// This again should *not* lint, since X = () and I = std::array::IntoIter<i32, 3>,
// and `[i32; 3]: Helper<()>` is not true (only `std::array::IntoIter<i32, 3>: Helper<()>` is).
foo2::<(), _>([1, 2, 3].into_iter());

// This should lint. Removing the `.into_iter()` means that `I` gets substituted with `[i32; 3]`,
// and `i32: Helper2<[i32, 3]>` is true, so this call is indeed unncessary.
foo3([1, 2, 3].into_iter());
}

fn ice() {
struct S1;
impl S1 {
pub fn foo<I: IntoIterator>(&self, _: I) {}
}

S1.foo([1, 2].into_iter());

// ICE that occured in itertools
trait Itertools {
fn interleave_shortest<J>(self, other: J)
where
J: IntoIterator,
Self: Sized;
}
impl<I: Iterator> Itertools for I {
fn interleave_shortest<J>(self, other: J)
where
J: IntoIterator,
Self: Sized,
{
}
}
let v0: Vec<i32> = vec![0, 2, 4];
let v1: Vec<i32> = vec![1, 3, 5, 7];
v0.into_iter().interleave_shortest(v1.into_iter());

trait TraitWithLifetime<'a> {}
impl<'a> TraitWithLifetime<'a> for std::array::IntoIter<&'a i32, 2> {}

struct Helper;
impl<'a> Helper {
fn with_lt<I>(&self, _: I)
where
I: IntoIterator + TraitWithLifetime<'a>,
{
}
}
Helper.with_lt([&1, &2].into_iter());
}
}

#[derive(Copy, Clone)]
struct Foo<const C: char>;

Expand Down
Loading

0 comments on commit 59636a2

Please sign in to comment.