From 95ca8a937ae20967923895a6944bdc70482e4b3e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 15 Nov 2024 20:05:42 -0600 Subject: [PATCH 01/23] Add skeleton --- text/0000-nit-lint-level.md | 97 +++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 text/0000-nit-lint-level.md diff --git a/text/0000-nit-lint-level.md b/text/0000-nit-lint-level.md new file mode 100644 index 00000000000..a2ab4c4c8a6 --- /dev/null +++ b/text/0000-nit-lint-level.md @@ -0,0 +1,97 @@ +- Feature Name: (fill me in with a unique ident, `my_awesome_feature`) +- Start Date: (fill me in with today's date, YYYY-MM-DD) +- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) + +# Summary +[summary]: #summary + +One paragraph explanation of the feature. + +# Motivation +[motivation]: #motivation + +Why are we doing this? What use cases does it support? What is the expected outcome? + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +Explain the proposal as if it was already included in the language and you were teaching it to another Rust programmer. That generally means: + +- Introducing new named concepts. +- Explaining the feature largely in terms of examples. +- Explaining how Rust programmers should *think* about the feature, and how it should impact the way they use Rust. It should explain the impact as concretely as possible. +- If applicable, provide sample error messages, deprecation warnings, or migration guidance. +- If applicable, describe the differences between teaching this to existing Rust programmers and new Rust programmers. +- Discuss how this impacts the ability to read, understand, and maintain Rust code. Code is read and modified far more often than written; will the proposed feature make code easier to maintain? + +For implementation-oriented RFCs (e.g. for compiler internals), this section should focus on how compiler contributors should think about the change, and give examples of its concrete impact. For policy RFCs, this section should provide an example-driven introduction to the policy, and explain its impact in concrete terms. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +This is the technical portion of the RFC. Explain the design in sufficient detail that: + +- Its interaction with other features is clear. +- It is reasonably clear how the feature would be implemented. +- Corner cases are dissected by example. + +The section should return to the examples given in the previous section, and explain more fully how the detailed proposal makes those examples work. + +# Drawbacks +[drawbacks]: #drawbacks + +Why should we *not* do this? + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +- Why is this design the best in the space of possible designs? +- What other designs have been considered and what is the rationale for not choosing them? +- What is the impact of not doing this? +- If this is a language proposal, could this be done in a library or macro instead? Does the proposed change make Rust code easier or harder to read, understand, and maintain? + +# Prior art +[prior-art]: #prior-art + +Discuss prior art, both the good and the bad, in relation to this proposal. +A few examples of what this can include are: + +- For language, library, cargo, tools, and compiler proposals: Does this feature exist in other programming languages and what experience have their community had? +- For community proposals: Is this done by some other community and what were their experiences with it? +- For other teams: What lessons can we learn from what other communities have done here? +- Papers: Are there any published papers or great posts that discuss this? If you have some relevant papers to refer to, this can serve as a more detailed theoretical background. + +This section is intended to encourage you as an author to think about the lessons from other languages, provide readers of your RFC with a fuller picture. +If there is no prior art, that is fine - your ideas are interesting to us whether they are brand new or if it is an adaptation from other languages. + +Note that while precedent set by other languages is some motivation, it does not on its own motivate an RFC. +Please also take into consideration that rust sometimes intentionally diverges from common language features. + +# Unresolved questions +[unresolved-questions]: #unresolved-questions + +- What parts of the design do you expect to resolve through the RFC process before this gets merged? +- What parts of the design do you expect to resolve through the implementation of this feature before stabilization? +- 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? + +# Future possibilities +[future-possibilities]: #future-possibilities + +Think about what the natural extension and evolution of your proposal would +be and how it would affect the language and project as a whole in a holistic +way. Try to use this section as a tool to more fully consider all possible +interactions with the project and language in your proposal. +Also consider how this all fits into the roadmap for the project +and of the relevant sub-team. + +This is also a good place to "dump ideas", if they are out of scope for the +RFC you are writing but otherwise related. + +If you have tried and cannot think of any future possibilities, +you may simply state that you cannot think of anything. + +Note that having something written down in the future-possibilities section +is not a reason to accept the current or a future RFC; such notes should be +in the section on motivation or rationale in this or subsequent RFCs. +The section merely provides additional information. From a8d493236d31e1b4abce912abd356fed19eb09d1 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 08:56:28 -0600 Subject: [PATCH 02/23] feat: Initial draft --- text/0000-nit-lint-level.md | 376 +++++++++++++++++++++++++++++++----- 1 file changed, 324 insertions(+), 52 deletions(-) diff --git a/text/0000-nit-lint-level.md b/text/0000-nit-lint-level.md index a2ab4c4c8a6..dfe1b0d8981 100644 --- a/text/0000-nit-lint-level.md +++ b/text/0000-nit-lint-level.md @@ -1,97 +1,369 @@ -- Feature Name: (fill me in with a unique ident, `my_awesome_feature`) -- Start Date: (fill me in with today's date, YYYY-MM-DD) +- Feature Name: `nit-lint-level` +- Start Date: 2024-11-15 - RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) # Summary [summary]: #summary -One paragraph explanation of the feature. +Add a new visible lint level below `warn` to allow linters to act like a pair-programmer / code-reviewer where feedback is evaluated on a case-by-case basis. +- `cargo` does not display these lints by default, requiring an opt-in +- This RFC assumes LSPs/IDEs will opt-in +- CIs could opt-in and open issues in the code review to show nits introduced in the current PR but not block merge on them + +There is no expectation that crates will be `nit`-free. + +*Note:* The name `nit` is being used for illustrative purposes and is assumed to not be the final choice + +*Note:* This RFC leaves the determination of what lints will be `nit` by +default to the respective teams. Any lints referenced in this document are +for illustrating the intent of this feature and how teams could reason about +this new lint level. # Motivation [motivation]: #motivation -Why are we doing this? What use cases does it support? What is the expected outcome? +By its name and literal usage, the `warn` level is non-blocking. +However, most projects treat `warn` as a soft-error. +It doesn't block for local development but CI blocks it from being merged. +This convention is not new with the Rust community; many C++ projects have take +this approach before Rust came to be with "warnings clean" being a goal for +correctness. +Cargo is looking to further cement this meaning by adding +[`CARGO_BUILD_WARNINGS=deny`](https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#warnings) +for CIs to set rather than `RUSTFLAGS=-Dwarnings`. + +This leaves a gap in the developer user experience for truly non-blocking lints. +- Have maintainers `allow` a lint means they can't benefit from it +- Have contributors `#[allow]`ing the lints to bless code, + - The weight of `warn` may encourage contributors to do what it says, rather than `#[allow]` it + - Sprinkling `#[allow]` in code can be noisy and the lint may not be worth it + - If CI is using `stable`, then new lints can break CI + - This is also true for lints changing behavior but that isn't a problem as often +- Having maintainers `-W` or `-A` these lints through `RUSTFLAGS` in CI + - This makes it more difficult for users to reproduce this locally. + - `RUSTFLAGS` also comes with its own set of problems and the Cargo team is interested in finding alternatives to maintainers setting `RUSTFLAGS`, + see [cargo#12738](https://github.com/rust-lang/cargo/issues/12738), [cargo#12739](https://github.com/rust-lang/cargo/issues/12739) + +Another problem is when adopting lints. +This experience is more taken from legacy C++ code bases but the assumption is +that this can become a problem in our future as the Rust code bases grow over +time. +A complaint that can happen when adopting a linter or lints is the time it takes to clean up the code base to be lint free +because their only choice is to block on a lint or completely ignoring it. +If we had a non-blocking lint level, a project could switch interested lints to +that level and at least limit the introduction of new lint violations, +either viewing that as good enough or while the existing violations are resolved in parallel. + +A secondary benefit of non-blocking lints is that more lints could move out of `allow`, raising their visibility. +It can take effort to sift through all of the +[default-`allow`ed lints](https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow) +for which ones a maintainer may want to turn into a soft or hard error. + +## Example: `clap` + +Each lint below from `clap`s `Cargo.toml` represents a lint that could be useful but not worthwhile enough to `allow`: +```toml +[workspace.lints.clippy] +bool_assert_comparison = "allow" +branches_sharing_code = "allow" +collapsible_else_if = "allow" +if_same_then_else = "allow" +let_and_return = "allow" # sometimes good to name what you are returning +multiple_bound_locations = "allow" +assigning_clones = "allow" +blocks_in_conditions = "allow" +``` + +Also, CI runs `clippy` with `-D warnings -A deprecated` +- Dependency upgrades should not be blocked on an API becoming deprecated +- The maintainers want visibility into what is deprecated though +- Combined with other circumstances, this also blocks [`clippy-sarif`](https://crates.io/crates/clippy-sarif) from opening issues in PRs for deprecated APIs. + +Part of this stems from `deprecated` having two purposes: +- This API is broken and you should severely question any use of it +- This API will go away with the next major version you should migrate to the new API at some point in time + +The second case is very useful for compiler-guided upgrades through breaking changes +(e.g. [winnow's migration guide](https://github.com/winnow-rs/winnow/blob/v0.6.0/CHANGELOG.md)). + +Allowing dependency upgrades in the presence of deprecations is not just something that `clap` is interested in for itself but +users of clap reported that they want to handle deprecations on their own schedule ([clap#3822](https://github.com/clap-rs/clap/issues/3822)). +This led to adding a `deprecated` feature to `clap` so deprecations became opt-in +([clap#3830](https://github.com/clap-rs/clap/pull/3830)). +However, this can be easy for users to miss and won't catch new uses of the deprecated APIs. + +## Example: `cargo` + +In 2021 ([cargo#9356](https://github.com/rust-lang/cargo/pull/9356)), the Cargo team switched their `src/lib.rs` to: +```rust +#![allow(clippy::all)] +#![warn(clippy::needless_borrow)] +#![warn(clippy::redundant_clone)] +``` +due to false positives and the level of subjectivity of the improvements. +This approach was first relaxed in 2024 ([cargo#11722](https://github.com/rust-lang/cargo/pull/11722)). + +Over time and with the addition of the `[lints]` table, this eventually led to ([cargo#12178](https://github.com/rust-lang/cargo/pull/12178)): +```toml +[workspace.lints.clippy] +all = { level = "allow", priority = -1 } +dbg_macro = "warn" +disallowed_methods = "warn" +print_stderr = "warn" +print_stdout = "warn" +self_named_module_files = "warn" +``` + +In a recent PR, a bug that clippy would have found without our level overrides (`derive_ord_xor_partial_ord`) +was only caught in review because one of the reviewers has seen clippy report +this lint many times in other projects. +After some discussion, the `Cargo.toml` was updated to ([cargo#14796](https://github.com/rust-lang/cargo/pull/14796)): +```toml +[workspace.lints.clippy] +all = { level = "allow", priority = -2 } +correctness = { level = "warn", priority = -1 } +dbg_macro = "warn" +disallowed_methods = "warn" +print_stderr = "warn" +print_stdout = "warn" +self_named_module_files = "warn" +``` + +If more clippy lints were non-blocking, maybe the Cargo team would not have set +`all` to `allow` and been able to catch this more easily. +Granted, a non-blocking lint level could still lead to low quality "lint +reduction" PRs being posted and avoiding those was one of the original +motivations for the initial change. # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -Explain the proposal as if it was already included in the language and you were teaching it to another Rust programmer. That generally means: +## Example workflow + +*This will be reported from perspective of a Cargo user.* + +Rust users will have a new `nit` lint level for providing optional coding feedback. + +If introduced today without any changes to levels for existing lints, +a maintainer may decide that they want `clippy::let_and_return` to be optional. +In their `Cargo.toml`, they would set: +```toml +[package.lints.clippy] +let_and_return = "nit" +``` + +A contributor may write: +```rust +fn foo() -> String { + if condition() { + let semantically_meaningful_name_one = bar(); + semantically_meaningful_name_one + } else { + let semantically_meaningful_name_two = baz(); + semantically_meaningful_name_two + } +} +``` + +Rust-analyzer would give them feedback that they wrote extraneous code that could be reduced down, +with a suggested fix. +However, they feel the variable name is acting as a form of documentation and want to keep it. + +When the contributor checks for lints, they run +```console +$ cargo clippy + Compiling foo + Finished `foo` profile [optimized + debuginfo] target(s) in 15.85s +``` +No lints reported. + +A non-rust-analyzer user could run the following to look for feedback: +```console +$ CARGO_BUILD_NITS=nit cargo clippy + Compiling foo +note: creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block +... + Finished `foo` profile [optimized + debuginfo] target(s) in 15.85s +``` + +They then go and post a PR. +On the PR, the code above gets the following issue opened: +``` +X Check failure + +Code scanning clippy + +creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block (warning) + +Show more details +``` +This gives the contributor an opportunity to know of this potential improvement +if they missed it and for the code reviewer to double check if they agreed with +the author's determination to ignore it. -- Introducing new named concepts. -- Explaining the feature largely in terms of examples. -- Explaining how Rust programmers should *think* about the feature, and how it should impact the way they use Rust. It should explain the impact as concretely as possible. -- If applicable, provide sample error messages, deprecation warnings, or migration guidance. -- If applicable, describe the differences between teaching this to existing Rust programmers and new Rust programmers. -- Discuss how this impacts the ability to read, understand, and maintain Rust code. Code is read and modified far more often than written; will the proposed feature make code easier to maintain? +For future PRs, Github should not report this as it should recognize that the report is for existing code. -For implementation-oriented RFCs (e.g. for compiler internals), this section should focus on how compiler contributors should think about the change, and give examples of its concrete impact. For policy RFCs, this section should provide an example-driven introduction to the policy, and explain its impact in concrete terms. +## Choosing lint levels + +When creating a lint or overriding a default lint level, +its important to consider the semantics, something like: + +`error` is for hard errors that even prevent reporting of errors further in the build, running of tests, etc. +This should be reserved for when there is no point building or testing anything further because of how broken the code is. + +`warn` generally has the semantics of a soft-error. +Its non-blocking locally but will be blocked when the code is merged upstream. +This should be used when eventual correctness or consistency is desired. + +`nit` is for coding suggestions for users that may or may not improve the code or may not need to be done yet. + +`allow` is for when there is no right answer (e.g. mutually exclusive lints), there are false positives, the lint is overly noisy, or the lint is of limited value. + +*Note:* This is for communicating the intent of this RFC and to get people started with this new feature and is not meant to precisely redefine the +[lint levels](https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-levels). # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -This is the technical portion of the RFC. Explain the design in sufficient detail that: +## Rustc + +Rustc focuses on the mechanics of a new, visible lint level `nit`. + +Add support for the new lint: +```rust +#![nit(deprecated)] +``` +```console +$ rustc --nit deprecated ... +``` +```console +$ rustc -Ndeprecated ... +``` + +This will be rendered like a `note:`. + +Rustc will have a dynamic lint group of all `nit`s, much like `warnings` is all lints at level `warn` at the time of processing, `-Anits`. + +## Cargo + +Cargo implements the semantics for this to be a first-class non-blocking lint level. + +Add support for the new lint: +```toml +[workspace.lints.rust] +deprecated = "nit" +``` + +A new config field will be added to mirror `build.warnings`: -- Its interaction with other features is clear. -- It is reasonably clear how the feature would be implemented. -- Corner cases are dissected by example. +### `build.nits` -The section should return to the examples given in the previous section, and explain more fully how the detailed proposal makes those examples work. +- Type: string +- Default: `allow` +- Environment: `CARGO_BUILD_NITS` + +Controls how Cargo handles nits. Allowed values are: + +- `nit`: warnings are emitted as warnings. +- `allow`: warnings are hidden (default). # Drawbacks [drawbacks]: #drawbacks -Why should we *not* do this? +Rust-analyzer might be limited in what it can do through LSP to avoid overwhelming the user with `nit`s. + +This requires a non-standard CI setup to report these messages. + +Non-rust-analyzer users won't get feedback until CI runs and then will have to know to run their linter in a special way to reproduce CI's output locally. + +Users running `CARGO_BUILD_NITS=nit cargo clippy` will have a hard time finding what lints are relevant for their change. +If we had a way to filter lints by part of the API or by "whats new since run X", +then that could be resolved. + +As `nit`s will always be enabled in the linter, just silenced in Cargo, the performance hit of running +them will be present unless `RUSTFLAGS=-Anits` is applied by the user. # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives -- Why is this design the best in the space of possible designs? -- What other designs have been considered and what is the rationale for not choosing them? -- What is the impact of not doing this? -- If this is a language proposal, could this be done in a library or macro instead? Does the proposed change make Rust code easier or harder to read, understand, and maintain? +- `CARGO_BUILD_NITS=allow` is the default to avoid "warning fatigue" +- The default level for lints is left unchanged by this RFC to keep the scope of what is designed and reviewed to the minimum +- Rustc shows `nit`s by default, rather than hide them and require a separate opt-in mechanism for Cargo to see them + - Problem: People directly using rustc or using a third-party build tool may be inundated with these + - Lint levels may change without breaking compatibility + - If stabilize with no `nit`s or only downgrading `warn`s to `nit`s, then there will be no difference in the amount of output + - We already have a mechanism, let's not invent a new one +- `nits` lint group is created to give rustc-only or third-party build tools a built-in way to control these besides ignoring them like Cargo +- Cargo could print a message that `nit`s are present to help raise awareness of how to show them but they will likely always be present, so this is noise to experienced users. # Prior art [prior-art]: #prior-art -Discuss prior art, both the good and the bad, in relation to this proposal. -A few examples of what this can include are: - -- For language, library, cargo, tools, and compiler proposals: Does this feature exist in other programming languages and what experience have their community had? -- For community proposals: Is this done by some other community and what were their experiences with it? -- For other teams: What lessons can we learn from what other communities have done here? -- Papers: Are there any published papers or great posts that discuss this? If you have some relevant papers to refer to, this can serve as a more detailed theoretical background. +IntelliJ IDEA has the following [lint levels](https://www.jetbrains.com/help/idea/configuring-inspection-severities.html): +- Error: Syntax errors +- Warning: Code fragments that might produce bugs or require enhancement +- Weak Warning: Code fragments that can be improved or optimized (redundant code, duplicated code fragments, and so on) +- Server Problem +- Grammar Error +- Typo +- Consideration: Code fragments that can be improved. This severity level is not marked on the error stripe and does not have a default highlighting style, but you can select one from the list of existing styles or configure your own. +- No highlighting (but fix available) -This section is intended to encourage you as an author to think about the lessons from other languages, provide readers of your RFC with a fuller picture. -If there is no prior art, that is fine - your ideas are interesting to us whether they are brand new or if it is an adaptation from other languages. +LSP has the following [diagnostic levels](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnostic): +- Error +- Warning +- Information +- Hint -Note that while precedent set by other languages is some motivation, it does not on its own motivate an RFC. -Please also take into consideration that rust sometimes intentionally diverges from common language features. +with the following diagnostic tags: +- Unnecessary +- Deprecated # Unresolved questions [unresolved-questions]: #unresolved-questions -- What parts of the design do you expect to resolve through the RFC process before this gets merged? -- What parts of the design do you expect to resolve through the implementation of this feature before stabilization? -- 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? +- `CARGO_BUILD_NITS=nit` is modeled off of `CARGO_BUILD_WARNINGS=warn` which is just saying "treat X level violations as Y level" + - `CARGO_BUILD_WARNINGS=warn` is "weird" but shouldn't show up too often as its the default + - However, the frequency is swapped with `CARGO_BUILD_NITS=nit` + - We may also want the terms to be consistent across the different level-control fields + - Reminder: `CARGO_BUILD_WARNINGS` is unstable and we can still change it -# Future possibilities -[future-possibilities]: #future-possibilities +## Naming + +The name should clearly communicate that there is no authoriative weight +encouraging them to resolve them but that it is left to them and their reviewer +to weigh whether to resolve or ignore them. -Think about what the natural extension and evolution of your proposal would -be and how it would affect the language and project as a whole in a holistic -way. Try to use this section as a tool to more fully consider all possible -interactions with the project and language in your proposal. -Also consider how this all fits into the roadmap for the project -and of the relevant sub-team. +The verb form is used for: +- attribute +- long flag -This is also a good place to "dump ideas", if they are out of scope for the -RFC you are writing but otherwise related. +Lint names are expected to be read with their level for what should be done, e.g. +- `deny(let_and_return)` -> "deny the use of let-and-return in this code" +- `warn(let_and_return)` -> "warn on the use of let-and-return in this code" -If you have tried and cannot think of any future possibilities, -you may simply state that you cannot think of anything. +The noun form is used for: +- Dynamic lint group -Note that having something written down in the future-possibilities section -is not a reason to accept the current or a future RFC; such notes should be -in the section on motivation or rationale in this or subsequent RFCs. -The section merely provides additional information. +Options +- `#[note]` / `--note` / `-N` / `-Wnotes` + - "note the let-and-return in this code" +- `#[notice]` / `--notice` / `-N` / `-Wnotices` + - "notice the let-and-return in this code" +- `#[consider]` / `--consider` / ~~`-C` (taken)~~ / `-Wconsiderations` (IntelliJ) + - "consider the let-and-return in this code" + - Verb and noun have a larger divergence +- `#[inform]` / `--inform` / `-I` / `-Winformation` (LSP) + - "inform you of the let-and-return in this code" + - Verb and noun have a larger divergence +- `#[hint]` / `--hint` / `-H` / `-Whints` (LSP) + - "hint of a let-and-return in this code" +- `#[remark]` / `--remark` / `-R` (supposedly LLVM) / `-Wremarks` + - "remark on the let-and-return in this code" +- ~~`#[suggest]` / `--suggest` / `-S` / `-Wsuggestions`~~ + - When read with the lint name, it sounds like its to find where you *should* do it rather than finding where you are doing it + - Verb and noun have a larger divergence + +# Future possibilities +[future-possibilities]: #future-possibilities From ee5c90e43cb798d0dde68ce6da875932c56a4ea5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 11:38:21 -0600 Subject: [PATCH 03/23] fix: Rename the file for RFC# --- text/{0000-nit-lint-level.md => 3730-nit-lint-level.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename text/{0000-nit-lint-level.md => 3730-nit-lint-level.md} (100%) diff --git a/text/0000-nit-lint-level.md b/text/3730-nit-lint-level.md similarity index 100% rename from text/0000-nit-lint-level.md rename to text/3730-nit-lint-level.md From 066712c3f4178a2c65a1142f87a7df9d8ad6e26d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 11:38:44 -0600 Subject: [PATCH 04/23] fix: Update body with RFC# --- text/3730-nit-lint-level.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index dfe1b0d8981..6aa27e8e61c 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -1,6 +1,6 @@ - Feature Name: `nit-lint-level` - Start Date: 2024-11-15 -- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- RFC PR: [rust-lang/rfcs#3730](https://github.com/rust-lang/rfcs/pull/3730) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) # Summary From 91be5d26fdefbdb841eadd1358f8ca4cd008e4c7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 12:03:53 -0600 Subject: [PATCH 05/23] fix: Be less prescriptive on rendering --- text/3730-nit-lint-level.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 6aa27e8e61c..727284d615d 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -241,7 +241,8 @@ $ rustc --nit deprecated ... $ rustc -Ndeprecated ... ``` -This will be rendered like a `note:`. +These will be rendered like other lint levels with a diagnostic level and coloring that conveys the non-blocking, helpful nature. +For example, this could be rendered like a `note:` or `help:` diagnostic level. Rustc will have a dynamic lint group of all `nit`s, much like `warnings` is all lints at level `warn` at the time of processing, `-Anits`. From 1bc7f95e4059de7fa5acb2bce51b99bb4539d175 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 12:06:27 -0600 Subject: [PATCH 06/23] feat: Add nit as an option --- text/3730-nit-lint-level.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 727284d615d..add71ca1274 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -348,6 +348,8 @@ The noun form is used for: - Dynamic lint group Options +- `#[nit]` / `--nit` / `-N` / `-Wnits` + - Unsure how to word this as a sentence - `#[note]` / `--note` / `-N` / `-Wnotes` - "note the let-and-return in this code" - `#[notice]` / `--notice` / `-N` / `-Wnotices` From 883fb825efc09c65c7b40a2e296739ad2d3c0caa Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 13:04:01 -0600 Subject: [PATCH 07/23] fix: Clarify the nit concern --- text/3730-nit-lint-level.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index add71ca1274..cf8f1c2e63a 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -349,7 +349,7 @@ The noun form is used for: Options - `#[nit]` / `--nit` / `-N` / `-Wnits` - - Unsure how to word this as a sentence + - Unsure how to word `nit(let_and_return)` as a sentence like the other lint levels - `#[note]` / `--note` / `-N` / `-Wnotes` - "note the let-and-return in this code" - `#[notice]` / `--notice` / `-N` / `-Wnotices` From b7093ce1c54556d4782dc72e0064d608a1e85dc7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 13:04:29 -0600 Subject: [PATCH 08/23] feat: Add 'mention' as an option --- text/3730-nit-lint-level.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index cf8f1c2e63a..4246c89e542 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -354,6 +354,8 @@ Options - "note the let-and-return in this code" - `#[notice]` / `--notice` / `-N` / `-Wnotices` - "notice the let-and-return in this code" +- `#[mention]` / `--mention` / `-M` / `-Wmentions` + - "mention the let-and-return in this code" - `#[consider]` / `--consider` / ~~`-C` (taken)~~ / `-Wconsiderations` (IntelliJ) - "consider the let-and-return in this code" - Verb and noun have a larger divergence From 644528155be4090cd88b9659efbc44a491bd4b17 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 13:10:38 -0600 Subject: [PATCH 09/23] fix: Cover warning fatigue --- text/3730-nit-lint-level.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 4246c89e542..cba60a29a49 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -26,6 +26,10 @@ this new lint level. By its name and literal usage, the `warn` level is non-blocking. However, most projects treat `warn` as a soft-error. It doesn't block for local development but CI blocks it from being merged. +This is an attempt to balance final correctness with rapid prototyping. +Requiring "warnings clean" code also avoids warnings fatigue where warnings +make it hard to see "relevant' compiler output and +act as proverbial [broken windows](https://en.wikipedia.org/wiki/Broken_windows_theory). This convention is not new with the Rust community; many C++ projects have take this approach before Rust came to be with "warnings clean" being a goal for correctness. From 56876e2bac318562a7d08d53613c8813bdff0197 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 13:47:51 -0600 Subject: [PATCH 10/23] fix: Link to the original internals thread --- text/3730-nit-lint-level.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index cba60a29a49..3f1fd3f789a 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -2,6 +2,7 @@ - Start Date: 2024-11-15 - RFC PR: [rust-lang/rfcs#3730](https://github.com/rust-lang/rfcs/pull/3730) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) +- [Internals](https://internals.rust-lang.org/t/forbid-deny-warn-allow-and-notice/19986) # Summary [summary]: #summary From f82b8ca5b3382148f8e41c49fb122062fe9c1c59 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 14:13:54 -0600 Subject: [PATCH 11/23] fix: Avoid broken windows Co-authored-by: Josh Triplett --- text/3730-nit-lint-level.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 3f1fd3f789a..ffc65b98875 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -29,8 +29,9 @@ However, most projects treat `warn` as a soft-error. It doesn't block for local development but CI blocks it from being merged. This is an attempt to balance final correctness with rapid prototyping. Requiring "warnings clean" code also avoids warnings fatigue where warnings -make it hard to see "relevant' compiler output and -act as proverbial [broken windows](https://en.wikipedia.org/wiki/Broken_windows_theory). +make it hard to see "relevant" compiler output, and make a codebase feel +lower-quality in a way that does not inspire people or invite people to +help solve the problem. This convention is not new with the Rust community; many C++ projects have take this approach before Rust came to be with "warnings clean" being a goal for correctness. From fb61bb239d69c776d6f0a4a85d958ef6af6a53ec Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 14:22:32 -0600 Subject: [PATCH 12/23] feat: Add Github prior art --- text/3730-nit-lint-level.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index ffc65b98875..ee8c7e79451 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -327,6 +327,11 @@ with the following diagnostic tags: - Unnecessary - Deprecated +Github/Sarif support the following [severities](https://docs.github.com/en/code-security/code-scanning/managing-code-scanning-alerts/about-code-scanning-alerts#about-alert-severity-and-security-severity-levels) +- Error +- Warning +- Note + # Unresolved questions [unresolved-questions]: #unresolved-questions From 9e3c40aee83d6495e3e8a72a36fd239a091f6534 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 15:05:51 -0600 Subject: [PATCH 13/23] feat: Start talking to how CI can be done --- text/3730-nit-lint-level.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index ee8c7e79451..4768c8d6665 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -210,6 +210,18 @@ the author's determination to ignore it. For future PRs, Github should not report this as it should recognize that the report is for existing code. +## Integrating with CI + +The above potential experience is dependent on being able to integrate linting with CI to report only "new lints" or "lints for changed code". + +For Github users, you can use [`clippy-sarif`](https://crates.io/crates/clippy-sarif) to report lints as SARIF alerts. +Github [tracks these alerts across runs](https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#providing-data-to-track-code-scanning-alerts-across-runs). + +These can also be implemented manually with any setup. +For example, in a large, corporate, legacy C++ code base that [epage](https://github.com/epage/) +worked with, a linter used a similar database to track what lints were "new" and reported those back to +[ReviewBoard](https://www.reviewboard.org/). + ## Choosing lint levels When creating a lint or overriding a default lint level, From a03f5d2a6124954619834590def481a4ee98f6ad Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 19 Nov 2024 15:58:28 -0600 Subject: [PATCH 14/23] feat(motivation): Talk to migrating allows to warns --- text/3730-nit-lint-level.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 4768c8d6665..ea51a4f0fef 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -66,6 +66,11 @@ It can take effort to sift through all of the [default-`allow`ed lints](https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow) for which ones a maintainer may want to turn into a soft or hard error. +Another secondary benefit is that this could provide a smoother path for linter +teams to migrate `allow`s to `warn`s by having a period of time in a +non-blocking lint level so people can benefit immediately while having more +flexibility on when they pay the cost for turning the lint into a soft-error. + ## Example: `clap` Each lint below from `clap`s `Cargo.toml` represents a lint that could be useful but not worthwhile enough to `allow`: From 5e307601ca3116a0be45326246860dc312fa7bfd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 20 Nov 2024 11:40:48 -0600 Subject: [PATCH 15/23] feat(future): Discuss first-party SARIF support --- text/3730-nit-lint-level.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index ea51a4f0fef..7a26fb44224 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -400,3 +400,16 @@ Options # Future possibilities [future-possibilities]: #future-possibilities + +## First-class SARIF support + +Like wanting [JUnit support for libtest](https://github.com/rust-lang/rust/issues/85563), +having SARIF support in first-party Rust linters (rustc, rustdoc, rust-clippy, or their Cargo wrappers) +would help in integrating Rust with third-party systems without needing a third-party tool like +[`clippy-sarif`](https://crates.io/crates/clippy-sarif). +See also [rust-clippy#8121](https://github.com/rust-lang/rust-clippy/issues/8121). + +In moving forward with this, we'd need to figure out +- What layer this should be supported in +- The semantics for output teeing (specifying the location, separate output format from teed format, etc) +- Versioning or flavors of these documents From 14faa69481b3365dc0e174e8752ce4f012c2c5ff Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 20 Nov 2024 11:57:41 -0600 Subject: [PATCH 16/23] feat(alt): Discuss pedantic --- text/3730-nit-lint-level.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 7a26fb44224..60b97cf0253 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -321,6 +321,17 @@ them will be present unless `RUSTFLAGS=-Anits` is applied by the user. - `nits` lint group is created to give rustc-only or third-party build tools a built-in way to control these besides ignoring them like Cargo - Cargo could print a message that `nit`s are present to help raise awareness of how to show them but they will likely always be present, so this is noise to experienced users. +## `clippy::pedantic` + +In purpose, this has a lot of overlap with +[`clippy::pedantic`](https://rust-lang.github.io/rust-clippy/master/index.html?groups=pedantic#/print_stderr) +in that these are "too noisy" of lints. +A mechanism could be devised for hiding/showing this group within Cargo. + +However, +- This is clippy specific. The conversation would instead shift to getting an agreed-to convention for this group. +- The group is linter-defined and users can't override it, lints into or out of `pedantic` and `allow` or `warn`. + # Prior art [prior-art]: #prior-art From 2410bb1ee7a2c2c225ba651229e52938fb541941 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 20 Nov 2024 12:00:50 -0600 Subject: [PATCH 17/23] feat(ref): Discuss clippy --- text/3730-nit-lint-level.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 60b97cf0253..e5a1611785c 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -269,6 +269,24 @@ For example, this could be rendered like a `note:` or `help:` diagnostic level. Rustc will have a dynamic lint group of all `nit`s, much like `warnings` is all lints at level `warn` at the time of processing, `-Anits`. +## Clippy + +Like Rustc, Clippy would focus on the mechanism. +Add support for the new lint: +```rust +#![nit(deprecated)] +``` +```console +$ rustc --nit clippy::pedantic ... +``` +```console +$ rustc -Nclippy::pedantic ... +``` + +These will be rendered like Rustc. + +These will be part of Rustc's dynamic lint group, much like `warnings`. + ## Cargo Cargo implements the semantics for this to be a first-class non-blocking lint level. From 21f543aabc1a53983697c209dec11e734d67491b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 21 Nov 2024 15:00:36 -0600 Subject: [PATCH 18/23] feat(motivation): Call out the avoid-breaking-exported-api casee --- text/3730-nit-lint-level.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index e5a1611785c..0642c1a02b7 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -71,6 +71,20 @@ teams to migrate `allow`s to `warn`s by having a period of time in a non-blocking lint level so people can benefit immediately while having more flexibility on when they pay the cost for turning the lint into a soft-error. +One area worth exploring for how lints are structured that we will not be +addressing is when they become `allow`ed under alternative circumstances. +These range from +[`avoid-breaking-exported-api`](https://doc.rust-lang.org/clippy/lint_configuration.html#avoid-breaking-exported-api) +which would be of interest to know about when developing new APIs +to +[`allow-dbg-in-tests`](https://doc.rust-lang.org/clippy/lint_configuration.html#allow-dbg-in-tests) +which the developer may not care to have called out. +As default lint levels are being left to the teams, +the handling of these cases are also being left to the relevant teams. +This is being included for completeness of different workflows of lints and in +case it inspires an alternative design that can better encompass cases like +this. + ## Example: `clap` Each lint below from `clap`s `Cargo.toml` represents a lint that could be useful but not worthwhile enough to `allow`: From d5814384b304a9a2baa2f5f4a91546fdab2025c7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 21 Nov 2024 15:28:26 -0600 Subject: [PATCH 19/23] feat(motivation): Cover soft-errors that block release --- text/3730-nit-lint-level.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 0642c1a02b7..4559d42bc7c 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -85,6 +85,15 @@ This is being included for completeness of different workflows of lints and in case it inspires an alternative design that can better encompass cases like this. +A linting workflow that this RFC intentionally does not try to support is for soft-errors that block on release, +in addition to soft-errors that block CI. +Historically, the software industry has had separate development and hardening phases. +The more recent trend has focused on always being in a releasable state through +the aid of code review and automated testing in CI. +While there are still times for more explicit hardening phases (gated features, human acceptance testing), +we don't see enough benefit for deferrings lints to a hardening phase to outweigh the costs of +smoothing out this workflow being misapplied. + ## Example: `clap` Each lint below from `clap`s `Cargo.toml` represents a lint that could be useful but not worthwhile enough to `allow`: From eaf01459eeb0cd5970b5b25dab585460e8acaf20 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 22 Nov 2024 11:54:23 -0600 Subject: [PATCH 20/23] feat(rationale): Highlight the strengths of this solution --- text/3730-nit-lint-level.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 4559d42bc7c..4bc95ba052a 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -352,6 +352,15 @@ them will be present unless `RUSTFLAGS=-Anits` is applied by the user. # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives +Overall, this solution benefits from +- Users have adopted `[lints]` for controlling their project lint semantics, so this fits right in. + - In particular, `[lints]` design is optimized for setting only `level`. +- The `CARGO_BUILD_WARNINGS=deny` work for further cements the relationship of lint levels with the semantic levels discusse +- Users or tooling that request to see `nits` through Cargo can do so without + recompilation because Rusts always repors `nits`, Cargo records this and + replays it on cache hits, and the choice to display is made after this. + +For some specifics - `CARGO_BUILD_NITS=allow` is the default to avoid "warning fatigue" - The default level for lints is left unchanged by this RFC to keep the scope of what is designed and reviewed to the minimum - Rustc shows `nit`s by default, rather than hide them and require a separate opt-in mechanism for Cargo to see them From a9d8e560f309d4c395d3b90229462c56cee76119 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 22 Nov 2024 11:55:31 -0600 Subject: [PATCH 21/23] fix(rationale): Call out that CARGO_BUILD_NITS=deny doesn't make sense --- text/3730-nit-lint-level.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 4bc95ba052a..63dee1424a3 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -362,6 +362,8 @@ Overall, this solution benefits from For some specifics - `CARGO_BUILD_NITS=allow` is the default to avoid "warning fatigue" + - Users should not have to feel the need to resolve these for the sake of satisfying the tool + - Therefore, adding support for `CARGO_BUILD_NITS=deny` in the future would run counter to the goals and intents of this RFC. The user might as well switch the `nits` to `warn`. - The default level for lints is left unchanged by this RFC to keep the scope of what is designed and reviewed to the minimum - Rustc shows `nit`s by default, rather than hide them and require a separate opt-in mechanism for Cargo to see them - Problem: People directly using rustc or using a third-party build tool may be inundated with these From 9f279265c70223a8eec18210383757da32cd0f80 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 22 Nov 2024 13:50:54 -0600 Subject: [PATCH 22/23] fix(rationale): Call out LSP reporting benefit --- text/3730-nit-lint-level.md | 1 + 1 file changed, 1 insertion(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 63dee1424a3..38fc0309438 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -359,6 +359,7 @@ Overall, this solution benefits from - Users or tooling that request to see `nits` through Cargo can do so without recompilation because Rusts always repors `nits`, Cargo records this and replays it on cache hits, and the choice to display is made after this. +- LSPs, Sarif, etc being able to report soft-errors and nits differently to the user For some specifics - `CARGO_BUILD_NITS=allow` is the default to avoid "warning fatigue" From bf52f759586e44a84ee43cd2bdb72491cd38571b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 30 Nov 2024 08:38:16 -0600 Subject: [PATCH 23/23] feat(ref): Discuss cargo-fix behavior --- text/3730-nit-lint-level.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/3730-nit-lint-level.md b/text/3730-nit-lint-level.md index 38fc0309438..bb373973c0d 100644 --- a/text/3730-nit-lint-level.md +++ b/text/3730-nit-lint-level.md @@ -320,6 +320,8 @@ Add support for the new lint: deprecated = "nit" ``` +`cargo fix` will not fix these lints by default. + A new config field will be added to mirror `build.warnings`: ### `build.nits`