Clippy warns "unneeded `return` statement" for `#[tokio::{main,test}]` fns on latest nightly

[oxalica]

Version

tokio 1.40.0
rust toolchain 1.83.0-nightly (9e394f551 2024-09-25)

Platform

Linux invar 6.10.10 #1-NixOS SMP PREEMPT_DYNAMIC Thu Sep 12 09:13:13 UTC 2024 x86_64 GNU/Linux

Description

I tried this code:

// lib.rs
#[tokio::main(flavor = "current_thread")]
async fn main() {
    println!("hello");
}

with

# Cargo.toml
[package]
name = "poc"
edition = "2021"
version = "0.0.0"

[dependencies]
tokio = { version = "1.40.0", features = ["rt", "macros"] }

When running cargo clippy --tests -- -Dclippy::needless_return

I expected to see this happen: [no warnings]

Instead, this happened:

    Checking poc v0.0.0 (/home/oxa/tmp/rust-poc)
error: unneeded `return` statement
 --> src/main.rs:3:22
  |
3 |     println!("hello");
  |                      ^
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
  = note: requested on the command line with `-D clippy::needless-return`
help: remove `return`
  |
3 |     println!("hello")println!("hello");
  |                      ~~~~~~~~~~~~~~~~~~

error: could not compile `poc` (bin "poc" test) due to 1 previous error

By expanding macros via cargo rustc -- -Zunpretty=expanded, the warning seens correct: the generated code contains a needless "return" expression at trailing position, and there is no explicit #[allow(..)]. It seems the return here can be trivially removed?

It also reproduces on #[tokio::test].

   Compiling poc v0.0.0 (/home/oxa/tmp/rust-poc)
#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
fn main() {
    let body = async { { ::std::io::_print(format_args!("hello\n")); }; };

    #[allow(clippy :: expect_used, clippy :: diverging_sub_expression)]
    {
        return tokio::runtime::Builder::new_current_thread().enable_all().build().expect("Failed building the Runtime").block_on(body);
    }
}
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.05s

[taiki-e]

As always, this is a clippy bug that warns code generated by external macros (upstream report: rust-lang/rust-clippy#13457).

(Similar to #5616, #3830, etc.)

[oxalica]

As always, this is a clippy bug that warns code generated by external macros (upstream report: rust-lang/rust-clippy#13457).

Thanks for the reference. I also found that the return is intentionally introduced from #4636. Maybe we could still have a #[allow(..)] then?

[taiki-e]

Maybe we could still have a #[allow(..)] then?

Yeah, I would accept a PR to add the corresponding #[allow()] attribute to the generated code (as with #5616 (comment), #3830 (comment)).

[y21]

I've put up a PR for clippy, but as was pointed out on the PR (rust-lang/rust-clippy#13464 (comment)) I do wonder if there's a reason for why tokio doesn't use Span::call_site().located_at(t.span()) here?

tokio/tokio-macros/src/entry.rs

Lines 403 to 404 in 2c14f88

	let start = last_stmt.next().map_or_else(Span::call_site, |t| t.span());
	let end = last_stmt.last().map_or(start, |t| t.span());

That should tell clippy that it's from a macro expansion and avoid linting but still maintains the input location so rustc points type errors at ts span I think?
(Maybe quote_spanned should just do that by itself instead of passing the span as-is to .set_span(). edit: nevermind that, bad idea)

[jieyouxu]

I do wonder if there's a reason for why tokio doesn't use Span::call_site().located_at(t.span()) here?

Nit: I think you might've meant Span::mixed_site(), tokio currently uses call_site hygiene, which would cause Span::from_expansion to return false (for clippy and rustc). Passing the proper span hygiene will not only help with clippy lints but also rustc diagnostics and lints.

I didn't look at the tokio-macros code in detail, but I think there's another place that uses Span::call_site that's possibly incorrect w.r.t. span hygiene.

[taiki-e]

I would accept a PR that adjusts the span as long as it does not worsen diagnostics.

I do wonder if there's a reason for why tokio doesn't use Span::call_site().located_at(t.span()) here?

Nit: I think you might've meant Span::mixed_site(), tokio currently uses call_site hygiene, which would cause Span::from_expansion to return false (for clippy and rustc).

I think call_site is fine and what from_expansion can't handle correctly is the generated code using the span from the input.

why tokio doesn't use Span::call_site().located_at(t.span()) here

tokio-macros is a crate that has been existed before located_at stabilized, and no one was interested in working on replacing existing spans with a potential alternative while checking that changes would not cause regressions.

[y21]

Nit: I think you might've meant Span::mixed_site(), tokio currently uses call_site hygiene, which would cause Span::from_expansion to return false (for clippy and rustc). Passing the proper span hygiene will not only help with clippy lints but also rustc diagnostics and lints.

Not that it matters but AFAIK both Span::call_site() and Span::mixed_site() have the "same from_expansion behavior" (that is, in both cases clippy will know that it's from a macro), so either one of the two works.

Span::mixed_site only has a different Transparency which determines how identifiers are resolved.
In the case of this macro specifically, there are no identifiers to be resolved, so both should work identically

This does not produce a clippy warning

#[proc_macro]
pub fn foo(ts: TokenStream) -> TokenStream {
    let input = ts.into_iter().next().unwrap().span();
    let sp = proc_macro2::Span::call_site().located_at(input.into());
    quote::quote_spanned!(sp=> fn f() -> i32 { return 42 }).into()
}

I would accept a PR that adjusts the span as long as it does not worsen diagnostics.

I believe the only change is that at the end of the diagnostic it will say "the error originates from the macro tokio::main" or something like that, which it didn't before. I don't think it's that significant though. But it means that tokio won't get clippy warnings anymore in its proc macro

[jieyouxu]

Ah good point, my bad, I think the problem is that we can't naively forward user spans without providing any distinguishable context?