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

Unable to allow same_item_push on beta and released Rust version #6002

Closed
bgurney-rh opened this issue Sep 2, 2020 · 7 comments
Closed

Unable to allow same_item_push on beta and released Rust version #6002

bgurney-rh opened this issue Sep 2, 2020 · 7 comments
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@bgurney-rh
Copy link

As part of preparing devicemapper-rs in Stratis for the 1.46 release, I was addressing a lint error message from clippy that appears to be a false one, since it's on a non-constant value (which would correspond to issue #5902).

(Related pull request from devicemapper-rs: stratis-storage/devicemapper-rs#576)

"rustup default beta" (rustc 1.47.0-beta.1)
...
error: it looks like the same item is being pushed into this Vec
   --> src/core/dm.rs:582:17
    |
582 |                 targets.push((targ.sector_start, targ.length, target_type, params));
    |                 ^^^^^^^
    |
    = note: `-D clippy::same-item-push` implied by `-D warnings`
    = help: try using vec![(targ.sector_start, targ.length, target_type, params);SIZE] or targets.resize(NEW_SIZE, (targ.sector_start, targ.length, target_type, params))
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#same_item_push

As part of continuous integration testing, clippy is executed on both the beta, and the released version. However, in the case of 1.46.0, and same_item_push, the lint cannot be found in the released version:

"rustup default 1.46.0" (1.46.0-x86_64-unknown-linux-gnu)
...
error: unknown clippy lint: clippy::same_item_push
   --> src/core/dm.rs:558:13
    |
558 |     #[allow(clippy::same_item_push)]
    |             ^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::unknown-clippy-lints` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unknown_clippy_lints
    Checking tempfile v3.1.0
error: aborting due to previous error

For now, we changed the allow to #[allow(clippy::all)], but it would be preferable to be able to allow the specific lint on both the beta and released version.

@bgurney-rh bgurney-rh added the C-bug Category: Clippy is not doing the correct thing label Sep 2, 2020
@mulkieran
Copy link

We, like to run clippy on beta on our CI for various reasons. The principle one is so that when we move up to the new stable version of Rust we will have already fixed all the lints. But, as the above illustrates if the lint is a false positive, we will not be able to fix it, and if it is new we will not be able to allow it on stable. We have chosen to use clippy::all, which allows this lint that can't be named on stable. But, it's on a pretty largish expression, so that may allow other lints to creep in as well.

Ideally, the lint name would be introduced in one version, and then the lint would be implemented in the next version. This way, on stable, we could just allow the lint by name.

However, we may choose to drop the beta lint task from our CI, as this, while it has some benefits, and has caught at least one lint regression, it also has drawbacks.

@ebroto
Copy link
Member

ebroto commented Sep 3, 2020

One option would be to allow the lint from the command line only for the beta pipeline:

cargo clippy -- -A clippy::same_item_push

See here for the relevant documentation.

@mulkieran
Copy link

One option would be to allow the lint from the command line only for the beta pipeline:

cargo clippy -- -A clippy::same_item_push

See here for the relevant documentation.

That is true (and would not require radical surgery on our CI). Thanks for the suggestion!

@ghost
Copy link

ghost commented Sep 4, 2020

Or allow clippy::unknown-clippy-lints

@mulkieran
Copy link

Or allow clippy::unknown-clippy-lints

Thanks!

@ebroto
Copy link
Member

ebroto commented Sep 4, 2020

@bgurney-rh @mulkieran does the last suggestion solve your problem? Just asking in case we can close this issue :)

@mulkieran
Copy link

Please feel free to close. thx.

@ebroto ebroto closed this as completed Sep 4, 2020
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

No branches or pull requests

3 participants