diff --git a/text/2383-lint-reasons.md b/text/2383-lint-reasons.md new file mode 100644 index 00000000000..5d324bcee30 --- /dev/null +++ b/text/2383-lint-reasons.md @@ -0,0 +1,331 @@ +- Feature Name: `lint_reasons` +- Start Date: 2018-04-02 +- RFC PR: [rust-lang/rfcs#2383](https://github.com/rust-lang/rfcs/pull/2383) +- Rust Issue: [rust-lang/rust#54503](https://github.com/rust-lang/rust/issues/54503) + +# Summary +[summary]: #summary + +Rust has a number of code lints, both built into the compiler and provided +through external tools, which provide guidelines for code style. The linter +behavior can be customized by attaching attributes to regions of code to allow, +warn, or forbid, certain lint checks. + +The decision for allowing, warning on, or forbidding, specific lints is +occasionally placed in a comment above the attribute or, more often, left +unstated. This RFC proposes adding syntax to the lint attributes to encode the +documented reason for a lint configuration choice. + +# Motivation +[motivation]: #motivation + +The style guide for a project, team, or company can cover far more than just +syntax layout. Rules for the semantic shape of a codebase are documented in +natural language and often in automated checking programs, such as the Rust +compiler and Clippy. Because the decisions about what rules to follow or ignore +shape the codebase and its growth, the decisions are worth storing in the +project directly with the settings they affect. + +It is common wisdom that only the text the environment can read stays true; text +it ignores will drift out of sync with the code over time, if it was even in +sync to begin. Lint settings should have an explanation for their use to explain +why they were chosen and where they are or are not applicable. As they are text +that is read by some lint program, they have an opportunity to include an +explanation similar to the way Rust documentation is a first-class attribute on +code. + +The RFC template asks three questions for motivation: + +- Why are we doing this? + +We are adding this behavior to give projects a mechanism for storing human +design choices about code in a manner that the tools can track and use to +empower human work. For example, the compiler could use the contents of the +lint explanation when it emits lint messages, or the documenter could collect +them into a set of code style information. + +- What use cases does it support? + +This supports the use cases of projects, teams, or organizations using specific +sets of code style guidelines beyond the Rust defaults. This also enables the +creation and maintenance of documented practices and preferences that can be +standardized in a useful way. Furthermore, this provides a standardized means of +explaining decisions when a style gude must be violated by attaching an +overriding lint attribute to a specific item. + +- What is the expected outcome? + +The expected outcome of this RFC is that projects will have more information +about the decisions and expectations of the project, and can have support from +the tools to maintain and inform these decisions. Global and specific choices +can have their information checked and maintained by the tools, and the Rust +ecosystem can have a somewhat more uniform means of establishing code guidelines +and practices. + +I expect Clippy will be a significant benefactor of this RFC, as Clippy lints +are far more specific and plentiful than the compiler lints, and from personal +experience much more likely to want explanation for their use or disuse. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +When a linting tool such as the compiler or Clippy encounter a code span that +they determine breaks one of their rules, they emit a message indicating the +problem and, often, how to fix it. These messages explain how to make the linter +program happy, but carry very little information on why the code may be a +problem from a human perspective. + +These lints can be configured away from the default settings by the use of an +attribute modifying the code span that triggers a lint, or by setting the linter +behavior for a module or crate, with attributes like `#[allow(rule)]` and +`#![deny(rule)]`. + +It is generally good practice to include an explanation for why certain rules +are set so that programmers working on a project can know what the project +expects from their work. These explanations can be embedded directly in the lint +attribute with the `reason = "Your reasoning here"` attribute. + +For example, if you are implementing `Ord` on an enum where the discriminants +are not the correct ordering, you might have code like this: + +```rust +enum Severity { Red, Blue, Green, Yellow } +impl Ord for Severity { + fn cmp(&self, other: &Self) -> Ordering { + use Severity::*; + use Ordering::*; + match (*self, *other) { + (Red, Red) | + (Blue, Blue) | + (Green, Green) | + (Yellow, Yellow) => Equal, + + (Blue, _) => Greater, + (Red, _) => Less, + + (Green, Blue) => Less, + (Green, _) => Greater, + + (Yellow, Red) => Greater, + (Yellow, _) => Less, + } + } +} +``` + +The ordering of the left hand side of the match branches is significant, and +allows a compact number of match arms. However, if you're using Clippy, this +will cause the `match_same_arms` lint to trip! You can silence the lint in this +spot, and provide an explanation that indicates you are doing so deliberately, +by placing this attribute above the `match` line: + +```rust +#[allow(match_same_arms, reason = "The arms are order-dependent")] +``` + +Now, when the lints run, no warning will be raised on this specific instance, +and there is an explanation of why you disabled the lint, directly in the lint +command. + +Similarly, if you want to increase the strictness of a lint, you can explain why +you think the lint is worth warning or forbidding directly in it: + +```rust +#![deny(float_arithmetic, reason = "This code runs in a context without floats")] +``` + +With a warning or denial marker, when a linting tool encounters such a lint trap +it will emit its builtin diagnostic, but also include the reason in its output. + +For instance, using the above Clippy lint and some floating-point arithmetic +code will result in the following lint output: + +```text +error: floating-point arithmetic detected +reason: This code runs in a context without floats + --> src/lib.rs:4:2 + | +4 | a + b + | ^^^^^ + | +note: lint level defined here + --> src/lib.rs:1:44 + | +1 | #![cfg_attr(deny(float_arithmetic, reason = "..."))] + | ^^^^^^^^^^^^^^^^ + = help: for further information visit ... +``` + +## `expect` Lint Attribute + +This RFC adds an `expect` lint attribute that functions identically to `allow`, +but will cause a lint to be emitted when the code it decorates ***does not*** +raise a lint warning. This lint was inspired by Yehuda Katz: + +> [@ManishEarth](https://twitter.com/ManishEarth) has anyone ever asked for +> something like #[expect(lint)] which would basically be like #[allow(lint)] +> but give you a lint warning if the problem ever went away? +> +> I basically want to mark things as ok while doing initial work, but I need to +> know when safe to remove +> +> — Yehuda Katz ([@wycats](https://twitter.com/wycats)) +> +> [March 30, 2018](https://twitter.com/wycats/status/979742693378019329) + +When the lint passes run, the `expect` attribute suppresses a lint generated by +the span to which it attached. It does not swallow any other lint raised, and +when it does not receive a lint to suppress, it raises a lint warning itself. +`expect` can take a `reason` field, which is printed when the lint is raised, +just as with the `allow`/`warn`/`deny` markers. + +This is used when prototyping and using code that will generate lints for now, +but will eventually be refactored and stop generating lints and thus no longer +need the permission. + +```rust +#[expect(unused_mut, reason = "Everything is mut until I get this figured out")] +fn foo() -> usize { + let mut a = Vec::new(); + a.len() +} +``` + +will remain quiet while you're not mutating `a`, but when you do write code that +mutates it, or decide you don't need it mutable and strip the `mut`, the +`expect` lint will fire and inform you that there is no unused mutation in the +span. + +```rust +#[expect(unused_mut, reason = "...")] +fn foo() { + let a = Vec::new(); + a.len() +} +``` + +will emit + +```text +warning: expected lint `unused_mut` did not appear +reason: Everything is mut until I get this figured out + --> src/lib.rs:1:1 + | +1 | #[expect(unused_mut, reason = "...")] + | -------^^^^^^^^^^----------------- + | | + | help: remove this `#[expect(...)]` + | + = note: #[warn(expectation_missing)] on by default +``` + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +This RFC adds a `reason = STRING` element to the three lint attributes. The +diagnostic emitter in the compiler and other lint tools such as Clippy will need +to be made aware of this element so that they can emit it in diagnostic text. + +This RFC also adds the `expect(lint_name, reason = STRING)` lint attribute. The +`expect` attribute uses the same lint-suppression mechanism that `allow` does, +but will raise a new lint, `expectation_missing` (name open to change), when the +lint it expects does not arrive. + +The `expectation_missing` lint is itself subject to +`allow`/`expect`/`warn`/`deny` attributes in a higher scope, so it is possible +to suppress expectation failures, lint when no expectation failures occur, or +fail the build when one occurs. The default setting is +`#![warn(expectation_missing)]`. + +That’s pretty much it, for technical details. + +## OPTIONAL — Yet Another Comment Syntax + +A sugar for lint text MAY be the line comment `//#` or the block comment +`/*# #*/` with `U+0023 # NUMBER SIGN` as the signifying character. These +comments MUST be placed immediately above a lint attribute. They are collected +into a single string and collapsed as the text content of the attribute they +decorate using the same processing logic that documentation comments (`///` and +`//!` and their block variants) currently use. Example: + +```rust +//# Floating Point Arithmetic Unsupported +//# +//# This crate is written to be run on an AVR processor which does not have +//# floating-point capability in hardware. As such, all floating-point work is +//# done in software routines that can take a significant amount of time and +//# space to perform. Rather than pay this cost, floating-point work is +//# statically disabled. All arithmetic is in fixed-point integer math, using +//# the `FixedPoint` wrapper over integer primitives. +#![deny(float_arithmetic)] +``` + +The `#` character is chosen as the signifying character to provide room for +possible future expansion – these comments MAY in the future be repurposed as +sugar for writing the text of an attribute that declares a string parameter that +can accept such comments. + +This comment syntax already pushes the edge of the scope of this RFC, and +extension of all attributes is certainly beyond it. + +Implementing this comment syntax would require extending the existing transform +pass that replaces documentation comments with documentation attributes. +Specifically, the transform pass would ensure that all lint comments are +directly attached to a lint attribute, and then use the strip-and-trim method +that the documentation comments experience to remove the comment markers and +collapse the comment text, across multiple consecutive comment spans, into a +single string that is then inserted as `reason = STRING` into the attribute. + +Given that this is a lot of work and a huge addition to the comment grammar, the +author does not expect it to be included in the main RFC at all, and is writing +it solely to be a published prior art in case of future desire for such a +feature. + +# Drawbacks +[drawbacks]: #drawbacks + +Why should we *not* do this? + +Possibly low value add for the effort. + +# Rationale and alternatives +[alternatives]: #alternatives + +- Why is this design the best in the space of possible designs? + + Attributes taking descriptive text is a common pattern in Rust. + +- What other designs have been considered and what is the rationale for not + choosing them? + + None. + +- What is the impact of not doing this? + + None. + +# Prior art +[prior-art]: #prior-art + +The `stable` and `unstable` attributes both take descriptive text parameters +that appear in diagnostic and documentation output. + +# Unresolved questions +[unresolved]: #unresolved-questions + +- What parts of the design do you expect to resolve through the RFC process + before this gets merged? + + The name of the `reason` parameter. + +- What parts of the design do you expect to resolve through the implementation + of this feature before stabilization? + + The use sites of the `reason` parameter. + +- What related issues do you consider out of scope for this RFC that could be + addressed in the future independently of the solution that comes out of this + RFC? + + The means of filling the `reason` parameter, or what tools like `rustdoc` + would do with them.