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

Remove .iter() calling suggestion when Iterator isn't implemented #52178

Closed
wants to merge 1 commit into from

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented Jul 9, 2018

Closes #50773
r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2018
@varkor
Copy link
Member

varkor commented Jul 9, 2018

This is not the correct fix. Often, when users try this, calling .iter() is what they want to do. What you want to do instead is check to see whether it's possible to call .iter(), and only suggest calling it if it's available. In other words, the suggestion should be contextual, otherwise we regress the error messages.

@csmoe
Copy link
Member Author

csmoe commented Jul 9, 2018

@varkor this error lint only emits when Iterator is not implemented for given expr as the error message says:
the trait bound u8: std::iter::Iterator is not satisfied.

@estebank
Copy link
Contributor

estebank commented Jul 9, 2018

The changes to the tests are a net improvement, but we would fail to recommend iter() when appropriate, which granted, I don't think is possible at the moment using rustc_on_unimplemented. Let me get back to this soon once I've had some time to think about it.

@csmoe csmoe added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jul 14, 2018
@kennytm kennytm removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Jul 14, 2018
@stokhos
Copy link

stokhos commented Jul 21, 2018

Ping from triage! @estebank any update on this PR?

@TimNN
Copy link
Contributor

TimNN commented Jul 24, 2018

Ping form triage, @estebank / @rust-lang/compiler: This PR requires your review.

@estebank
Copy link
Contributor

If we merge this change we'll need to cut a new ticket to go back to the drawing board and enable the compiler to suggest using .iter() when appropriate. @csmoe, do you mind if we keep this PR open until we come up with a clearer plan for that? Otherwise, at the very least we should add a rustc_on_unimplemented check for array literals, where .iter() is the right suggestion:

error[E0277]: the trait bound `[{integer}; 3]: std::iter::Iterator` is not satisfied
 --> src/main.rs:3:14
  |
3 |     for y in x {
  |              ^ `[{integer}; 3]` is not an iterator; maybe try calling `.iter()` or a similar method
  |
  = help: the trait `std::iter::Iterator` is not implemented for `[{integer}; 3]`
  = note: required by `std::iter::IntoIterator::into_iter`

Doing so would imply modifying the flags supported by rustc_on_unimplemented (like in this PR) to signal that the type is an array. This work can happen as a separate PR, but I would want to merge all of them within a short period of time to avoid a stable release with a regression in the suggestions.

@csmoe
Copy link
Member Author

csmoe commented Jul 26, 2018

@estebank ok, I'll leave this open.

@csmoe csmoe added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 26, 2018
@TimNN
Copy link
Contributor

TimNN commented Aug 28, 2018

Ping from triage! What is the status of the changes proposed here / the changes required to merge this PR?

Per our current triage guidelines, a blocked PR will generally be closed after two weeks if no immediate progress is expected. Since there is no issue tracking the blockage of this PR, I wanted to ask about the current status before closing.

@estebank
Copy link
Contributor

@TimNN created #53766 to track the feature needed before being able to merge this PR.

@csmoe I'm closing this PR and will reopen it once the above ticket is fixed, to keep the queue clean. Will you have time in the future to rebase against master and introduce the use of the new filter into this?

@estebank estebank closed this Aug 28, 2018
@csmoe
Copy link
Member Author

csmoe commented Aug 28, 2018

@estabank yes, I'll keep this in my todo until that happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants