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

attributes: auto skip unused parameters #1443

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Folyd
Copy link
Contributor

@Folyd Folyd commented Jun 23, 2021

Motivation

The unused parameters always need to be skipped in most cases. However, keep writing
#[tracing::instrument(skip(_arg1, _arg2))] is unnecessary and unconcise.

Rarely, someone would use underscore parameter as normal, but that should be
ascribed to a code style issue.

fn foo(_arg: usize) {
    println!("{}", _arg);
}

Solution

Support auto skips unused parameters for the tracing::instrument attribute.
Raise a compile error if an unnecessary skipping found:

#[tracing::instrument(skip(_arg))]
fn foo(_arg: usize) {
}

error: unnecessary skipping: the unused parameter is auto skipped by default
    |
  2 |     #[tracing::instrument(skip(_arg))]
    |                                ^^^^

error: aborting due to previous error

@Folyd Folyd requested review from davidbarsky, hawkw and a team as code owners June 23, 2021 10:13
@Folyd
Copy link
Contributor Author

Folyd commented Jun 23, 2021

error: using `clone` on type `metadata::Level` which implements the `Copy` trait
   --> tracing-core/src/metadata.rs:835:47
    |
835 |                     let actual: LevelFilter = level.clone().into();
    |                                               ^^^^^^^^^^^^^ help: try dereferencing it: `(*level)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy

I guess this is an unrelated Clippy warning, which should be fixed in another PR.

@hawkw
Copy link
Member

hawkw commented Jun 23, 2021

error: using `clone` on type `metadata::Level` which implements the `Copy` trait
   --> tracing-core/src/metadata.rs:835:47
    |
835 |                     let actual: LevelFilter = level.clone().into();
    |                                               ^^^^^^^^^^^^^ help: try dereferencing it: `(*level)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy

I guess this is an unrelated Clippy warning, which should be fixed in another PR.

fixed this in #1444 :)

@vi
Copy link

vi commented Jun 24, 2021

error: unnecessary skipping: the unused parameter is auto skipped by default

unused parameter

Maybe "parameter for which unused warning is silenced" or "underscored parameter" would be better?

Also maybe it should be a warning, not error?

@bryangarza
Copy link
Member

AFAICT we could still merge this, after a rebase and review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants