fix: ignore implicit std dependencies in unused-crate-dependencies lint#16677
fix: ignore implicit std dependencies in unused-crate-dependencies lint#16677biscuitrescue wants to merge 4 commits intorust-lang:masterfrom
unused-crate-dependencies lint#16677Conversation
rustbot
added
A-build-execution
|
r? @ehuss rustbot has assigned @ehuss. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
The CI failure is due to rust-src not being available. I will update the test. One thing I'm not sure of though, should this test reside instead in |
|
|
Note that we ask for PRs to be focused on how they should be reviewed and merged, and not how they were implemented. This includes making the commits atomic. One twist on that is we've found it helpful to add tests in the commit before the fix, with them passing, reproducing the undesired behavior. The fix commit would then update them to show the new behavior and the diff between them shows how behavior changed. See also https://doc.crates.io/contrib/process/working-on-cargo.html#submitting-a-pull-request |
| p.cargo("build -Zbuild-std") | ||
| .masquerade_as_nightly_cargo(&["build-std"]) | ||
| .env("RUSTFLAGS", "-W unused-crate-dependencies") | ||
| // Use .with_stderr() with a pattern to be more flexible | ||
| .with_stderr_contains("[WARNING] extern crate `bar` is unused in crate `buildstd_test`") | ||
| .with_stderr_contains("[..]unused-crate-dependencies[..]") | ||
| .with_stderr_does_not_contain("extern crate `core` is unused") | ||
| .with_stderr_does_not_contain("extern crate `alloc` is unused") | ||
| .with_stderr_does_not_contain("extern crate `compiler_builtins` is unused") |
There was a problem hiding this comment.
Normally, we try to avoid use of does_not_contain tests because the format of the output could change and then we'd never know if this regressed.
We normally use snapsnhots. How much to include needs to balance that we don't want to overly constrain rustc in the output because it makes updates to the compiler more difficult
| if nounused { | ||
| opts.push("nounused"); | ||
| *unstable_opts = true; | ||
| } |
There was a problem hiding this comment.
What does the path to stabilizing this look like?
There was a problem hiding this comment.
I also wonder if we should start tracking all of the different unstable features of rustc build-std is relying on to not get surprised when we stabilize it.
CC @davidtwco
|
On a related note, I'm working on integrating this lint into Cargo so it can be properly reported, #16600. As part of this, I've posted rust-lang/rfcs#3920. I wonder if I should tie |
4 participants
fixes 152561
What does this PR try to solve?
Cargo explicitly passes standard library crates (core, alloc, etc.) as
extern dependencies when using
-Zbuild-std, which causes "erroneous"warnings because these crates are not listed in the user's Cargo.toml.
The working
This was initially issued on the
rustrepo, for which i filed a PR, usingnopreludeto exclude linting, but it was correctly pointed out that this was not the correct approach.Following Zulip discussions, I added a
nounusedflag to theUnitDepsstruct which is similar in working to thenopreludeflag.--extern nounused:fooalready exists as an unstable compiler option.How to test and review this PR?
rustup run nightly cargo test build_std_does_not_warn_about_implicit_std_deps.core,alloc,compiler_builtinsdo not throw unused warnings.Currently,
nounusedis emitted only for implicitly injected standard library dependencies when using-Zbuild-std.If Cargo ever gains support for explicit builtin dependencies declared by users, it may be desirable to revisit whether
nounusedshould apply in those cases. This PR does not change behavior for explicit dependencies.