Add a [lints] table to Cargo.toml
#3389
Conversation
epage
added
the
T-cargo
|
Thank you for creating the RFC. I think this would be interesting for everyone in Clippy as well. Adding an option to configure lints has been requested several times :) cc: @rust-lang/clippy :) |
| 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` |
There was a problem hiding this comment.
I actually think this is probably necessary for this RFC to make sense. I feel this makes more sense in the local (potentially-but-not-necessarily checked-in) config as opposed to the crate config.
There was a problem hiding this comment.
Though with workspace inheritance perhaps it's not that necessary.
There was a problem hiding this comment.
Could you clarify why you feel it makes more sense to put this in .cargo/config.toml over Cargo.toml?
In starting this with Cargo.toml, I'm looking to address the existing problems with doing this via rustflags in .cargo/config.toml (project configuration being directory dependent). EmbarkStudios/rust-ecosystem#59 is a good example of people using both source code and .cargo/config.toml for lints that are project-specific and would benefit from this. This was also spurred by the cargo team itself wanting to use this feature.
There was a problem hiding this comment.
I think it makes sense to allow it in both.
The reason being that there are multiple lint workflows: sometimes you lint in CI and want everyone to follow the same lint set. However, some people may wish to opt in to additional lints locally for their own code, and being able to set that is useful.
But I'm fine with starting out in Cargo.toml at least. I would prefer to try and solve it for both.
There was a problem hiding this comment.
The reason being that there are multiple lint workflows: sometimes you lint in CI and want everyone to follow the same lint set. However, some people may wish to opt in to additional lints locally for their own code, and being able to set that is useful.
The part I'm missing is why users can't just use rustflags for this in the short term? In the long term, if we add configurable lints, there isn't an alternative and I can see it becoming a higher priority then. This is the main reason I put it as a future possibility, rather than including it.
There was a problem hiding this comment.
When you are cross-compiling, there is no way to set rustflags via .config/cargo.toml that applies to everything (built for host or built for target), so even just for having something that works across proc-macros and normal crates within a workspace, this would be great.
There was a problem hiding this comment.
We will still be fingerprinting [lints] and causing rebuilds
Why? Lints don't affect the compiled binaries. It seems like pure overhead to me.
While I'd want to be able to set lints globally (for every project on my computer), there is a design issue of config priority which is unclear to me. Currently afaik the only option duplicated in both config.toml and Cargo.toml is [profile], and in its case the config.toml has priority. In particular, a [profile] section in $HOME/.cargo/config.toml would override the [profile] in any other Cargo project.
That may be reasonable for [profile], but would likely be incorrect for lints. Most of the time I'd want to use the lints from the specific project I work on, with the config.toml defaults (particularly defined in non-project directories) used only occasionally. So the correct priority looks like $WORKSPACE/.cargo/config.toml > $WORKSPACE/subcrate/Cargo.toml > $WORKSPACE/Cargo.toml > $UPPER_DIR/.cargo/config.toml. This looks a bit convoluted and inconsistent with the behaviour of other config.toml keys.
Maybe there should be a cargo command-line flag which would control whether Cargo.toml or config.toml lint settings should be used?
Actually, if cargo build/run/test would have flags to set lint levels per invocation, that would be sufficient. That would also avoid the question of config priorities.
There was a problem hiding this comment.
Why? Lints don't affect the compiled binaries. It seems like pure overhead to me.
If a user adds additional lints and run cargo check, they would expect to see the result of those lints. This would only be possible by rebuilding which will require finger printing.
While I'd want to be able to set lints globally (for every project on my computer), there is a design issue of config priority which is unclear to me.
For now, I am leaving [lints] for .cargo/config.toml as a future possibility rather than removing it or making it required to move this proposal along. Keeping it as a future possibility has no cost. Adding it into the proposal has a larger cost, in design, decision making, and implementation. It would require a very strong motivation to block incremental improvement ([lints] in Cargo.toml) on getting a full solution (e.g. including [lints] in .cargo/config.toml).
With that context, maybe we can temper how much design work we put into the feature or how much we debate whether we should or shouldn't do it in the future.
There was a problem hiding this comment.
Sure, I wouldn't want future extensions to block this long-waited feature either.
When you say "rebuilding", do you mean rebuilding the workspace crates or all dependencies? I am most concerned about the latter, even though for large projects there could be no difference. I am not familiar with the inner details of cargo check.
There was a problem hiding this comment.
When you say "rebuilding", do you mean rebuilding the workspace crates or all dependencies?
Rebuilding the package when its [lints] table changes and not its dependencies.
When writing up my previous message, I realized this was ambiguous in the RFC and I have since clarified it.
text/3389-manifest-lint.md
Outdated
| Instead of traditional workspace inheritance where there is a single value to | ||
| inherit with `workspace = true`, we could have `[workspace.lints.<preset>]` | ||
| which defines presets and the user could do `lints.<preset> = true`. The user | ||
| could then name them as they wish to avoid collision with rustc lints. |
There was a problem hiding this comment.
I wanted to call attention to this item. Would presets or custom lint groups of interest enough for us to design around?
text/3389-manifest-lint.md
Outdated
| 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). |
There was a problem hiding this comment.
Would it work for cargo to just hard code what lint tool goes to what CLI?
The problem is then dealing with the potential future state of third-party lint tools. If those were opted into via cargo instead of a source-level attribute, we might be able to get the information we need to direct the lints to the appropriate CLI (or ignore as needed)
There was a problem hiding this comment.
Couldn't cargo always pass all lints defined in the [lint] section to all tools? If a tool is not used and therefore not registered with rustc, rustc will just ignore that lint and that it was passed to it. Or am I misunderstanding the problem? 🤔
There was a problem hiding this comment.
I've not touched that side of things so I am unsure if that would be a problem or not.
ehuss also brought up the possibility of using this to minimize which lints are included in fingerprinting for reducing recompiles for lint changes (e.g. adding clippy lints doesn't cause cargo build to rebuild). Its mentioned in the RFC just after this paragraph.
rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
|
Huzzah! The @rust-lang/cargo team has decided to accept this RFC. To track further discussion, subscribe to the tracking issue here: |
feat: `lints` feature ### What does this PR try to resolve? Implement rust-lang/rfcs#3389 which shifts a subset of `.cargo/config.toml` functionality to `Cargo.toml` by adding a `[lints]` table. This **should** cover all of the user-facing aspects of the RFC - This doesn't reduce what flags we fingerprint - This will fail if any lint name as `::` in it. What to do in this case was in the RFC discussion but I couldn't find the thread to see what all was involved in that discussion - This does not fail if a `[lints]` table is present or malformed unless nightly with the `lints` feature enabled - The idea is this will act like a `[lints]` table is present in an existing version of rust, ignore it - The intent is to not force an MSRV bump to use it. - When disabled, it will be a warning - When disabled, it will be stripped so we don't publish it Tracking issue for this is #12115. ### How should we test and review this PR? 1. Look at this commit by commit to see it gradually build up 2. Look through the final set of test cases to make sure everything in the RFC is covered I tried to write this in a way that will make it easy to strip out the special handling of this unstable feature, both in code and commit history ### Additional information I'd love to bypass the need for `cargo-features = ["lints"]` so users today can test it on their existing projects but hesitated for now. We can re-evaluate that later. I broke out the `warn_for_feature` as an experiment towards us systemitizing this stabilization approach which we also used with #9732. This works well when we can ignore the new information which isn't too often but sometimes happens. This does involve a subtle change to `profile.rustflags` precedence but its nightly and most likely people won't notice it? The benefit is its in a location more like the rest of the rustflags.
epage
added
the
call-for-testing
Testing instructionsRequires
Steps:
Example: rust-lang/cargo#12178 Please leave feedback on rust-lang/cargo#12115 |
|
This Tracking Issue will appear in the Call for Testing section of the next issue (# 497) of This Week in Rust (TWiR). I have removed the |
U007D
removed
the
call-for-testing
new cargo feature has became stable at nightly: <rust-lang/rfcs#3389 (comment)>
…ed cargo-lints Since the implementation of rust-lang/rfcs#3389, it is now possible to specify workspace-level lints for rustc and clippy. This PR updates the cargo configuration and CI to use this new feature instead of cargo-lints. Note that it was only stabilized in `nightly-2023-09-10`. Using it with out current toolchain requires either a -Zlints flag or a modification to `.cargo/config.toml`: ``` [unstable] lints = true ``` Note that unlike the original suggestion in hyperledger#3622, this doesn't make the lints crate level, but merely replaces a clunky unmaintained tool with a standard solution for configuring lints. In particular this PR: - Removes old lints.toml configuration files for cargo-lints - Adds [lint] tables to Cargo.toml of the root and wasm workspaces. The lints are duplicated between the two - Replaces `cargo lints clippy` invocations with `cargo clippy -Zlints` in CI scripts - Silences/fixes some new lints that popped up due to differences in how between `cargo lints` and workspaces - Does a drive-by fix to iroha_genesis: it now too shares cargo metadata as do other crates Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
Since the implementation of rust-lang/rfcs#3389, it is now possible to specify workspace-level lints for rustc and clippy. This PR updates the cargo configuration and CI to use this new feature instead of cargo-lints. Note that it was only stabilized in `nightly-2023-09-10`. Using it with out current toolchain requires either a -Zlints flag or a modification to `.cargo/config.toml`: ``` [unstable] lints = true ``` Note that unlike the original suggestion in #3622, this doesn't make the lints crate level, but merely replaces a clunky unmaintained tool with a standard solution for configuring lints. In particular this PR: - Removes old lints.toml configuration files for cargo-lints - Adds [lint] tables to Cargo.toml of the root and wasm workspaces. The lints are duplicated between the two - Replaces `cargo lints clippy` invocations with `cargo clippy -Zlints` in CI scripts - Silences/fixes some new lints that popped up due to differences in how between `cargo lints` and workspaces - Does a drive-by fix to iroha_genesis: it now too shares cargo metadata as do other crates Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
Since the implementation of rust-lang/rfcs#3389, it is now possible to specify workspace-level lints for rustc and clippy. This PR updates the cargo configuration and CI to use this new feature instead of cargo-lints. Note that it was only stabilized in `nightly-2023-09-10`. Using it with out current toolchain requires either a -Zlints flag or a modification to `.cargo/config.toml`: ``` [unstable] lints = true ``` Note that unlike the original suggestion in #3622, this doesn't make the lints crate level, but merely replaces a clunky unmaintained tool with a standard solution for configuring lints. In particular this PR: - Removes old lints.toml configuration files for cargo-lints - Adds [lint] tables to Cargo.toml of the root and wasm workspaces. The lints are duplicated between the two - Replaces `cargo lints clippy` invocations with `cargo clippy -Zlints` in CI scripts - Silences/fixes some new lints that popped up due to differences in how between `cargo lints` and workspaces - Does a drive-by fix to iroha_genesis: it now too shares cargo metadata as do other crates Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
Since the implementation of rust-lang/rfcs#3389, it is now possible to specify workspace-level lints for rustc and clippy. This PR updates the cargo configuration and CI to use this new feature instead of cargo-lints. Note that it was only stabilized in `nightly-2023-09-10`. Using it with out current toolchain requires either a -Zlints flag or a modification to `.cargo/config.toml`: ``` [unstable] lints = true ``` Note that unlike the original suggestion in #3622, this doesn't make the lints crate level, but merely replaces a clunky unmaintained tool with a standard solution for configuring lints. In particular this PR: - Removes old lints.toml configuration files for cargo-lints - Adds [lint] tables to Cargo.toml of the root and wasm workspaces. The lints are duplicated between the two - Replaces `cargo lints clippy` invocations with `cargo clippy -Zlints` in CI scripts - Silences/fixes some new lints that popped up due to differences in how between `cargo lints` and workspaces - Does a drive-by fix to iroha_genesis: it now too shares cargo metadata as do other crates Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
Since the implementation of rust-lang/rfcs#3389, it is now possible to specify workspace-level lints for rustc and clippy. This PR updates the cargo configuration and CI to use this new feature instead of cargo-lints. Note that it was only stabilized in `nightly-2023-09-10`. Using it with out current toolchain requires either a -Zlints flag or a modification to `.cargo/config.toml`: ``` [unstable] lints = true ``` Note that unlike the original suggestion in #3622, this doesn't make the lints crate level, but merely replaces a clunky unmaintained tool with a standard solution for configuring lints. In particular this PR: - Removes old lints.toml configuration files for cargo-lints - Adds [lint] tables to Cargo.toml of the root and wasm workspaces. The lints are duplicated between the two - Replaces `cargo lints clippy` invocations with `cargo clippy -Zlints` in CI scripts - Silences/fixes some new lints that popped up due to differences in how between `cargo lints` and workspaces - Does a drive-by fix to iroha_genesis: it now too shares cargo metadata as do other crates Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
This proposal is trying to make it easier managing project-specific lint overrides.
Rendered (book)