rustlings icon indicating copy to clipboard operation
rustlings copied to clipboard

clippy2 hint / clarification

Open carneeki opened this issue 4 years ago • 2 comments

I'm running from v4.4.0; the hint for clippy2 might be confusing. Currently it reads

`for` loops over Option values are more clearly expressed as an `if let`

The for implies that there might be multiple elements to be checked, but only a single Option is present. If the desire is for a student to use the if let syntax, I suggest rewording:

`for` loops over a single Option value are more clearly expressed as an `if let`

But this got me thinking... while let covers both a single and multiple cases, and perhaps that is more preferable in general?

`for` loops over Option values are more clearly expressed as a `while let`

As in

fn main() {
    let mut res = 42;
    let option = Some(12);
    while let Some(x) = option {
        res += x;
    }
    println!("{}", res);
}

But also, I feel like I didn't learn anything clippy specific at all in the clippy exercises compared to previous exercises. This leaves me wondering if I did them correctly or not.

carneeki avatar Jun 26 '21 21:06 carneeki

Without modifications the output is...

! Compilation of exercises/clippy/clippy2.rs failed!, Compiler error message:

    Checking clippy2 v0.0.1 (/home/makolb/Source/Repos/rustlings/exercises/clippy)
error: for loop over `option`, which is an `Option`. This is more readably written as an `if let` statement
 --> clippy2.rs:9:14
  |
9 |     for x in option {
  |              ^^^^^^
  |
  = note: `#[deny(clippy::for_loops_over_fallibles)]` on by default
  = help: consider replacing `for x in option` with `if let Some(x) = option`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles

error: aborting due to previous error

error: could not compile `clippy2`

So the hint is referring the clippy error: error: for loop over option, which is an Option. This is more readably written as an if let statement.

But you learn that Option is loopable/iteratable because it implements IntoIterator-trait and provides iter().

Clippy provides you the solution for better readable code with the if let, although the unchanged code would compile without strict clippy.

Maybe the hint should explain why and when you'd like to iterate an Option?!
Which make IMO only sense in combination with other iteratables like in Iterating over an Option.

Overall this explains also that for Option it would be more readable to use if let Some(x) = option instead of while let, because this would never fail assert!((0..=1).contains(&option.iter().len())) and there is is nothing to loop.

kolbma avatar Jun 29 '21 17:06 kolbma

Maybe the hint should explain why and when you'd like to iterate an Option?!

Absolutely! This would be a great thing to learn in Rustlings, I'd suggest in a lesson on options or iterators! -- however --

This is stepping away from a lesson about Clippy. I feel this listen hasn't really taught me much about Clippy. To improve it, I'd like to propose the following:

  1. Starting off with an example that compiles, but using a method that would normally generate a warning.
  2. The first task would be to configure Clippy such that the example throws an error instead of a warning.
  3. The second task student must complete the exercise by changing the code (rather than the Clippy config) to compile with the re-configured Clippy.

That would make the lesson more about Clippy and its use (particularly for projects that may prefer enforce specific lint options). The first task would demonstrate being able to configure these lints and the second task to adhere to them.

carneeki avatar Jul 03 '21 07:07 carneeki