Clippy plans

[nrc]

• [ ] Define stability
• [x] Move to the nursery
• [x] Submodule into Rust repo
• [ ] Distribute with Rustup - enables stable Rust
• [x] lint audit, leading to 1.0
• More...

[Manishearth]

https://internals.rust-lang.org/t/rust-ci-and-submodule-crates/5232/3

[nrc]

On the move to Rust repo and Rustup distribution, there are three blockers - defining stability for Clippy, settling the stability policy questions for Rustup distribution, and getting experience with tools and Rustup with the RLS and Rustfmt.

[Manishearth]

I'm not sure if the stability plans need more articulation than this (lmk if so)

Clippy considers a breaking change to be a removal of a lint. It makes no guarantees on whether or not your code will always compile with deny(some_lint), and it definitely makes no guarantees about deny(clippy). The same as rustc. We will try to limit changes that cause swathes of new warnings. We already do this and stick deprecated lints in a box of allow lints that do nothing.

With this in mind it is effectively stable already. The only issue is working with stable rust, which is the whole submodule/rustup deal.

We definitely need to do a lint audit. Or, really, we already know what false positives there are, and we should tag some of those as 1.0 blockers (which can lead to the lint being temporarily made allow if the bug isn't fixed). We should also go through all the lints and make a general judgement as to what should be allow/warn, and perhaps decide if we do want more lint groups (https://github.com/Manishearth/rust-clippy/issues/632#issuecomment-180891984). Mostly minor stuff, can be done easily when the time comes.

See also: https://github.com/Manishearth/rust-clippy/issues/1358

Does this sound good?

cc @llogiq @oli-obk @mcarton

[nrc]

Clippy considers a breaking change to be a removal of a lint

All lints, even those that are allow? I fear this means that any new lint is insta-stable and must be maintained forever. I think it is worth accommodating unstable lints some how. Perhaps linking this to the channel the user is on.

It makes no guarantees on whether or not your code will always compile with deny(some_lint)

I wonder if this is too weak. This effectively says any lint can change in anyway, i.e., it is legal to change a lint to always pass or always fail, which is effectively the same as removing the lint.

Again, it might be worth categorising the stability of lints. Ideally, for the most stable lint (presumably those that are stable and deny by default), then they have similar guarantees to compiler errors, rather than compiler warnings. I also wonder whether we need something even stronger than that - given that the reason to use Clippy is to catch bugs, we might want to promise that a lint will continue to fail for certain programs. I'm not at all sure how to phrase that though.

[Manishearth]

I fear this means that any new lint is insta-stable and must be maintained forever.

No, because we can "remove" lints by marking them as deprecated.

When I say "Clippy considers a breaking change to be a removal of a lint" I mean that clippy considers it a breaking change to stop having a lint of that name. When we want to remove lints, we just replace them with stubs that always pass.

This is because rustc doesn't like it when you do #[allow(lint_that_doesnt_exist)].

which is effectively the same as removing the lint.

(by design)

Ideally, for the most stable lint (presumably those that are stable and deny by default), then they have similar guarantees to compiler errors, rather than compiler warnings.

Yeah, deny by default lints being frozen such that they will never cause new errors on a clippy upgrade is fine with me.

They still should be open to having false positives removed (i.e., so that they don't error on something that was previously an error). This also means they can be removed entirely via the "always pass" mechanism.

I'd rather not get into "it will always fail for code X".

[oli-obk]

There's no sensible way to guarantee that a lint will always be emitted for a given piece of code. Even if we change nothing in clippy, if inference changes (which is not a breaking change as far as I understand the commentary around it) our type resolution in clippy changes, and may break a lint. I'm sure there are many other things that can change and will invalidate even our simplest checks.

That said, I think we could state that except for removing false positives, we will strive to not remove lints or make them less powerful. We do want to minimize false negatives, but not at arbitrary cost and most definitely not if that would incur false positives.

[Manishearth]

We will "remove" (read: make always pass) lints that are no longer the recommendation, though.

E.g. lints that don't make sense anymore in light of new features (the clippy ref lint might be one of these with the match ergonomics stuff), or lints that are no longer the accepted style.

[nrc]

@Manishearth That sounds basically reasonable, it is probably worth being explicit about deprecated lints.

This is because rustc doesn't like it when you do #[allow(lint_that_doesnt_exist)].

Does this mean that if someone adds annotations to a crate expecting Clippy, that it can no longer be compiled (without warnings) if not using Clippy?

I wonder if it is worth having a policy for removing deprecated lints? Perhaps it is enough to say that you'll only do that at major version increments. It seems otherwise that in 10 years time you could have thousands of deprecated lints.

[nrc]

And to clarify, how are 'deprecated' lints and 'removed' (that is always pass) lints related - is one a subset of the other, or are they the equivalent?

[oli-obk]

I'm not sure how we could do major version bumps if clippy is distributed with the compiler, since it will be usable without specifying it in the cargo.toml

Not sure what a deprecated lint means. Printing a deprecation method seem weird.

[Manishearth]

And to clarify, how are 'deprecated' lints and 'removed' (that is always pass) lints related - is one a subset of the other, or are they the equivalent?

They're the same, I use the terms loosely.

Does this mean that if someone adds annotations to a crate expecting Clippy, that it can no longer be compiled (without warnings) if not using Clippy?

Yeah, that too.

Of course, it is rare to run clippy on dependencies, so this has the same effect as cap-lints in the sense that it limits breakages to your own crates. But I don't want to break a compile in any case other than "oops we ICEd" and "you wrote #[deny()]".

It seems otherwise that in 10 years time you could have thousands of deprecated lints.

Fine by me, really 😄 . They're just names registered in the plugin registry, and they don't do anything (like i said, deprecated lints are "always pass"/"removed" lints)

[oli-obk]

Does this mean that if someone adds annotations to a crate expecting Clippy, that it can no longer be compiled (without warnings) if not using Clippy?

Interesting point. With the current setup that is true. But #![plugin(foo)] isn't stable, so we can think about how to design the clippy integration. Is clippy always on? But allow-by-default? Then there's no issue. Or will #![plugin(clippy)] be allowed on stable? Then we could decide to automatically enable the existing clippy cfg

[Manishearth]

I assume we'll have a #![clippy(args)] and -Zinclude-clippy=(args) stable form of the unstable plugin/extra-plugins args. The #![clippy] arg doesn't need to exist but explicitly included lints are easier to make work with bespoke build systems.

[phansch]

I guess with the RFC merged and Clippy being available via rustup, this can be closed, right?