Feature Request: Don't warn for unused variables in unimplemented!() methods

The title says it all, it would be really nice if

fn foo(arg: u32) {
    unimplemented!()
}

didn't throw an unused variable warning, both because it pollutes the terminal (and even editor for those of use who use Sublime with the Rust Enhanced package), and because when I'm stubbing things out the last thing I want is to be creating argument renaming work for future me.

7 Likes

It's an interesting thought. Would you propose only doing this if the full body of a function consists only of unimplemented(), or also if it has some contents before the unimplemented!()? Personally I almost never use the former, but the latter would be interesting to me. I do wonder if it this is a case where todo!() should be handled differently than unimplemented!() (eliding the warning makes more sense for todo!() than for unimplemented!() because the latter has less of an implication of wanting to change it).

4 Likes

Yeah, I definitely like this idea, but there's some interesting questions about how aggressive it should be in suppressing the warnings. I think I would personally like it if the rule was "any variable that is in-scope on a unimplemented!() is assumed to be used", but there are various alternative rules that could be used, like not suppressing the warning if the unimplemented!() is inside a conditional such that there's a way the function could return normally.

1 Like

@djc I had been thinking the latter as well. I often leave the unimplemented in place until I'm completely done writing the method. Especially tests:

#[test]
fn test_the_thing() {
    let computed = do_the_thing().unwrap();
    unimplemented!() // TODO: assert stuff
}

is pretty much the first thing I write once I have do_the_thing stubbed in. That way I can run it as a smoke test but can't forget to eventually flesh it out.

On the question of todo vs unimplemented, do you really want the warnings if you're using unimplemented to avoid claiming that you intend to implement the thing? I'm not so sure.

@elidupree I like your rule too. If there's an unimplemented in there anywhere it means the function is incomplete, and thus it has bigger issues than unused variables.

If you expect to flesh out the unimplemented!() before a major release, then why is it not todo!()? To me unimplemented!() is a long-term placeholder for something that might be added at some point, or not, such as adding floating point to an integer-only micro-computer. I think this post captured the distinction between the two macros quite well.

2 Likes

It's a terrible answer but a true one: because whatever version of the Rust Enhanced Sublime plugin I'm using has a snippet that makes typing unimplemented!() into three keystrokes and so far I've been too lazy to look up how to create a snippet for todo. To a lesser extent there's also a degree of "old dog, new tricks" going on.

But that's just my workflow, I'm not yet convinced that todo and unimplemented should be treated differently wrt to this warning.

I'm curious why just using _ instead of arg is not a sufficient answer? If later, you implement it, and that implementation needs to use the parameter, you'll need to change the argument from _ to arg, but, why is that an issue? I just don't see how adding a feature like this pulls any useful weight.

3 Likes

The reason I don't like using underscores to suppress the warning is that I'm forgetful. If I forget to use some variable there's a good chance I'll also forget to rename it back to something that will permit the warning. I also like being able to use the leading underscore to imply that there's a reason I'm keeping whatever it is around.

2 Likes

Yeah, I get that, but, I'm not trying to be argumentative, but, I am trying to better understand your position on this so I can understand how this might be a useful "feature" (because at first reading, I can't understand the usefulness). This is how I see it. You have a function, that takes one or more arguments that you intend to provide an implementation for at some point (otherwise, why define the function in the first place). You don't want to get the "unused parameter" warning. The currently provided mechanism for avoiding the warning when a parameters is not used, is to use _ as the parameter name. Later, when you implement it, you'll have to change the parameter to a legitimate name; otherwise, you won't be able to write the implementation (unless the implementation still doesn't need the parameter). What is there to "forget" to do? I just can't seem to wrap my mind around what could be forgotten.

1 Like

Let's say that I'm writing a function that reads and reassembles a message from a TcpStream:

pub fn get_message(sock: &TcpStream, timeout: Duration) -> Result<MessageType>

I could plausibly forget to use the timeout parameter, and under normal circumstances the method would still appear to work perfectly.

Why is that any different than having the parameter but implementing the code that "uses" it in a manner that makes the algorithm not work as intended? I agree with @gbutler that this burdens the language learner unnecessarily. Use the existing features _ and todo!(), plus inline or top-of-function comments, to assist your future analysis, or that of the person who picks up your code and attempts to complete it.

The difference between the bug in my example and other bugs is that the one in my example can be readily detected at compile time. Doesn't using todo!() still permit unused variable warnings? I was under the impression that they were functionally identical.

Besides, what are those of us who want unused variable warnings supposed to do? If my code is littered with unimplemented!() and todo!(), I want warnings. This is non-working code. I don't want it to silently compile without any notice. Warnings are warnings, they are there to warn you of things. You can just ignore them if you think they should be ignored. Drop #![ignore(unused_variables)] at the top of your library/module/function.

1 Like

I've been wishing for this as well.

I'd say the warning should be suppressed only if the entire body of the function is unimplemented or todo. If there is some other code, a warning could indicate a potential issue in that code, so it could be valuable. But if there is no other code at all, it's always clear that all variables are intentionally unused yet.

The semantic difference between unimplemented and todo is too subtle, so people will often use them interchangeably. I don't think the warning policy should depend on this difference.

The whole point of creating a stub function is to declare its interface. Argument names are a part of the interface, along with argument types and function name. Even if the function is not implemented yet, it can be used in other places. Without argument names, it may be difficult to remember what the function is supposed to do with its arguments. IDEs can also show argument names of functions in auto completion. Another issue, as was already pointed out, is the danger to forget to use the variable.

This is not really relevant to this topic. Warnings about unused variables are not going to protect you from unimplemented and todo slipping into production code because there are functions with no arguments. If you want that, you have to ensure it in other ways (like adding a pre-commit or CI check). Warnings about unused variables in this case just add meaningless noise because they do not report the root problem with the code (the presence of unimplemented).

3 Likes

I am up for this, as long as the todo! / unimplemented! call itself triggers a warning. TODOs, FIXMEs, etc. should all trigger some form of warning to prevent them from being accidentally published. Currently, unused_ warnings are the poor man's version of that lint (and, imho, that ought to change), but at least you are (usually, but not always) getting a warning for them.

The main reason warnings are annoying is that they may be printed / displayed before hard errors, and for those of us who still use a console for their workflow, that can be quite annoying indeed.

  • For this issue, I personally do things like (sometimes through a script that toggles -Awarnings in a .cargo/config file, so that my editor uses the flag too, and the console and it do not "fight" for the compilation environment):

    RUSTFLAGS=-Awarnings cargo watch -c -s "cargo check --tests --quiet 2>&1 | head -n 40"
    
2 Likes

Just noting there exists a Rust issue for this already. The implementation would not be entirely straightforward, because you would have to special-case unimplemented!() and todo!(), rather than just relying on their behaviour as panic!s.

1 Like

Could unimplemented warn only in release mode? (and/or some other situation that's not part of edit-compile-debug cycle)

While I'm developing code I know lots of stuff is messy and unfinished, so such warnings aren't helpful. They create noise in my editor and are a distraction.

2 Likes

Sure, it could even be an allow-by-default lint that one can deny for some form of CI / git hook. I personally think that unused_ warning could also belong to that category, as they are usually kind of a pedantic lint (it's great to avoid committing code with unused stuff, but it is annoying to have it pollute the "edit-compile-debug cycle" in general). As I've shown, I've found that tweaking the lint levels through the "environment" (variables or .config files) ends up being the most flexible solution.

3 Likes

I'd be fine with warnings about the presence of unimplemented/todo as long as I can disable them. Something like being able to write:

#![allow(incomplete_code)]  // TODO: remove this and clean up ASAP
2 Likes

Agree with @Riateche that the warning can probably be reasonably suppressed if the entire body of the function is either of the macros.

For other situations, e.g. if there's an todo! / unimplemented! in some branch of a function, could the macro accept the variables that this branch intends to use, and then they'd not be considered unused?

Simple example:

fn partial_func(a: u32, b: u32, c: u32) {
    if a > 3 {
        println!("{}", b);
    } else {
        unimplemented!(c);
    }
}

That way, the simple "full function" case is ergonomic, and the more complex cases can also be resolved manually (without having to add an extra let _ = c line for example).