rust-lang / rust-lang/rust

Policy for lint expansions

Open
#122,759 17 comments 5 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-diagnostics A-lints C-discussion disposition-merge I-lang-radar proposed-final-comment-period T-lang
Dominant language
Rust
Stars
119k
Forks
16.1k
PR merge metrics
PR metrics pending

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Read the Background and Guidelines sections, then review the linked lang-team discussion for context on the proposed policy. Consult the linked crater bot documentation for the experiment command; the work is complete when maintainers have decided whether the lint-expansion and revert guidelines should be adopted or revised.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers, documentation
Issue type
Documentation
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.