Flag for enforcing explicit 'return' keyword

[UebelAndre]

Is there a flag that would cause rustfmt to appropriately add an explicit return keyword at the end of functions?

Before:

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        return false;
    }

    lhs % rhs == 0
}

After:

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        return false;
    }

    return lhs % rhs == 0;
}

Diff:

diff --git a/example.rs b/example.rs
index 96b10a6..5af00c8 100644
--- a/example.rs
+++ b/example.rs
@@ -3,5 +3,5 @@ fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
         return false;
     }

-    lhs % rhs == 0
+    return lhs % rhs == 0;
 }
\ No newline at end of file

[calebcartwright]

Thank you for the question @UebelAndre. No, there is no flag nor config option that will have rustfmt convert the last item to a return expression. Needless return expressions are generally considered to be bad style and non-idiomatic in Rust (there's even a Clippy lint that will warn on them).

If you prefer to use utilize explicit return expressions within your code then you can certainly do so, and rustfmt will still happily format them. However, IMHO rustfmt should not be in the business of trying to change these expression types as part of style formatting, particularly to one that will cause clippy to emit warnings.

[UebelAndre]

@calebcartwright Thank you so much for your reply! 😃

I would agree that rustfmt should not start adding ways to fight clippy, however, I don't at all understand the known issue or the justification for why this is bad in the link you sent. Could you elaborate on those to help me understand this better?

[ayazhafiz]

My understanding is that elision of the return keyword is preferred because rust is intended to be more of an expression-based than statement-based language. For example, fn foo() { 1 } evaluates to the expression 1, and while fn foo() { return 1; } does the same thing in practice, to a reader, the latter example may read as the last statement of foo instructing the return operator to pass 1 to the caller, which feels a bit different than an expression evaluating to something.

This convention comes from more functional languages where return doesn't make much sense to begin with because every function evaluates to something (even if it is the unit type, or () in rust).

The rust book has some more insight on statements vs expressions: https://doc.rust-lang.org/reference/statements-and-expressions.html.

[ayazhafiz]

I guess there's a question of whether rustfmt should do the reverse, ie convert terminating explicit returns into expressions.

[calebcartwright]

@UebelAndre

I don't at all understand the known issue or the justification for why this is bad in the link you sent

Yeah the lint description there isn't exactly detailed is it 😆 As @ayazhafiz noted, it really is more about convention, and like with any convention, there's not always explicit origin story or obvious explanation, but conventions arise nevertheless. The explanation @ayazhafiz shared is similar to what I've heard before elsewhere, and I've also found the avoidance of return expressions to be helpful in other places, like closures.

[calebcartwright]

@ayazhafiz

I guess there's a question of whether rustfmt should do the reverse, ie convert terminating explicit returns into expressions.

TBH I wouldn't be too keen on trying to take that on for the same reason as the requested inverse in the OP, and I'd rather defer this to the developer. Obviously different story if our hand gets forced by the style guide or it becomes a popular ask from the community, but for now it just feels like it'd be another vector for bugs.

[UebelAndre]

The explanation @ayazhafiz shared is similar to what I've heard before elsewhere, and I've also found the avoidance of return expressions to be helpful in other places, like closures.

I feel like the intention of the implicit return is great but the execution could be improved. There should be a way to write code that doesn't require the use of the return keyword for control flow as well. The example in the rust documentation

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        return false;
    }

    lhs % rhs == 0;
}

The mix of a return keyword and the implicit return from the expression make it unclear as to how to read this. I'm most interested in making sure my code is consistent and readable so if there was a smarter way to handle control flow that avoids this situation then I'd love to learn about it. I otherwise feel adding return at the end is the nicer thing to do.

[calebcartwright]

I feel like the intention of the implicit return is great but the execution could be improved. There should be a way to write code that doesn't require the use of the return keyword for control flow as well.

Perhaps I am misunderstanding you here, but I feel like this is quickly getting off topic and really outside the scope of rustfmt anyway. Requests/suggestions for changes to the language itself need to be made elsewhere. rustfmt is just the code formatting tool.

The example in the rust documentation

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        return false;
    }

    lhs % rhs == 0;
}

Please take note that your snippet here is not the same as the book reference (you've added a semicolon to the last line making it a statement), and it actually will not compile because that path doesn't return a boolean value.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=05b76e95debd0932b5bdfa558b28bddb

The mix of a return keyword and the implicit return from the expression make it unclear as to how to read this. I'm most interested in making sure my code is consistent and readable so if there was a smarter way to handle control flow that avoids this situation then I'd love to learn about it. I otherwise feel adding return at the end is the nicer thing to do.

Again, not really sure what to make of this considering your snippet is invalid 😄 That trivial example from the book is simply illustrative, showing both usage of an early return as well as last-line expressions for implicit returns.

There's a lot of alternative ways you could choose to write this example from the book, for example:

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        false
    } else {
        lhs % rhs == 0
    }
}

or

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        return false;
    }

    return lhs % rhs == 0;
}

You're welcome to use any approach you prefer, but as discussed above, rustfmt isn't going to convert your code from one of those approaches to another; rustfmt will ensure that, whichever approach you prefer and use, it follows the formatting/style established by the Style Guide.

[UebelAndre]

Again, not really sure what to make of this considering your snippet is invalid

My apologies, this is a typo, I copied the wrong snippet above and forgot to remove that but it looks like you understood what I was getting at with that.

Perhaps I am misunderstanding you here, but I feel like this is quickly getting off topic and really outside the scope of rustfmt anyway. Requests/suggestions for changes to the language itself need to be made elsewhere. rustfmt is just the code formatting tool.

This has shifted toward a discussion about the language and I can happily move the conversation. Before opening this ticket, it was unclear to me that the implicit return was "enforced" by clippy, which to me means the conversation should be at least be moved over there and I think this is reason enough to close this issue. However, if it weren't the case that this is "enforced" in clippy, I do feel it would be appropriate for rustfmt to provide this functionality.

[calebcartwright]

Before opening this ticket, it was unclear to me that the implicit return was "enforced" by clippy

A clippy warning does not qualify as enforcement. As noted above, it's convention and considered to be idiomatic to use an expression over an explicit return expression statement.

However, if it weren't the case that this is "enforced" in clippy, I do feel it would be appropriate for rustfmt to provide this functionality

I understand your ask, but I just disagree. rustfmt's job is to ensure that any two programs parsed to the same AST are formatted the same way, in accordance with the style guide. It's not a design goal of rustfmt to support changing the AST structure while maintaining program logic.

IMHO, rustfmt should not support allowing the conversion of last-line-in-block expressions to an explicit return expression statement because it is out of scope, the cons of effort/code/etc. outweigh any user preference, and the output from rustfmt would be flagged by clippy.

I'm inclined to close this as wontfix but cc @topecongiro for your thoughts in case you disagree

[UebelAndre]

A clippy warning does not qualify as enforcement

This is for lack of a better word. It does appear that clippy seems to have more authority over this decision so any decision making would be deferred to the decisions made there. If that makes more sense.

I understand your ask, but I just disagree. rustfmt's job is to ensure that any two programs parsed to the same AST are formatted the same way, in accordance with the style guide.

It does seem you understand my ask but I think I fail to communicate that I also understand your responses. I completely agree with your stance on the responsibility of rustfmt. This simply isn't the place to discuss the language or style guide. I just want it to be clear that if this gets flagged with wontfix that it's because of the decisions being made in clippy, something specified in the style guide, or the engineering overhead by supporting this feature.

[calebcartwright]

This is for lack of a better word. It does appear that clippy seems to have more authority over this decision so any decision making would be deferred to the decisions made there. If that makes more sense.

I really want to stress that there's not anywhere to go if you're looking for something that will rewrite your code from:

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        return false;
    }

    lhs % rhs == 0
}

To a functionally equivalent alternative like:

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        false
    } else {
        lhs % rhs == 0
    }
}

// Or
fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    if rhs == 0 {
        return false;
    }

    return lhs % rhs == 0;
}

// Or
fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    match rhs {
        0 => false,
        _ => lhs % rhs == 0, 
    }
}

// Or...

because which features of the language to use to implement logic is really left to the discretion of the programmer.

Conventions/best practices/official approaches/etc. often stem from collective experience and wisdom around what the experts and community at large have found to be optimal, and how to best take advantage of the features provided by the language. It's stylistic in nature, but explicit return expression statements are similar here because they're redundant and don't take advantage of the expression oriented nature of the language (you may find https://users.rust-lang.org/t/explicit-and-implicit-return-in-functions/17089/5 helpful as well).

There's other common/preferred stylistic conventions such as if let. For a highly contrived example, while you could write something like this:

fn greet(user: Option<&str>) {
    if user.is_some() {
        let name = user.unwrap();
        println!("Greetings {}", name);
    } else {
        println!("Hello world!");
    }
}

It's more conventional to leverage if let (or perhaps a match), like:

fn greet(user: Option<&str>) {
    if let Some(name) = user {
        println!("Greetings {}", name);
    } else {
        println!("Hello world!");
    }
}

Similarly to how you seem to prefer the explicit return expression statements, there's probably someone out there who stylistically would prefer the is_some()/unwrap() combo from the above example, and that's fine!

Conventions are just that, and folks can (and do) go against the conventions sometimes. However, you probably shouldn't be surprised that the official tools won't help you enforce anti-convention nor convert convention-following code to non-convention-following code.

All that being said, if you'd like more info (or to challenge) clippy's needless return lint then the clippy repo (https://github.com/rust-lang/rust-clippy) would indeed be the place to do so.

Lastly, if you'd like to suggest changes to Rust keywords/syntax/constructs/etc., then you'd need to follow the RFC process https://github.com/rust-lang/rfcs.

I just want it to be clear that if this gets flagged with wontfix that it's because of the decisions being made in clippy, something specified in the style guide, or the engineering overhead by supporting this feature.

No, that's not what I said. What I said is that you're welcome to write in whichever style you prefer and rustfmt will format either, but that I view this type of conversion of the input code as out of scope.

Additionally, as noted above, I don't think official tools should invest in support for converting from convention to anti-conventions, but your ask here is for rustfmt to do just that. The fact that clippy (the linting tool) warns about this is just another point of evidence that your ask is for something that goes against convention; clippy doesn't have any decision making authority over rustfmt and vice versa.

Lastly, even in the absence of all the above issues, although rustfmt could probably implement the ask from a technical perspective, IMO it would add undo overhead to the rustfmt source and I suspect would likely be a source of bugs and idempotency issues in "real" code scenarios.

[UebelAndre]

I really want to stress that there's not anywhere to go if you're looking for something that will rewrite your code from:

Yeah, this is not the feature I was requesting.

To a functionally equivalent alternative like:

But the alternatives you give here are awesome and satisfy my desire for the return keyword. In especially like:

fn is_divisible_by(lhs: u32, rhs: u32) -> bool {
    match rhs {
        0 => false,
        _ => lhs % rhs == 0, 
    }
}

So thank you so much for all the info and taking the time to explain everything to me 😄

[calebcartwright]

So thank you so much for all the info and taking the time to explain everything to me smile

Sure! Sounds like we've come to a logical resolution on this, so going to go ahead and close this issue.