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

Forgetting .iter() in for loop causes confusing type error message #8649

Closed
catamorphism opened this issue Aug 20, 2013 · 15 comments
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-type-system Area: Type system P-medium Medium priority

Comments

@catamorphism
Copy link
Contributor

I accidentally wrote:

for p in foo() { ... }

where foo returns a vector. The error:

search.rs:31:4: 37:8 error: type `&mut ~[std::path::PosixPath]` does not implement any method in scope named `next`

was not exactly useful. The problem was that I left out the .iter() after foo(). But my first instinct would be to look for where I'm calling next(), and I'm not calling it explicitly.

@thestinger
Copy link
Contributor

I think the only way to completely fix the strangeness of these errors would be to implement for loops in the compiler beyond just a transformation on the AST.

We would also need that to fix issue #8372, and possibly to support both an Iterable trait and Iterator.

@emberian
Copy link
Member

This is pretty bad, and should probably be fixed for 1.0

@nikomatsakis
Copy link
Contributor

A hacky fix is to have a global function like

fn next<T,I:Iterator<T>>(i: &mut I) -> Option<T> {
    i.next()
}

and call that from the generated function. It'd give an improved
error message. It's obviously not a good long-term solution.

@pnkfelix
Copy link
Member

We should do something here for 1.0, if only the hack niko suggested.

@eddyb
Copy link
Member

eddyb commented Jan 9, 2014

I think there's a better hack (just one call, more of a noop, relevant name):

{ // in for loop expansion:
    let iter = ::std::iter::assert_iter(&mut rhs);
    ...
}
// in std::iter
pub fn assert_iter<'a, T, I: Iterator<T>>(iter: &'a mut I) -> &'a mut I {iter}

An Iterable trait is also desired, to remove some verbosity (and getting an Iterator from an Iterable could go through a library function as well).

@thestinger
Copy link
Contributor

Adding more code to the macro is going to cause further problems at --opt-level=0 and --opt-level=1. It's also one more thing to bring into scope when you aren't using the prelude.

@nikomatsakis
Copy link
Contributor

Let's not overstate: s/cause further problems/"mildly lenghten compile times"/

@thestinger
Copy link
Contributor

@nikomatsakis: I don't think we can ignore performance without optimizations enabled, because LLVM doesn't offer support for debugging while optimizing like GCC (-Og, -fvar-tracking-assignments). A demanding interactive application could easily become unusable without optimizations...

It should just be done properly instead of adding more hacks. Every hack we add makes it less likely that it's going to be written properly any time soon. Giving sane error messages and not bloating the code are important.

@nikomatsakis
Copy link
Contributor

@thestinger Here are my thoughts:

  1. I think that expanding for into equivalent code is actually the correct way to implement it. Writing custom code in trans, borrow checker, etc is a waste of time and just going to lead to more bugs and a more confusing compiler.
  2. However, we ought to give better error messages, such that the expansion of for into equivalent code is not user visible.
  3. I have no idea how much impact it would have on compile time to add an extra function call, or use a next() helper instead of calling iter.next() directly, but I suspect it is minimal. Measurements might be helpful.

In my ideal world, we would build up some infrastructure for doing this sort of rewriting. I know that some (e.g. @pcwalton) have their doubts about the utility of this. However, if we were careful we could reduce the Rust language to a truly small set of operations. This would have the further benefit that this would be quite close to a language that we can formalize and prove sound etc etc.

@nikomatsakis
Copy link
Contributor

@thestinger I guess what I mean is, I'd rather live with the current hack, especially if we can improve end-user experience, then invest effort writing custom code for for in the various phases of the compiler, since I don't think that's the way it ought to be done. This is particularly true as we push towards 1.0. That said, I do think we should avoid having a "duck typing" scenario for 1.0, since that is not what we intend to support in the future.

@thestinger
Copy link
Contributor

@nikomatsakis: I think it's important that it doesn't unnecessarily borrow an lvalue for the loop body though. I don't really see how this can be done without it being part of the compiler. You should be able to use the iterator within a for loop using the iterator.

for x in xs.iter() {
    if cond(x) {
        xs.next(); // skip the next value
    }
    foo(x);
}

@nikomatsakis
Copy link
Contributor

@thestinger I guess you mean for x in xs { -- however, this could be fixed without further integration by making the expander expand into one of two different expansions, depending on whether the path to the iterable is a "pure lvalue" or not. Anyway, just to be clear, I'm not claiming the current setup is ideal, though I think it does have some desirable qualities that I'd like to find a way to preserve.

@pnkfelix
Copy link
Member

@thestinger I thought you currently couldn't write that code anyway, since it is pretty much the exact example motivating issue #8372 ? (Or are you just providing this as a separate argument for why iterator support should be integrated into the compiler rather than left as a preprocessor expansion?)

@thestinger
Copy link
Contributor

@pnkfelix: I'm providing it as an example for why integrating it into the compiler would be nice. If we can have a branch doing this in the expansion, then it doesn't matter.

@alexcrichton
Copy link
Member

Closed by #15809

flip1995 pushed a commit to flip1995/rust that referenced this issue Jun 30, 2022
add [`manual_find`] lint for function return case

part of the implementation discussed in rust-lang#7143

changelog: add [`manual_find`] lint for function return case
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 A-type-system Area: Type system P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

7 participants