Moving forward on forward-compatibility lints

compiler
1

At the moment, we have a policy to use forward-compatibility lints, but no policy on how to remove them. @petrochenkov has been heroically pursuing this on their onw. @arielb1 and I were talking and wanted to propose the following strategy. The ideal pattern is as follows. Some points need refinement. =)

  1. Begin with a warning period for some number of cycles N >= 1
  2. Do one cycle of “deny by default”
  3. After beta is issued with “deny by default” enabled, support may be removed on stable

Conditions for each stage:

Stage 1: warning. No prior conditions, just have to write a PR. The proper procedure is described in RFC 1589.

Stage 2: deny. We have to do a crater run and at least notify all affected crate authors. Ideally, there should be pending PRs. Something like what @petrochenkov has done seems ideal.

QUESTION: I think we also want some kind of “absolute” criteria along the lines of “there shouldn’t be a lot of people affected in practice”. This means more than we have a pending PR to fix it: it means that, if a major crate is affected, the PR should have landed in a release available on crates.io so that people can just “cargo update” to get the release. Ideally, the old release would also be yanked, and this would issue some sort of warning that people get which advises them to cargo update, but that would work best in conjunction with cargo #2608. (cc @wycats, @alexcrichton)

Stage 3: The deny-by-default branch must have reached beta. There should be not be significant controversy on the tracking issue. =)

QUESTION: Should we also require some absolute period of time? In principle, you could land a patch on nightly, branch beta, and then two days later rip out support from nightly. This has happened in the past and people who live in the nightly ecosystem were somewhat annoyed. I hope that with we can wean people from nightly with macros 1.1 and make this a non-issue, but it’s something to think about.

Exemptions from the rules

Note that I said that these are the ideal requirements. I think we always have the option to do things differently (e.g., skip stage 2) if there are other concerns, but this is a decision that must be “ratified” by the compiler team. We can use RFC bot to do this.

How do we decide when to move between stages?

This raises one final point – do we always require consent by compiler team to move to stage 2 or 3? Or should we say that if the above criteria are met, we can just move without gaining consent from the whole team?

Background on RFC bot

If you haven’t heard much about RFC bot, the way it works is that we tag the issue with T-compiler and then send a message like @rfcbot fcp merge. This will create a checklist for every member of T-compiler to check off their name. You can find the list of pending RFCs on your personal dashboard http://rusty-dash.com/fcp/YOURNAMEHERE. The idea is that this is all very lightweight: you can just check off your name with a quick click if you are happy. You can also register concerns that must be resolved before consensus is reached.

If there are inactive members of the compiler team, we’ll just check their names for them, but if people are consistently inactive, we should talk to them about moving to alumni status (I think there are several inactive members of compiler team right now, so this will probably come up. =)

My thoughts on the “open questions”:

  • I think we should just always use rfcbot to move between stages.
  • To move to deny by default:
    • We should require that less than 30 total crates are affected (not root crates)
    • There should be open PRs for all root crates; if opening a PR is not possible, the authors should be notified
  • To move to a hard error:
    • I say as soon as the beat is branched, it’s fair game

cc @brson @petrochenkov

2

I just realized a wrinkle. When we test a “deny-by-default” PR, the “cap-lints” feature ensures that we don’t get a complete picture of how many affected crates there are. Also, crater’s limitations prevent us from having a feeling for windows. (Hopefully the latter question will be solved soon.)

On the topic of cap-lints: deny-by-default still means that if people are using a crate as a dependency, the lint will be silenced. This is a good thing, I would think, but it does mean that the “total crates affected” metric is not telling us one important piece of data: how many people will be affected when we move to a hard error.

It seems like at minimum the criteria to move to stage 3 should be changed, e.g., to do another crater run.

3

(Also, on a related note, I want to get more organized about release planning, and in particular to schedule a particular compiler meeting where we will go over outstanding forward-compatibility lints, unsoundness issues, as well as roadmap goals etc, and try to plan out work for the upcoming release cycle, but that seems like a separate topic from this discussion.)

4

The plan as outlined seems fine to me. When turning them into a hard error, it seems prudent to me to convert them to forbid by default or to otherwise leave the compiler understanding that the lint did exist. That way stragglers mentioning the lint will get something better than an error saying the lint doesn’t exist.

How could crater better support testing here? Does cap-lints affect forbidden lints? ISTM that just converting the lint to forbid and running through crater should be sufficient.

I guess I’d prefer these lints can be simply switched to some final ‘enabled’ state, that preserves the existence of the lint name, that can’t be disabled, isn’t impacted by cap-lints, and can be switched on with a one-liner for ease of cratering.

5

The step “deny-by-default” => “hard error” seems to be more serious than “warn-by-default” => “deny-by-default”. “Deny-by-default” is a strict but friendly reminder, it doesn’t affect dependencies, nothing needs to be forcefully updated, it may be annoying for the “root” crate’s maintainer(s) at worst, but even then it can be trivially allowed if the proper fix is non-trivial or not immediately possible for some reasons (e.g. compatibility with older stable Rust versions). Hard error is an end of everything. I personally expected a time frame of (at least) 6 months or so for making lints from https://github.com/rust-lang/rust/pull/36894 hard errors.

6

In many cases reporting these lints requires keeping some legacy code around. For example, private_in_public requires keeping the whole old private-in-public checker, and RFC 1214 lint required even more. I think i may make sense to keep some list of removed lints in unknown_lints, but not not to keep reporting them in their current form.

1 Like
7

So I think that the missing part of this plan is some way to help ensure that delinquent dependency authors get notified that work is needed. In other words, if you have some library that is no longer maintained, how can we get it patched up? I've talked to @wycats one and off about this and sort of forget what we said; seems like the only option is really to fork the repo and apply a patch, or encourage others to do so (and hopefully maintain the result). If that is done, we might have some way to annotate the crate so as to redirect people to this patched up project.

Another related note is getting downstream users to cargo update -- but the fix for that, I think, is the "cargo yank with reason" but that I linked to.

8

I didn't put any time frame -- I'm not sure it makes sense to wait for "a long time", but maybe it does. I guess it depends on how much you trust cargo results.

If we had LTO releases -- which we may want to adopt at some point -- then it seems like it'd be natural to tie the warnings to that cycle. i.e., the warning must have made it into an LTO release, or something like that.

9

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.