New lint: unit_as_impl_trait #13925
New lint: unit_as_impl_trait #13925
Conversation
rustbot
added
the
S-waiting-on-review
samueltardieu
force-pushed
the
push-uzulsnsxpqmn
branch
from
6b2e881 to
6c3ca5b
Compare
I feel that having On the other hand, the suggestion is not too helpful in my opinion, given my reasoning stated above. |
Yes, and as seen in the issue, it exists in production, for example in Axum. |
| "consider being explicit, and terminate the body with `()`", | ||
| ); | ||
| } else if let Some(last) = block.stmts.last() { | ||
| diag.span_note(last.span, "this statement evaluates to `()` because it ends with `;`") |
There was a problem hiding this comment.
Shouldn't we in this case at least check that the expression the semicolon consumed also implements the required trait? Otherwise I could create an example that won't compile when removing the semicolon, and reducing false positives is always a good thing.
| /// if returning `()` is intentional. | ||
| #[clippy::version = "1.85.0"] | ||
| pub UNIT_AS_IMPL_TRAIT, | ||
| suspicious, |
There was a problem hiding this comment.
Given that this is warn by default, I'd like to make sure we remove all possible false positives:
- Whenever the swallowed expression would not impl the returned trait
- Perhaps there are some
#[cfg(..)]shenanigans going on that change the final call's return to()because there's nothing meaningful to return on some systems
| diag.span_note(expr.span, "this expression evaluates to `()`") | ||
| .span_help( | ||
| expr.span.shrink_to_hi(), | ||
| "consider being explicit, and terminate the body with `()`", |
There was a problem hiding this comment.
also the messaging might be shorter:
| "consider being explicit, and terminate the body with `()`", | |
| "add `; ()` for explicitness", |
| "if this is intentional, consider being explicit, and terminate the body with `()`", | ||
| ); | ||
| } else { | ||
| diag.span_note(block.span, "the empty body evaluates to `()`") |
There was a problem hiding this comment.
Especially in this case, having some #[cfg(..)] in the plaintext source would suggest this is intentional.
| "this function returns `()` which implements the required trait", | ||
| |diag| { | ||
| if let Some(expr) = block.expr { | ||
| diag.span_note(expr.span, "this expression evaluates to `()`") |
There was a problem hiding this comment.
Here it gets tricky, because the expression might return something else on a different target.
() implements traits from And I think it's safe to assume that people in general won't make MyHash and MyEq traits just to avoid this. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
This lint implements the suggestion from #13909, and requires that an explicit
()is used when a function returns()while declaring an impl trait return type.I have used specialized messages for three different situations:
()One should note that adding the explicit
()as suggested will cause theunused_unitlint (default: warn) to trigger. This should be possible to silence it inside functions returning impl traits if needed, to avoid having contradictory suggestions. Thoughts?Close #13909
changelog: [
unit_as_impl_trait]: new lint