feat(diag): Stabilize cargo-lints - #17298
Conversation
rustbot
added
A-build-execution
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
Just thought about this: Do we dogfood this yet? Maybe before stabilization we can do it in rust-lang/cargo and rust-lang/rust and get an sense of how it looks like in real world.
Not a blocker btw
There was a problem hiding this comment.
I have tried it out a couple times on Cargo. I have not on rust-lang/rust.
There was a problem hiding this comment.
I just tried in rustc
- several
cargo::manual_readme - several
cargo::non_kebab_case_bins - A warning about
#![expect(unused_crate_dependencies)]being unfulfilled.
There was a problem hiding this comment.
Thanks for the info!
Should we configure our CI to adopt that?
e7f3786 to
424ad37
Compare
This comment has been minimized.
This comment has been minimized.
5d91e48 to
426df5f
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Could you share why we need to allow cargo::default here for many tests? Were they got lint warnings for the same reason, or case by case?
There was a problem hiding this comment.
Most test have unused dependencies. There were a smattering of others. I figured it would be easier to do this globally than resolving lints, allowing on a case-by-case basis, etc.
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
|
@rfcbot fcp merge T-cargo We have discussed this during the sync weekly meeting. Ed has written a good review guide. Would be appreciated if people find a time reviewing them. |
|
@weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rust-rfcbot
added
proposed-final-comment-period
|
@rfcbot concern blanket_hint_mostly_unused Should this be behind |
|
I believe so. |
|
Since I won the race for |
This test is trying to assert for presence and absence but can't use exhaustive output because it is dependent on the users build environment. This makes it so we can check exactly what we are looking for.
This comment has been minimized.
This comment has been minimized.
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
4 participants
View all comments
What does this PR try to resolve?
Fixes #12235
Unstable feature docs: https://doc.rust-lang.org/stable/cargo/reference/unstable.html#lintscargo
We are adding a linting system to workspaces and packages controlled by
[lints.cargo].As the presence of
[lints.cargo]was a hard error before 1.79, by-default warn/deny lints are ignored whenpackage.rust-versionis set below that.Workspace lints are controlled by
[workspace.lints]if present, otherwise[lints]. This was inspired byworkspace.resolver.The rust-version used for the workspace is the lowest among the workspace members.
In vetting the design, we have a fully general parse-pass that runs during build operations as well as
cargo fetxh(matching deferred warnings and errors from manifest parsing). We also have a one-off lint that runs against the completed build units.The initial batch of lints being stabilized is documented at https://doc.rust-lang.org/nightly/cargo/reference/lints.html
For more on the behavior of these lints, see https://github.com/rust-lang/cargo/tree/master/tests/testsuite/lints
Relevant docs:
How to test and review this PR?
At #t-cargo > Last items for linting system: `unused_dependencies`, I raised concern over
unused_dependenciesname in case we want to add other kinds of unused dependencies (like artifacts) as separate lints. No one expressed interest in that, so I kept it as a general lint name. I didn't addpackageto the name since our existing dept tables don't have that and the intent is likely clear.