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 in the ecosystem. In particular, certain challenges occur in large codebases. Maintainers 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 so that other developers see the warnings and don't add more such code.

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 a set of guidelines to help decide whether an existing lint should be expanded, as well as mitigations for the above problems when expansions occur.

These guidelines are based on the lang team discussion of #121708.

cc @estebank @petrochenkov

Guidelines

Deciding on new lint vs expansion

We don't have hard-and-fast rules for when to use a new lint versus expanding an existing one, but we want to take several factors into account:

• How important is fixing the issue the lint identifies, to the maintainer of the code or the health of the ecosystem?
• Are the cases of the old and new lints of the same severity?
• Is it possible to fix code to no longer trigger the lint without making the code depend on the new version of Rust?
• Is there a clear description of the distinction between the old and new cases, such that it's easy to give a logical name to the new cases?
• What fraction of the ecosystem will encounter the lint? (Crater can give a good idea of this.)
• Is there an automatic machine-applicable fix?

We'd like to avoid having a disproportionately painful impact on the ecosystem compared to the value provided. We'd also like to avoid having a painful impact on large codebases that need to migrate their code to take the new lint into account.

When proposing or reviewing an expansion of an existing lint, please take the above factors into account. If in doubt about whether to use a new or existing lint, please consult with lang, and seek a decision via FCP.

Mitigations for lint expansions

When an existing lint is expanded to include many new cases, please consider providing at least one of:

1. A new lint name under the existing group, so that users may opt out of the expansion at least temporarily. This is particularly desirable if there's a clear description of the new cases being added.
2. A MachineApplicable fix for the lint. This fix must produce code that compiles without new lints or errors under the most recent stable compiler. This helps maintainers, particularly of large codebases, quickly fix all existing instances so they can upgrade/deny the lint, without racing with new code additions that trigger the lint. (Note, though, that a MachineApplicable fix doesn't suffice if applying the fix would make code fail to compile on the immediately prior version of Rust.)

To evaluate whether a lint produces "many new cases", try doing a crater run on the top 1000 crates. If the lint addition impacts more than 1% of the top-1000 crates on crates.io, or is producing a large number of warnings within a small number of crates in that set, treat it as producing many new cases. See below for details on how to run crater.

A crater run is not required before landing for every lint expansion. Reviewers should use their best judgment to decide if one is required.

Crater command

To measure the impact of a lint on the top 1000 crates, 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.

Lint revert policy

In general, if a lint expansion does not meet the above guidelines, and in particular especially if it has neither a new lint name nor a machine applicable fix, it should be flagged for review and likely reverted before it hits stable Rust.

[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.

[joshtriplett]

We still need to figure out where this should live (e.g. rustc-dev-guide, somewhere on forge), but 👍 for documenting guidelines for how we evaluate both new lints and expansions of existing lints.

Let's see if we have consensus on the proposal in the top post here:

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[RalfJung]

I don't quite understand how expanding an existing lint is any worse than adding a new (warn-by-default) lint. Reading the motivation here

Maintainers 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 so that other developers see the warnings and don't add more such code.

This applies equally to the case of landing a new lint, doesn't it? If you have developers using outdated compilers, they may submit code that would trigger a lint on newer compilers. If you allow the lint, you may miss out on genuine bugs.

We have a technology that can solve issues like "automatically ensure developers submit code that adheres to a certain standard", it's called CI. Why should Rust compiler developers have to bear the cost for people not updating their compilers?