Stability tracking - Clippy

Rust distribution:

move to nursery
move tests back to ui tests
~~Wait for RLS/rustfmt distro issues to be worked out~~ Stability tracking - Clippy #18 (comment)
Add to Rust repo submodule
- Add building miri to Rust CI
- Add miri's tests to Rust CI
Add to Rust distro for nightly only
Start recommending the distro version
Make cargo clippy require rustup component add clippy, clean up cargo clippy's impl
Clippy 1.0, if it's not happened already (probably will have)
Turn it on for stable too

What are the preconditions for the nursery move?

What are the preconditions for the nursery move?

None. We can do it asap.

Can we do an intermediate step and move clippy to the rust repository, but keep publishing it in the current manner? There have been complaints and a lot of "me too" github votes on nightly breakage that isn't fixed within a day or two.

Note that the "move clippy to the rust repo" plan is probably going to be via submoduling (perhaps submoduling a fork/branch to make it easier to do in-place fixes without waiting for clippy review). So the repo stays here, but it also goes in tree and gets CId.

I'm open to doing that step now (adding a submodule and possibly setting up CI, but no distribution). Is there anything we should be worried about before this?

I think the logical progression here would be:

Add to Rust repo submodule
Add to Rust CI
Wait for RLS/rustfmt distro issues to be worked out
Add to Rust distro for nightly only
Start recommending the distro version
Make cargo clippy require rustup component add clippy, clean up cargo clippy's impl
Clippy 1.0, if it's not happened already (probably will have)
Turn it on for stable too

I edited the OP to reflect these steps.

Is there anything we should be worried about before this?

I should probably move our tests back to ui tests so they integrate better with rustc's test suite.

Add to Rust repo submodule

I think this should be ok to do, I'll need to check with some of the core team

Add to Rust CI
Wait for RLS/rustfmt distro issues to be worked out

We're probably going to need to do these in the opposite order since once you're in the CI, you block PRs landing which mean all issues with the updating procedure, etc. need to be worked out.

Add to Rust distro for nightly only
...
Turn it on for stable too

Although we did this for the RLS, there is a strong feeling that we shouldn't do this for Rustfmt and Clippy. That is, once it hits the distro, it should be ready to ride the trains. We can use a clippy-beta name rather than just clippy to indicate that it is not 1.0 and there will be infrastruture to automatically rename to just clippy when we're ready

I should probably move our tests back to ui tests so they integrate better with rustc's test suite

I think tests would be run separately from the compiler's (this is what we do with Cargo and RLS), so I don't think this is necessary (can still do it if you want, of course).

We're probably going to need to do these in the opposite order since once you're in the CI, you block PRs landing which mean all issues with the updating procedure, etc. need to be worked out.

is there a tracking issue for that or some other central place where the issues are discussed?

is there a tracking issue for that or some other central place where the issues are discussed?

Not really sorry. There's been a lot of ad hoc discussion on this topic - some on i.r-l.o and some in meetings, etc.

Some issues are tracked here: #15

This doc discusses some things: https://github.com/nrc/dev-tools-team/blob/master/stability/README.md

And there are some 'unknown unknowns' that we'll discover as we push on the RLS distribution

We're probably going to need to do these in the opposite order since once you're in the CI, you block PRs landing which mean all issues with the updating procedure, etc. need to be worked out.

I don't agree. The distro issues are a completely different set of issues from the CI issues, and tying them together seems like an unnecessary delay.

(To be clear, when I said "distro issues", I was talking about the whole deal of "do we choose to have partial nightlies?", "how does this interact with the dist build", "how does rustup component add work with this?", "installed by default?".)

I would like to add to CI early perhaps with the option for PR authors to turn it off to get stuff landed, as we work out the details of the process. IMO this will at least give us an early warning system, and by encouraging rustc devs to fix it on the clippy side too we can gauge how easy the process is and work towards improving it, all the while having an escape hatch for rustc devs when the process gets too hard.

In general having a submodule which PR authors are allowed to redirect temporarily to a fork or a branch is a pretty smooth process, with a bit of burden on clippy devs to resync to master afterwards (which is fine).

I don't really think this must be figured out for RLS and rustfmt first either, especially because it seems like these discussions for rustfmt and RLS haven't really gotten anywhere yet -- I'm willing to coordinate/push whatever decision making is necessary for clippy to move forward here.

That is, once it hits the distro, it should be ready to ride the trains.

I don't really see a reason to do this, but I see a major reason to not. Specifically, cargo-clippy does really weird things now so that cargo install clippy is possible, and this can be greatly simplified once the plugin dylib is in the distro, but this is a nontrivial amount of work requiring both rustc and clippy changes and is best done after clippy is in the nightly distro since it's easier to do incrementally.

It would also make life much easier for clippy users without requiring us to fix all the problems (which we can continue to work on incrementally); since then it won't break every two nightlies.

I don't agree. The distro issues are a completely different set of issues from the CI issues, and tying them together seems like an unnecessary delay.

I'm not talking about the distro issues, I'm talking about issues (both technical and political) of having tools CI blocking Rust PRs. There has been a lot of discussion and a lot of ideas floated, and in that situation we need to get actual experience with the processes before over-committing.

perhaps with the option for PR authors to turn it off to get stuff landed

sure, but this needs to be implemented, documented, and communicated to other teams.

In general having a submodule which PR authors are allowed to redirect temporarily to a fork or a branch is a pretty smooth process, with a bit of burden on clippy devs to resync to master afterwards (which is fine).

This is what we do with the RLS, but it is a bit more complicated than this. One constraint is that we don't want to point a submodule at a branch that might disappear, which rules out branches on individual's repos. That means one of the RLS/Clippy maintainers has to push the branch to upstream (and make sure it never gets deleted). This has proven to be enough of a speed bump that nobody has done this except me (a couple of times we've merged PRs to the RLS or Rustfmt that can solve a problem backwards compatibly).

I don't really think this must be figured out for RLS and rustfmt first either, especially because it seems like these discussions for rustfmt and RLS haven't really gotten anywhere yet

This is not correct - the RLS is in tree and part of the CLI and we're getting useful information from working with it.

I don't really see a reason to do this

This is an implementation issue - we simply don't have anything in the infrastructure to have something part of nightly and not ride the trains. For the RLS, it has been manually removed with every release, but that is not something that can be generalised (in fact people are pretty unhappy about doing it for the RLS). Core team feeling when we discussed tool stability was that there was little benefit from the nightly-only situation. However, it sounds like for Clippy specifically it might be useful to ease implementation - if we landed at the start of a cycle, would 6 weeks be enough time to do that implementation?

I'm not talking about the distro issues

I meant that I was 😄 . The "figure out CI plan" was a part of "add to CI" (not "Wait for RLS/rustfmt distro issues to be worked out"), I didn't explicitly mention it, my bad.

That means one of the RLS/Clippy maintainers has to push the branch to upstream

I'd have a separate repo/fork on rust-lang which all rust reviewers have push for, and clippy/devtools devs keep the two in sync.

Also, submodules pin to commits, not branches, and IIRC github doesn't delete commits after the first few minutes of their existence. We'd have to check this.

Even then, all that is sufficient to make this work is to merge submodule contributions via git merge never git rebase, because then that same hash will be in the clippy tree forever (as it is reachable from master). This lets us delete branches if we want.

This is not correct - the RLS is in tree and part of the CLI and we're getting useful information from working with it.

Fair; I wasn't aware of this.

if we landed at the start of a cycle, would 6 weeks be enough time to do that implementation?

maaaybe.

I guess a simple hack is to continue to have cargo-clippy (which so far we're keeping out of distro; though I'm open to this changing) enforce nightly. The current version has nightly-only code by necessity, and it will continue to do so during most of the process of refactoring implementation. Once most of that is torn out we can move to a stable-compat cargo-clippy, but we can also just keep a dummy #[feature(rustc_private)] in there until we're ready for it to ride the trains. In the meantime, the distributed clippy will be available for download on beta, but utterly useless there.

The nice thing is that this process is something totally compatible with the "maaaybe" I gave you -- we can try to clean things up in 6 weeks. If done in time we remove the feature flag; if not, not much harm done, we build a useless dylib on a couple platforms that can be fetched via rustup but does nothing once fetched.

@eddyb

New issue: @eddyb mentions that it is desirable to get rid of all plugins. Since clippy is currently a plugin, I see two ways forward:

link clippy to the compiler
require cargo clippy to stay as it is by being its own driver and injecting the clippy lints (that would suggest moving cargo clippy to rustc, too though)

I don't see an issue with 1. since clippy will be part of the distribution anyhow

IMO custom drivers are the way forward, and the API should change to be more ergonomic.

That sounds ok to me if the API is becoming more ergonomic.

I think as a first step keeping it a regular plugin would help make transitioning easy.

+1 for custom drivers. More ergonomics there would of course be great. I'm not really sure what Clippy's needs are, but happy to review code/design suggestions (the current API is a bit ad hoc and has basically evolved for the RLS and whatever crazy tool idea I was experimenting with at the time).

To be clear about what 'plugin' means, we're talking about the pluggable lints facility?

https://rust-lang-nursery.github.io/rust-toolstate/ clippy has now been built the same system that currently builds rls and rustfmt in the compiler

So I guess as a next step we can try distributing clippy?

So I guess as a next step we can try distributing clippy?

Yes, I think it is probably a good time! Let me check with Alex about infra stuff, and the core team to check they are OK about it.

@oli-obk

I talked to the core team about Clippy using Rustup for distribution and the answer was pretty much a firm 'no'. There are basically worried about the availability of Clippy on nightly. Some facts:

the RLS has had a bumpy ride - there have been lots of unexpected hiccups around breakage and the various ways we've tried to work around that. We're pretty much in a good place right now, but it has been a rough journey. Furthermore, a lot of the problems have been things that weren't predicted and the worry is that Clippy will have similarly unpredictable problems.
when a tool is available from rustup, even just on nightly, there is an expectation that it will be always available. No matter how much we message that it might be missing sometimes, this has a negative effect on users and puts the whole project in a bad light (this is really frustrating from our point of view because we know the quality guarantees and live up to those expectations, plus message them, but anything less than 'perfection' brings headaches)
Currently we block nightlies on the RLS working. This is OK because breakage to the RLS is now rare, and usually fixed quickly.

And some opinions:

breakage in Clippy is likely to happen more often than the RLS/rustfmt and there is no obvious way to mitigate that (such as putting libsyntax on crates.io, which has pretty much solved the breakage problem in rustfmt completely and 90% for the RLS).
there is not confidence that Clippy breakage can always be fixed quickly enough to block nightlies on Clippy (which leaves us in a bit of a no-win situation - we can't block nightlies but we can't not to do that because we don't know any other way to guarantee availability for Clippy).
there is a lot of resistance to moving any of the maintenance burden of Clippy onto the compiler devs

On the bright side, there was agreement that Clippy is an important tool and we need to find some way of distributing it reliably and conveniently.

Some things we can do next:

wait - as we get more confidence that RLS and Rustfmt are working now, the core team is more likely to view Clippy as lower risk, and also that we are not piling risk on top of risk.
brainstorm ideas for 'partial availability' - using libsyntax on crates.io has worked really well for Rustfmt. Perhaps there is an equivalent for Clippy? I don't think putting the whole compiler on crates.io will work because of the extra compilation time, but perhaps we can pin to binaries or something?
think about a higher level interface between Clippy and the compiler. I think we should do this in any case for many reasons, but this would certainly lower the risk around distributing Clippy and isolate maintenance on the compiler side. My fear is that such a thing would be too long-term to help with the distribution issues, but perhaps it can be part of the story.
I propose we meet at the all-hands in Berlin to discuss the problem and possible solutions. We could separately meet with stakeholders on the core team to bounce ideas around.

Sorry for the 'stop energy' here, but I am still very keen to get Clippy distributed on official channels and optimistic that this is a speed bump rather than a wall.

cc @oli-obk @Manishearth

I think waiting is fine, but overall I'm a bit surprised/confused by this response because some of the points stated seem to go against past discussions we've had.

there is no obvious way to mitigate that
..
think about a higher level interface between Clippy and the compiler.

I thought we'd discussed this recently? There are ways to mitigate breakage by moving bunches of clippy's utils into the compiler (and making more common tasks into utils so that you never interact directly with compiler functions). I was going to wait for it to be shipped with rustup so that it's easier to make these changes (we can make these changes atomically instead of having clippy break a lot in this process). But that's not strictly necessary, and these changes could be made within the clippy repo itself and then moved all at once.

This is a bit frustrating because I recall having this conversation twice very recently (and a couple times in the past). We haven't started doing this work (because as mentioned I was waiting to move it into rustup first), but we could do it, and it seems like it's being assumed that it won't work?

I don't think this will ever get us to rustfmt levels of stability, but it may get us at RLS levels. We'll still be breaking when people change the AST/HIR, and if there are major fundamental changes to how things work, but mostly it will be pretty stable.

there is an expectation that it will be always available
...
which leaves us in a bit of a no-win situation - we can't block nightlies but we can't not to do that because we don't know any other way to guarantee availability for Clippy

Wasn't the plan to allow for nightlies to not contain clippy, and just have it so that rustup update will not update to partial nightlies? (and be explicit about this). I'm confused as to when this changed, or maybe there was a miscommunication. I don't think nightly users would find it a problem if they missed out on one nightly a week (at worst -- the rate of breakages is less than this and we'd often be able to fix it beforehand).

It might put the nightlies in a slightly bad light, but IMO we're already currently in a very bad light because RLS and clippy are not stable. Most of the time people complain about Rust requiring nightlies these days they're talking about RLS, clippy, and sometimes Rocket. No amount of "just use cargo +nightly for dev tools" messaging will fix that.

there is a lot of resistance to moving any of the maintenance burden of Clippy onto the compiler devs

The majority of clippy breakages are breakages where the dev had to fix a whole bunch of other librustc_foo crates anyway; it's no big deal to apply the same fix to clippy (OTOH it takes work for us to figure out the fix after the fact, and to cut a new release each time). Just posting the patch on the PR and ccing us would be enough, and failing that we can always just figure out the fixes ourselves knowing the changes causing the breakage.

This even works if we're going to make it so that travis gates on a clippy build and nightlies must contain clippy: if clippy breaks tick the "allow contributors to push to this PR" thing and we'll fix it.

Historically we've been pretty fast at fixing broken nightlies. The ability to just work with the PR instead of waiting for a nightly to break -- even if the PR author isn't helping -- would already be a major win for us. The only impact on the compiler would be that it may take an extra few hours, perhaps a day, until it gets r+d -- and let's face it, bors cycle times are already much worse than that (hoping to work a fix for that into homu eventually, but I have no bandwidth for homu work right now)

Rereading this I might be coming off a bit harsh, apologies for that. I'm overall fine with the speed bump; just very confused.

I'm not sure if this is on anybody's radar, but I have a partial solution for rls + clippy users: rust-lang/rust#48097

Essentially rls ships with clippy iff clippy builds, otherwise it ships without clippy. This won't help cargo clippy users though.

It might put the nightlies in a slightly bad light, but IMO we're already currently in a very bad light because RLS and clippy are not stable. Most of the time people complain about Rust requiring nightlies these days they're talking about RLS, clippy, and sometimes Rocket. No amount of "just use cargo +nightly for dev tools" messaging will fix that.

This is the major point I think. The goal is not to get clippy into nightlies. It's to get it into stable. And we are confident that we can get clippy working for stable releases.

Speaking not from a rustc dev's but from a users perspective: What is the short-term story here? What do I need to do to get a great setup with all the super-useful dev tools?

Do you I need to install a known-good-and-complete nightly? Can I use the latest beta (assuming I don't need nightly-only features in my own code)?

Can we extend rustup to automatically pick a known-good-and-complete nightly?

From the perspective of someone telling people to try Rust: Can I say "install the VSCode plugin and click 'yes' when it asks you to automatically install some tools"? What needs to happen so that I can say this?

We do have https://rust-lang-nursery.github.io/rust-toolstate/ so there is a way to programmatically get a nightly that has the desired tools. The vscode plugin could query that and select a good nightly to use

I'm not sure if this is on anybody's radar, but I have a partial solution for rls + clippy users: rust-lang/rust#48097

It certainly is on my radar, I'm really excited about it! However, it doesn't help with Clippy's stability, only the integration of Clippy + RLS.

This is the major point I think. The goal is not to get clippy into nightlies. It's to get it into stable. And we are confident that we can get clippy working for stable releases.

There might be something interesting here. If we never distribute Clippy with the nightly channel, we don't need to worry about it breaking! We'd have to figure out the mechanics of ensuring there is a working version ready for beta though.

... What needs to happen so that I can say this?

That is kind of the big question here and what we've been solving with the RLS over the past 4 months or so. I think we're basically in a good place right now. Although tool breakage doesn't block landing a PR, it does block a release. So anytime you use Rustup you always have the tools you'd expect. The downside is that sometimes we skip nightlies altogether, although that is rare.

Essentially, between that constraint, rust-toolstate, and a few other things, we're in a good place right now. The question is therefore not how we can get to a good place, but how we can stay in the current good place while we add Clippy to the mix of supported tools.

If we never distribute Clippy with the nightly channel, we don't need to worry about it breaking!

That seems kinda backwards though? People still default to nightlies.

But we can guarantee clippy in stable and let people opt in to having clippy on beta/nightlies with the caveat that you won't be able to update to a nightly that doesn't have clippy (or the caveat that some nightlies won't have clippy).

It seems like no clippy on nightly is strictly worse than clippy on nightly, but sometimes.

I thought we'd discussed this recently?

We have discussed this a few times, which is why I included it in my comment.

There are ways to mitigate breakage by moving bunches of clippy's utils into the compiler (and making more common tasks into utils so that you never interact directly with compiler functions)

This sounds good and I think you should work on it. I don't have a good idea of how long a project this is. I've assumed that it would take quite a long time to implement and that the stability wins only come at the end. In any case, I think we need to prove the improved stability before we can block nightlies on Clippy.

I think it is important that the design here is a lightweight abstraction layer in the compiler and the hard work is still done in Clippy, rather than moving the guts of Clippy into the compiler. This is what I mean about not increasing the maintenance burden on the compiler.

We haven't started doing this work (because as mentioned I was waiting to move it into rustup first), but we could do it, and it seems like it's being assumed that it won't work?

I don't think anyone is assuming it won't work. But we do want to see it work before we tighten the integration with Clippy.

Wasn't the plan to allow for nightlies to not contain clippy, and just have it so that rustup update will not update to partial nightlies? (and be explicit about this). I'm confused as to when this changed

This changed as we learned what users expect from the RLS. Even though we clearly messaged that nightly RLS would not always be available, users got pretty annoyed when it didn't work and this reflected badly on the RLS and on the whole Rust project. I would expect the same with Clippy.

It might put the nightlies in a slightly bad light, but IMO we're already currently in a very bad light because RLS and clippy are not stable

I think this is a very different kind of frustration (and of course we want to address it asap, but...) - it is much easier to never offer something than to offer it and then take it away, especially if that happens unpredictably and inconsistently.

The ability to just work with the PR instead of waiting for a nightly to break

This approach has not really worked out. The reasoning is that it is already hard enough to get a PR into Rust and making contributors think about tool breakage (even if they don't have to do the fix themselves) is just too much to ask. We have the toolstate thing now which lets PRs land if they break tools and then emails the tool maintainers with the breaking PR. I think this is as much info as tools folk could hope to have without blocking Rust PRs.

Does that clear up some things? I probably should have shared how the RLS integration was going as we went along as that would probably have helped you understand the core team thinking a bit better.

That seems kinda backwards though? People still default to nightlies.

Could they still use Cargo to install Clippy on nightly? I think we might need different names for the binaries, but otherwise it should work.

But we can guarantee clippy in stable and let people opt in to having clippy on beta/nightlies with the caveat that you won't be able to update to a nightly that doesn't have clippy (or the caveat that some nightlies won't have clippy).

Some nightlies missing a tool is something we definitely don't want though. So we would need to block nightlies on Clippy, which we're not comfortable to do yet.

It seems like no clippy on nightly is strictly worse than clippy on nightly, but sometimes.

Whilst this is true technically, I think it is wrong due to the psychology of the thing: better to be consistently and predictably missing than to be inconsistently and unpredictably missing, even if you're missing more often.

This changed as we learned what users expect from the RLS. Even though we clearly messaged that nightly RLS would not always be available, users got pretty annoyed when it didn't work and this reflected badly on the RLS and on the whole Rust project. I would expect the same with Clippy.

But this is different?

There are two ways of doing partial nightlies:

rustup update will often delete your RLS/clippy because it doesn't exist (this is what you describe)
rustup update will refuse to update to partial nightlies if you have the clippy component installed (and installing such a component will mention this explicitly)

The former will definitely lead to lots of frustration.

But as for the latter? I don't see the average nightly user caring that they get the absolute latest nightly, as long as it's very recent. Skipping a nightly seems like no big deal.

Some nightlies missing a tool is something we definitely don't want though.
..
Whilst this is true technically,

(we're talking about different things re: broken nightlies)

Could they still use Cargo to install Clippy on nightly? I think we might need different names for the binaries, but otherwise it should work.

cargo install clippy is rather fragile, especially on Windows, and not something we want to stick with. It also prevents us from making various kinds of improvements.

If cargo install provided the ability to set an install script, then this wouldn't be that much of an issue.

making contributors think about tool breakage

As I already said, we get breakage less than once a week, I don't think it's something contributors need to worry about too much. But that's fine, this is only a problem if we want to block the build on clippy -- it seems like we don't have to.

rustup update will refuse to update to partial nightlies if you have the clippy component installed (and installing such a component will mention this explicitly)

I implemented most of this in Rustup. One approach to trying to get Clippy distributed would be to press on down this road - we'd need to add a bit more to Rustup (e.g., if a component is missing on one nightly, then it is like it doesn't exist at all, whereas component add should know that it is usually available, just not now). We'd also need to do some work on the Rust CI to make this work.

Just in terms of risk appetite, we'd need to wait a little while before doing this since we want to be sure the RLS is really solid before taking on more risk. Probably worth discussing in Berlin.

Alright, going down that route seems better.

(I was under the impression that this was always the plan)

Navigation Menu

Repository navigation

Stability tracking - Clippy #18

20 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Stability tracking - Clippy #18

Description

Activity

oli-obk commented on Jul 13, 2017

nrc commented on Jul 13, 2017

oli-obk commented on Aug 1, 2017

Manishearth commented on Aug 1, 2017

oli-obk commented on Aug 1, 2017

nrc commented on Aug 2, 2017

oli-obk commented on Aug 2, 2017

nrc commented on Aug 2, 2017

Manishearth commented on Aug 2, 2017

nrc commented on Aug 2, 2017

Manishearth commented on Aug 3, 2017

oli-obk commented on Aug 4, 2017

eddyb commented on Aug 4, 2017

Manishearth commented on Aug 4, 2017

nrc commented on Aug 9, 2017

20 remaining items

oli-obk commented on Feb 5, 2018

nrc commented on Feb 11, 2018

nrc commented on Feb 22, 2018

Manishearth commented on Feb 22, 2018

Manishearth commented on Feb 22, 2018

oli-obk commented on Feb 22, 2018

killercup commented on Feb 22, 2018

oli-obk commented on Feb 22, 2018

nrc commented on Feb 23, 2018

Manishearth commented on Feb 23, 2018

nrc commented on Feb 23, 2018

nrc commented on Feb 23, 2018

Manishearth commented on Feb 23, 2018

nrc commented on Feb 23, 2018

Manishearth commented on Feb 23, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions