Have for pattern in iter not borrow the iterator

[SimonSapin]

Test case:

let mut iter = range(1, 10);
for i in iter {
    printfln!(i);
    // error: cannot borrow `iter` as mutable more than once at a time
    printfln!(iter.next());
}

The above currently fails, but I think it should be valid and equivalent to:

let mut iter = range(1, 10);
loop { match iter.next() { None => break, Some(i) => {
    printfln!(i);
    printfln!(iter.next());
}}

In other words, the for loop should only borrow the iterator long enough to call next(), leaving the loop body free to use it.

I understand that this might not be possible when a more complex expression is used as the iterator. The for loop needs to evaluate the expression once and keep the result in an anonymous variable, thus borrowing. Maybe there could be a special case when the iterator expression is "simple" enough: a variable, struct field access, etc?

[thestinger]

This means implementing it as more than a simple parser feature. It's not possible to check the type of the passed in variable at the moment and decide whether a temporary is needed.

[huonw]

@thestinger for is likely to need to be a non-parser feature anyway: it is currently duck-typed, and gives horrible error messages. (As I'm sure you know.)

[SimonSapin]

@thestinger what you say might still be true, but this is the type of AST node (eg. identifier) for the iterator, not Rust type (eg. std::vec::VecIterator.)

[SimonSapin]

Also, this is a "nice to have", but not blocking because I can just use loop with match. I even have a macro for it:

// Work around "error: cannot borrow `iter` as mutable more than once at a time"
// when using a normal for loop.
macro_rules! for_iter(
    ($iter: expr, $pattern: pat, $loop_body: expr) => (
        loop {
            match $iter.next() { None => break, Some($pattern) => $loop_body }
        }
    );
)

[bluss]

Your macro should probably take an ident, not an expression since it doesn't work well with expressions!

[SimonSapin]

Good point, @blake2-ppc.

// Work around "error: cannot borrow `iter` as mutable more than once at a time"
// when using a normal for loop.
macro_rules! for_iter(
    ($iter: ident, $pattern: pat, $loop_body: expr) => (
        loop {
            match $iter.next() { None => break, Some($pattern) => $loop_body }
        }
    );
)

[huonw]

triage, no progress.

[pnkfelix]

It would be good to decide what we're going to do about these things. (This and #8649)

But its not clear whether it should block 1.0.

[nikomatsakis]

cc me

[ezyang]

Maybe there could be a special case when the iterator expression is "simple" enough: a variable, struct field access, etc?

It seems to me that there is precedent for language constructs to act specially when the expression in question is an lvalue, c.f. match; that would be a natural choice here.

[emberian]

Another painful example:

fn main() {
    let s = "foobar bazbang";
    let mut chars = s.chars();

    loop {
        match chars.next() {
            Some(c) => {
                if c == ' ' {
                    chars.next();
                } else {
                    print!("{}", c);
                }
            },
            None => { break; }
        }
    }
}

is good, but:

fn main() {
    let s = "foobar bazbang";
    let mut chars = s.chars();

    for c in chars {
        if c == ' ' {
            chars.next();
        } else {
            print!("{}", c);
        }
    }
}

is not.

[bluss]

Is it possible to migrate to a for-by-value by recommending users who need the reference behaviour to use Iterator::by_ref() ?

[huonw]

@blake2-ppc could you write the expansion you are thinking of? (I'm specifically wondering how that would allow for something like for x in foo.iter() { ... }, while also allowing for x in bar { bar.next() } to do the right thing.)

[pczarn]

Solved by #15809

[SimonSapin]

@pczarn, indeed it is, thanks for the heads up! Updated for language/lib changes, the initial test case now works as expected:

fn main() {
    let mut iter = range(1i, 10);
    for i in iter {
        println!("{}", i);
        println!("{}", iter.next());
    }
}

1
Some(2)
3
Some(4)
5
Some(6)
7
Some(8)
9
None

[SimonSapin]

Re-opening, see #17101.

[huonw]

The implementation we had was incorrect and memory unsafe.

[mahkoh]

That's what it says in #17101.

[pnkfelix]

@mahkoh I do not think your macro properly captures what people expect from for-loops.

In particular, I ported the example from #16820 over to the macro above, and (even after generalizing it to take a pat and an expr instead of two idents), you can see a clear problem: http://is.gd/ZeRT1O
Namely, the expression for the iterator is re-evaluated on each iteration through the loop. You did not observe this in your version because you had restricted your version to take an ident.

But then if you fix that problem in the "obvious" manner, you are back where you started: http://is.gd/3EQAly

This is the problem that one must solve in some manner, the one that I referred to in my comment here: #17101 (comment)

[mahkoh]

@pnkfelix: I chose ident over expr precisely to avoid this re-evaluation problem. This issue is about iterators that are stored in identifiers and not arbitrary rvalue iterators.

I don't know how the for loop works, but if it simply rewrites the AST so that it looks like a loop + match, then it could rewrite it to the version in my macro above if the iterator is an identifier and it could rewrite it to a version with a temporary variable otherwise.

[SimonSapin]

I’d expect the for loop to have a special case when the iterator expression can be re-evaluated without side effects (e.g. an ident, a field access, etc.) and behave like the macro. Only when necessary should the loop have a temporary to avoid re-evaluation.

[huonw]

Yeah definitely, the idea is sound, the implementation just wasn't actually implementing that idea.

(I have to admit, I'm confused why @mahkoh decided to ask "why was this removed?" despite acknowledging that they knew the reason was the implementation was wrong. In any case, the situation we have now is just a temporary revert back to the old behaviour that we had when for was a parser feature, i.e. there was only a narrow window (9 days) in which this was "fixed", and so I wouldn't expect this to really break that much.)

[mahkoh]

From the comments in this thread it looks like this worked from July 30th until at least September 5th. I don't see where you get the 9 days from. And I don't understand why the rvalue bug had to be fixed so urgently that this case had to be removed with it. Even simply checking if the "iterator" is a single identifier would have kept the cases discussed in this issue working. I don't know if the old version was also unsafe in that case, but if not, then simply adding an if-block around the code added by @pcwalton should do the trick, no?

[huonw]

(Oh, whoops, read those dates wrong; even so, only 40 days, and we had lived with this for nearly a year before that.)

Being memory safe and correct is very urgent: as it stood, our for loops allowed for iterator invalidation in safe code. Squashing those problems is significantly more important than relatively minor features like this, which has perfectly good synonyms, as your macro demonstrates and as everyone who met this before July 30th knew. In fact the replacement is just 2 lines: for pat in iter { becomes

loop {
    let pat = match iter.next() { Some(x) => x, None => break };

This doesn't seem particularly burdensome compared to 'have an incorrect compiler'.

The fixing-the-horrible-bug patch is being conservative, but it is has the rather large advantage of being correct (and is clearly so).

[TyOverby]

Am I correct in my assumption that this could be fixed very easily with a syntax extension? I don't see how something so simple that macro_rules could solve it could lead to an "incorrect compiler".

[thestinger]

It's simple to write a syntax extension for lvalues where it doesn't borrow the iterator within the body of the loop. It does have to borrow the iterator temporarily at the top of the loop body to call next. The compiler's borrow checking rules would need to gain more understanding of the for construct since it's all wired into the compiler.

[SimonSapin]

@TyOverby Sure, this can be done with macro_rules! (see earlier in this thread). So can every for loop, but that doesn’t mean we should remove for from the language.

It’s unfortunate that we have to switch to an entirely different syntactic construct for something this semantically is a for loop, just because the current implementation of for borrows too conservatively.

[Stebalien]

You can now just use while let:

while let Some(item) = iterator.next() {
    // ...
}

It's not perfect but but better than using a macro (IMHO).

[SimonSapin]

Oooh, this is much nicer indeed. In particular since there is only one nesting level of {}. Thanks for the tip!

[smosher]

As far as blocking 1.0 goes, it won't bother me personally if this doesn't get fixed until after. But it might be confusing and offputting to newcomers who already have a lot to learn about ownership. (I know I've invented a few new things to say after "why the ...?" beacuse of this issue.)

Might it be possible to detect the situation and emit a hint about the workaround?

[nikomatsakis]

Now that while let is an option, I am not sure if this should be
fixed. Particularly since for was defined to invoke IntoIterator
on its argument -- and thus in a sense takes ownership of its argument
(though not quite, is that a backwards compat hazard @aturon?). If you
plan to mess with the iterator itself during iteration, it seems to me
that using a distinct kind of loop is a good indication of this (I
certainly don't expect it when I see a for loop).

On Thu, Dec 18, 2014 at 07:46:54PM -0800, Stephen Mosher wrote:

As far as blocking 1.0 goes, it won't bother me personally if this doesn't get fixed until after. But it might be confusing and offputting to newcomers who already have a lot to learn about ownership. (I know I've invented a few new things to say after "why the ...?" beacuse of this issue.)

Might it be possible to detect the situation and emit a hint about the workaround?

---

Reply to this email directly or view it on GitHub:
#8372 (comment)

[emberian]

I used to be annoyed about this a ton, but now that we have while let,
I agree.

On Sat, Dec 20, 2014 at 9:08 PM, Niko Matsakis notifications@github.com
wrote:

Now that while let is an option, I am not sure if this should be
fixed. Particularly since for was defined to invoke IntoIterator
on its argument -- and thus in a sense takes ownership of its argument
(though not quite, is that a backwards compat hazard @aturon?). If you
plan to mess with the iterator itself during iteration, it seems to me
that using a distinct kind of loop is a good indication of this (I
certainly don't expect it when I see a for loop).

On Thu, Dec 18, 2014 at 07:46:54PM -0800, Stephen Mosher wrote:

As far as blocking 1.0 goes, it won't bother me personally if this
doesn't get fixed until after. But it might be confusing and offputting to
newcomers who already have a lot to learn about ownership. (I know I've
invented a few new things to say after "why the ...?" beacuse of this
issue.)

Might it be possible to detect the situation and emit a hint about the
workaround?

---

Reply to this email directly or view it on GitHub:
#8372 (comment)

—
Reply to this email directly or view it on GitHub
#8372 (comment).

http://octayn.net/

[SimonSapin]

I still kinda wish this was fixed, but I understand @nikomatsakis’s argument and I don’t mind as much with the while let alternative.

[japaric]

#20790 implemented the new for-loops that take the iterator by value, so this issue can't be fixed (without changing the semantics in a backwards incompatible way). This should be closed.

[SimonSapin]

Fair enough. I’m happy with the while let Some(value) = iterator.next() { trick.