Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't suggest .into_iter() on iterators #132760

Merged
merged 1 commit into from
Nov 9, 2024
Merged

Conversation

dianne
Copy link
Contributor

@dianne dianne commented Nov 8, 2024

This makes the the suggestion to call .into_iter() only consider unsatisfied Iterator bounds for the receiver type itself. That way, it ignores predicates generated by trying to auto-ref the receiver (the result of which usually won't implement Iterator).

Fixes #127511

Unfortunately, the error in that case is still confusing: it labels Iterator as an unsatisfied bound because &impl Iterator: Iterator can't be satisfied, despite that not being required or helpful. I'd like to handle that in a separate PR. I'm hoping fixing #124802 will fix it too. It doesn't look connected to that issue. Still, I think it'd be clearest to visually distinguish unsatisfied predicates from different attempts at pick_method; I'll make a PR for that soon.

@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2024

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 8, 2024
if let Some(iterator_trait) = self.tcx.get_diagnostic_item(sym::Iterator)
&& self
.type_implements_trait(iterator_trait, [ty], self.param_env)
.must_apply_modulo_regions()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be necessary 🤔 why is the impl Iterator: Iterator in the unsatisfied_predicates. I feel like there's something bigger going on which likely also affects other diagnostics.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, the reason is that unsatisfied_predicates also contains all the unsatisfied predicates from auto-ref and the deref-steps, e.g.

trait Trait {
    fn method(self)
}

trait Bound {}
impl<T: Bound> Trait for T {
    fn method(self) {}
}

fn main() {
    0u32.method()
}

results in

error[E0599]: the method `method` exists for type `u32`, but its trait bounds were not satisfied
  --> src/main.rs:11:10
   |
11 |     0u32.method()
   |          ^^^^^^ method cannot be called on `u32` due to unsatisfied trait bounds
   |
note: the following trait bounds were not satisfied:
      `&mut u32: Bound`
      `&u32: Bound`
      `u32: Bound

as you can see in the new error message for this issue, it's talking about the following unsatisfied obligations:

      `&impl Iterator: Iterator`
      `&impl Iterator: Missing`
      `&mut impl Iterator: Missing`
      `impl Iterator: Missing`

so instead of checking whether ty implements iterator, modify is_iterator_predicate to only trigger if the self-type of the iterator predicate is ty itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch! I was thinking along those lines for making that error less confusing, but I totally missed that it could be applied to is_iterator_predicate too. it's much nicer now; thank you

@lcnr
Copy link
Contributor

lcnr commented Nov 9, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 9, 2024

📌 Commit cea82ed has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 9, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2024
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#132755 (Do not reveal opaques in the param-env, we got lazy norm instead)
 - rust-lang#132757 (Get rid of `check_opaque_type_well_formed`)
 - rust-lang#132760 (Don't suggest `.into_iter()` on iterators)
 - rust-lang#132778 (update io::Error::into_inner to acknowledge io::Error::other)
 - rust-lang#132780 (use verbose for path separator suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b598849 into rust-lang:master Nov 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2024
Rollup merge of rust-lang#132760 - dianne:iter-into-iter, r=lcnr

Don't suggest `.into_iter()` on iterators

This makes the the suggestion to call `.into_iter()` only consider unsatisfied `Iterator` bounds for the receiver type itself. That way, it ignores predicates generated by trying to auto-ref the receiver (the result of which usually won't implement `Iterator`).

Fixes rust-lang#127511

Unfortunately, the error in that case is still confusing: it labels `Iterator` as an unsatisfied bound because `&impl Iterator: Iterator` can't be satisfied, despite that not being required or helpful. I'd like to handle that in a separate PR. ~~I'm hoping fixing rust-lang#124802 will fix it too.~~ It doesn't look connected to that issue. Still, I think it'd be clearest to visually distinguish unsatisfied predicates from different attempts at `pick_method`; I'll make a PR for that soon.
@dianne dianne deleted the iter-into-iter branch November 9, 2024 22:22
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
Don't suggest `.into_iter()` on iterators

This makes the the suggestion to call `.into_iter()` only consider unsatisfied `Iterator` bounds for the receiver type itself. That way, it ignores predicates generated by trying to auto-ref the receiver (the result of which usually won't implement `Iterator`).

Fixes rust-lang#127511

Unfortunately, the error in that case is still confusing: it labels `Iterator` as an unsatisfied bound because `&impl Iterator: Iterator` can't be satisfied, despite that not being required or helpful. I'd like to handle that in a separate PR. ~~I'm hoping fixing rust-lang#124802 will fix it too.~~ It doesn't look connected to that issue. Still, I think it'd be clearest to visually distinguish unsatisfied predicates from different attempts at `pick_method`; I'll make a PR for that soon.
mati865 pushed a commit to mati865/rust that referenced this pull request Nov 12, 2024
…kingjubilee

Rollup of 5 pull requests

Successful merges:

 - rust-lang#132755 (Do not reveal opaques in the param-env, we got lazy norm instead)
 - rust-lang#132757 (Get rid of `check_opaque_type_well_formed`)
 - rust-lang#132760 (Don't suggest `.into_iter()` on iterators)
 - rust-lang#132778 (update io::Error::into_inner to acknowledge io::Error::other)
 - rust-lang#132780 (use verbose for path separator suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
4 participants