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 many_single_char_names is broken #3198

Closed
gnzlbg opened this issue Sep 19, 2018 · 11 comments · Fixed by #4095
Closed

allow many_single_char_names is broken #3198

gnzlbg opened this issue Sep 19, 2018 · 11 comments · Fixed by #4095
Assignees
Labels
E-needs-test Call for participation: writing tests good-first-issue These issues are a good way to get started with Clippy

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 19, 2018

Playground:

#[cfg_attr(
    feature = "cargo-clippy",
    allow(clippy::many_single_char_names)
)]
pub fn append_to() {
    let (a, b, c, d, e) = (0, 0, 0, 0, 0);
}

produces

warning: 5th binding whose name is just one char
 --> src/main.rs:7:22
  |
7 |     let (a, b, c, d, e) = (0, 0, 0, 0, 0);
  |                      ^
  |
  = note: #[warn(clippy::many_single_char_names)] on by default
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#many_single_char_names

That lint should not trigger, since it is allowed for the function.

@flip1995
Copy link
Member

Applying the allow to a function does not change the behavior of the lint inside the function. This is weird, since the lint is produced through EarlyLintPass::check_[impl_]item() and a function is an item.

It works if you turn the cfg_attr into an inner attribute. Probably something you don't want to do.

Removing the clippy:: and checking this with cargo +stable clippy doesn't produce a warning. I'm afraid that this could have something to do with tool_lints...

@flip1995 flip1995 added the C-bug Category: Clippy is not doing the correct thing label Sep 19, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 19, 2018

I'm hitting this issue with other lints like shadow_unrelated as well. Moving the cfg_attr into an inner attribute does not work around this form me, can you show on the play ground how you are doing that ? This still produces the warning for me:

pub fn append_to() {
    #![cfg_attr(
        feature = "cargo-clippy",
        allow(clippy::many_single_char_names)
    )]
    let (a, b, c, d, e) = (0, 0, 0, 0, 0);
}

@flip1995
Copy link
Member

Sure: Playground

#![feature(tool_lints)]

#![cfg_attr(
    feature = "cargo-clippy",
    allow(clippy::many_single_char_names)
)]

#[allow(unused_variables)] 
pub fn main() {
    let (a, b, c, d, e) = (0, 0, 0, 0, 0);
}

@flip1995
Copy link
Member

flip1995 commented Sep 19, 2018

Hm maybe something changed how lint attributes are processed... Since when do you have these issues?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Sep 19, 2018

Ah, you meant making it an inner attribute of the scope where the function is, yeah I'd prefer not to do that. I've just disabled the lint globally as a workaround for now.

Since when do you have these issues?

I just started updating criterion to the latest clippy, and the last Rust nightly version criterion used was from june... so... I'd guess between june and today, which isn't probably very helpful.

@flip1995
Copy link
Member

I'm looking into that!

@flip1995 flip1995 self-assigned this Sep 19, 2018
@flip1995
Copy link
Member

flip1995 commented Sep 19, 2018

This error started with nightly-2018-08-24. The previous nightly-2018-08-19 does not produce a warning for your example. (That means, it is not connected to the tool_lints 🎉)

Repo: https://github.com/flip1995/test_allow_on_fn
Travis: https://travis-ci.org/flip1995/test_allow_on_fn/builds/430632901

@flip1995
Copy link
Member

flip1995 commented Sep 21, 2018

One of these commits are at fault here:

rust-lang/rust@758239c
rust-lang/rust@d2048b6
rust-lang/rust@6bf6d50

Travis: https://travis-ci.org/flip1995/test_allow_on_fn

phansch added a commit that referenced this issue Oct 5, 2018
This collapses both lint tests into one file.
Somehow allowing the other lint in the respective files did not work
correctly. Maybe that's fixed as part of fixing #3198.
@flip1995
Copy link
Member

This is no longer an issue: Playground

@gnzlbg
Copy link
Contributor Author

gnzlbg commented May 14, 2019

Is there a test that prevents this from breaking again?

@flip1995
Copy link
Member

Oh good point!

@flip1995 flip1995 reopened this May 14, 2019
@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy E-needs-test Call for participation: writing tests and removed C-bug Category: Clippy is not doing the correct thing labels May 14, 2019
flip1995 added a commit to flip1995/rust-clippy that referenced this issue May 14, 2019
bors added a commit that referenced this issue May 14, 2019
Add test for #3198

Closes #3198

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-needs-test Call for participation: writing tests good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants