rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.67k stars 12.63k forks source link

Suppress warnings caused by todo!() #75277

Open SOF3 opened 4 years ago

SOF3 commented 4 years ago

todo!() is often used to quickly fill expressions that the developer currently does not have time/want to finish. However, use of todo!() often leads to the unreachable_code warning since it panics and the subsequent is not executable.

It is currently possible to suppress this warning at the crate level, #![allow(unreachable_code)], but this would result in suppressing legitimate unreachable code bugs as well. On the other hand, it is also possible to suppress this warning for each function that uses todo!(), but this introduces significantly more boilerplate (compare #[allow(unreachable_code)] 26 chars, need to move cursor to declaring block, vs todo!(), 7 chars in place).

According to the docs for todo!(), "todo! conveys an intent of implementing the functionality later". This implies that the TODO itself is already implies the current code is meant to be somewhat incorrect, and it is unlikely that a developer writing code after todo!() does not know it is unreachable.

Hence, subsequent code after todo!() shall not be considered unreachable, and the compiler shall suppress such warnings that would not have happened without todo!().

nbdd0121 commented 4 years ago

todo!() gives a ! and considering the code after it reachable would basically just mess up the reachability analysis. If you intend to fill it later, why would a warning do any harm?

SOF3 commented 4 years ago

In some larger projects, some values might be computed by somewhat complex logic that needs longer time to think about how to write, and one might create a lot of them quickly. For myself, I often use todo!() to fill in missing expressions and compile the crate before I go on so that I can e.g. make sure I didn' t make any type errors. Reachability warnings interferes with this workflow greatly as they are mixed with actual warnings (and maybe errors0 that might matter.

Mubelotix commented 3 years ago

There should be no warning of unreachable code with todo!, but there should be a warning for unimplemented code. It's easy to forget that there are still todo!s in the code.

detly commented 1 year ago

One of the problems is that these warnings cascade, and that's unhelpful. For example:

struct Magic(u32);

fn fully_fleshed_out_function(magic: Magic) -> Option<u32> {
    Some(magic.0)
}

pub fn main() {
    #![allow(unreachable_code)]

    // Gosh, I don't know how to make one of these yet.
    let magic: Magic = todo!();

    // But I know what to DO with it!
    let result = fully_fleshed_out_function(magic);

    println!("Result: {result:?}");
}

Even with the lint off, I get a warning:

   Compiling playground v0.0.1 (/playground)
warning: unused variable: `magic`
  --> src/lib.rs:11:9
   |
11 |     let magic: Magic = todo!();
   |         ^^^^^ help: if this is intentional, prefix it with an underscore: `_magic`
   |
   = note: `#[warn(unused_variables)]` on by default

warning: `playground` (lib) generated 1 warning (run `cargo fix --lib -p playground` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 0.44s

The thing is though, magic is not unused from the perspective of "designing the calling code". Yes yes, the divergence at todo!() means that it never gets used in a running program, but if todo!() is actually meant to be an aid to design rather than a straight synonym for panic!(), this warning simply isn't helpful.

If there is more code after the todo!(), you get warnings for all of it. And warnings for any helper functions called only after that. And for any data types used only after that. And the modules that they're in. etc. There's no way to distinguish these from warnings unrelated to the todo!(). Linting unfinished-but-serious code becomes less valuable because of this.

I suppose what I'm asking for is a way to "launder out" the un-used-ness of magic, or to silence warnings by provenance, but I don't know how either of those things could be implemented.

jcdyer commented 1 year ago

Also, the unused warning about magic is simply wrong from the perspective of the type checker. Whether the code after the todo!() gets run or not, the program would fail to compile if you change let magic to let _magic, because the variable is used in fully_fleshed_out_function. I shouldn't have to write fully_fleshed_out_function(_magic) to satisfy the linter.

There should be no warning of unreachable code with todo!, but there should be a warning for unimplemented code. It's easy to forget that there are still todo!s in the code.

I'd be good with this solution. One of my problems with the current state of things is that the warning lands on the code after the todo, which is generally finished code, whereas the code that needs addressing is the todo!() itself. That's where the little squiggles should go.

This is what I'm looking at instead: image

ntc2 commented 9 months ago

Another vote to not create a cascade of warnings every time todo!() is used. If this can't be the default, then something like #[allow(todo)] would be sufficient: when I want to be sure I didn't leave some WIP code I can #[warn(todo)].

Here's everything I needed to write to stop spurious warnings about todo!() during a refactor:

    // Stop clippy whining about todo!().
    #[allow(unreachable_code)]
    #[allow(unused)]
    #[allow(clippy::diverging_sub_expression)]
    fn foo() { ... }