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

#[allow(clippy::single_call_fn)] won't work #12182

Closed
exalted opened this issue Jan 21, 2024 · 4 comments · Fixed by #12183
Closed

#[allow(clippy::single_call_fn)] won't work #12182

exalted opened this issue Jan 21, 2024 · 4 comments · Fixed by #12183
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@exalted
Copy link

exalted commented Jan 21, 2024

Summary

I "enabled" clippy::single-call-fn lint via -D clippy::single-call-fn to cargo clippy, which expectedly resulted in an error for a single call function, however when I added #[allow(clippy::single_call_fn)] to the function, the error didn't go away.

It does work, however, if I were to add #![allow(clippy::single_call_fn)] at the top of the main.rs.

$ cargo clippy --version
clippy 0.1.75 (82e1608d 2023-12-21)

Reproducer

I tried this code:

fn main() {
    let some_text = "foo";
    print_foo(some_text);
}

#[allow(clippy::single_call_fn)]
fn print_foo(text: &str) {
    println!("{text}");
}

I expected to see this happen:

Thanks to #[allow(clippy::single_call_fn)], cargo clippy should give 0 errors.

Instead, this happened:

Albeit #[allow(clippy::single_call_fn)], cargo clippy will still complain as such:

error: this function is only used once
  --> src/main.rs:55:1
   |
55 | / fn print_foo(text: &str) {
56 | |     println!("{text}");
57 | | }
   | |_^
   |
help: used here
  --> src/main.rs:51:5
   |
51 |     print_foo(some_text);
   |     ^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_call_fn
   = note: requested on the command line with `-D clippy::single-call-fn`

Version

rustc 1.75.0 (82e1608df 2023-12-21)
binary: rustc
commit-hash: 82e1608dfa6e0b5569232559e3d385fea5a93112
commit-date: 2023-12-21
host: aarch64-apple-darwin
release: 1.75.0
LLVM version: 17.0.6

Additional Labels

No response

@exalted exalted added the C-bug Category: Clippy is not doing the correct thing label Jan 21, 2024
@exalted
Copy link
Author

exalted commented Jan 22, 2024

@y21 hey, thanks for the quick fix! Do you think this issue only applies to single_call_fn, or a broader check could/should be done for all the lints, in case there are other lints that don't respect/work in case #[allow()] is applied to them?

@y21
Copy link
Member

y21 commented Jan 22, 2024

For the majority of the lints (e.g. method calls lints), this sort of issue can't happen, because clippy/rustc correctly considers the lint level attributes when a warning is emitted for the current node being checked.
It can only happen when a warning is emitted for a different item than the one currently checked.

In this specific case, the single_call_fn lint doesn't immediately emit a warning in check_fn but instead remembers the function's id and then later emits a warning in check_crate_post if there is exactly one use of it (because we only know the number of times a function is called after the whole crate is checked). That's why it only respected attributes at the crate root.

That said, it does come up every now and then for some lints. I've also been wondering if we can have an internal lint that tries to catch this, but it seems hard to generalize.
Maybe one possible (but less general) implementation that would have caught this issue (and similar ones) could be looking for a call to one of the span_lint_* functions (and also no call to is_lint_allowed) inside of a check_crate_post method, and suggest using one of the span_lint_hir* functions instead (like the linked PR does), which takes the lint level attributes of the given node into account.

@y21
Copy link
Member

y21 commented Jan 22, 2024

The idea of an internal lint like mentioned above is now tracked over at #12189.

@bors bors closed this as completed in 0b6e7e2 Jan 22, 2024
@exalted
Copy link
Author

exalted commented Jan 23, 2024

@y21 I appreciate your description and feedback, that was useful for me to understand the gist of the issue.

Also, I think the "linting clippy itself" approach with an internal lint is quite clever!

My two cents: the test case you wrote to avoid regression on this issue might be something to add for all the lints (i.e., each lint could have #[!][<allow|warn|...(<lint>)] test cases —ideally automatically added to them.)

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants