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

Be able to disable/enable Clippy lints globally #22

Open
4 of 5 tasks
repi opened this issue Apr 11, 2019 · 22 comments
Open
4 of 5 tasks

Be able to disable/enable Clippy lints globally #22

repi opened this issue Apr 11, 2019 · 22 comments
Labels
cargo have-workaround Issues we have / are using a workaround for

Comments

@repi
Copy link
Contributor

repi commented Apr 11, 2019

In our monorepo we want to have the same baseline of Clippy lints enabled & disabled across our crates. Right now we need to set this up manually for every crate in lib.rs / bin.rs which is error prone and cumbersome.

This is what almost all of our crates have right now:

#![warn(clippy::all)]
#![warn(rust_2018_idioms)]
#![allow(clippy::new_ret_no_self)] // believe this is fixed in nightly https://github.com/rust-lang-nursery/rust-clippy/issues/3313

We would strongly prefer to instead be able to specify this in our workspace Cargo.toml or root Clippy.toml.

This is tracked in:

@repi repi added the cargo label Apr 11, 2019
@repi
Copy link
Contributor Author

repi commented Apr 11, 2019

Ran into this again today with trying to upgrade to Rust 1.34 and where the redundant_closure had been enabled.

All the cases it triggered on was for small member wrapper functions for simple conversions and where it would be a lot more verbose and quite annoying to use the full written out types, examples:

warning: redundant closure found
   --> br/src/network/ml_service.rs:147:45
    |
147 |     TcpListener::bind(&socket_addr).map_err(|e| e.into())
    |                                             ^^^^^^^^^^^^ help: remove closure as shown: `std::convert::Into::into`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

///////////////////////////////////////////

warning: redundant closure found
   --> texture-synth/src/applet.rs:505:18
    |
505 |             .map(|s| s.to_string())
    |                  ^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string`
    |
note: lint level defined here
   --> texture-synth/src/lib.rs:1:9
    |
1   | #![warn(clippy::all)]
    |         ^^^^^^^^^^^
    = note: #[warn(clippy::redundant_closure)] implied by #[warn(clippy::all)]
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

////////////////////////////////////////

warning: redundant closure found
   --> texture-synthesis/src/service.rs:110:62
    |
110 |                     example_guides: source_guides.iter().map(|a| a.to_rgba()).collect(),
    |                                                              ^^^^^^^^^^^^^^^ help: remove closure as shown: `image::dynimage::DynamicImage::to_rgba`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure

We also had a few of these cases where the suggested code didn't work because the types it suggested didn't have the crate:: prefix. Looks like it is tracked in this issue: rust-lang/rust-clippy#3071.

Regardless, this is a case where we would like to disable the redundant_closure lint by default across our crates. But for now likely will have to just add it to existing crates that run into this issue, and maybe manually propagate that lint toggle to the rest of our crates to avoid our devs running into this clippy warning and changing the code in a non-ideal way.

repi added a commit that referenced this issue Apr 14, 2019
@repi
Copy link
Contributor Author

repi commented Jun 12, 2019

We now have 81 crates in our monorepo, all having to specify #![warn(clippy::all)] & co in every lib.rs to get the behavior we want here. It works but it is a bit messy :(

@yorickpeterse
Copy link

@repi I ran into this myself for a project I am working on. When running Clippy you can disable lints via the CLI. I ended up putting this in an already existing Makefile, for example:

clippy:
	touch src/lib.rs
	${CARGO_CMD} clippy -- -Dwarnings \
		-Arenamed_and_removed_lints \
		-Aclippy::new_without_default \
		-Aclippy::needless_range_loop

Here the touch src/lib.rs is needed because otherwise Clippy does not (or at least did not in the past) build the changed files. Here the flag -A is used to globally disable certain lints.

Clippy also supports using a configuration file, but last I checked you can't disable lints using it.

@repi
Copy link
Contributor Author

repi commented Dec 7, 2019

@yorickpeterse thanks, that does look like a workable solution for CI.

Though for working locally for users and with tools like RLS I really want a solution where everyone can just run cargo clippy and it has our set of additionally enabled or disabled lints so everyone runs the same lints. Hopefully the clippy config can be extended for that. Or even better: in Cargo.toml

@yorickpeterse
Copy link

yorickpeterse commented Dec 7, 2019

@repi There is a discussion about adding support for enabling/disabling lints in Cargo.toml in rust-lang/cargo#5034, but it seems that at least since 2018 (example: rust-lang/cargo#5728) not a lot of progress has been made. I wouldn't be surprised if it will take a while for configuration file support to land in Clippy.

@repi
Copy link
Contributor Author

repi commented Dec 20, 2019

Yeah doesn't look like very active development of it unfortunately, but hope Cargo/Clippy gets it eventually.

Just added these lines on top of 30 lib.rs files in our workspace to enable more lints for our codebase.

#![warn(clippy::all)]
#![warn(clippy::doc_markdown)]
#![warn(clippy::dbg_macro)]
#![warn(clippy::todo)]
#![warn(clippy::empty_enum)]
#![warn(clippy::enum_glob_use)]
#![warn(clippy::pub_enum_variant_names)]
#![warn(clippy::mem_forget)]
#![warn(clippy::use_self)]
#![warn(clippy::filter_map_next)]
#![warn(clippy::needless_continue)]
#![warn(clippy::needless_borrow)]
#![warn(rust_2018_idioms)]

Copy'n'paste is not pretty, but works.

@repi
Copy link
Contributor Author

repi commented Sep 8, 2020

This is getting a bit more silly without this support, we have like 30 crates in our repo where all of their lib.rs start something like this:

#![forbid(unsafe_code)]
#![warn(
    clippy::all,
    clippy::doc_markdown,
    clippy::dbg_macro,
    clippy::todo,
    clippy::empty_enum,
    clippy::enum_glob_use,
    clippy::pub_enum_variant_names,
    clippy::mem_forget,
    clippy::use_self,
    clippy::filter_map_next,
    clippy::needless_continue,
    clippy::needless_borrow,
    clippy::match_wildcard_for_single_variants,
    clippy::if_let_mutex,
    clippy::mismatched_target_os,
    clippy::await_holding_lock,
    clippy::match_on_vec_items,
    clippy::imprecise_flops,
    clippy::suboptimal_flops,
    clippy::lossy_float_literal,
    clippy::rest_pat_in_fully_bound_structs,
    clippy::fn_params_excessive_bools,
    clippy::exit,
    clippy::inefficient_to_string,
    clippy::linkedlist,
    clippy::macro_use_imports,
    clippy::option_option,
    clippy::verbose_file_reads,
    clippy::unnested_or_patterns,
    rust_2018_idioms,
    future_incompatible,
    nonstandard_style
)]

Does work though!

@repi
Copy link
Contributor Author

repi commented Nov 25, 2020

We now have ~170 crates across ~20 repositories so have an even larger list of Clippy lints we enable by default across them now scattered and duplicated across 170 locations, which makes it hard to add and esp easy to miss when creating new crates.

We really do need the proper Cargo/Clippy solution for a clippy.toml where we can specify the above list of warn/deny/forbid lints that can be applied for the entire repository: rust-lang/cargo#5034. We'll try and investigate if we can help push the development of that and clippy further. It is a fantastic tool that we want to use more of!

@repi
Copy link
Contributor Author

repi commented Feb 11, 2021

We had a good sync with the Clippy team a couple of weeks ago and this is one of the top issues in the Clippy 2021 Roadmap!

@cztomsik
Copy link

omg yes!

@repi repi added the have-workaround Issues we have / are using a workaround for label Oct 18, 2021
@repi
Copy link
Contributor Author

repi commented Oct 19, 2021

We finally have a workaround for this that works quite well that we are transitioning our projects for, and that is to use .cargo/config.toml and set lint allow/deny CLI parameters with rustflags:

[target.'cfg(all())']
rustflags = [
    # BEGIN - Embark standard lints v0.4
    # do not change or add/remove here, but one can add exceptions after this section
    # for more info see: <https://github.com/EmbarkStudios/rust-ecosystem/issues/59>
    "-Dunsafe_code",
    "-Wclippy::all",
    "-Wclippy::await_holding_lock",
    "-Wclippy::char_lit_as_u8",
]

from #68.

There are some tradeoffs with this approach but overall it works well and dramatically easier to maintain and work with, our main private repo with 80+ crates no longer have to specify our long list of standard lints in every lib.rs/main.rs.

Still would prefer proper Cargo support of a lints.toml or [lints] section to be able to have different lints in different workspaces (in the same repo) and to have a cleaner syntax for this. Though that has been slow moving but tracked in:

So will leave this issue open for now, but tagged as have-workaround

@jplatte
Copy link

jplatte commented Oct 19, 2021

There seems to be a downside to this in that it only works as of Rust 1.55 (earlier versions raise an error when using -Wclippy::lint with a non-clippy command). Crates should still work with older compilers when used as dependencies, but for MSRV CI jobs it's a problem.

@repi
Copy link
Contributor Author

repi commented Oct 19, 2021

Ah interesting, we are on Rust 1.55 and now with our v5 lint set (#67) it also requires 1.55 as we use new lints from it.

Thought initially this approach wouldn't work as also believed that Cargo/Rust would have failed on those unknown clippy lints that only clippy knows about, but was glad to see that it didn't well at least not with 1.55. But good to know that it seems to require 1.55 so must have been a change in how they handle the allow/deny flags (hopefully an intentional one).

@jplatte
Copy link

jplatte commented Oct 19, 2021

Maybe it's fallout from rustdoc lints being moved to their own namespace a few versions earlier, and people wanting to keep activating them (with new names) in some way where they would be passed to non-rustdoc invocations. It's definitely a reasonable change to just ignore unknown tool-specific lints so I'm sure it will be kept even if it wasn't intentional 😄

DmitrySamoylov added a commit to lumeohq/api-client that referenced this issue Dec 7, 2021
DmitrySamoylov added a commit to lumeohq/api-client that referenced this issue Dec 8, 2021
Herschel added a commit to Herschel/ruffle that referenced this issue Jan 29, 2022
Currently it is not directly possible to configure lints for the
entire workspace via TOML, which forced us to repeat `#![allow]`
blocks in each crate.

embark pointed out this workaround to configure lints at the
workspace level via RUSTFLAGS:

EmbarkStudios/rust-ecosystem#22 (comment)

Remove the common `#![allow]` blocks and switch to this method for
global lint config.

Temporarily allow `needless_borrow` lint, buggy pending this fix:
rust-lang/rust-clippy#8355
Herschel added a commit to ruffle-rs/ruffle that referenced this issue Jan 29, 2022
Currently it is not directly possible to configure lints for the
entire workspace via TOML, which forced us to repeat `#![allow]`
blocks in each crate.

embark pointed out this workaround to configure lints at the
workspace level via RUSTFLAGS:

EmbarkStudios/rust-ecosystem#22 (comment)

Remove the common `#![allow]` blocks and switch to this method for
global lint config.

Temporarily allow `needless_borrow` lint, buggy pending this fix:
rust-lang/rust-clippy#8355
emilk added a commit to emilk/egui that referenced this issue Mar 21, 2022
That way they apply to all crates equally.

See EmbarkStudios/rust-ecosystem#22 for why.
emilk added a commit to emilk/egui that referenced this issue Mar 21, 2022
That way they apply to all crates equally.

See EmbarkStudios/rust-ecosystem#22 for why.
@jplatte
Copy link

jplatte commented Jun 7, 2022

We just hit a very weird issue with the .cargo/config.toml-based workaround, where cargo <command> and cargo <command> --target foo would partially invalidate each other's caches. Turned out a historical design decision in Cargo was the culprit: rust-lang/cargo#4423

Luckily, there is a workaround, you just add this to the config.toml to fix the problem on Nightly (I don't know of any solutions for stable): (see latest comments)

# outside any section
target-applies-to-host = false

[unstable]
target-applies-to-host = true

PR that fixes this for our project, with extra comments: https://github.com/matrix-org/matrix-rust-sdk/pull/737/files

@repi repi mentioned this issue Jun 7, 2022
44 tasks
@jplatte
Copy link

jplatte commented Jun 10, 2022

Regarding my previous comment, do not use that nightly feature if you also want to pass RUSTDOCFLAGS to rustdoc through cargo +nightly doc, the flags will silently get ignored: rust-lang/cargo#10744

@jplatte
Copy link

jplatte commented Jun 13, 2022

One more update: I've found a good-ish workaround: Just reset target-applies-to-host to its default value when building docs, for example by setting the environment variable CARGO_TARGET_APPLIES_TO_HOST to true. (cc matrix-org/matrix-rust-sdk#757)

@9SMTM6
Copy link

9SMTM6 commented Jun 28, 2022

Btw, I've found a different (kinda horrible) workaround:

You can add another binary that declares all the other entrypoints to be linted as modules, and then add the lints in that binary.

It seems to have a number of drawbacks that you'll probably find out quickly (ie it triggers dead_code), but I just thought I'd leave that here, perhaps it turns out to be the least evil for some and/or the main problems can be fixed.

@repi
Copy link
Contributor Author

repi commented Jun 28, 2022

Ah thanks for sharing! Kinda horrible indeed but good to know about!

@jplatte
Copy link

jplatte commented Jun 30, 2022

Ugh, I found that target-applies-to-host entirely disables rustflags-based clippy configuration (it's not just rustdoc) on nightly for regular builds without an explicit --target. This feature breaks more things than it fixes 🙄

@ericseppanen
Copy link

I wrote cargo-cranky, which allows easy clippy lint configuration. Lint config goes in a Cranky.toml file and then cargo cranky just runs cargo clippy with the right command line that enables those lints. It's pretty basic so far, but I find it a nice improvement, as it can handle every bin/lib/example in a workspace.

@avantgardnerio
Copy link

I need this for the single_range_in_vec_init rule, which is telling me valid code is invalid and offering fixes that would alter behavior:

warning: a `Vec` of `Range` that is only one element
   --> datafusion/physical-expr/src/window/rank.rs:291:32
    |
291 |         test_f64_result(&r, 7, vec![0..7], expected)?;
    |                                ^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_range_in_vec_init
help: if you wanted a `Vec` that contains the entire range, try
    |
291 |         test_f64_result(&r, 7, (0..7).collect::<std::vec::Vec<usize>>(), expected)?;
    |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
help: if you wanted a `Vec` of len 7, try
    |
291 |         test_f64_result(&r, 7, vec![0; 7], expected)?;
    |                                     ~~~~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cargo have-workaround Issues we have / are using a workaround for
Projects
None yet
Development

No branches or pull requests

7 participants