Proposal: Else clauses for for and while loops by Starwort · Pull Request #3163...

Copy link

This would be redundant given RFC rust-lang/rust#63177

Starwort commented 9 days ago

Perhaps I'm just tired - it's 11:30 PM here - but I fail to see the connection between that pull and this proposal

lebensterben commented 9 days ago •

See https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.try_find

Applies function to the elements of iterator and returns the first true result or the first error.

Your motivating example is

let mut found = false;
for value in data {
   if value == target {
       found = true;
       break;
   }
}
if !found {
   println!("Couldn't find {}", target);
   return;
}

Specifically for the motivating example, you only need

let found = if let Some(value) = data.iter().find(|value| *value == target) {
  true
} else {
  println!("Couldn't find {}", target);
  return;
};

If you think this is too complex, which is not at all, you can use the syntax proposed in the recently-merged let-else PR:

let Some(found) = data.iter().find(|value| *value = target) {
  true
} else {
  println!("Couldn't find {}", target);
  return;
}

If you only care about whether it's found, regardless of the actual matched item, it's simply

if data.iter().find(|value| *value =  target).is_none() {
  println!("Couldn't find {}", target);
  return;
}

For a more general case:

try_findreturns early either on a match (when the closure returns anOk(true)`), or on a error.

For an Iterator<Item = T>, this returns Result<Option<T>>, which contains all the information you possibly need:

• Whether a failure occured
• Whether a match was found and if so the matched item

So you can write something like:

if let Ok(Some(needle)) = hay.iter().try_find(|value| Ok(value == needle)) {
  /* do something */
} else {
  /* do something else */
}

lebensterben commented 9 days ago •

The PR contains the following examples, which can all be written in a concise and readble way without proposed new syntax:

Proposed syntax:

let x = for i in 0..10 {
    if (...) {
        break i;
    }
} else {
    11 // default value
};

With Iterator::find():

let x = (0..10).find(|v| Ok(v == target)).unwrap_or_else(11);

Proposed syntax:

let x = while (...) {
    // do work
    if (...) {
        break Some(value);
    }
} else {
    Some(42) // default value
};

With std::iter::successors() and Iterator::try_find()

use std::iter::successors;

let x = successors(Some(initial_value), |v| {
  Some(/* do something on v */)
}).try_find(|v| {
  if /* outer conditional */ {
    Ok(/* inner conditional */)
  } else {
    Err()
  }
}).unwrap_or_else(42);

You can use std::iter::successors() with Iterator::last():

use std::iter::successors;

let x = successors(Some(initial_value), |v| {
  if /* check on v */ {
    Some(/* do something on v */)
  } else {
    None
  }
}).last().flatten().unwrap_or_else(42);

clarfonthey commented 9 days ago •

I feel like using dedicated syntax for this instead of just iterator adapters does have promise, and could help in a fair few numbers of cases where such adapters aren't possible, e.g. easily returning to the outer function or async code.

I dunno. I like filling in this gap where while and for can't break values, but loop can.

SoniEx2 commented 9 days ago

Honestly would rather have

#![feature(label_break_value)]
fn main() {
    let x = 'foo: {
        for &a in [1, 2, 3].iter() {
            if a % 2 == 0 {
                break 'foo a;
            }
        }
        0
    };
}

In fact, you can do this today, on nightly: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=51e6caacaf3e6fae2d6dd68dfbc56c77

clarfonthey commented 9 days ago

Both comments state that they're open to revisiting this.

Starwort commented 9 days ago

The PR contains the following examples, which can all be written in a concise and readble way without proposed new syntax:

1. I am already planning to follow this up with some real-world examples, I just have current prior arrangements; you can probably expect them some time in the next few days
2. I'm all for one-liners, but (in my subjective opinion) your proposed alternatives lose a lot of readability compared to the original, and even knowing my original code I struggle to understand the purpose of your code samples. I see your point now that it's possible to perform the same function with that PR, but it represents such a massive transformation from the original code that it doesn't feel like the 'right' solution to the problem.

I'm open to changing the else keyword to one such as nobreak if it would make the drawback of programmers not understanding the feature small enough to get approved (although my personal opinion is that else is a perfectly fine keyword for this), and it feels like a more natural solution to the issue than transforming a (potentially large) section of imperative code into a functional style. I'll try to add some more motivating examples after I've got back from work today

Starwort commented 9 days ago •

That said: Here's a piece of code from one of my own projects (a Connect 4 AI). I cannot see a simple way of transforming this into any of the cases you propose; however it uses the exact pattern for/else is supposed to improve (in this case reaching the else clause indicates we've found a diagonal):

let owner = col.pieces[y];
let mut found_line = true;
for offset in 0..4 {
    let col = &self.cols[x + offset];
    if col.count < y + offset {
        found_line = false;
        break;
    }
    if col.pieces[y + offset] != owner {
        found_line = false;
        break;
    }
}
if found_line {
    return Some(match owner {
        Piece::White => GameResult::WhiteWin,
        Piece::Black => GameResult::BlackWin,
    });
}

I'd be interested in seeing how complex the functional approach can get with a real example (although obviously you do not have to write code for me if you do not want to)
EDIT: It occurs to me that my match arms do not work correctly. I will need to figure out a fix for that fixed

lebensterben commented 9 days ago •

@Starwort

let owner = col.pieces[y];

let found_line = &self.cols[x..x + 4]
    .iter()
    .enumerate()
    .find(|(offset, col)| col.count < y + offset || col.pieces[y + offset] != owner)
    .is_none();

if found_line {
    return Some(match owner {
        Piece::White => GameResult::WhiteWin,
        Piece::Black => GameResult::BlackWin,
    });
}

You can make it shorter:

let owner = col.pieces[y];

&self.cols[x..x + 4]
    .iter()
    .enumerate()
    .find(|(offset, col)| col.count < y + offset || col.pieces[y + offset] != owner)
    .is_none()
    .then(|| match owner {
        Piece::White => GameResult::WhiteWin,
        Piece::Black => GameResult::BlackWin,
    })

SoniEx2 commented 9 days ago •

If you consider the label-break-value desugaring:

#![feature(label_break_value)]
fn main() {
    let x = 'foo: {
        for &a in [1, 2, 3].iter() {
            if a % 2 == 0 {
                break 'foo a;
            }
        }
        0
    };
}

then a good keyword for it would be "then":

fn main() {
    let x =
        for &a in [1, 2, 3].iter() {
            if a % 2 == 0 {
                break a;
            }
        } then {
            0
        };
}

but do we really want to add a new keyword vs stabilizing an existing, already-implemented feature?

(Also this is impossible to get wrong: if you forget to break the label you get an error ^-^)

mcarton commented 9 days ago

The “Prior art” section gives really good reasons not to have this syntax.

This feature is in both Python (with identical semantics) and Guarded Command Language. Additionally, there has been a proposal for this feature in Golang (closed by bot) and in JuliaLang (with many people proposing/preferring the Python-influenced semantics used in the JuliaLang thread, in addition to backing up the motivation).

The Go proposal mentioned in the RFC got 23 and no support vote, while the Julia proposal has been open since 2012 with little activity. This seems to indicate that people generally feel no need for such syntax.

Unfortunately, many Python users are unfamiliar with the syntax; of an informal survey, only around 25% knew the meaning and 55% gave an incorrect meaning.

If only 1 out of 4 Python users know what it means, and 1 out of 2 think it means something else, this indicates to me that copying this syntax with this semantic is a very bad idea. It's too niche for most users to know, and not intuitive for users who don't know.

Starwort commented 8 days ago •

It's too niche for most users to know, and not intuitive for users who don't know.

As Rust is a systems language, I would expect that more people would have an understanding of the desugar (at its core, a while loop just uses a guard to goto the loop's beginning; therefore an else can be attached to that guard) than Python, which is a very high-level language. Additionally, the point of that survey was that the responders couldn't look at documentation; those who already know the syntax could use it fine, those who don't can use the guard version, and those who encounter it can look at the feature's documentation and join the first group.

then a good keyword for it would be "then":

I believe I mentioned this but the reason then was rejected was due to it reading as if it always occurs after a loop; with label-break-value this makes sense but it is less obvious that the entire block would be skipped (as the previous one closes first)

@lebensterben: It takes me far longer to parse that code than the equivalent for-else construct (which is really the point of the construct) - I can just about manage but it is definitely harder to read, which kinda supports my point. I'd be interested in a poll between the three four options for readability though (please vote for the most readable version, in your opinion)

let owner = col.pieces[y];
for offset in 0..4 {
    let col = &self.cols[x + offset];
    if col.count < y + offset {
        break;
    }
    if col.pieces[y + offset] != owner {
        break;
    }
} else {
    return Some(match owner {
        Piece::White => GameResult::WhiteWin,
        Piece::Black => GameResult::BlackWin,
    });
}

let owner = col.pieces[y];

let found_line = &self.cols[x..x + 4]
    .iter()
    .enumerate()
    .find(|(offset, col)| col.count < y + offset || col.pieces[y + offset] != owner)
    .is_none();

if found_line {
    return Some(match owner {
        Piece::White => GameResult::WhiteWin,
        Piece::Black => GameResult::BlackWin,
    });
}

    'found: {
        let owner = col.pieces[y];
        for offset in 0..4 {
            let col = &self.cols[x + offset];
            if col.count < y + offset {
                break 'found;
            }
            if col.pieces[y + offset] != owner {
                break 'found;
            }
        }
        return Some(match owner {
            Piece::White => GameResult::WhiteWin,
            Piece::Black => GameResult::BlackWin,
        });
    }

let owner = col.pieces[y];

&self.cols[x..x + 4]
    .iter()
    .enumerate()
    .find(|(offset, col)| col.count < y + offset || col.pieces[y + offset] != owner)
    .is_none()
    .then(|| match owner {
        Piece::White => GameResult::WhiteWin,
        Piece::Black => GameResult::BlackWin,
    })

ETA: did a quick poll of my (small) office and all of them agreed the first is clearest

This comment has been hidden.

clarfonthey commented 8 days ago

I actually do like the idea of then as a keyword a lot. One of my biggest issues with the Python syntax is that the word "else" implies that the block will be run instead of the loop, instead of unless it breaks. With if-else, we mentally separate out the two branches as separate paths, whereas for-else are combined paths.

A for-then or while-then gets rid of that problem. I don't think that adding in an extra keyword should be seen as that big of a detriment, but we technically could use do instead, although I'd hesitate against it since it would be too similar to do-while loops.

To me, for-then says "do this loop, then that" and it's not too difficult to understand that a break would skip the then. Whereas for-else is "do this loop, otherwise do this if it doesn't break," which is much harder to understand.

Starwort commented 8 days ago •

What would you think about the following instead:

for i in 0..2 {
    // code
} broke (value) {
    value
} else {
    other_value
}

The broke clause could be omitted for the semantics as above, would not need the (value) if bare breaks were used, and is probably clearer to those opposing for/else
The loop would still need a consistent type for each break-with-value, but it would only have to match the type of the else if the broke clause was removed.

Does that seem like an acceptable compromise?

N.B. The syntax for the broke clause is based on catch clauses from other languages and could be revised prior to formal introduction to the RFC

Starwort commented 8 days ago

Now that I'm thinking about it more, I actually like while/broke/else even more than my original RFC as common cleanup code could be moved into the broke clause, while still allowing everything the original proposal does

clarfonthey commented 8 days ago

broke feels like it's getting to the point where you're just mimicking try blocks. At that rate, you're replacing break with yeet and catch with broke.

The "else" block avoids storing extra state whereas the "broke" block just calls a function for every broken value.

Starwort commented 8 days ago

That's the primary reason why it would be optional; in most cases for/else would be used but in cases with complicated exit code the broke clause would be used for DRY purposes

PatchMixolydic commented 3 days ago •

#3152 suggests nobreak as a replacement for else (also mentioned in passing in this comment), which significantly clarifies when the second branch is run compared to for-else:

let owner = col.pieces[y];

for offset in 0..4 {
    let col = &self.cols[x + offset];
    if col.count < y + offset {
        break;
    }

    if col.pieces[y + offset] != owner {
        break;
    }
} nobreak { // `k#nobreak` before Rust 2024 if RFC 3098 is merged
    return Some(match owner {
        Piece::White => GameResult::WhiteWin,
        Piece::Black => GameResult::BlackWin,
    });
}

Personally, I'd still prefer the iterator or label_break_value approaches. Neither of them would require reserving a unique keyword or utilizing a strange take on an existing keyword, and the label_break_value solution seems much more readable in my eyes, especially to those unfamiliar with for-else constructs.

ssokolow commented 2 days ago •

As Rust is a systems language, I would expect that more people would have an understanding of the desugar (at its core, a while loop just uses a guard to goto the loop's beginning; therefore an else can be attached to that guard) than Python, which is a very high-level language. Additionally, the point of that survey was that the responders couldn't look at documentation; those who already know the syntax could use it fine, those who don't can use the guard version, and those who encounter it can look at the feature's documentation and join the first group.

As someone who has been programming in Python for ~18 years and is familiar with the Python equivalent to this proposal, I'd say that Rust being a systems language is more reason not to implement such a feature.

Rust is already a fairly complex language and we don't want to become C++. This doesn't feel like it benefits enough to be worth what it costs to the language's complexity budget.

Heck, as someone who was introduced to this kind of else right at the beginning by the O'Reilly Python 2.3 book I learned from, someone who spent a fair amount of time being "too clever for my own good" as far as maintainability goes, and who's gone through just about every programming style Python allows over the last two decades (I still have some codebases that I need to refactor away from "enterprise overload" OOP-style), I think I used for/else maybe once or twice and I'm not sure I ever used while/else.

In my experience, they're just too niche to be worth it.

For the record, I would expect a while/else and for/else to trigger the else if the loop does not loop.

In python, it has this rfc behavior: https://book.pythontips.com/en/latest/for_-_else.html (which seems controversial)
But in Twig, it has an "empty loop behavior" https://twig.symfony.com/doc/2.x/tags/for.html#the-else-clause

The else also forces to check the in-loop behavior and should not be confused with a parent if.

nikomatsakis commented 2 days ago •

@rfcbot fcp close

I thank the author for taking the time to prepare the RFC, but I tend to agree with those who have said that the history of this feature in Python is actually evidence against its inclusion. It's just a bit too niche and confusing, and the meaning of else is not obvious or intuitive (as evidenced by 55% of folks in the survey assigning it the wrong meaning), even if it can obviously be useful. I therefore move to close.

rfcbot commented 2 days ago •

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

durka commented 8 minutes ago

In Python I find this feature both useful (as this RFC says at the beginning, it's a common pattern that is cumbersome to solve with a boolean) and also very confusing.

I can never remember that else means "loop did not break" because, to me, break is sort of an exceptional thing to do, so I first think that should jump to the else clause, the same way exiting early from try ends up in catch... but wait, that's not how for/else works, is it... unless... oh dear, better Google it this time just like every time.

So yeah, I agree this is a problem, but I do not support using else as the solution. Perhaps some macro crates can experiment with other ideas like broke suggested in previous comments.