-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
disallowed_methods should allow for reasons in the toml file #7609
Comments
+1 for this! We just recently enabled this lint and disallowed a set of functions and I found myself writing brief motivations / reasons for the different functions we are disallowing for our codebase for my coworkers and future contributors to be able to see what to use instead. Our current small # for `disallowed_method`: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_method
disallowed-methods = [
# std functions
"std::env::temp_dir", # we use the tempdir crate instead
"std::env::home_dir", # deprecated, and we use app_dirs2 crate
# use the faster & simpler non-poisonable primitives in `parking_lot` instead
"std::sync::Mutex::new",
"std::sync::RwLock::new",
"std::sync::Condvar::new",
# "std::sync::Once::new", # enabled for now as the `log_once` macro uses it internally
# use `ark_future::block_on` instead for named profiler markers
"futures::executor::block_on",
# SHA-1 is cryptographically broken, and we are building new code so should not use it
"sha1::Digest::new",
] which was a nice start, but if this lint is improved with reasons built in it would be even faster and easier for users to see when they run into a linting error of what to use instead and why, instead of having to find and lookup Clippy.toml for the description. With this support I think our current small list could look something like this: disallowed-methods = [
# std functions
{ path = "std::env::temp_dir", reason = "we use the tempdir crate instead" },
{ path = "std::env::home_dir", reason = "method iis deprecated, use app_dirs2 crate instead" },
{ path = "std::sync::Mutex::new", reason = "use the faster & simpler non-poisonable `parking_lot::Mutex` instead" },
{ path = "std::sync::RwLock::new", reason = "use the faster & simpler non-poisonable `parking_lot::RwLock` instead" },
{ path = "std::sync::Condvar::new", reason = "use the faster & simpler non-poisonable `parking_lot::Condvar` instead" },
# "std::sync::Once::new", # enabled for now as the `log_once` macro uses it internally
{
method = "futures::executor::block_on",
reason = "Use `ark_future::block_on` instead. This requires specifying a name for the blocking time and enables us to get nice instrumentation in profilers of wait times"
},
{ method = "sha1::Digest::new", reason = "SHA-1 is cryptographically broken, and we are building new code so should not use it" },
] |
Another potential improvement would be to allow for a "rename" subkey that lets you ask the method be automatically replaced with another with the same signature, unfortunately this will typically require inserting an (ugly) full path without local type info. I wouldn't do that as part of this issue but it's an extension we can file a followup for that will likely need more careful design work. |
Considering almost all of the cases where you disallow one thing you actually want to point people to some other thing I wonder if it wouldn't make sense to encode that. disallowed-methods = [
# std functions
{ path = "std::sync::Mutex::new", replacement = "parking_lot::Mutex::new", reason = "prefer faster & simpler non-poisonable mutex" },
] While obviously |
@mitsuhiko yeah that's what I suggested in my previous comment; the tricky thing is dealing with signatures and replacements in a good way there. For now let's get reasons in, that would be a followup imo |
Taking a look at the impl for this |
Is there a good reason not to allow for reasons in |
Nope, feel free to implement it if you'd like 😄 |
…iraffate Allow giving reasons for `disallowed_types` Similar to #7609 but for the `disallowed_type` lint. The permitted form of configuration is the same as for `disallowed_methods`. changelog: Allow giving reasons for [`disallowed_type`]
We currently have the
disallowed_methods
(added in #6081) which allows for aclippy.toml
config ofdisallowed-methods = ["foo", "bar::baz"]
, etc.It would be nice if the config also allowed for reasons, something like:
so that the warning can output the reason when it comes across this methods
See https://twitter.com/nokusu/status/1432339032705544193, https://twitter.com/fasterthanlime/status/1432333612251238402
This shouldn't be hard to implement, happy to mentor!
The text was updated successfully, but these errors were encountered: