comprehensive-rust icon indicating copy to clipboard operation
comprehensive-rust copied to clipboard

Don't silence warnings in exercises

Open QuineDot opened this issue 2 years ago • 4 comments

All the exercises start with something like

// TODO: remove this when you're done with your implementation.
#![allow(unused_variables, dead_code)]

You're training people that it's normal and a good idea to ignore warnings during development. Instead, continue to emphasize what you mention in passing elsewhere -- Rust has great compiler errors!

Of course it's going to have a lot of warnings at the start since you've given them incomplete code. You can

  • Just let them deal with it
  • Or mention RUSTFLAGS=-A... cargo ... perhaps

But you shouldn't imply ignoring warnings until you're "done with your implementation" is a good idea, or IMO that ignoring warnings in your source code (as opposed to a command line flag) is a good idea.

QuineDot avatar Dec 27 '22 01:12 QuineDot

But you shouldn't imply ignoring warnings until you're "done with your implementation" is a good idea, or IMO that ignoring warnings in your source code (as opposed to a command line flag) is a good idea.

I generally agree... except that most students use the playground for the exercises. At least for the first 1-2 days. I did it like that to minimize the work I (and other instructors) would have to get things started: I used Linux and know little about how to debug Windows firewall settings which prevents people from installing Cargo and Rust on their laptops.

I added the warnings after the first few classes internally: I noticed that people would get a ton of warnings on the playground and that they had a hard time scrolling around in all the output.

Perhaps there is a way to re-design the exercises to not trigger warnings? That would be ideal. One way would be to comment out the unused code — though that also leads to less coverage by mdbook test.

I would be happy to hear ideas for improving this.

mgeisler avatar Dec 27 '22 16:12 mgeisler

Hey @cchiw, I see you've assigned this issue to yourself. How do you plan on tackling this?

mgeisler avatar Feb 16 '23 10:02 mgeisler

Hola,

After trying it out, I thought we could remove the line (to silence warnings) by either:

  1. Just commenting out the minimal amount of code necessary Example PR. Note might be a lot for some exercises.
  2. Using the unused code. That can include all the fields in a struct Example PR or parameters to a function Example PR and Example.

What do you think? @mgeisler

cchiw avatar Feb 17 '23 19:02 cchiw

  1. Just commenting out the minimal amount of code necessary Example PR. Note might be a lot for some exercises.

This is the style where you comment out the functions in the exercise, but leaves them active in the solution? Like this on the exercise page:

// fn compare_with_10(x: i8) -> bool {
    unimplemented!()
}

and this in solution.rs:

fn compare_with_10(x: i8) -> bool {
    x == 10
}

The problem with this is that we lose the connection between the exercise page and the solution.

  1. Using the unused code. That can include all the fields in a struct Example PR or parameters to a function Example PR and Example.

I'm less of a fan of this solution, as I just wrote on #391: I don't think adding print!("Use {foo}") statements is a good way to silence the warnings. Why not? Because it not how actual code looks like. It removes the warnings without actually helping the students (with the #![allow(unused_variables, dead_code)] line, students can easily double-check that there is no dead code left — we remove that ability if we put in spurious print! statements).

I think there could be an option 3 and 4

  1. Let the students deal with the warnings — they might even use them as a guide for what to implement next. That's basically what @QuineDot suggested above.

  2. Use function arguments in the todo! macro like this:

    // fn compare_with_10(x: i8) -> bool {
        todo!("Compare {x} with 10")
    }
    

    This results in a single warning for the function:

    warning: function `compare_with_10` is never used
     --> src/main.rs:1:4
      |
    1 | fn compare_with_10(x: i8) -> bool {
      |    ^^^^^^^^^^^^^^^
      |
      = note: `#[warn(dead_code)]` on by default
    

    The good thing about this approach is that a) we can reuse the function signatures when stating the exercise, b) it's more or less normal code and c) the students only get one warning per function, instead of a warning per parameter.

mgeisler avatar Mar 25 '23 10:03 mgeisler