Policy for lint expansions

[tmandry]

Background

When a lint is expanded to include many new cases, it adds significant complexity to the rollout of a toolchain to large codebases. Maintainers of these codebases are stuck with the choice of

1. Disabling the existing lint while the toolchain is updated and new cases are fixed
2. Fixing cases manually and updating the toolchain immediately

Both of these come with the problem of racing with other developers in a codebase who may land new code which triggers the expanded lint in a new compiler, but does not trigger the lint in an old compiler.

While it would be nice to solve this "raciness" once and for all, there are other considerations at play. Instead, we propose to support these users by either providing them with a new lint name to temporarily opt out of OR a machine-applicable fix which eases the pain of any races which might occur.

Note that this requirement only applies to significant lint expansions as measured by crater.

This policy is based on the lang team discussion of #121708.

cc @estebank @petrochenkov

Policy

When an existing lint is expanded to include many new cases, we must provide either:

1. A new lint name under the existing group, so that users may opt out of the expansion at least temporarily, or
2. A MachineApplicable fix for the lint.

Exceptions to this policy may be made via Language Team FCP.

Here, we define "many new cases" as impacting more than 5% 1% of the top-1000 crates on crates.io. This can be measured by counting the number of regressions from a crater run like the one below.

A crater run is not required before landing for every lint expansion. Reviewers should use their best judgment to decide if one is required. However, if a lint expansion lands that violates this requirement, or is strongly suspected to violate this requirement based on other impact, it should be reverted.

Crater command

To measure the impact of a lint as defined by this policy, you can use the following crater command:

@craterbot run name=<name> start=master#<hash1>+rustflags=-D<lint_name> end=master#<hash2>+rustflags=-D<lint_name> crates=top-1000 mode=check-only p=1

See the crater docs for more information.

[tmandry]

As discussed in last week's lang team meeting, I've drafted a policy for the team to review.

@rustbot label I-lang-nominated

[estebank]

Here, we define "many new cases" as impacting more than 5% of the top-1000 crates on crates.io. This can be measured by counting the number of regressions from a crater run like the one below.

I would likely be more stringent than that. Affecting just .5% would already make me consider mitigations.

---

There's an additional approach we could take: annotating all lints with a metadata field for "last significantly updated". This would allow tooling to account for Rust versions when triggering any lints. Then we could make #![deny(warnings)] be coupled with an additional attribute (or Cargo.toml field) to state "I want to mark warn-by-default lints as errors up until the version I support, and later warns remain as warnings at most". (I'm aware this is a separate issue, but believe the same set of users are affected by both simultaneously.)

---

For the original issue, I endorse splitting the lint into a new family.

[tmandry]

I would likely be more stringent than that. Affecting just .5% would already make me consider mitigations.

Yeah, I pulled a number out out of thin air. Let me continue doing that here. How about: If .5% would make you consider mitigations, maybe the policy line should be 1%?

There's an additional approach we could take: annotating all lints with a metadata field for "last significantly updated". This would allow tooling to account for Rust versions when triggering any lints. Then we could make #![deny(warnings)] be coupled with an additional attribute (or Cargo.toml field) to state "I want to mark warn-by-default lints as errors up until the version I support, and later warns remain as warnings at most". (I'm aware this is a separate issue, but believe the same set of users are affected by both simultaneously.)

I like this idea too. It would work well for those releasing to their codebase from discrete Rust versions. Those releasing from nightly might not have such a good time unless we manage to include the merge date somehow.