Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macros: fix wrong error messages #4067

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

c0va23
Copy link
Contributor

@c0va23 c0va23 commented Aug 24, 2021

Motivation

The macros tokio::main and tokio::test return invalid error messages if the function being wrapped has no return value. This was because the semicolon from the last statement was missing.

Also, since version 0.1.54 (or earlier), clippy::semicolon_if_nothing_returned triggered false-negative when using the macros tokio::test and tokio::main on functions without a result type.

Fixes: rust-lang/rust-clippy#7438

Solution

We can copy the semicolon and the return keyword from the last
statement from the body of the wrapped function.

@c0va23
Copy link
Contributor Author

c0va23 commented Aug 24, 2021

I'm not sure that this problem should be solved on the tokio-macros' side, and not on the clippy side. And I'm not sure that this fix won't break some scenarios for using the tokio::test and tokio::mainmacros.

But this change made it possible to update 'tokio-macros' without disabling 'clippy::semicolon_if_nothing_returned' on my project.

I will try to add tests for this problem. For now, I want to open a discussion to understand on which side the problem should be solved (clippy or tokio).

@Darksonn Darksonn added A-tokio-macros Area: The tokio-macros crate M-macros Module: macros in the main Tokio crate labels Aug 24, 2021
@Darksonn
Copy link
Contributor

Honestly, I would prefer if this was fixed on the clippy side.

@c0va23 c0va23 force-pushed the fix/macros-clippy-semicolon branch from 15e3972 to 244bc50 Compare August 25, 2021 05:26
Comment on lines -10 to -28
/* TODO(taiki-e): one of help messages still wrong
help: consider using a semicolon here
|
16 | return Ok(());;
|
*/
return Ok(());
}

#[tokio::main]
async fn extra_semicolon() -> Result<(), ()> {
/* TODO(taiki-e): help message still wrong
help: try using a variant of the expected enum
|
29 | Ok(Ok(());)
|
29 | Err(Ok(());)
|
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taiki-e, i removed these todos, because maybe I fixed them. Correct me if I'm wrong.

@c0va23
Copy link
Contributor Author

c0va23 commented Aug 25, 2021

I found tests in which it was planned to make similar changes.

And I redid the PR. Instead of adding a semicolon if the function does not have a result type. I check whether the last expression in the function body had a semicolon, and if so, I check whether this expression was 'return'.

@c0va23 c0va23 force-pushed the fix/macros-clippy-semicolon branch from 244bc50 to 4206e37 Compare August 25, 2021 05:57
@Darksonn
Copy link
Contributor

Since this improves some existing bad error messages, I'm more positive for accepting this version. Can you add some tests that look like this?

#[tokio::main]
async fn main() -> Result<(), ()> {
    return Ok(());
}

#[tokio::main]
async fn main() -> Result<(), ()> {
    loop {
    }
}

It's ok if they don't have great error messages.

@c0va23
Copy link
Contributor Author

c0va23 commented Aug 25, 2021

@Darksonn, I will add such tests and fix the fallen tests (locally I ran tests on the latest version of the changes and they passed, but they fell on CI).

I think that it will still be necessary to correct the description of the PR, so that it better reflects the reason for these changes.

@c0va23 c0va23 force-pushed the fix/macros-clippy-semicolon branch from 4206e37 to 1664480 Compare August 25, 2021 09:06
@c0va23 c0va23 changed the title macros: fix clippy::semicolon_if_nothing_returned macros: fix wrong error messages Aug 25, 2021
@c0va23 c0va23 force-pushed the fix/macros-clippy-semicolon branch from 1664480 to b8682d8 Compare August 25, 2021 09:12
The macros `tokio::main` and `tokio::test` return invalid error
messages if the function being wrapped has no return value. This was
because the semicolon from the last statement was missing.

Also, since version 0.1.54 (or earlier),
`clippy::semicolon_if_nothing_returned` triggered false-negative when
using the macros `tokio::test` and `tokio::main` on functions without
a result type.

We can copy the semicolon and the `return` keyword from the last
statement from the body of the wrapped function.

Fixes: rust-lang/rust-clippy#7438
@c0va23 c0va23 force-pushed the fix/macros-clippy-semicolon branch from b8682d8 to 9990139 Compare August 25, 2021 14:31
@c0va23
Copy link
Contributor Author

c0va23 commented Aug 25, 2021

Since this improves some existing bad error messages, I'm more positive for accepting this version. Can you add some tests that look like this?

#[tokio::main]
async fn main() -> Result<(), ()> {
    return Ok(());
}

#[tokio::main]
async fn main() -> Result<(), ()> {
    loop {
    }
}

It's ok if they don't have great error messages.

@Darksonn, i add this tests and fix filed tests.

And i update subject and description for PR and commit message.

@c0va23 c0va23 marked this pull request as ready for review August 25, 2021 14:37
@Darksonn
Copy link
Contributor

Darksonn commented Aug 25, 2021

Ah, actually, I think the tests I wanted was more like this:

#[tokio::main]
async fn main() {
    return Ok(());
}

#[tokio::main]
async fn main() {
    loop {
        return Ok(());
    }
}

I wanted to see what the error messages were in these cases. Sorry about posting the wrong version.

@c0va23
Copy link
Contributor Author

c0va23 commented Aug 25, 2021

Ah, actually, I think the tests I wanted was more like this:

#[tokio::main]
async fn main() {
    return Ok(());
}

This test already exists

#[tokio::main]
async fn missing_return_type() {
return Ok(());
}

#[tokio::main]
async fn main() {
loop {
return Ok(());
}
}


I wanted to see what the error messages were in these cases. Sorry about posting the wrong version.

Second test

async fn main() -> Result<(), ()> {
loop {
if !never() {
return Ok(());
}
}
}
. But it compilled successful, without error messages.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what I wanted with the tests then. I must have confused myself. The PR looks good as is.

@Darksonn Darksonn merged commit 80bda3b into tokio-rs:master Aug 25, 2021
@c0va23 c0va23 deleted the fix/macros-clippy-semicolon branch August 26, 2021 08:32
@c0va23
Copy link
Contributor Author

c0va23 commented Aug 26, 2021

@Darksonn, thanks a lot for helping with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-macros Area: The tokio-macros crate M-macros Module: macros in the main Tokio crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

semicolon-if-nothing-returned false positive with tokio macro
2 participants