From b80c2ffd1ac7702cc54057a5e87b85bfdf49e9d5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 14:30:50 -0600 Subject: [PATCH 01/64] chore: Start manifest-lint RFC --- text/0000-manifest-lint.md | 97 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 text/0000-manifest-lint.md diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md new file mode 100644 index 00000000000..a2ab4c4c8a6 --- /dev/null +++ b/text/0000-manifest-lint.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 28f51c33fe0bc0f63e4b1316ab4a988ff341948c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 14:31:18 -0600 Subject: [PATCH 02/64] feat: Start manifest-lint RFC --- text/0000-manifest-lint.md | 193 ++++++++++++++++++++++++++----------- 1 file changed, 137 insertions(+), 56 deletions(-) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index a2ab4c4c8a6..dbb5eb8031f 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -1,97 +1,178 @@ -- 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: `manifest-lint` +- Start Date: 2023-02-14 - 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 `package.lint` table to `Cargo.toml` to configure reporting levels for +rustc and other tool lints. # Motivation [motivation]: #motivation -Why are we doing this? What use cases does it support? What is the expected outcome? +Currently, you can configure lints through +- `#[()]` or `#![()]`, like `#[forbid(unsafe)]` + - But this doesn't scale up with additional targets (benches, examples, + tests) or workspaces +- On the command line, like `cargo check -- --forbid unsafe` + - This puts the burden on the caller +- Through `RUSTFLAGS`, like `RUSTFLAGS=--forbid=unsafe cargo check` + - This puts the burden on the caller +- In `.cargo/config.toml`'s `target.*.rustflags` + - This couples you to the running in specific directories and not running in + the right directory causes rebuilds + - The cargo team has previously stated that + [they would like to see package-specific config moved to manifests](https://internals.rust-lang.org/t/proposal-move-some-cargo-config-settings-to-cargo-toml/13336/14?u=epage) + +We would like a solution that makes it easier to share across targets and +packages for all tools. + +See also +- [rust-lang/rust-clippy#1313](https://github.com/rust-lang/rust-clippy/issues/1313) +- [rust-lang/cargo#5034](https://github.com/rust-lang/cargo/issues/5034) +- [EmbarkStudios/rust-ecosystem#59](https://github.com/EmbarkStudios/rust-ecosystem/issues/59) # 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. +A new `package.lint` table would be added to configure lints: +```toml +[package.lint] +unsafe = "forbid" +``` +and `cargo` would pass these along as flags to `rustc` and `clippy`. + +This would work with +[RFC 2906 `workspace-deduplicate`](https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html?highlight=2906#): +```toml +[workspace.package.lint] +unsafe = "forbid" + +[package] +lint = "workspace" +``` + +## Documentation Updates + +Note: +[Workspace: The Package Table](https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table) +would be updated to say this works for workspace-inheritance + +As a new [`[package]` entry](https://doc.rust-lang.org/cargo/reference/manifest.html#the-package-section): + +**The `lint` field** + +Override the default level of lints by assigning them to a new level in a +table, for example: +```toml +[package.lint] +unsafe = "forbid" +``` + +Supported levels include: +- `forbid` +- `deny` +- `warn` +- `allow` + +**Note:** TOML does not support `:` in unquoted keys, requiring tool-specific +lints to be quoted, like +```toml +[package.lint] +"clippy::enum_glob_use" = "warn" +``` # 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. +When parsing a manifest, cargo will resolve workspace inheritance for +`package.lint.workspace = true` as it does with other fields. -The section should return to the examples given in the previous section, and explain more fully how the detailed proposal makes those examples work. +When running rustc, cargo will transform the lints from `lint = level` to +`--level lint` and pass them on the command line before `RUSTFLAGS`, allowing +user configuration to override package configuration. # Drawbacks [drawbacks]: #drawbacks -Why should we *not* do this? +A concern brought up in +[rust-lang/rust-clippy#1313](https://github.com/rust-lang/rust-clippy/issues/1313) +was that this will pass lints unconditionally to the underlying tool, leading +to "undefined lint" warnings when used on earlier versions, requiring that +warning to also be suppressed, reducing its value. However, in the "Future +possibility's", we mention direct support for tying lints to rust versions. # 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? +This could be left to `clippy.toml` but that leaves `rustc` without a solution. + +`[package.lint]` could be `[lint]` but the lints are tied to the package and +this aligns within the existing design space for workspace inheritance. + +We could support platform or feature specific settings, like with +`[lint.]` or `[target..lint]` but +- There isn't a defined use case for this yet besides having support for `cfg(feature = "clippy")` or + which does not seem high enough priority to design + around. +- `[lint.]` runs into ambiguity issues around what is a `` + entry vs a `` entry in the `[lint]` table. +- We have not yet defined semantics for sharing something like this across a + workspace + +Instead of the `[package.lint]` table being `lint = "level"`, we could organize +it around `level = ["lint", ...]` like some other linters do (like +[ruff](https://beta.ruff.rs/docs/configuration/)) but this works better for +logically organizing lints, highlighting what changed in diffs, and for +possibly adding lint-specific configuration in the future. # 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: +Python +- [flake8](https://flake8.pycqa.org/en/latest/user/configuration.html) + - Format is `level = [lint, ...]` +- [pylint](https://github.com/PyCQA/pylint/blob/main/examples/pylintrc#L402) + - Format is `level = [lint, ...]` but the [config file is a reflection of the CLI](https://pylint.pycqa.org/en/latest/user_guide/configuration/index.html) +- [ruff](https://beta.ruff.rs/docs/configuration/)) + - Format is `level = [lint, ...]`, likely due to past precedence in ecosystem (see above) -- 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. +Javascript +- [eslint](https://eslint.org/docs/latest/use/configure/rules) + - Format is `lint = level` / `lint = [ level, additional config ]` -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. +Go +- [golangci-lint](https://golangci-lint.run/usage/configuration/) + - Format is `level = [lint, ...]` # 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 +Should it be `lint` or `lints`? -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. +How does this affect fingerprinting / recompilation and how should it? -This is also a good place to "dump ideas", if they are out of scope for the -RFC you are writing but otherwise related. +How should we hand rustdoc lint levels or, in the future, cargo lint levels? +The current proposal takes all lints and passes them to rustc like `RUSTFLAGS` +but rustdoc uses `RUSTDOCFLAGS` and cargo would use neither. -If you have tried and cannot think of any future possibilities, -you may simply state that you cannot think of anything. +# Future possibilities +[future-possibilities]: #future-possibilities -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. +## Configurable lints + +We can extend basic lint syntax: +```toml +[package.lint] +cyclomatic_complexity = "allow" +``` +to support configuration, whether for cargo or the lint tool: +```toml +[package.lint] +cyclomatic_complexity = { level = "allow", rust-version = "1.23.0", threshold = 30 } +``` +Where `rust-version` is used by cargo to determine whether to pass along this +lint and `threshold` is used by the tool. We'd need to define how to +distinguish between reserved and unreserved field names. From 6ac518ebc0c634a6d57927ecb7f9b8cdd60b6481 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 15:00:01 -0600 Subject: [PATCH 03/64] fix: Remove stray paren --- text/0000-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index dbb5eb8031f..d644ab357df 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -136,7 +136,7 @@ Python - Format is `level = [lint, ...]` - [pylint](https://github.com/PyCQA/pylint/blob/main/examples/pylintrc#L402) - Format is `level = [lint, ...]` but the [config file is a reflection of the CLI](https://pylint.pycqa.org/en/latest/user_guide/configuration/index.html) -- [ruff](https://beta.ruff.rs/docs/configuration/)) +- [ruff](https://beta.ruff.rs/docs/configuration/) - Format is `level = [lint, ...]`, likely due to past precedence in ecosystem (see above) Javascript From aa8695b1f3040f41db85867cc9c2f3338d7e1cae Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 15:13:09 -0600 Subject: [PATCH 04/64] fix: Pluralize the table --- text/0000-manifest-lint.md | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index d644ab357df..192e9322e40 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -6,7 +6,7 @@ # Summary [summary]: #summary -Add a `package.lint` table to `Cargo.toml` to configure reporting levels for +Add a `package.lints` table to `Cargo.toml` to configure reporting levels for rustc and other tool lints. # Motivation @@ -37,9 +37,9 @@ See also # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -A new `package.lint` table would be added to configure lints: +A new `package.lints` table would be added to configure lints: ```toml -[package.lint] +[package.lints] unsafe = "forbid" ``` and `cargo` would pass these along as flags to `rustc` and `clippy`. @@ -47,11 +47,11 @@ and `cargo` would pass these along as flags to `rustc` and `clippy`. This would work with [RFC 2906 `workspace-deduplicate`](https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html?highlight=2906#): ```toml -[workspace.package.lint] +[workspace.package.lints] unsafe = "forbid" [package] -lint = "workspace" +lints = "workspace" ``` ## Documentation Updates @@ -62,12 +62,12 @@ would be updated to say this works for workspace-inheritance As a new [`[package]` entry](https://doc.rust-lang.org/cargo/reference/manifest.html#the-package-section): -**The `lint` field** +**The `lints` field** Override the default level of lints by assigning them to a new level in a table, for example: ```toml -[package.lint] +[package.lints] unsafe = "forbid" ``` @@ -80,7 +80,7 @@ Supported levels include: **Note:** TOML does not support `:` in unquoted keys, requiring tool-specific lints to be quoted, like ```toml -[package.lint] +[package.lints] "clippy::enum_glob_use" = "warn" ``` @@ -88,7 +88,7 @@ lints to be quoted, like [reference-level-explanation]: #reference-level-explanation When parsing a manifest, cargo will resolve workspace inheritance for -`package.lint.workspace = true` as it does with other fields. +`package.lints.workspace = true` as it does with other fields. When running rustc, cargo will transform the lints from `lint = level` to `--level lint` and pass them on the command line before `RUSTFLAGS`, allowing @@ -109,20 +109,22 @@ possibility's", we mention direct support for tying lints to rust versions. This could be left to `clippy.toml` but that leaves `rustc` without a solution. -`[package.lint]` could be `[lint]` but the lints are tied to the package and +`[package.lints]` could be `[lints]` but the lints are tied to the package and this aligns within the existing design space for workspace inheritance. +`[lints]` could be `[lint]` but we decided to follow the precedence of `[dependencies]`. + We could support platform or feature specific settings, like with -`[lint.]` or `[target..lint]` but +`[lints.]` or `[target..lints]` but - There isn't a defined use case for this yet besides having support for `cfg(feature = "clippy")` or which does not seem high enough priority to design around. -- `[lint.]` runs into ambiguity issues around what is a `` - entry vs a `` entry in the `[lint]` table. +- `[lints.]` runs into ambiguity issues around what is a `` + entry vs a `` entry in the `[lints]` table. - We have not yet defined semantics for sharing something like this across a workspace -Instead of the `[package.lint]` table being `lint = "level"`, we could organize +Instead of the `[package.lints]` table being `lint = "level"`, we could organize it around `level = ["lint", ...]` like some other linters do (like [ruff](https://beta.ruff.rs/docs/configuration/)) but this works better for logically organizing lints, highlighting what changed in diffs, and for @@ -150,8 +152,6 @@ Go # Unresolved questions [unresolved-questions]: #unresolved-questions -Should it be `lint` or `lints`? - How does this affect fingerprinting / recompilation and how should it? How should we hand rustdoc lint levels or, in the future, cargo lint levels? @@ -165,12 +165,12 @@ but rustdoc uses `RUSTDOCFLAGS` and cargo would use neither. We can extend basic lint syntax: ```toml -[package.lint] +[package.lints] cyclomatic_complexity = "allow" ``` to support configuration, whether for cargo or the lint tool: ```toml -[package.lint] +[package.lints] cyclomatic_complexity = { level = "allow", rust-version = "1.23.0", threshold = 30 } ``` Where `rust-version` is used by cargo to determine whether to pass along this From 680fe11dc6dd9d1d8aea9372775185ae2759ebaa Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 15:24:35 -0600 Subject: [PATCH 05/64] fix: Make lints table top-level --- text/0000-manifest-lint.md | 62 +++++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 18 deletions(-) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index 192e9322e40..ca8c494a81e 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -6,7 +6,7 @@ # Summary [summary]: #summary -Add a `package.lints` table to `Cargo.toml` to configure reporting levels for +Add a `[lints]` table to `Cargo.toml` to configure reporting levels for rustc and other tool lints. # Motivation @@ -37,9 +37,9 @@ See also # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -A new `package.lints` table would be added to configure lints: +A new `lints` table would be added to configure lints: ```toml -[package.lints] +[lints] unsafe = "forbid" ``` and `cargo` would pass these along as flags to `rustc` and `clippy`. @@ -47,27 +47,23 @@ and `cargo` would pass these along as flags to `rustc` and `clippy`. This would work with [RFC 2906 `workspace-deduplicate`](https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html?highlight=2906#): ```toml -[workspace.package.lints] -unsafe = "forbid" +[lints] +workspace = true -[package] -lints = "workspace" +[workspace.lints] +unsafe = "forbid" ``` ## Documentation Updates -Note: -[Workspace: The Package Table](https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table) -would be updated to say this works for workspace-inheritance - -As a new [`[package]` entry](https://doc.rust-lang.org/cargo/reference/manifest.html#the-package-section): +## The `lints` section -**The `lints` field** +*as a new ["Manifest Format" entry](https://doc.rust-lang.org/cargo/reference/manifest.html#the-manifest-format)* Override the default level of lints by assigning them to a new level in a table, for example: ```toml -[package.lints] +[lints] unsafe = "forbid" ``` @@ -80,15 +76,44 @@ Supported levels include: **Note:** TOML does not support `:` in unquoted keys, requiring tool-specific lints to be quoted, like ```toml -[package.lints] +[lints] "clippy::enum_glob_use" = "warn" ``` +## The `lints` table + +*as a new [`[workspace]` entry](https://doc.rust-lang.org/cargo/reference/workspaces.html#the-workspace-section)* + +The `workspace.lints` table is where you define lint configuration to be inherited by members of a workspace. + +Specifying a workspace lint configuration is similar to package lints. + +Example: + +```toml +# [PROJECT_DIR]/Cargo.toml +[workspace] +members = ["crates/*"] + +[workspace.lints] +unsafe = "forbid" +``` + +```toml +# [PROJECT_DIR]/crates/bar/Cargo.toml +[package] +name = "bar" +version = "0.1.0" + +[lints] +workspace = true +``` + # Reference-level explanation [reference-level-explanation]: #reference-level-explanation When parsing a manifest, cargo will resolve workspace inheritance for -`package.lints.workspace = true` as it does with other fields. +`lints.workspace = true` as it does with other fields. When running rustc, cargo will transform the lints from `lint = level` to `--level lint` and pass them on the command line before `RUSTFLAGS`, allowing @@ -109,8 +134,9 @@ possibility's", we mention direct support for tying lints to rust versions. This could be left to `clippy.toml` but that leaves `rustc` without a solution. -`[package.lints]` could be `[lints]` but the lints are tied to the package and -this aligns within the existing design space for workspace inheritance. +`[lints]` could be `[package.lints]`, tying it to the package unlike `[patch]` +and other fields that are more workspace related. Instead, we used +`[dependencies]` as our model. `[lints]` could be `[lint]` but we decided to follow the precedence of `[dependencies]`. From 054de5173fd10405ad74407b4cf299573e8829b5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 15:26:40 -0600 Subject: [PATCH 06/64] fix: Call out workspace lint name reservation --- text/0000-manifest-lint.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index ca8c494a81e..5b775553fce 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -119,6 +119,8 @@ When running rustc, cargo will transform the lints from `lint = level` to `--level lint` and pass them on the command line before `RUSTFLAGS`, allowing user configuration to override package configuration. +**Note:** This reserves the lint name `workspace` to allow workspace inheritance. + # Drawbacks [drawbacks]: #drawbacks From 379605636f6749e23a25184331e19bedd6b3657b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 17:19:16 -0600 Subject: [PATCH 07/64] fix: Link to user-defined attribute RFC --- text/0000-manifest-lint.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index 5b775553fce..d683f65bb90 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -184,7 +184,9 @@ How does this affect fingerprinting / recompilation and how should it? How should we hand rustdoc lint levels or, in the future, cargo lint levels? The current proposal takes all lints and passes them to rustc like `RUSTFLAGS` -but rustdoc uses `RUSTDOCFLAGS` and cargo would use neither. +but rustdoc uses `RUSTDOCFLAGS` and cargo would use neither. This also starts +to get into +[user-defined tool attributes](https://rust-lang.github.io/rfcs/2103-tool-attributes.html). # Future possibilities [future-possibilities]: #future-possibilities From cfbd2314eecec6835eeb8ee9591bc9e16b7c174e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 14 Feb 2023 19:02:08 -0600 Subject: [PATCH 08/64] feat: Add a couple more brainstorming ideas --- text/0000-manifest-lint.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index d683f65bb90..d3cbb533fc3 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -152,6 +152,18 @@ We could support platform or feature specific settings, like with - We have not yet defined semantics for sharing something like this across a workspace +Instead of using workspace inheritance for `[lint]`, we could make it +workspace-level configuration, like `[patch]` which is automatically applied to +all workspace members. However, `[patch]` and friends are because they affect +the resolver / `Cargo.toml` and so they can only operate at the workspace +level. `[lints]` is more like `[dependencies]` in being something that applies +at the package level but we want shared across workspaces. + +Instead of traditional workspace inheritance where there is a single value to +inherit with `workspace = true`, we could have `[workspace.lints.]` +which defines presets and the user could do `lints. = true`. The user +could then name them as they wish to avoid collision with rustc lints. + Instead of the `[package.lints]` table being `lint = "level"`, we could organize it around `level = ["lint", ...]` like some other linters do (like [ruff](https://beta.ruff.rs/docs/configuration/)) but this works better for From c57a51040d5ca2a207367a2740df9273320535fc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 08:44:05 -0600 Subject: [PATCH 09/64] fix: Use correct --forbid syntax --- text/0000-manifest-lint.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index d3cbb533fc3..861e363eecc 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -16,9 +16,9 @@ Currently, you can configure lints through - `#[()]` or `#![()]`, like `#[forbid(unsafe)]` - But this doesn't scale up with additional targets (benches, examples, tests) or workspaces -- On the command line, like `cargo check -- --forbid unsafe` +- On the command line, like `cargo clippy -- --forbid unsafe` - This puts the burden on the caller -- Through `RUSTFLAGS`, like `RUSTFLAGS=--forbid=unsafe cargo check` +- Through `RUSTFLAGS`, like `RUSTFLAGS=--forbid=unsafe cargo clippy` - This puts the burden on the caller - In `.cargo/config.toml`'s `target.*.rustflags` - This couples you to the running in specific directories and not running in From 42010e8df73990268bb25ffe36b4280dd676ab32 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 09:07:04 -0600 Subject: [PATCH 10/64] fix: Apply ehuss' feedback --- text/0000-manifest-lint.md | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/text/0000-manifest-lint.md b/text/0000-manifest-lint.md index 861e363eecc..1de6a945fe6 100644 --- a/text/0000-manifest-lint.md +++ b/text/0000-manifest-lint.md @@ -33,6 +33,7 @@ See also - [rust-lang/rust-clippy#1313](https://github.com/rust-lang/rust-clippy/issues/1313) - [rust-lang/cargo#5034](https://github.com/rust-lang/cargo/issues/5034) - [EmbarkStudios/rust-ecosystem#59](https://github.com/EmbarkStudios/rust-ecosystem/issues/59) +- [Proposal: Cargo Lint configuration](https://internals.rust-lang.org/t/proposal-cargo-lint-configuration/9135) # Guide-level explanation [guide-level-explanation]: #guide-level-explanation @@ -117,7 +118,8 @@ When parsing a manifest, cargo will resolve workspace inheritance for When running rustc, cargo will transform the lints from `lint = level` to `--level lint` and pass them on the command line before `RUSTFLAGS`, allowing -user configuration to override package configuration. +user configuration to override package configuration. These flags will be +finterprinted so changing them will cause a rebuild. **Note:** This reserves the lint name `workspace` to allow workspace inheritance. @@ -131,6 +133,8 @@ to "undefined lint" warnings when used on earlier versions, requiring that warning to also be suppressed, reducing its value. However, in the "Future possibility's", we mention direct support for tying lints to rust versions. +This does not allow sharing lints across workspaces. + # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives @@ -142,6 +146,12 @@ and other fields that are more workspace related. Instead, we used `[lints]` could be `[lint]` but we decided to follow the precedence of `[dependencies]`. +Instead of using `::` as a separator between tool and lint (e.g. +`clippy::enum_glob_use`), we could use TOML dotted keys for this (e.g. +`clippy.enum_glob_use`). This has the advantage of allowing unquoted keys at +the cost of not being able to copy/paste the lint name from the tool's output +into the fileV + We could support platform or feature specific settings, like with `[lints.]` or `[target..lints]` but - There isn't a defined use case for this yet besides having support for `cfg(feature = "clippy")` or @@ -164,7 +174,7 @@ inherit with `workspace = true`, we could have `[workspace.lints.]` which defines presets and the user could do `lints. = true`. The user could then name them as they wish to avoid collision with rustc lints. -Instead of the `[package.lints]` table being `lint = "level"`, we could organize +Instead of the `[lints]` table being `lint = "level"`, we could organize it around `level = ["lint", ...]` like some other linters do (like [ruff](https://beta.ruff.rs/docs/configuration/)) but this works better for logically organizing lints, highlighting what changed in diffs, and for @@ -192,14 +202,23 @@ Go # Unresolved questions [unresolved-questions]: #unresolved-questions -How does this affect fingerprinting / recompilation and how should it? - How should we hand rustdoc lint levels or, in the future, cargo lint levels? The current proposal takes all lints and passes them to rustc like `RUSTFLAGS` but rustdoc uses `RUSTDOCFLAGS` and cargo would use neither. This also starts to get into [user-defined tool attributes](https://rust-lang.github.io/rfcs/2103-tool-attributes.html). +Should we only apply/fingerprint lints for the appropriate tool? For example, +we would not include and fingerprint `clippy::` lints when running builds, +allowing them to change without forcing a rebuild. We likely already need to +be tool-aware for built-in tools to handle `rustdoc::` lints (see above) so +this isn't much more of a step. + +How do we allow controling precedence between lints and lint groups? We are +using a TOML table with the keys as lint names which does not allow controlling +ordering. Even if we switched to `level = [lint, ...]`, you get a hard coded +precedence between levels that the user can't control. + # Future possibilities [future-possibilities]: #future-possibilities @@ -207,14 +226,21 @@ to get into We can extend basic lint syntax: ```toml -[package.lints] +[lints] cyclomatic_complexity = "allow" ``` to support configuration, whether for cargo or the lint tool: ```toml -[package.lints] +[lints] cyclomatic_complexity = { level = "allow", rust-version = "1.23.0", threshold = 30 } ``` Where `rust-version` is used by cargo to determine whether to pass along this lint and `threshold` is used by the tool. We'd need to define how to distinguish between reserved and unreserved field names. + +## Extending the syntax to `.cargo/config.toml` + +Similar to `profile` and `patch` being in both files, we could support +`[lints]` in both files. This allows more flexibility for experimentation with +this feature, like conditionally applying them or applying them via environment +variables. For now, users still have the option of using `rustflags`. From b79b792163e528e52936a2931135082482dda58c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 09:12:03 -0600 Subject: [PATCH 11/64] fix: Add RFC number --- text/{0000-manifest-lint.md => 3389-manifest-lint.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename text/{0000-manifest-lint.md => 3389-manifest-lint.md} (99%) diff --git a/text/0000-manifest-lint.md b/text/3389-manifest-lint.md similarity index 99% rename from text/0000-manifest-lint.md rename to text/3389-manifest-lint.md index 1de6a945fe6..9ff85b77e3b 100644 --- a/text/0000-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -1,6 +1,6 @@ - Feature Name: `manifest-lint` - Start Date: 2023-02-14 -- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- RFC PR: [rust-lang/rfcs#3389](https://github.com/rust-lang/rfcs/pull/3389) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) # Summary From 4f4107f22c325fa62bd958ae2c5379949e935167 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 09:15:29 -0600 Subject: [PATCH 12/64] fix: Link out to rubocop --- text/3389-manifest-lint.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 9ff85b77e3b..2a7ded970f7 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -199,6 +199,10 @@ Go - [golangci-lint](https://golangci-lint.run/usage/configuration/) - Format is `level = [lint, ...]` +Ruby +- [rubocop](https://docs.rubocop.org/rubocop/1.45/configuration.html) + - Format is `Lint: Enabled: true` + # Unresolved questions [unresolved-questions]: #unresolved-questions From bf6d9fc2d20abae91a2293151dc1c5b835bfe5be Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 10:37:10 -0600 Subject: [PATCH 13/64] fix: Typos --- text/3389-manifest-lint.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 2a7ded970f7..507dae91f2e 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -150,7 +150,7 @@ Instead of using `::` as a separator between tool and lint (e.g. `clippy::enum_glob_use`), we could use TOML dotted keys for this (e.g. `clippy.enum_glob_use`). This has the advantage of allowing unquoted keys at the cost of not being able to copy/paste the lint name from the tool's output -into the fileV +into the file. We could support platform or feature specific settings, like with `[lints.]` or `[target..lints]` but @@ -218,7 +218,7 @@ allowing them to change without forcing a rebuild. We likely already need to be tool-aware for built-in tools to handle `rustdoc::` lints (see above) so this isn't much more of a step. -How do we allow controling precedence between lints and lint groups? We are +How do we allow controlling precedence between lints and lint groups? We are using a TOML table with the keys as lint names which does not allow controlling ordering. Even if we switched to `level = [lint, ...]`, you get a hard coded precedence between levels that the user can't control. From d136690e43d6214094b6fa16c964213e0fcf2955 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 10:46:47 -0600 Subject: [PATCH 14/64] fix: Include cargo-cranky as prior art --- text/3389-manifest-lint.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 507dae91f2e..28721098b5b 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -183,6 +183,9 @@ possibly adding lint-specific configuration in the future. # Prior art [prior-art]: #prior-art +Rust +- [cargo cranky](https://github.com/ericseppanen/cargo-cranky) + Python - [flake8](https://flake8.pycqa.org/en/latest/user/configuration.html) - Format is `level = [lint, ...]` From 75198c24d36ed2b716502f306e06058ef35b1523 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 10:58:50 -0600 Subject: [PATCH 15/64] fix: Typo --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 28721098b5b..1030b6bc0b5 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -119,7 +119,7 @@ When parsing a manifest, cargo will resolve workspace inheritance for When running rustc, cargo will transform the lints from `lint = level` to `--level lint` and pass them on the command line before `RUSTFLAGS`, allowing user configuration to override package configuration. These flags will be -finterprinted so changing them will cause a rebuild. +fingerprinted so changing them will cause a rebuild. **Note:** This reserves the lint name `workspace` to allow workspace inheritance. From 4a267a6b227cb46c14f69212c575d2173a48647b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 11:04:55 -0600 Subject: [PATCH 16/64] fix: Be more explicit on workspace inheritance --- text/3389-manifest-lint.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 1030b6bc0b5..1d0caada85d 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -114,7 +114,9 @@ workspace = true [reference-level-explanation]: #reference-level-explanation When parsing a manifest, cargo will resolve workspace inheritance for -`lints.workspace = true` as it does with other fields. +`lints.workspace = true` as it does with basic fields, when `workspace` is +present, no other fields are allowed to be present. This precludes having the +package override the workspace on a lint-by-lint basis. When running rustc, cargo will transform the lints from `lint = level` to `--level lint` and pass them on the command line before `RUSTFLAGS`, allowing @@ -245,6 +247,14 @@ Where `rust-version` is used by cargo to determine whether to pass along this lint and `threshold` is used by the tool. We'd need to define how to distinguish between reserved and unreserved field names. +## Packages overriding inherited lints + +Currently, it is a hard error to mix `workspace = true` and lints. We could +open this up in the future for the package to override lints from the +workspace. This would not be a breaking change as we'd be changing an error +case into a working case. We'd need to ensure we had a path forward for the +semantics for configurable lints. + ## Extending the syntax to `.cargo/config.toml` Similar to `profile` and `patch` being in both files, we could support From c726cf3e97196c45007cd99484221f702d8f0a98 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 11:11:54 -0600 Subject: [PATCH 17/64] feat: Add external file possibility --- text/3389-manifest-lint.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 1d0caada85d..0d904eee50f 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -231,6 +231,12 @@ precedence between levels that the user can't control. # Future possibilities [future-possibilities]: #future-possibilities +## External file + +Like with `package.license`, users might want to refer to an external file for +their lints. This especially becomes useful for copy/pasting lints between +projects. + ## Configurable lints We can extend basic lint syntax: From 508e92f09c62c2b36bc12ebae440bd9b6793a966 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 11:39:23 -0600 Subject: [PATCH 18/64] feat: Add cargo lints as a future possibility --- text/3389-manifest-lint.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 0d904eee50f..75909d37b20 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -267,3 +267,12 @@ Similar to `profile` and `patch` being in both files, we could support `[lints]` in both files. This allows more flexibility for experimentation with this feature, like conditionally applying them or applying them via environment variables. For now, users still have the option of using `rustflags`. + +## Cargo Lints + +The cargo team has expressed interest in producing warnings for more situations +but this requires defining a lint control system for it. The overhead of doing +so has detered people from adding additional warnings. This would provide an +MVP for controlling cargo lints, unblocking the cargo team from adding more +warnings. This just leaves the question of whether these belong more in cargo +or in clippy which already has some cargo-specific lints. From 1c7ef777faf52375c807caf16dac57e98a675d72 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 12:11:36 -0600 Subject: [PATCH 19/64] fix: Note that cargo-metadata support is needed for configurable lints --- text/3389-manifest-lint.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 75909d37b20..1d92c8c04d9 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -253,6 +253,9 @@ Where `rust-version` is used by cargo to determine whether to pass along this lint and `threshold` is used by the tool. We'd need to define how to distinguish between reserved and unreserved field names. +`cargo metadata` would need to report the `lints` table so `clippy` could read +it without re-implementing workspace inheritance. + ## Packages overriding inherited lints Currently, it is a hard error to mix `workspace = true` and lints. We could From 4bfc5453d37eb1888131497fbed371acda7812d2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 12:27:40 -0600 Subject: [PATCH 20/64] fix: Update to lints.tool.lint --- text/3389-manifest-lint.md | 43 +++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 1d92c8c04d9..9dad2e38fb0 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -40,7 +40,7 @@ See also A new `lints` table would be added to configure lints: ```toml -[lints] +[lints.rust] unsafe = "forbid" ``` and `cargo` would pass these along as flags to `rustc` and `clippy`. @@ -61,10 +61,10 @@ unsafe = "forbid" *as a new ["Manifest Format" entry](https://doc.rust-lang.org/cargo/reference/manifest.html#the-manifest-format)* -Override the default level of lints by assigning them to a new level in a +Override the default level of lints from different tools by assigning them to a new level in a table, for example: ```toml -[lints] +[lints.rust] unsafe = "forbid" ``` @@ -74,12 +74,10 @@ Supported levels include: - `warn` - `allow` -**Note:** TOML does not support `:` in unquoted keys, requiring tool-specific -lints to be quoted, like -```toml -[lints] -"clippy::enum_glob_use" = "warn" -``` +To know which tool a lint falls under, it is the part before `::` in the lint +name. If there isn't a `::`, then the tool is `rust`. For example a warning +about `unsafe` would be `lints.rust.unsafe` but a lint about +`clippy::enum_glob_use` would be `lints.clippy.enum_glob_use`. ## The `lints` table @@ -96,7 +94,7 @@ Example: [workspace] members = ["crates/*"] -[workspace.lints] +[workspace.lints.rust] unsafe = "forbid" ``` @@ -123,7 +121,7 @@ When running rustc, cargo will transform the lints from `lint = level` to user configuration to override package configuration. These flags will be fingerprinted so changing them will cause a rebuild. -**Note:** This reserves the lint name `workspace` to allow workspace inheritance. +**Note:** This reserves the tool name `workspace` to allow workspace inheritance. # Drawbacks [drawbacks]: #drawbacks @@ -148,11 +146,11 @@ and other fields that are more workspace related. Instead, we used `[lints]` could be `[lint]` but we decided to follow the precedence of `[dependencies]`. -Instead of using `::` as a separator between tool and lint (e.g. -`clippy::enum_glob_use`), we could use TOML dotted keys for this (e.g. -`clippy.enum_glob_use`). This has the advantage of allowing unquoted keys at -the cost of not being able to copy/paste the lint name from the tool's output -into the file. +Instead of `.`, we could use `::` (e.g. +`"clipp::enum_glob_use"` instead of `clippy.enum_glob_use`), like in the +diagnostic messages. This would make it easier to copy/paste lint names but it +will requiring quoting the keys and is more difficult to add tool-level +configuration in the future. We could support platform or feature specific settings, like with `[lints.]` or `[target..lints]` but @@ -241,19 +239,26 @@ projects. We can extend basic lint syntax: ```toml -[lints] +[lints.clippy] cyclomatic_complexity = "allow" ``` to support configuration, whether for cargo or the lint tool: ```toml -[lints] +[lints.clippy] cyclomatic_complexity = { level = "allow", rust-version = "1.23.0", threshold = 30 } ``` Where `rust-version` is used by cargo to determine whether to pass along this lint and `threshold` is used by the tool. We'd need to define how to distinguish between reserved and unreserved field names. -`cargo metadata` would need to report the `lints` table so `clippy` could read +Tool-wide configuration would be in in the `lints..metadata` table and be +completely ignored by `cargo`. For example: +```toml +[lints.clippy.metadata] +avoid-breaking-exported-api = true +``` + +Tools will need `cargo metadata` to report the `lints` table so they can read it without re-implementing workspace inheritance. ## Packages overriding inherited lints From f8071a315f38841d17173077bd5a0541094aca48 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 15 Feb 2023 12:29:01 -0600 Subject: [PATCH 21/64] feat: Add open question about rustfmt --- text/3389-manifest-lint.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 9dad2e38fb0..c8658c7c72d 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -226,6 +226,11 @@ using a TOML table with the keys as lint names which does not allow controlling ordering. Even if we switched to `level = [lint, ...]`, you get a hard coded precedence between levels that the user can't control. +What does this mean for rustfmt? If this feature slowly absorbs the role of +`clippy.toml`, a `rustfmt.toml` file might stick out like a sore thumb. Do we +accept that, shoe-horn it in, generalize this feature into "tools" and "rules" +from "linters" and "lints"? + # Future possibilities [future-possibilities]: #future-possibilities From 1e43928544a01646e91311f5ddd5fc1edfef36a7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 17 Feb 2023 09:10:04 -0600 Subject: [PATCH 22/64] fix: Discuss all supported lint tools --- text/3389-manifest-lint.md | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index c8658c7c72d..b22b4dc158e 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -116,10 +116,21 @@ When parsing a manifest, cargo will resolve workspace inheritance for present, no other fields are allowed to be present. This precludes having the package override the workspace on a lint-by-lint basis. -When running rustc, cargo will transform the lints from `lint = level` to -`--level lint` and pass them on the command line before `RUSTFLAGS`, allowing -user configuration to override package configuration. These flags will be -fingerprinted so changing them will cause a rebuild. +cargo will contain a mapping of tool to underlying command (e.g. `rust` to +`rustc`, `clippy` to `rustc` when clippy is the driver, `rustdoc` to +`rustdoc`). When running the underlying command, cargo will transform the +lints from `lint = level` to `--level lint` and pass them on the command line +before other configuration, `RUSTFLAGS`, allowing user configuration to +override package configuration. These flags will be fingerprinted so changing +them will cause a rebuild. + +Initially, the only supported tools will be: +- `rust` +- `clippy` +- `rustdoc` + +Addition of third-party tools would fall under their +[attributes for tools](https://github.com/rust-lang/rust/issues/44690). **Note:** This reserves the tool name `workspace` to allow workspace inheritance. @@ -209,12 +220,6 @@ Ruby # Unresolved questions [unresolved-questions]: #unresolved-questions -How should we hand rustdoc lint levels or, in the future, cargo lint levels? -The current proposal takes all lints and passes them to rustc like `RUSTFLAGS` -but rustdoc uses `RUSTDOCFLAGS` and cargo would use neither. This also starts -to get into -[user-defined tool attributes](https://rust-lang.github.io/rfcs/2103-tool-attributes.html). - Should we only apply/fingerprint lints for the appropriate tool? For example, we would not include and fingerprint `clippy::` lints when running builds, allowing them to change without forcing a rebuild. We likely already need to From 3cd61251bbe54f57941c9adfdd42fa959ece7f26 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 21 Feb 2023 09:10:48 -0600 Subject: [PATCH 23/64] fix: Update for latest conversation Note the confusion over rustfmt not reading `package.edition`. Take a stab at resolving rustfmt by saying "no" and documenting it under alternatives. Be explicit on fingerprinting behavior and leave it out of the Unresolved section. --- text/3389-manifest-lint.md | 41 +++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index b22b4dc158e..03c927f5607 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -122,7 +122,7 @@ cargo will contain a mapping of tool to underlying command (e.g. `rust` to lints from `lint = level` to `--level lint` and pass them on the command line before other configuration, `RUSTFLAGS`, allowing user configuration to override package configuration. These flags will be fingerprinted so changing -them will cause a rebuild. +them will cause a rebuild only for the commands where they are used. Initially, the only supported tools will be: - `rust` @@ -137,6 +137,22 @@ Addition of third-party tools would fall under their # Drawbacks [drawbacks]: #drawbacks +There has been some user/IDE confusion about running commands like `rustfmt` +directly and expecting them to pick up configuration only associated with their +higher-level cargo-plugins despite that configuration (like `package.edition`) +being cargo-specific. By baking the configuration for rustc, rustdoc, and +clippy directly into cargo, we will be seeing more of this. A hope is that +this will actually improve with this RFC. Over time, tools will need to switch +to the model of runnign `cargo` to get confuguratio in response to this RFC. +As for users, if a tool's primary configuration is in `Cargo.toml`, that will +provide a strong coupling with `cargo` in users minds as compared to using an +external configuration file and overlooking the one or two fields read from +`Cargo.toml`. + +As this focuses on lints, this leaves out first-party tools that need +configuration but aren't linters, namely `rustfmt`, leading to an inconsistent +experience if `clippy.toml` goes away. + A concern brought up in [rust-lang/rust-clippy#1313](https://github.com/rust-lang/rust-clippy/issues/1313) was that this will pass lints unconditionally to the underlying tool, leading @@ -163,6 +179,18 @@ diagnostic messages. This would make it easier to copy/paste lint names but it will requiring quoting the keys and is more difficult to add tool-level configuration in the future. +We could possibly extend this new field to `rustfmt` by shifting the focus from +"lints" to "rules" (see +[eslint](https://eslint.org/docs/latest/use/configure/rules)). However, the +more we generalize this field, the fewer assumptions we can make about it. On +one extreme is `package.metadata` which is so free-form we can't support it +with workspace inheritance. A less extreme example is if we make the +configuration too general, we would preclude the option of supporting +per-package overrides as we wouldn't know enough about the shape of the data to +know how to merge it. There is likely a middle ground that we could make work +but it would take time and experimentation to figure that out which is at odds +with trying to maintain a stable file format. + We could support platform or feature specific settings, like with `[lints.]` or `[target..lints]` but - There isn't a defined use case for this yet besides having support for `cfg(feature = "clippy")` or @@ -220,22 +248,11 @@ Ruby # Unresolved questions [unresolved-questions]: #unresolved-questions -Should we only apply/fingerprint lints for the appropriate tool? For example, -we would not include and fingerprint `clippy::` lints when running builds, -allowing them to change without forcing a rebuild. We likely already need to -be tool-aware for built-in tools to handle `rustdoc::` lints (see above) so -this isn't much more of a step. - How do we allow controlling precedence between lints and lint groups? We are using a TOML table with the keys as lint names which does not allow controlling ordering. Even if we switched to `level = [lint, ...]`, you get a hard coded precedence between levels that the user can't control. -What does this mean for rustfmt? If this feature slowly absorbs the role of -`clippy.toml`, a `rustfmt.toml` file might stick out like a sore thumb. Do we -accept that, shoe-horn it in, generalize this feature into "tools" and "rules" -from "linters" and "lints"? - # Future possibilities [future-possibilities]: #future-possibilities From b02771a860f796e6494722a60ac039724a6ed042 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 21 Feb 2023 09:54:45 -0600 Subject: [PATCH 24/64] fix: Typo --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 03c927f5607..97a3a760f4e 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -143,7 +143,7 @@ higher-level cargo-plugins despite that configuration (like `package.edition`) being cargo-specific. By baking the configuration for rustc, rustdoc, and clippy directly into cargo, we will be seeing more of this. A hope is that this will actually improve with this RFC. Over time, tools will need to switch -to the model of runnign `cargo` to get confuguratio in response to this RFC. +to the model of running `cargo` to get confuguratio in response to this RFC. As for users, if a tool's primary configuration is in `Cargo.toml`, that will provide a strong coupling with `cargo` in users minds as compared to using an external configuration file and overlooking the one or two fields read from From 5a70d45f956c916b00b6b1d2f58d631588234d2f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 21 Feb 2023 13:17:53 -0600 Subject: [PATCH 25/64] feat: Take a stab at lint precedence --- text/3389-manifest-lint.md | 79 ++++++++++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 97a3a760f4e..af6a29cdf9b 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -68,13 +68,22 @@ table, for example: unsafe = "forbid" ``` -Supported levels include: +This is short-hand for: +```toml +[lints.rust] +unsafe = { level = "forbid", priority = 1 } +``` + +`level` corresponds to the lint levels in `rustc`: - `forbid` - `deny` - `warn` - `allow` -To know which tool a lint falls under, it is the part before `::` in the lint +`priority` controls which lints override other lints: +- `0` is lowest priority, being overridden by all, and shows up first on the command-line to tools like `rustc` + +To know which table under `[lints]` a particular lint belongs under, it is the part before `::` in the lint name. If there isn't a `::`, then the tool is `rust`. For example a warning about `unsafe` would be `lints.rust.unsafe` but a lint about `clippy::enum_glob_use` would be `lints.clippy.enum_glob_use`. @@ -119,10 +128,11 @@ package override the workspace on a lint-by-lint basis. cargo will contain a mapping of tool to underlying command (e.g. `rust` to `rustc`, `clippy` to `rustc` when clippy is the driver, `rustdoc` to `rustdoc`). When running the underlying command, cargo will transform the -lints from `lint = level` to `--level lint` and pass them on the command line -before other configuration, `RUSTFLAGS`, allowing user configuration to -override package configuration. These flags will be fingerprinted so changing -them will cause a rebuild only for the commands where they are used. +lints from `lint = level` to `--level lint`, sort them by priority and then +lint name, and pass them on the command line before other configuration, +`RUSTFLAGS`, allowing user configuration to override package configuration. +These flags will be fingerprinted so changing them will cause a rebuild only +for the commands where they are used. Initially, the only supported tools will be: - `rust` @@ -201,6 +211,14 @@ We could support platform or feature specific settings, like with - We have not yet defined semantics for sharing something like this across a workspace +Instead of the `[lints]` table being `lint = "level"`, we could organize +it around `level = ["lint", ...]` like some other linters do (like +[ruff](https://beta.ruff.rs/docs/configuration/)) but this works better for +logically organizing lints, highlighting what changed in diffs, and for +possibly adding lint-specific configuration in the future. + +## Workspace Inheritance + Instead of using workspace inheritance for `[lint]`, we could make it workspace-level configuration, like `[patch]` which is automatically applied to all workspace members. However, `[patch]` and friends are because they affect @@ -213,11 +231,45 @@ inherit with `workspace = true`, we could have `[workspace.lints.]` which defines presets and the user could do `lints. = true`. The user could then name them as they wish to avoid collision with rustc lints. -Instead of the `[lints]` table being `lint = "level"`, we could organize -it around `level = ["lint", ...]` like some other linters do (like -[ruff](https://beta.ruff.rs/docs/configuration/)) but this works better for -logically organizing lints, highlighting what changed in diffs, and for -possibly adding lint-specific configuration in the future. +## Lint Predence + +The priority field is meant to allow mimicing +- `-Aclippy::all -Wclippy::doc_markdown` +- `-D future-incompatible -A semicolon_in_expressions_from_macros` + +We can't order lints based on the level as which we want first is dependent on the context. + +We can't rely on the order of the keys in the table as that is undefined in TOML. + +We could use an array instead of a table: + +Unconfigurable: +```toml +[lints] +clippy = [ + { all = "Alow" }, + { doc_markdown = "Worn" }, +] +``` + +Configurable: +```toml +[[lints.clippy.all] +level = "Alow" +[[lints.clippy.doc_markdown] +level = "Worn" +``` +Where the order is based on how to pass them on the command-line. + +Complex TOML arrays tend to be less friendly to work with including the fact +that TOML 1.0 does not allow multi-line inline tables. + +For the most part, people won't need granularity, so we could instead start +with a `priority: bool` field. This might get confusing to mix with numbers +though (what does `false` and `true` map to?). There is also the problem that +generally people will want to opt a specific lint into being low-priority (the +group) and the leave the exceptions at default but making `priority = true` the +default would read weird (everything is a priorty but one or two items). # Prior art [prior-art]: #prior-art @@ -248,11 +300,6 @@ Ruby # Unresolved questions [unresolved-questions]: #unresolved-questions -How do we allow controlling precedence between lints and lint groups? We are -using a TOML table with the keys as lint names which does not allow controlling -ordering. Even if we switched to `level = [lint, ...]`, you get a hard coded -precedence between levels that the user can't control. - # Future possibilities [future-possibilities]: #future-possibilities From 08f7190eec6c377ecff960c00df05fc3e65e6a75 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 22 Feb 2023 16:07:44 -0600 Subject: [PATCH 26/64] fix: Make priority signed, giving a clear center value --- text/3389-manifest-lint.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index af6a29cdf9b..ad412392c01 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -71,7 +71,7 @@ unsafe = "forbid" This is short-hand for: ```toml [lints.rust] -unsafe = { level = "forbid", priority = 1 } +unsafe = { level = "forbid", priority = 0 } ``` `level` corresponds to the lint levels in `rustc`: @@ -80,8 +80,10 @@ unsafe = { level = "forbid", priority = 1 } - `warn` - `allow` -`priority` controls which lints override other lints: -- `0` is lowest priority, being overridden by all, and shows up first on the command-line to tools like `rustc` +`priority` is a signed value that controls which lints override other lints: +- lower (particularly negative) numbers have lower priority, being overridden + by higher numbers, and shows up first on the command-line to tools like + `rustc` To know which table under `[lints]` a particular lint belongs under, it is the part before `::` in the lint name. If there isn't a `::`, then the tool is `rust`. For example a warning From 7d81bf16970f193bb220a8bb77610d05d36b5dcb Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 22 Feb 2023 16:24:51 -0600 Subject: [PATCH 27/64] fix: Add another reason against 'rules' --- text/3389-manifest-lint.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index ad412392c01..fcec4f68930 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -201,7 +201,10 @@ configuration too general, we would preclude the option of supporting per-package overrides as we wouldn't know enough about the shape of the data to know how to merge it. There is likely a middle ground that we could make work but it would take time and experimentation to figure that out which is at odds -with trying to maintain a stable file format. +with trying to maintain a stable file format. Another problem with `rules` is +that it is divorced from any context. In eslint, it is in an eslint-specific +config file but a `[rules]` table is not a clear as a `[lints]` table as to +what role it fulfills. We could support platform or feature specific settings, like with `[lints.]` or `[target..lints]` but From 31c9587d522476f249d85717984cc495bd68396e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 22 Feb 2023 16:25:48 -0600 Subject: [PATCH 28/64] fix: Some TOML formatting --- text/3389-manifest-lint.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index fcec4f68930..788bedcd738 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -259,9 +259,9 @@ clippy = [ Configurable: ```toml -[[lints.clippy.all] +[[lints.clippy.all]] level = "Alow" -[[lints.clippy.doc_markdown] +[[lints.clippy.doc_markdown]] level = "Worn" ``` Where the order is based on how to pass them on the command-line. From cf9a148148ebaa69656a7713ffbc535cc028921b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 22 Feb 2023 21:16:16 -0600 Subject: [PATCH 29/64] fix: Clarify we are overriding lint groups --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 788bedcd738..94bc476c1fb 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -80,7 +80,7 @@ unsafe = { level = "forbid", priority = 0 } - `warn` - `allow` -`priority` is a signed value that controls which lints override other lints: +`priority` is a signed value that controls which lints or lint groups override other lint groups: - lower (particularly negative) numbers have lower priority, being overridden by higher numbers, and shows up first on the command-line to tools like `rustc` From 1adde1b477168ab157b31935a21231c49a76ccdc Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 23 Feb 2023 10:21:02 -0600 Subject: [PATCH 30/64] fix: Typo Co-authored-by: teor --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 94bc476c1fb..a9aa3b4425a 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -155,7 +155,7 @@ higher-level cargo-plugins despite that configuration (like `package.edition`) being cargo-specific. By baking the configuration for rustc, rustdoc, and clippy directly into cargo, we will be seeing more of this. A hope is that this will actually improve with this RFC. Over time, tools will need to switch -to the model of running `cargo` to get confuguratio in response to this RFC. +to the model of running `cargo` to get configuration in response to this RFC. As for users, if a tool's primary configuration is in `Cargo.toml`, that will provide a strong coupling with `cargo` in users minds as compared to using an external configuration file and overlooking the one or two fields read from From 78083edfdae5ea656b2802105b19ffbcf37e7791 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 23 Feb 2023 11:00:09 -0600 Subject: [PATCH 31/64] fix: Document future idea for lint-level source --- text/3389-manifest-lint.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index a9aa3b4425a..d7f5f3a7ab6 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -308,6 +308,13 @@ Ruby # Future possibilities [future-possibilities]: #future-possibilities +## rustc reporting `Cargo.toml` as lint-level source + +Currently Rust tells you where a lint level was enabled when it emits a lint. +`rustc` only sees that these lints are coming in from the command-line and +doesn't know about `[lints]`. +It would be nice if it could also point to Cargo.toml for this. + ## External file Like with `package.license`, users might want to refer to an external file for From 72cdd44a3b97020a716921cd7ae89bf582d29e9b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 23 Feb 2023 11:01:16 -0600 Subject: [PATCH 32/64] fix: Typos --- text/3389-manifest-lint.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index d7f5f3a7ab6..e1b1bc7dd59 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -238,7 +238,7 @@ could then name them as they wish to avoid collision with rustc lints. ## Lint Predence -The priority field is meant to allow mimicing +The priority field is meant to allow mimicking - `-Aclippy::all -Wclippy::doc_markdown` - `-D future-incompatible -A semicolon_in_expressions_from_macros` @@ -252,17 +252,17 @@ Unconfigurable: ```toml [lints] clippy = [ - { all = "Alow" }, - { doc_markdown = "Worn" }, + { all = "allow" }, + { doc_markdown = "worn" }, ] ``` Configurable: ```toml [[lints.clippy.all]] -level = "Alow" +level = "allow" [[lints.clippy.doc_markdown]] -level = "Worn" +level = "worn" ``` Where the order is based on how to pass them on the command-line. @@ -274,7 +274,7 @@ with a `priority: bool` field. This might get confusing to mix with numbers though (what does `false` and `true` map to?). There is also the problem that generally people will want to opt a specific lint into being low-priority (the group) and the leave the exceptions at default but making `priority = true` the -default would read weird (everything is a priorty but one or two items). +default would read weird (everything is a priority but one or two items). # Prior art [prior-art]: #prior-art From 70732661ab14212bf163de1b5f1795fac976c4f8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 27 Feb 2023 12:44:45 -0600 Subject: [PATCH 33/64] fix: Spelling and language --- text/3389-manifest-lint.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index e1b1bc7dd59..bf5d060ae23 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -80,9 +80,9 @@ unsafe = { level = "forbid", priority = 0 } - `warn` - `allow` -`priority` is a signed value that controls which lints or lint groups override other lint groups: +`priority` is a signed integer that controls which lints or lint groups override other lint groups: - lower (particularly negative) numbers have lower priority, being overridden - by higher numbers, and shows up first on the command-line to tools like + by higher numbers, and show up first on the command-line to tools like `rustc` To know which table under `[lints]` a particular lint belongs under, it is the part before `::` in the lint @@ -170,7 +170,7 @@ A concern brought up in was that this will pass lints unconditionally to the underlying tool, leading to "undefined lint" warnings when used on earlier versions, requiring that warning to also be suppressed, reducing its value. However, in the "Future -possibility's", we mention direct support for tying lints to rust versions. +possibilities" section, we mention direct support for tying lints to rust versions. This does not allow sharing lints across workspaces. @@ -236,7 +236,7 @@ inherit with `workspace = true`, we could have `[workspace.lints.]` which defines presets and the user could do `lints. = true`. The user could then name them as they wish to avoid collision with rustc lints. -## Lint Predence +## Lint Precedence The priority field is meant to allow mimicking - `-Aclippy::all -Wclippy::doc_markdown` From 86b0b64e5ced89d867cdc49ddb92408c4a48af8e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 27 Feb 2023 13:11:27 -0600 Subject: [PATCH 34/64] fix: remindme priority with multiple lint sources --- text/3389-manifest-lint.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index bf5d060ae23..b5649662677 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -362,6 +362,9 @@ Similar to `profile` and `patch` being in both files, we could support this feature, like conditionally applying them or applying them via environment variables. For now, users still have the option of using `rustflags`. +In doing so, we would need to define how `priority` interacts with different +sources of `[lints]`. + ## Cargo Lints The cargo team has expressed interest in producing warnings for more situations From a270d2854efbf91ea639e4f39d52d628b8bd993c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 27 Feb 2023 13:20:48 -0600 Subject: [PATCH 35/64] fix: Be explicit that lints does not affect dependencies --- text/3389-manifest-lint.md | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index b5649662677..ab33f884428 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -129,12 +129,15 @@ package override the workspace on a lint-by-lint basis. cargo will contain a mapping of tool to underlying command (e.g. `rust` to `rustc`, `clippy` to `rustc` when clippy is the driver, `rustdoc` to -`rustdoc`). When running the underlying command, cargo will transform the -lints from `lint = level` to `--level lint`, sort them by priority and then -lint name, and pass them on the command line before other configuration, -`RUSTFLAGS`, allowing user configuration to override package configuration. -These flags will be fingerprinted so changing them will cause a rebuild only -for the commands where they are used. +`rustdoc`). When running the underlying command for the specified package, +cargo will transform the lints from `lint = level` to `--level lint`, sort them +by priority and then lint name, and pass them on the command line before other +configuration, `RUSTFLAGS`, allowing user configuration to override package +configuration. These flags will be fingerprinted so changing them will cause a +rebuild only for the commands where they are used. + +Note that this means that `[lints]` does not affect dependencies which is +normally not an issue due to `--cap-lints` being used for dependencies. Initially, the only supported tools will be: - `rust` @@ -149,6 +152,10 @@ Addition of third-party tools would fall under their # Drawbacks [drawbacks]: #drawbacks +Since `[lints]` only affects the associated package, and not dependencies, it +will not work with `future-incompat` lints that are meant to be applied to +dependencies. This may cause some user confusion. + There has been some user/IDE confusion about running commands like `rustfmt` directly and expecting them to pick up configuration only associated with their higher-level cargo-plugins despite that configuration (like `package.edition`) @@ -362,6 +369,9 @@ Similar to `profile` and `patch` being in both files, we could support this feature, like conditionally applying them or applying them via environment variables. For now, users still have the option of using `rustflags`. +We would need to define whether this only affects local packages as-if the user +set it in `Cargo.toml` or if it also affects dependencies. + In doing so, we would need to define how `priority` interacts with different sources of `[lints]`. From 78ab70d4c078c3c4fae2d272135facb82e949418 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 27 Feb 2023 13:36:47 -0600 Subject: [PATCH 36/64] fix: Typo --- text/3389-manifest-lint.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index ab33f884428..e0721f810b8 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -260,7 +260,7 @@ Unconfigurable: [lints] clippy = [ { all = "allow" }, - { doc_markdown = "worn" }, + { doc_markdown = "warn" }, ] ``` @@ -269,7 +269,7 @@ Configurable: [[lints.clippy.all]] level = "allow" [[lints.clippy.doc_markdown]] -level = "worn" +level = "warn" ``` Where the order is based on how to pass them on the command-line. From cf93e59f1a90ba5a2e95ac55ee107241180ccf01 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 27 Feb 2023 20:49:14 -0600 Subject: [PATCH 37/64] fix: Expand on lint source future --- text/3389-manifest-lint.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index e0721f810b8..6313fcb3b92 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -320,7 +320,9 @@ Ruby Currently Rust tells you where a lint level was enabled when it emits a lint. `rustc` only sees that these lints are coming in from the command-line and doesn't know about `[lints]`. -It would be nice if it could also point to Cargo.toml for this. +It would be nice if it could also point to Cargo.toml for this. This could be +as simple as a `--lint-source=Cargo.toml` with rustc knowing just enough about +the `[lints]` table to process it directly. ## External file From e92a52b1e7e03a84277f69f34d8b76cc2e0e81d7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Mar 2023 06:28:53 -0600 Subject: [PATCH 38/64] fix: Isolate array precedence --- text/3389-manifest-lint.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 6313fcb3b92..fdd271cfa81 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -249,10 +249,17 @@ The priority field is meant to allow mimicking - `-Aclippy::all -Wclippy::doc_markdown` - `-D future-incompatible -A semicolon_in_expressions_from_macros` -We can't order lints based on the level as which we want first is dependent on the context. +We can't order lints based on the `level` as which we want first is dependent on the context (see above). We can't rely on the order of the keys in the table as that is undefined in TOML. +For the most part, people won't need granularity, so we could instead start +with a `priority: bool` field. This might get confusing to mix with numbers +though (what does `false` and `true` map to?). There is also the problem that +generally people will want to opt a specific lint into being low-priority (the +group) and the leave the exceptions at default but making `priority = true` the +default would read weird (everything is a priority but one or two items). + We could use an array instead of a table: Unconfigurable: @@ -276,13 +283,6 @@ Where the order is based on how to pass them on the command-line. Complex TOML arrays tend to be less friendly to work with including the fact that TOML 1.0 does not allow multi-line inline tables. -For the most part, people won't need granularity, so we could instead start -with a `priority: bool` field. This might get confusing to mix with numbers -though (what does `false` and `true` map to?). There is also the problem that -generally people will want to opt a specific lint into being low-priority (the -group) and the leave the exceptions at default but making `priority = true` the -default would read weird (everything is a priority but one or two items). - # Prior art [prior-art]: #prior-art From ef224ec086b97cac8a0951c84e9c1173f5387861 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 7 Mar 2023 06:33:57 -0600 Subject: [PATCH 39/64] fix: Document 'auto-priority' alternative --- text/3389-manifest-lint.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index fdd271cfa81..8e97b083c11 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -260,6 +260,17 @@ generally people will want to opt a specific lint into being low-priority (the group) and the leave the exceptions at default but making `priority = true` the default would read weird (everything is a priority but one or two items). +We could pass this to the tool and say "you figure it out" based on which is +the most specific and least specific. This requires lint groups to be +subsets or disjoint of each other to avoid ambiguity. While this might hold +today, I'm a bit cautious of putting this requirement on us forever. For +example, if we merged `cargo semver-check`, there are proposed lint groups +based on what a lint's user-overideable semver-level is which couldn't be +guarenteed to meet these requirements. On the implementation side, this will +require a new channel to communicate these lints to the tools (unless we infer +order for flags/config as well) and require them to organize their data in a +way for it to inferred. + We could use an array instead of a table: Unconfigurable: From ef4a490a77714db88699eaae3b31b76f259527b5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 18 Mar 2023 13:31:28 -0500 Subject: [PATCH 40/64] fix: Add missing namespacing of rust lints --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 8e97b083c11..33a2882c155 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -51,7 +51,7 @@ This would work with [lints] workspace = true -[workspace.lints] +[workspace.lints.rust] unsafe = "forbid" ``` From 86932bd9c88149105fc2e424caf0fe397ad2fd0f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 10:57:45 -0500 Subject: [PATCH 41/64] fix: Call out rust/rustc category confusion --- text/3389-manifest-lint.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 33a2882c155..d4070e01c87 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -144,6 +144,8 @@ Initially, the only supported tools will be: - `clippy` - `rustdoc` +A downside to naming the category `rust` is it might be confusing if we ever expose `rustc::` lints. + Addition of third-party tools would fall under their [attributes for tools](https://github.com/rust-lang/rust/issues/44690). From 5afa0cf0c5ff317b0154d42beeaf84c36d99c975 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 11:04:30 -0500 Subject: [PATCH 42/64] fix: Be more explicit in how the lints table is loaded --- text/3389-manifest-lint.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index d4070e01c87..32ef552ad21 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -130,11 +130,16 @@ package override the workspace on a lint-by-lint basis. cargo will contain a mapping of tool to underlying command (e.g. `rust` to `rustc`, `clippy` to `rustc` when clippy is the driver, `rustdoc` to `rustdoc`). When running the underlying command for the specified package, -cargo will transform the lints from `lint = level` to `--level lint`, sort them -by priority and then lint name, and pass them on the command line before other -configuration, `RUSTFLAGS`, allowing user configuration to override package -configuration. These flags will be fingerprinted so changing them will cause a -rebuild only for the commands where they are used. +cargo will: +1. Transform the lints from `tool.lint = level` to `--level tool::lint` + - Leaving off the `tool::` when it is `rust` + - cargo will error if `lint` contains `::` as the first part is assumed to be + a tool and it should be listed in that tool's table +2. Sort them by priority and then lint name +3. Pass them on the command line before other configuration like +`RUSTFLAGS`, allowing user configuration to override package configuration. + - These flags will be fingerprinted so changing them will cause a rebuild only + for the commands where they are used. Note that this means that `[lints]` does not affect dependencies which is normally not an issue due to `--cap-lints` being used for dependencies. From a54d985c9c9854e439953be612f233a39449daff Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 11:07:38 -0500 Subject: [PATCH 43/64] fix: Clarified this isn't limited to rustc/clippy --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 32ef552ad21..7c9aed596a1 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -43,7 +43,7 @@ A new `lints` table would be added to configure lints: [lints.rust] unsafe = "forbid" ``` -and `cargo` would pass these along as flags to `rustc` and `clippy`. +and `cargo` would pass these along as flags to `rustc`, `clippy`, or other lint tools. This would work with [RFC 2906 `workspace-deduplicate`](https://rust-lang.github.io/rfcs/2906-cargo-workspace-deduplicate.html?highlight=2906#): From de4405825d3fa522971e50e487c7847c36280052 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 11:10:52 -0500 Subject: [PATCH 44/64] fix: Explicitly call out why rust table exists' --- text/3389-manifest-lint.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 7c9aed596a1..ba96aa59e05 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -149,7 +149,10 @@ Initially, the only supported tools will be: - `clippy` - `rustdoc` -A downside to naming the category `rust` is it might be confusing if we ever expose `rustc::` lints. +The reason for `rust` existing, despite lints not being prefixed with `rust::`, is +to avoid ambiguity in the data model between `lint.` and +`lint..`. A downside to naming the tool `rust` is it might be +confusing if we ever expose `rustc::` lints. Addition of third-party tools would fall under their [attributes for tools](https://github.com/rust-lang/rust/issues/44690). From 155776772dbcf834c93c5d752f5eae381430d831 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 11:27:16 -0500 Subject: [PATCH 45/64] fix: Clarify I meant lint levels, not general lint configuration --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index ba96aa59e05..2e1bc82d9d8 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -169,7 +169,7 @@ dependencies. This may cause some user confusion. There has been some user/IDE confusion about running commands like `rustfmt` directly and expecting them to pick up configuration only associated with their higher-level cargo-plugins despite that configuration (like `package.edition`) -being cargo-specific. By baking the configuration for rustc, rustdoc, and +being cargo-specific. By baking the configured lint levels for rustc, rustdoc, and clippy directly into cargo, we will be seeing more of this. A hope is that this will actually improve with this RFC. Over time, tools will need to switch to the model of running `cargo` to get configuration in response to this RFC. From e1230cdb8c3fbef81ad8fc7f6a9f4f30a1bcdac6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 11:32:03 -0500 Subject: [PATCH 46/64] fix: Clarify clippy.toml isn't going away yet --- text/3389-manifest-lint.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 2e1bc82d9d8..d4451cf3c05 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -180,7 +180,8 @@ external configuration file and overlooking the one or two fields read from As this focuses on lints, this leaves out first-party tools that need configuration but aren't linters, namely `rustfmt`, leading to an inconsistent -experience if `clippy.toml` goes away. +experience if `clippy.toml` goes away in the future (if we act on the future +possibility of supporting linter configuration) A concern brought up in [rust-lang/rust-clippy#1313](https://github.com/rust-lang/rust-clippy/issues/1313) From 2f5f873847d3f5630e71d3afd25ddb9d11ff022b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 11:35:59 -0500 Subject: [PATCH 47/64] fix: Add clarification that an example is only an example --- text/3389-manifest-lint.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index d4451cf3c05..90216a8937f 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -378,6 +378,11 @@ avoid-breaking-exported-api = true Tools will need `cargo metadata` to report the `lints` table so they can read it without re-implementing workspace inheritance. +**Note:** At this time, there is no lint configuration for clippy, just tool +configuration. `lints.clippy.cyclomatic_complexity` exists for illustrative +purposes of what linters could support and is not indicative of any future +plans for clippy itself. + ## Packages overriding inherited lints Currently, it is a hard error to mix `workspace = true` and lints. We could From 354396739f69e10a01916d620990dc5ae6e1c57e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 24 Mar 2023 11:40:14 -0500 Subject: [PATCH 48/64] fix: Be explicit that lint configuration is a future possibility --- text/3389-manifest-lint.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 90216a8937f..7fb1c542c0c 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -388,8 +388,9 @@ plans for clippy itself. Currently, it is a hard error to mix `workspace = true` and lints. We could open this up in the future for the package to override lints from the workspace. This would not be a breaking change as we'd be changing an error -case into a working case. We'd need to ensure we had a path forward for the -semantics for configurable lints. +case into a working case. We should consider the possibility of adding +configurable lints in the future and what that would look like with +overridin of lints. ## Extending the syntax to `.cargo/config.toml` From 45766c486f4b406393fd0874aa86d748bf91942d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 29 Mar 2023 13:01:24 -0500 Subject: [PATCH 49/64] fix: Update now that we have confirmation on ruff's design choice --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 7fb1c542c0c..ecab32e66e7 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -317,7 +317,7 @@ Python - [pylint](https://github.com/PyCQA/pylint/blob/main/examples/pylintrc#L402) - Format is `level = [lint, ...]` but the [config file is a reflection of the CLI](https://pylint.pycqa.org/en/latest/user_guide/configuration/index.html) - [ruff](https://beta.ruff.rs/docs/configuration/) - - Format is `level = [lint, ...]`, likely due to past precedence in ecosystem (see above) + - Format is `level = [lint, ...]`, due to past precedence in ecosystem (see above) Javascript - [eslint](https://eslint.org/docs/latest/use/configure/rules) From fbf6f48e809f0af49ebdbfa20a48527147c5d31b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 30 Mar 2023 13:44:30 -0500 Subject: [PATCH 50/64] fix: Expand on why not `::` but separate tables --- text/3389-manifest-lint.md | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index ecab32e66e7..971429615bf 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -205,9 +205,13 @@ and other fields that are more workspace related. Instead, we used Instead of `.`, we could use `::` (e.g. `"clipp::enum_glob_use"` instead of `clippy.enum_glob_use`), like in the -diagnostic messages. This would make it easier to copy/paste lint names but it -will requiring quoting the keys and is more difficult to add tool-level -configuration in the future. +diagnostic messages. This would make it easier to copy/paste lint names. +However, with the schema being ` = `, this would require quoting +the keys rather than leaving them as bare words. This would also cause +problems for tool-level configuration. The first is that the lints wouldn't be +easily grouped with their config. The second is if we use ` = `, +we'd be mixing tool names in with lints, making it easier to conflict and +harder to collect all lints to forward to a tool. We could possibly extend this new field to `rustfmt` by shifting the focus from "lints" to "rules" (see From 935593f67f36d9d357cf86b1c48eddb0c3e98d95 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 30 Mar 2023 13:49:35 -0500 Subject: [PATCH 51/64] fix: Expand more on why not level=lint --- text/3389-manifest-lint.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 971429615bf..c5388af77f1 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -240,9 +240,18 @@ We could support platform or feature specific settings, like with Instead of the `[lints]` table being `lint = "level"`, we could organize it around `level = ["lint", ...]` like some other linters do (like -[ruff](https://beta.ruff.rs/docs/configuration/)) but this works better for -logically organizing lints, highlighting what changed in diffs, and for -possibly adding lint-specific configuration in the future. +[ruff](https://beta.ruff.rs/docs/configuration/)). This is less verbose. We +chose `lint` being the keys because +- Most reasons for organizing around `level` are preference from which ecosystem they came from (Python vs JS) +- Organizing around `lint` makes it harder to lose track of what section you + are in for large lint lists (which exist), people do with dependency tables + today +- TOML isn't great for nesting complex data structures and we have allow a + `priority` field and might allo other lint-level configuration. Organizing + around `lint` makes this cleaner. +- If we add support for packages overiding inherited workspace lints, it likely + better maps to a users model to just say the package lint table gets merged + into the workspace lint table. This is also easier to implement correctly. ## Workspace Inheritance From c3f932cc18f4b909c7a9b207362760a5bae08bb8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 31 Mar 2023 00:30:05 -0500 Subject: [PATCH 52/64] fix: Remove confusion over :: and tool-config --- text/3389-manifest-lint.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index c5388af77f1..c340e9e1940 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -207,11 +207,12 @@ Instead of `.`, we could use `::` (e.g. `"clipp::enum_glob_use"` instead of `clippy.enum_glob_use`), like in the diagnostic messages. This would make it easier to copy/paste lint names. However, with the schema being ` = `, this would require quoting -the keys rather than leaving them as bare words. This would also cause +the keys rather than leaving them as bare words. This might also cause problems for tool-level configuration. The first is that the lints wouldn't be -easily grouped with their config. The second is if we use ` = `, -we'd be mixing tool names in with lints, making it easier to conflict and -harder to collect all lints to forward to a tool. +easily grouped with their tool's config. The second is if we use `lints. = ` +and tool-level configuration goes under `lints.`, +then keys under `[lints]` are ambiguous as to whether they are a tool or a +lint, making it harder to collect all lints to forward tools. We could possibly extend this new field to `rustfmt` by shifting the focus from "lints" to "rules" (see From e73e6b9c572cc66546814658c4b2eea601c0f269 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 10 Apr 2023 11:21:30 -0500 Subject: [PATCH 53/64] fix: Add auto-sort as a future possibility --- text/3389-manifest-lint.md | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index c340e9e1940..47f2d3ba620 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -135,7 +135,8 @@ cargo will: - Leaving off the `tool::` when it is `rust` - cargo will error if `lint` contains `::` as the first part is assumed to be a tool and it should be listed in that tool's table -2. Sort them by priority and then lint name +2. Sort them by priority and then an unspecified order within priority that we may change in the [future](#future-possibilities). + - On initial release, the sort will likely be reverse alphabetical which would cause `all` to be last, making it unlikely to do what the user wants which would raise awareness of the need for setting `priority` for all groups. 3. Pass them on the command line before other configuration like `RUSTFLAGS`, allowing user configuration to override package configuration. - These flags will be fingerprinted so changing them will cause a rebuild only @@ -351,6 +352,32 @@ Ruby # Future possibilities [future-possibilities]: #future-possibilities +## Help the user with `priority` + +When running linters through cargo, we could warn the user when there is ambiguity, including +- A group and a lint at the same priority +- A group that is a superset of another group at the same priority +- Two intersecting groups at the same priority +- A lint or group that is masked by a group in a later priority + +We could then take this a step further and change the way we sort within a +priority level to put the most specific entry last, where ambiguity doesn't +exist. This would nearly eliminate the need for specifying `priority` with the +current groups. + +We specifically recommend warning, rather than error, so groups can evolve to +become intersecting without it being a breaking change. + +To implement this, either cargo needs to pass the lints down to the tool in a +way to communicate the priority batches, allow cargo to query the group +memberships from the linter, or we hard code this at compile-time like +rust-analyzer +([lints](https://rust-lang.github.io/rust-clippy/master/lints.json), +[generate](https://github.com/rust-lang/rust-analyzer/blob/a6464392c15fa8788215d669c4c0b1e46bcadeea/crates/ide-db/src/tests/sourcegen_lints.rs)). +One thing to keep in mind is the potential for [custom +tools](https://rust-lang.github.io/rfcs/2103-tool-attributes.html) in the +future. + ## rustc reporting `Cargo.toml` as lint-level source Currently Rust tells you where a lint level was enabled when it emits a lint. From d32801b23eb56fdab668c7fdc414766f67620286 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 10 Apr 2023 11:33:33 -0500 Subject: [PATCH 54/64] fix: Add high-level guidance --- text/3389-manifest-lint.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 47f2d3ba620..a2a0ee95a01 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -196,7 +196,17 @@ This does not allow sharing lints across workspaces. # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives -This could be left to `clippy.toml` but that leaves `rustc` without a solution. +When designing this, we wanted to keep in mind how things work today, including +- `clippy` defines all configuration as linter/tool config and not lint config (linter/lint config is a future possibility) +- All `clippy` lints are disjoint +- `rustdoc` has no plans for groups outside of `all` +- `rustc` today has some intersecting groups + +However, we also need to consider how decisions might limit us in the future and whether we want to bind future decisions with this RFC, including +- Whether existing decisions will be revisited +- When new tools are added, like `cargo` and `cargo-semver-check`, which haven't had lint levels and configuration long enough (or at all) to explore their problem and design space. + +This could be left to `clippy.toml` but that leaves `rustc`, `rustdoc`, and future linters without a solution. `[lints]` could be `[package.lints]`, tying it to the package unlike `[patch]` and other fields that are more workspace related. Instead, we used From 330782a2c2b6547a35e54af08d5c7a256d11f9de Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 10 Apr 2023 11:35:56 -0500 Subject: [PATCH 55/64] refactor: Break up Rationale / Alts into smaller sections --- text/3389-manifest-lint.md | 104 ++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 47 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index a2a0ee95a01..396432897cf 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -206,6 +206,8 @@ However, we also need to consider how decisions might limit us in the future and - Whether existing decisions will be revisited - When new tools are added, like `cargo` and `cargo-semver-check`, which haven't had lint levels and configuration long enough (or at all) to explore their problem and design space. +## Misc + This could be left to `clippy.toml` but that leaves `rustc`, `rustdoc`, and future linters without a solution. `[lints]` could be `[package.lints]`, tying it to the package unlike `[patch]` @@ -214,41 +216,7 @@ and other fields that are more workspace related. Instead, we used `[lints]` could be `[lint]` but we decided to follow the precedence of `[dependencies]`. -Instead of `.`, we could use `::` (e.g. -`"clipp::enum_glob_use"` instead of `clippy.enum_glob_use`), like in the -diagnostic messages. This would make it easier to copy/paste lint names. -However, with the schema being ` = `, this would require quoting -the keys rather than leaving them as bare words. This might also cause -problems for tool-level configuration. The first is that the lints wouldn't be -easily grouped with their tool's config. The second is if we use `lints. = ` -and tool-level configuration goes under `lints.`, -then keys under `[lints]` are ambiguous as to whether they are a tool or a -lint, making it harder to collect all lints to forward tools. - -We could possibly extend this new field to `rustfmt` by shifting the focus from -"lints" to "rules" (see -[eslint](https://eslint.org/docs/latest/use/configure/rules)). However, the -more we generalize this field, the fewer assumptions we can make about it. On -one extreme is `package.metadata` which is so free-form we can't support it -with workspace inheritance. A less extreme example is if we make the -configuration too general, we would preclude the option of supporting -per-package overrides as we wouldn't know enough about the shape of the data to -know how to merge it. There is likely a middle ground that we could make work -but it would take time and experimentation to figure that out which is at odds -with trying to maintain a stable file format. Another problem with `rules` is -that it is divorced from any context. In eslint, it is in an eslint-specific -config file but a `[rules]` table is not a clear as a `[lints]` table as to -what role it fulfills. - -We could support platform or feature specific settings, like with -`[lints.]` or `[target..lints]` but -- There isn't a defined use case for this yet besides having support for `cfg(feature = "clippy")` or - which does not seem high enough priority to design - around. -- `[lints.]` runs into ambiguity issues around what is a `` - entry vs a `` entry in the `[lints]` table. -- We have not yet defined semantics for sharing something like this across a - workspace +## Schema Instead of the `[lints]` table being `lint = "level"`, we could organize it around `level = ["lint", ...]` like some other linters do (like @@ -265,19 +233,18 @@ chose `lint` being the keys because better maps to a users model to just say the package lint table gets merged into the workspace lint table. This is also easier to implement correctly. -## Workspace Inheritance +## Linter Tables vs Linter Namespaces -Instead of using workspace inheritance for `[lint]`, we could make it -workspace-level configuration, like `[patch]` which is automatically applied to -all workspace members. However, `[patch]` and friends are because they affect -the resolver / `Cargo.toml` and so they can only operate at the workspace -level. `[lints]` is more like `[dependencies]` in being something that applies -at the package level but we want shared across workspaces. - -Instead of traditional workspace inheritance where there is a single value to -inherit with `workspace = true`, we could have `[workspace.lints.]` -which defines presets and the user could do `lints. = true`. The user -could then name them as they wish to avoid collision with rustc lints. +Instead of `.`, we could use `::` (e.g. +`"clipp::enum_glob_use"` instead of `clippy.enum_glob_use`), like in the +diagnostic messages. This would make it easier to copy/paste lint names. +However, with the schema being ` = `, this would require quoting +the keys rather than leaving them as bare words. This might also cause +problems for tool-level configuration. The first is that the lints wouldn't be +easily grouped with their tool's config. The second is if we use `lints. = ` +and tool-level configuration goes under `lints.`, +then keys under `[lints]` are ambiguous as to whether they are a tool or a +lint, making it harder to collect all lints to forward tools. ## Lint Precedence @@ -330,6 +297,49 @@ Where the order is based on how to pass them on the command-line. Complex TOML arrays tend to be less friendly to work with including the fact that TOML 1.0 does not allow multi-line inline tables. +## Workspace Inheritance + +Instead of using workspace inheritance for `[lint]`, we could make it +workspace-level configuration, like `[patch]` which is automatically applied to +all workspace members. However, `[patch]` and friends are because they affect +the resolver / `Cargo.toml` and so they can only operate at the workspace +level. `[lints]` is more like `[dependencies]` in being something that applies +at the package level but we want shared across workspaces. + +Instead of traditional workspace inheritance where there is a single value to +inherit with `workspace = true`, we could have `[workspace.lints.]` +which defines presets and the user could do `lints. = true`. The user +could then name them as they wish to avoid collision with rustc lints. + +## `rustfmt` + +We could possibly extend this new field to `rustfmt` by shifting the focus from +"lints" to "rules" (see +[eslint](https://eslint.org/docs/latest/use/configure/rules)). However, the +more we generalize this field, the fewer assumptions we can make about it. On +one extreme is `package.metadata` which is so free-form we can't support it +with workspace inheritance. A less extreme example is if we make the +configuration too general, we would preclude the option of supporting +per-package overrides as we wouldn't know enough about the shape of the data to +know how to merge it. There is likely a middle ground that we could make work +but it would take time and experimentation to figure that out which is at odds +with trying to maintain a stable file format. Another problem with `rules` is +that it is divorced from any context. In eslint, it is in an eslint-specific +config file but a `[rules]` table is not a clear as a `[lints]` table as to +what role it fulfills. + +## Target-specific lint + +We could support platform or feature specific settings, like with +`[lints.]` or `[target..lints]` but +- There isn't a defined use case for this yet besides having support for `cfg(feature = "clippy")` or + which does not seem high enough priority to design + around. +- `[lints.]` runs into ambiguity issues around what is a `` + entry vs a `` entry in the `[lints]` table. +- We have not yet defined semantics for sharing something like this across a + workspace + # Prior art [prior-art]: #prior-art From 9cbc977f3088c08fd0a01f831d93c2254e9eb4a6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 10 Apr 2023 11:51:33 -0500 Subject: [PATCH 56/64] fix: Rewrite :: section --- text/3389-manifest-lint.md | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 396432897cf..1fc3e1f1dbb 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -235,16 +235,18 @@ chose `lint` being the keys because ## Linter Tables vs Linter Namespaces -Instead of `.`, we could use `::` (e.g. -`"clipp::enum_glob_use"` instead of `clippy.enum_glob_use`), like in the -diagnostic messages. This would make it easier to copy/paste lint names. -However, with the schema being ` = `, this would require quoting -the keys rather than leaving them as bare words. This might also cause -problems for tool-level configuration. The first is that the lints wouldn't be -easily grouped with their tool's config. The second is if we use `lints. = ` -and tool-level configuration goes under `lints.`, -then keys under `[lints]` are ambiguous as to whether they are a tool or a -lint, making it harder to collect all lints to forward tools. +We started off with lints being referenced with their tool as a namespace (e.g. +`"clipp::enum_glob_use"`) like in diagnostic messages, making copy/paste easy. + +However, we switched to a more hierarchical data model (e.g. +`clippy.enum_glob_use`) to avoid quoting keys with the `lints. = ` schema. + +If we add lint/linter config in the future +- Being more hierarchical means lint and linter config are kept closer to each + other, making it easier to evaluate their impact on each other. +- `lints. = ` combined with `lints..metadata` makes it + harder for cargo to collect all the lints to pass down into the compiler + driver. ## Lint Precedence From 2f1b7994c52022a5f58d189829698abf36b748ac Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 10 Apr 2023 11:51:55 -0500 Subject: [PATCH 57/64] fix: Typos --- text/3389-manifest-lint.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 1fc3e1f1dbb..23ac0031258 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -229,7 +229,7 @@ chose `lint` being the keys because - TOML isn't great for nesting complex data structures and we have allow a `priority` field and might allo other lint-level configuration. Organizing around `lint` makes this cleaner. -- If we add support for packages overiding inherited workspace lints, it likely +- If we add support for packages overriding inherited workspace lints, it likely better maps to a users model to just say the package lint table gets merged into the workspace lint table. This is also easier to implement correctly. @@ -271,7 +271,7 @@ subsets or disjoint of each other to avoid ambiguity. While this might hold today, I'm a bit cautious of putting this requirement on us forever. For example, if we merged `cargo semver-check`, there are proposed lint groups based on what a lint's user-overideable semver-level is which couldn't be -guarenteed to meet these requirements. On the implementation side, this will +guaranteed to meet these requirements. On the implementation side, this will require a new channel to communicate these lints to the tools (unless we infer order for flags/config as well) and require them to organize their data in a way for it to inferred. From 660bcdbcebab24712e3ae3a758b318906bf9dee5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 10 Apr 2023 12:14:56 -0500 Subject: [PATCH 58/64] fix: Expand the schema section --- text/3389-manifest-lint.md | 67 ++++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 14 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 23ac0031258..901e77889d3 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -218,20 +218,59 @@ and other fields that are more workspace related. Instead, we used ## Schema -Instead of the `[lints]` table being `lint = "level"`, we could organize -it around `level = ["lint", ...]` like some other linters do (like -[ruff](https://beta.ruff.rs/docs/configuration/)). This is less verbose. We -chose `lint` being the keys because -- Most reasons for organizing around `level` are preference from which ecosystem they came from (Python vs JS) -- Organizing around `lint` makes it harder to lose track of what section you - are in for large lint lists (which exist), people do with dependency tables - today -- TOML isn't great for nesting complex data structures and we have allow a - `priority` field and might allo other lint-level configuration. Organizing - around `lint` makes this cleaner. -- If we add support for packages overriding inherited workspace lints, it likely - better maps to a users model to just say the package lint table gets merged - into the workspace lint table. This is also easier to implement correctly. +In evaluating prior art, we saw two major styles for configuring lint levels: + +Python-style: +```toml +[lints] +warn = [ + { lint = "rust_2018_idioms", priority = -1 }, + { lint = "clippy::all", priority = -1 }, + "clippy::await_holding_lock", + "clippy::char_lit_as_u8", + "clippy::checked_conversions", +] +deny = [ + "unsafe_code", +] +``` + +Inspired by ESLint-style: +```toml +[lints.rust] +rust_2018_idioms = { level = "warn", priority = -1} + +unsafe_code = "deny" + +[lints.clippy] +all = { level = "warn", priority = -1 } + +await_holding_lock = "warn" +char_lit_as_u8 = "warn" +checked_conversions = "warn" +``` +- More akin to `eslint` + +In a lot of areas, which to choose comes down to personal preference and what people are comfortable with: +- If you want to lookup everything for a level, Python-style works better +- If you want to look up the level for a lint, ESLint-style works better +- Python-style is more succinct +- Python-style has fewer jagged edges + +We ended up favoring more of the ESLint-style because: +- ESLint-style offers more syntax choices for lint config (inline tables, + standard tables, dotted keys). In general, the TOML experience for deeply + nested inline structures is not great. + - Right now, the only other lint field beside `level` is `priority`. In the future we may add lint configuration. While we shouldn't exclusively design for this possibility, all things being equal, we shouldn't make that potential future's experience worse +- ESLInt-style makes it easier to visually highlight groups and the lints related to those groups +- The cargo team has seen support issues that partially arise from a user + losing track of which `dependencies` table they are in because the list of + dependencies is large enough to have the header far enough away (or off + screen). This can similarly happen with Python-style as the context of the + level is in the table header. See [EmbarkStudios's lint list as an example of where this could happen](https://github.com/EmbarkStudios/rust-ecosystem/blob/81d62539a57add13f4b0f1c503e267b6de358f70/lints.toml) +- If we add support for packages to override some of the lints inherited from + the workspace, it is easier for users to map out this relationship with + ESLint-style. ## Linter Tables vs Linter Namespaces From 28f14ef7ca8b8b890bb135de134523754234f283 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Mon, 10 Apr 2023 15:09:59 -0500 Subject: [PATCH 59/64] fix: Expand on precedence options --- text/3389-manifest-lint.md | 124 +++++++++++++++++++++++++++---------- 1 file changed, 90 insertions(+), 34 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 901e77889d3..ec9674dae9c 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -289,54 +289,110 @@ If we add lint/linter config in the future ## Lint Precedence -The priority field is meant to allow mimicking +Currently, `rustc` allows lints to be controlled on the command-line with the +last level for a lint winning. They may also be specified as attributes with +the last instance winning. `cargo` adds the `RUSTFLAGS` environment variable +and `config.toml` entry. On top of this, there are lint groups that act as +aliases to sets of lints. These groups may be disjoint, supersets, or they may +even intersect. + +Example `RUSTFLAGS`: - `-Aclippy::all -Wclippy::doc_markdown` -- `-D future-incompatible -A semicolon_in_expressions_from_macros` +- `-Dfuture-incompatible -Asemicolon_in_expressions_from_macros` -We can't order lints based on the `level` as which we want first is dependent on the context (see above). +In providing lint-level configuration in `Cargo.toml`, users will need to be +able to set the lint level for group and then override individual lints within +that group while interacting with the existing `RUSTFLAGS` system. -We can't rely on the order of the keys in the table as that is undefined in TOML. +We have chosen **Option 6** with `priority` being in-scope for this RFC and +warnings and auto-sorting as a future possibility. -For the most part, people won't need granularity, so we could instead start -with a `priority: bool` field. This might get confusing to mix with numbers -though (what does `false` and `true` map to?). There is also the problem that -generally people will want to opt a specific lint into being low-priority (the -group) and the leave the exceptions at default but making `priority = true` the -default would read weird (everything is a priority but one or two items). - -We could pass this to the tool and say "you figure it out" based on which is -the most specific and least specific. This requires lint groups to be -subsets or disjoint of each other to avoid ambiguity. While this might hold -today, I'm a bit cautious of putting this requirement on us forever. For -example, if we merged `cargo semver-check`, there are proposed lint groups -based on what a lint's user-overideable semver-level is which couldn't be -guaranteed to meet these requirements. On the implementation side, this will -require a new channel to communicate these lints to the tools (unless we infer -order for flags/config as well) and require them to organize their data in a -way for it to inferred. +**Option 1: Auto-sort** +```rust +[lints.rust] +unsafe_code = "deny" +allow_dead_code = "allow" +all = "warn" +``` +- Unable to handle if two intersecting groups are assigned different levels -We could use an array instead of a table: +**Option 2: Ordered keys** +```rust +[lints.rust] +all = "warn" +allow_dead_code = "allow" +unsafe_code = "deny" +``` +- Relies on the order of keys in a TOML table which is undefined +- Without standard ordering semantics, like with `[]`, users or formatters + might naively reformat the table which would affect the semantics -Unconfigurable: +**Option 3: Array of tables** ```toml +# inline table [lints] -clippy = [ - { all = "allow" }, - { doc_markdown = "warn" }, +rust = [ + { lint = "all", level = "warn" }, + { lint = "allow_dead_code", level = "allow" }, + { lint = "unsafe_code", level = "deny" }, ] +# standard table +[[lints.clippy]] +lint = "all" +level = "warn" +[[lints.clippy]] +lint = "cyclomatic_complexity" +level = "allow" ``` +- The syntax for this seems overly verbose +- Complex, nested structures aren't the easiest to work with in TOML -Configurable: +**Option 4: Compact array of tables** ```toml -[[lints.clippy.all]] -level = "allow" -[[lints.clippy.doc_markdown]] -level = "warn" +[tools.rust] +lints = [ + { warn = "all" }, + { allow = "allow_dead_code" }, + { deny = "unsafe_code" }, +] +``` +- *Note:* `lints.rust = []` wasn't used as that won't work with linter configuration in the future +- *Note:* Top-level table was changed to avoid `lints.rust.lints` redundancy and would allow us to open this up to more tools in the future +- *Note:* ` = ` (instead of the other way) to keep the keys finite so we can add more fields in the future +- Mirrors the familiar `RUSTFLAGS` syntax +- Complex, nested structures aren't the easiest to work with in TOML + +**Option 5: `priority` field** +```rust +[lints.rust] +all = { level = "warn", priority = -1 } +allow_dead_code = "allow" +unsafe_code = "deny" ``` -Where the order is based on how to pass them on the command-line. +- Difficult for the user to figure out there is a problem or how to address it -Complex TOML arrays tend to be less friendly to work with including the fact -that TOML 1.0 does not allow multi-line inline tables. +**Option 6: `priority` field with warnings and maybe auto-sort** +```rust +[lints.rust] +all = "warn" +allow_dead_code = "allow" +unsafe_code = "deny" +``` +- Option 1 (auto-sort) but using Option 5 (`priority` field) to break ties +- Produces warnings to tell the user when `priority` may be needed +- As `priority` is a low-level subset, we can start with that as an MVP. Later, we can add warnings for all the ambiguity cases. As we gain confidence in this, we can then add auto-sorting. + +**Option 7: Explicit groups** +```rust +[lints.rust.groups] +all = "warn" +[lints.rust.lints] +allow_dead_code = "allow" +unsafe_code = "deny" +``` +- Hard codes knowledge of `all` +- Does not solve the intersecting group problem +- Names aren't validated as being from a group without duplicating the work needed for Option 1 (auto-sort) ## Workspace Inheritance From 51f498406b5f854052af04061bb9767c807cf9f2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 18 Apr 2023 13:35:29 -0500 Subject: [PATCH 60/64] fix: Move some discussion to stablization --- text/3389-manifest-lint.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index ec9674dae9c..37bcc50ecea 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -466,6 +466,11 @@ Ruby # Unresolved questions [unresolved-questions]: #unresolved-questions +Blocking for stablization +- Are we still comfortable with our schema choice? +- Are we still comfortable with our precedence choice? +- Can we fingerprint only the lints for the tool being run? + # Future possibilities [future-possibilities]: #future-possibilities From 2b10a5ee1702cf837a994e85733f24a413f54bb0 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 18 Apr 2023 20:11:00 -0500 Subject: [PATCH 61/64] fix: Be more precise when talking about disjoint groups --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 37bcc50ecea..b0a012cd28a 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -198,7 +198,7 @@ This does not allow sharing lints across workspaces. When designing this, we wanted to keep in mind how things work today, including - `clippy` defines all configuration as linter/tool config and not lint config (linter/lint config is a future possibility) -- All `clippy` lints are disjoint +- All `clippy` lint groups are disjoint - `rustdoc` has no plans for groups outside of `all` - `rustc` today has some intersecting groups From 4cb8421dfecf05766fb5439dbe1a8e0e8d42254b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 27 Apr 2023 16:41:03 -0500 Subject: [PATCH 62/64] fix: Clarify dependency situation --- text/3389-manifest-lint.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index b0a012cd28a..7276f2055f5 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -142,8 +142,12 @@ cargo will: - These flags will be fingerprinted so changing them will cause a rebuild only for the commands where they are used. -Note that this means that `[lints]` does not affect dependencies which is -normally not an issue due to `--cap-lints` being used for dependencies. +Note that this means that `[lints]` is only applied to the package where its +defined and not to its dependencies, local or not. This avoids having to unify +`[lints]` tables across local packages. Normally, lints for non-local +dependencies won't be shown anyways because of `--cap-lints`. As for local +dependencies, they will likely have their own `[lints]` table, most the same +one, inherited from the workspace. Initially, the only supported tools will be: - `rust` From a47520f231b665d1947bb38dca1e5cb04b01c3b9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 27 Apr 2023 16:45:37 -0500 Subject: [PATCH 63/64] fix: Call out reducing rebuilds for filtering lints --- text/3389-manifest-lint.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 7276f2055f5..4311af483a8 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -140,7 +140,9 @@ cargo will: 3. Pass them on the command line before other configuration like `RUSTFLAGS`, allowing user configuration to override package configuration. - These flags will be fingerprinted so changing them will cause a rebuild only - for the commands where they are used. + for the commands where they are used. By only including the lints for the + command in question, we reduce what is fingerprinted, reducing what gets + rebuilt when `[lints]` is changed. Note that this means that `[lints]` is only applied to the package where its defined and not to its dependencies, local or not. This avoids having to unify From 7aab0bd950dcdef908b6652e5433f40831290242 Mon Sep 17 00:00:00 2001 From: Eric Huss Date: Tue, 9 May 2023 08:16:39 -0700 Subject: [PATCH 64/64] Add tracking issue. --- text/3389-manifest-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/3389-manifest-lint.md b/text/3389-manifest-lint.md index 4311af483a8..d00df37dc65 100644 --- a/text/3389-manifest-lint.md +++ b/text/3389-manifest-lint.md @@ -1,7 +1,7 @@ - Feature Name: `manifest-lint` - Start Date: 2023-02-14 - RFC PR: [rust-lang/rfcs#3389](https://github.com/rust-lang/rfcs/pull/3389) -- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) +- Tracking Issue: [rust-lang/cargo#12115](https://github.com/rust-lang/cargo/issues/12115) # Summary [summary]: #summary