Create lint passes using Conf#13088
Conversation
rustbot
added
the
S-waiting-on-review
flip1995
left a comment
flip1995
left a comment
There was a problem hiding this comment.
A dependence on the config being a static reference is also added
Wasn't that that case already anyway, as the register_lints function already took the Conf as a 'static reference?
I really like those changes, thank you! Only one small comment.
| pub fn new(conf: &'static Conf) -> Self { | ||
| Self { | ||
| too_large_for_stack: conf.too_large_for_stack, | ||
| msrv: conf.msrv.clone(), |
There was a problem hiding this comment.
Why is MSRV cloned, while everything else that is not Copy is referenced?
There was a problem hiding this comment.
I assume it's because MSRV is sometimes mutated during linting when visiting #[clippy::msrv], while other configurations are not
There was a problem hiding this comment.
Might be a nice use case for Cow<'static, Msrv> since it'll avoid clones if code never uses #[clippy::msrv] but not sure if that's overdoing it
There was a problem hiding this comment.
It was taken as 'static, but it wasn't needed.
The config type should really be Option<RustcVersion>. I don't really know why Msrv exists as a type at all.
There was a problem hiding this comment.
If you mean we should always be using Option<RustcVersion> instead of Msrv (a stack of RustcVersions), then I don't see how nested #[clippy::msrv] could work (or even a single #[clippy::msrv] with a clippy.toml that also defines an msrv).
If a lint pass updates and overwrites its Option<RustcVersion> because of a #[clippy::msrv] then surely it loses the previous, enclosing msrv?
(unless you're implying that it's weird to have nested msrvs to begin with)
There was a problem hiding this comment.
The type stored in the config should be Option<...>. The Msrv should just be Vec<...> rather than it's own type.
There was a problem hiding this comment.
Ah, I see. Yeah, having Msrv in the config doesn't make much sense. The type exists, because there are some convenience functions implemented on it. I thought this was nicer back then. Not married to it though, if you want to change this.
But let's not block this PR on this.
|
@bors r+ |
Create lint passes using `Conf` This slightly reduces the amount of code changes needed to add a config to a lint and makes things makes things more consistent between passes. A dependence on the config being a static reference is also added. This would only ever be a problem if multiple crates end up compiled in a single process. Other changes include: * Removing useless `#[derive(..)]`s * Removing `#[must_use]` on lint pass constructors. * Unified the parsing of the `DisallowedPath` struct in lint passes. * Update `disallowed_types` and `await_holding_invalid` messages to be consistent with other disallowed lints. * Remove the `(from clippy.toml)` message. I plan on having all the configured lints point to point to a span in `clippy.toml` which will be more useful. changelog: none
|
💔 Test failed - checks-action_test |
|
Please run |
* Construct lint passes by taking `Conf` by reference. * Use `HashSet` configs in less places * Move some `check_crate` code into the pass constructor when possible.
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
6 participants
This slightly reduces the amount of code changes needed to add a config to a lint and makes things makes things more consistent between passes. A dependence on the config being a static reference is also added. This would only ever be a problem if multiple crates end up compiled in a single process.
Other changes include:
#[derive(..)]s#[must_use]on lint pass constructors.DisallowedPathstruct in lint passes.disallowed_typesandawait_holding_invalidmessages to be consistent with other disallowed lints.(from clippy.toml)message. I plan on having all the configured lints point to point to a span inclippy.tomlwhich will be more useful.changelog: none