Not a fan of semicolon_if_nothing_returned

[AdrianVovk]

Looked around and didn't find any discussion of this anywhere.

It seems to me (as a relative newcomer to Rust) that semicolon_if_nothing_returned is a detrimental lint by encouraging newcomers to make style choices that will make things more difficult to refactor later.

(I understand that I can turn the lint off, and I do. And I understand that pedantic is supposed to be opinionated. I'm just curious what others think about it, why it's worth including, etc.)

Let's say I have some code like this:

#![allow(dead_code)]
#![deny(clippy::semicolon_if_nothing_returned)]

const REQUEST_FOO: u32 = 1;
const REQUEST_BAR: u32 = 2;

struct Request;

impl Request {
    fn resolve(self) {
        // ...
    }

    // ...
}

fn handle_request(ty: u32, req: Request) {
    match ty {
        REQUEST_FOO => handle_foo(req),
        REQUEST_BAR => handle_bar(req),
        // ...
        _ => unreachable!(),
    }
}

fn handle_foo(req: Request) {
    // ...
    
    req.resolve()
}

fn handle_bar(req: Request) {
    // ...
    
    req.resolve()
}

This kind of code isn't all that surprising. Maybe resolving the request doesn't return any kind of error because whatever backend Request is wrapping has no error reporting. Maybe we're blindly twiddling bits on a GPIO pin, or something else like that. Alternatively, it's possible that we're dealing with some framework where an error to Request::resolve cannot be meaningfully handled by the caller (so instead we panic/abort/whatever).

Clippy complains about our code:

error: consider adding a `;` to the last statement for consistent formatting
  --> src/lib.rs:29:5
   |
29 |     req.resolve()
   |     ^^^^^^^^^^^^^ help: add a `;` here: `req.resolve();`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_if_nothing_returned

<snip>

So we listen to clippy and do what it asks. Now, months later whatever underlying framework we're using has changed, and resolving a request gives us some kind of Token back:

struct Token;

impl Request {
    fn resolve(self) -> Token {
        // ...
        todo!()
    }
}

And we've changed nothing else. At some of our call sites, we forgot to pass through the token. The result: slience.

Of course, we could mark our token as must_use, but that doesn't always make sense - maybe the token is helpful only occasionally, but should still be plumbed up to higher-level code (i.e. whatever calls handle_request)

If we didn't listen to clippy and skipped the semicolon, instead we'd have:

error[E0308]: mismatched types
  --> src/lib.rs:31:5
   |
31 |     req.resolve()
   |     ^^^^^^^^^^^^^ expected `()`, found `Token`
   |
help: consider using a semicolon here
   |
31 |     req.resolve();
   |                  +
help: try adding a return type
   |
28 | fn handle_foo(req: Request) -> Token {
   |                             ++++++++

<snip>

Ultimately, whether or not you put a semicolon at the end of a function body isn't a matter of just style, as the lint implies. You're deciding whether or not it would make sense for a potential future return value to propagate through through this function or not.

Another situation where this might come up: Non Generic Inner Functions. Clippy complains about the following:

#![allow(dead_code)]
#![allow(unused_variables)]
#![deny(clippy::semicolon_if_nothing_returned)]

fn outer<T: AsRef<str>>(arg: T) {
    fn inner(arg: &str) {
        // ...
    }
    inner(arg.as_ref())
}

But then if you listen to clippy, then later refactor the code:

fn outer<T: AsRef<str>>(arg: T) {
    fn inner(arg: &str) -> u32 {
        // ...
        todo!()
    }
    inner(arg.as_ref());
}

You get no warning! If you ignored Clippy, you get:

error[E0308]: mismatched types
  --> src/lib.rs:10:5
   |
10 |     inner(arg.as_ref())
   |     ^^^^^^^^^^^^^^^^^^^ expected `()`, found `u32`
   |
help: consider using a semicolon here
   |
10 |     inner(arg.as_ref());
   |                        +
help: try adding a return type
   |
5  | fn outer<T: AsRef<str>>(arg: T) -> u32 {
   |                                 ++++++

For more information about this error, try `rustc --explain E0308`.

Of course, in obvious cases like I'm bringing up here the mistake is obvious. But in a complicated code base the mistake won't necessarily be so obvious.

If you follow the link from the warning, you get this under "Why is it bad":

The semicolon might be optional but when extending the block with new code, it doesn’t require a change in previous last line.

Which is, to say the least, unconvincing. You're trading in a future compiler error that can warn you of bugs you introduced into the codebase, and in exchange you get to make a future git diff one line smaller? I don't think it's worth it.

---

So hopefully I've made clear why this lint might be harmful, but perhaps it could have some utility.

It would be useful to point out to newcomers that "this is a thing you should be thinking about"! I knew about this footgun before I even started writing Rust because of a YouTube video of "style tips" I watched at some point - I suspect most don't encounter it until it's too late.

The lint in its current form doesn't help. It just blanket-discourages skipping the semicolon, which does nothing towards informing the developer. So maybe instead the lint should act as a little nudge: "You're using semicolons inconsistently - did you know you should be carefully thinking about it?" Then once the developer is aware of this, they can turn off the lint.

Anyway, thoughts appreciated. Thanks

[flip1995]

"You're using semicolons inconsistently - did you know you should be carefully thinking about it?" Then once the developer is aware of this, they can turn off the lint.

You pointed out a lot of reasons why the pedantic group exists and why this lint is in pedantic. So I don't have to explain that part.

For me, if I read

fn handle_foo(req: Request) { // Might be off-screen, if the function is long
    // ...
    
    req.resolve()
}

I expect that resolve returns something. But if there is a ; I know that it doesn't return anything up. So for me it is also helpful for code readability.

You have a point about missing out on the error when you add the semicolon. But if the Token is not must_use and the function calling resolve as it's last instruction also was never returning anything, then did you really need it? I would assume, if you refactor something like this, you'll start from the other side: Something that calls handle_foo will need something returned, so you add a return type to handle_foo. Then you get an error, that it doesn't return anything, so you might modify resolve. And now if you have semicolons added everywhere, your refactoring is already done. Otherwise you'd have to go through every function and add it or return something.

But YMMV and what you prefer is opinionated. Which is fine for pedantic lints (by design).