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

suggest removing & when iterating over &<some iterator type> #47744

Closed
spastorino opened this issue Jan 25, 2018 · 7 comments
Closed

suggest removing & when iterating over &<some iterator type> #47744

spastorino opened this issue Jan 25, 2018 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@spastorino
Copy link
Member

504 | /         for (_idx, _constraint) in &self.constraints.iter().enumerate() {
505 | |         }
    | |_________^ `&std::iter::Enumerate<std::slice::Iter<'_, borrow_check::nll::region_infer::Constraint>>` 

I started going for constraint in &self.constraints { then move to use enumerate and forgot to remove the &.

I figuref that the error could say maybe remove the &, calling iter and having & doesn't make sense.

@nikomatsakis nikomatsakis added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 25, 2018
@nikomatsakis nikomatsakis changed the title Make this Iterator example's error message better suggest removing & when iterating over &<some iterator type> Jan 25, 2018
@nikomatsakis
Copy link
Contributor

cc @estebank

@ysiraichi
Copy link
Contributor

@nikomatsakis Can I give it a shot?

@estebank
Copy link
Contributor

@ysiraichi feel free to do so! Also feel free to reach out either on gitter or on this issue for help.

You'll have to look at librustc/traits/error_reporting.rs:report_selection_error (which is where the error E0277 is being emitted). There you have to verify wether self_ty is a &-ref, and wether the referenced ty does implement the trait being that failed to be found, std::iter::IntoIterator::into_iter in this case. Doing that second part is the hard part and I would have to dig a bit in order to be able to help you with it, others in the compiler team might have more context around it.

@nikomatsakis
Copy link
Contributor

There you have to verify whether self_ty is a &-ref

That doesn't quite seem like enough -- the type might be an & without the expression itself being a borrow expression. e.g., something like this:

let x = &(1..2);
for y in x { ... }

But indeed that is one of the tricky bits here: the place where the error is reported is rather 'divorced' from the original source code. We might be able to consult the cause information to find out where the error came from, I suppose, and hack something in that way?

@estebank
Copy link
Contributor

The further removed the borrow is from the use location the less likely you're to be able to change it without breaking something else. If the type is copy or clone, I'd instead suggest using * instead. Also, I thought that even if it is a binding a ref or what have you, you can follow the Tys until you get to a TyAdt.

@cuviper cuviper added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jan 27, 2018
@nikomatsakis
Copy link
Contributor

@estebank I think we should literally look for the case where the & appears in the for, and only that, specifically because of the danger of going wrong when doing this:

for x in &vec

to

for x in vec.iter().filter(...)

Also, adding a * will not help :( -- you can't move an iterator out from behind a borrowed reference

kennytm added a commit to kennytm/rust that referenced this issue Mar 19, 2018
…ebank

Suggest removing `&`s

This implements the error message discussed in rust-lang#47744.
We check whether removing each `&` yields a type that satisfies the requested obligation.
Also, it was created a new `NodeId` field in `ObligationCause` in order to iterate through the `&`s. The way it's implemented now, it iterates through the obligation snippet and counts the number of `&`.

r? @estebank
@estebank
Copy link
Contributor

Repro case for test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants