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

unit_arg lint for Rocket handlers returning unit causes a warning #6514

Closed
KamilaBorowska opened this issue Dec 29, 2020 · 5 comments · Fixed by #8694
Closed

unit_arg lint for Rocket handlers returning unit causes a warning #6514

KamilaBorowska opened this issue Dec 29, 2020 · 5 comments · Fixed by #8694
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work T-macros Type: Issues with macros and macro expansion

Comments

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Dec 29, 2020

Lint name:

unit_arg

I tried this code:

#[rocket::get("/")]
fn handler() {}

fn main() {
    println!("Hello, world!");
}

with the following Cargo.toml:

[package]
name = "rocket-example"
version = "0.1.0"
edition = "2018"

[dependencies.rocket]
git = "https://github.com/SergioBenitez/Rocket.git"
rev = "9671115796e42865eaebd020ac27542802027b02"

I expected to see this happen: no warning

Instead, this happened:

warning: passing a unit value to a function
 --> src/main.rs:2:4
  |
2 | fn handler() {}
  |    ^^^^^^^
  |
  = note: `#[warn(clippy::unit_arg)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
help: move the expression in front of the call and replace it with the unit literal `()`
  |
2 | fn handler;
3 | ()() {}
  |

warning: 1 warning emitted

See also rwf2/Rocket#1495.

Meta

  • cargo clippy -V: clippy 0.0.212 (2987785 2020-12-28)
  • rustc -Vv:
    rustc 1.51.0-nightly (2987785df 2020-12-28)
    binary: rustc
    commit-hash: 2987785df3d46d5ff144a5c67fbb8f5cca798d78
    commit-date: 2020-12-28
    host: x86_64-unknown-linux-gnu
    release: 1.51.0-nightly
    
@KamilaBorowska KamilaBorowska added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Dec 29, 2020
@KamilaBorowska KamilaBorowska changed the title unit_arg lint triggers in macros unit_arg lint for Rocket handlers returning unit Dec 29, 2020
@KamilaBorowska KamilaBorowska changed the title unit_arg lint for Rocket handlers returning unit unit_arg lint for Rocket handlers returning unit causes a warning Dec 29, 2020
@phansch phansch self-assigned this Jan 7, 2021
@phansch phansch added the E-needs-mcve Call for participation: This issue needs a Minimal Complete and Verifiable Example label Jan 11, 2021
@phansch

This comment has been minimized.

@phansch phansch added the T-macros Type: Issues with macros and macro expansion label Jan 11, 2021
@phansch
Copy link
Member

phansch commented Jan 11, 2021

Alright, I managed to extract a proc_macro_attribute function from Rocket's codegen code that reproduces the issue:

#[proc_macro_attribute]
pub fn rocket_get(_args: TokenStream, input: TokenStream) -> TokenStream {
    let input_fn: syn::ItemFn = syn::parse(input).unwrap();

    // This needs to be quote_spanned! with the span of the input signature otherwise the issue couldn't be reproduced
    let spanned = quote_spanned! { input_fn.sig.ident.span() =>
        fn unitfn(_: ()) {}
        #[allow(clippy::no_effect)]
        unitfn({ 1; });
    };

    let output = quote! {
        #[allow(unused, clippy::no_effect)]
        #input_fn

        fn foo() {
            #spanned
        }
    };
    output.into()
}

Unfortunately a simple external macro check didn't fix the issue, so I will continue working on this tomorrow =)

@phansch phansch removed the E-needs-mcve Call for participation: This issue needs a Minimal Complete and Verifiable Example label Jan 11, 2021
@phansch phansch added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Jan 12, 2021
@phansch
Copy link
Member

phansch commented Jan 12, 2021

Unfortunately, neither Clippy nor rustc currently have a way to tell apart a quote_spanned span from a non-macro span, so this is currently impossible to fix on our side (Also see #6249 (comment)).

@phansch phansch removed their assignment Jan 12, 2021
@camsteffen
Copy link
Contributor

I don't have much experience with procedural macros. Is this something that can/should be fixed in the library? Maybe using Span::mixed_site or something like that?

@phansch
Copy link
Member

phansch commented Jan 14, 2021

Personally, I would consider that only a workaround (I don't know if mixed_site would fix it). Given that the library is using stable proc_macro APIs that are causing these problems, I would say it's something that needs to be fixed in the compiler.

There has been some more discussion on Zulip: https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/Telling.20apart.20.60quote_spanned.60.20from.20non-macro.20.20spans.3F

Essentially, we would need some extra 'this span was produced by macro expansion' state that is not modified by any of the proc_macro APIs.

bors added a commit that referenced this issue Aug 8, 2022
More proc-macro detection

fixes #6514
fixes #8683
fixes #6858
fixes #6594

This is a more general way of checking if an expression comes from a macro and could be trivially applied to other lints. Ideally this would be fixed in rustc's proc-macro api, but I don't see that happening any time soon.

changelog: Don't lint `unit_arg` when generated by a proc-macro.
@bors bors closed this as completed in 10853f7 Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work T-macros Type: Issues with macros and macro expansion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants