Move+return in a for loop "moving out of captured outer immutable variable in a stack closure"

[SimonSapin]

Hi,

I have some code similar to this:

fn test() -> Option<~str> {
    let moved = ~"";
    for (~[1]).each |_| {
        return Some(moved)
    }
    None
}

Compiler output:

move_for.rs:4:20: 4:25 error: moving out of captured outer immutable variable in a stack closure
move_for.rs:4         return Some(moved)
                                  ^~~~~
error: aborting due to previous error

Making the variable mutable does not helpe. I am confident that this move should be fine: the variable is not used afterwards because of the return statement. A very similar code compiles fine when no closure is involved:

fn test() -> Option<~str> {
    let moved = ~"";
    loop {
        return Some(moved)
    }
}

Am I seeing a guarantee that the compiler doesn’t or is the compiler seeing a risk that I don’t?

[SimonSapin]

A work-around is to use a swap and a dummy value:

let temp = ~"";
temp <-> moved;
return Some(temp)

Not exactly convenient…

[Dretch]

I can only guess that the compiler complains because if vec.each didn't implement the for loop protocol correctly (which the compiler can't check) then the closure might be called twice.

[SimonSapin]

@Dretch, would that be a problem with return? I think there is magic rewriting return statements in for loops, but I don’t know the details. The outer function is returned, not just for’s closure.

[catamorphism]

Without looking at the compiler code that does this, my guess is that borrowck doesn't treat return inside a for-loop iterator specially, even though it is special (because it returns out of its enclosing function). The rule it's applying is: "You can't capture a variable x inside a closure and then move out of it, because you don't know who else is pointing to x." This rule is generally correct; the one place where it would be safe to break it, though, is the case here, where it's a return expression that captures x. I think borrowck just doesn't take that special case into account, and is being over-conservative.

I might be missing something here, though, since it's late :-)

[nikomatsakis]

The borrow checker is not being overly conservative. return from inside a for loop is not guaranteed to actually return if there is a bug in the each() method, as @Dretch pointed out. I think the best we can hope for here is perhaps an improved error message, but I don't know how one could improve it exactly.

[SimonSapin]

Is there a nicer work-around than temporary variables and <->? rust-cssparser/tree.rs#L236

Simplified a bit:

fn consume_declaration_value(parser: &Parser, name: ~str) -> Declaration {
    let mut name = name; // XXX see <-> below
    let mut value: ~[Primitive] = ~[];
    for parser.each_token |token| {
        match token {
            Delim('!') => {
                let mut name_ = ~"";
                let mut value_: ~[Primitive] = ~[];
                name_ <-> name;
                value_ <-> value;
                return consume_declaration_important(parser, is_nested, name_, value_)
            },
            Semicolon => break,
            _ => value.push(process(token))
        }
    }
    // Reached a Semicolon or EOF.
    Declaration{name: name, value: value, important: false})
}

[jruderman]

Why can't the compiler check that each() implements the for loop protocol correctly?

[nikomatsakis]

@SimonSapin the usual way is the so-called "option dance":

let mut name = Some(name);
...
for parser.each_token |token| {
    match token {
        Delim('!') => {
            let name = name.swap_unwrap(); // or `Option::swap_unwrap(&mut name);`
            ...

@jruderman in principle such a thing might be possible but it'd involve a new analysis that we haven't even begun to think about.

[SimonSapin]

@nikomatsakis good to know… Now I’m trying to decide which is less ugly.

[Dretch]

Could the generated for loop code fail at runtime if the return-inside-the-closure is called more than once? I think that would mean the return would be able to move safely.

[jruderman]

For the compiler to check that each() implements the for loop protocol correctly, you'd have to tell it which functions you intend to use with for loops. One possibility is declaring it as a loop fn rather than fn.

It seems preferable to put restrictions on the loop fns rather than there callers, because there are fewer loop fns and most of them are simple.

[SimonSapin]

If loop fn becomes a thing, does that make Python-style for…else constructs possible? The else part is executed after the for loop, except if break was used.

[nikomatsakis]

@SimonSapin actually the revised for loop protocol I suggested in a recent blog post would make for...else possible irrespective of whether we implement loop fn or not.

[nikomatsakis]

@jruderman interesting idea. I wonder how hard it'd be to implement a simple static analysis for this. It does seem plausible.

[mneumann]

I like the revised loop protocol very much. Seems absolutely plausible to do it that way! About for ... else, I find the naming a bit strange and I would better rename it to for ... then. Which could further extended to:

for list.each |i| {
  if i == 2 { break }
}
then {
  // on full exhaustion
}
else {
  // on "break"
}

And we could easily make the return value of for the unit type, while the return type of for ... then ... else (or any variant of it; for ... then or for ... else) would be that of the then/else block. For example this would make something like:

let mut found = false;
for list.each |i| {
  if i == 2 { found = true; break }
}
if found { ... }

turn into:

let found = for list.each |i| {
  if i == 2 { break }
} then { false } else { true }
if found { ... }

Well, not sure if it is really usable, though maybe there are some examples where this could be useful.

[graydon]

non-critical for 0.6, de-milestoning

[catamorphism]

Nominating for maturity milestone 1, "well-defined"

[graydon]

declined for milestone; maybe-someday

[bstrie]

@thestinger's for proposal would obviate this issue.

[SimonSapin]

@bstrie , do you mean changing the semantics of for-loops to use external iterators and make Iterator::advance a no-op? Yes please!

[SimonSapin]

In the meantime, std::util::replace makes this a bit less painful.

[SimonSapin]

I just came up with this macro to do for-loops on iterators without involving closures, thus eliminating this whole class of issues:

macro_rules! for_iter(
    ($iter: expr, $pattern: pat, $loop_body: expr) => (
        loop {
            match $iter.next() { None => break, Some($pattern) => $loop_body }
        }
    );
)

Usage:

    for_iter!(flux_capacitor_list.iter(), flux_capacitor, {
        feed_jigowatts(flux_capacitor)
        // …
    })

The }) at the end is unpleasant, but better than other workarounds.

When the semantics of for are changed to external iteration, the above can be replaced by:

    for flux_capacitor_list.iter() |flux_capacitor| {
        feed_jigowatts(flux_capacitor)
        // …
    }

[thestinger]

The old for loop is being removed, see #6997.