Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some unused crate dependencies. #126063

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

nnethercote
Copy link
Contributor

I found these by setting the unused_crate_dependencies lint temporarily to Warn.

r? @jackh726

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Jun 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 6, 2024

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

@nnethercote
Copy link
Contributor Author

There's an argument to be made that we should add #[deny(unused_crate_dependencies)] to most of the crates in the compiler, but I haven't done that here.

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make an MCP to change the lint to deny?

@@ -18,5 +18,5 @@ unwind = { path = "../unwind" }
compiler_builtins = "0.1.0"
cfg-if = "1.0"

[target.'cfg(not(all(windows, target_env = "msvc")))'.dependencies]
[target.'cfg(target_os = "emscripten")'.dependencies]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems weird?

@@ -1 +1,4 @@
// This is intentionally empty since this crate is only used to depend on other library crates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

@mati865
Copy link
Contributor

mati865 commented Jun 6, 2024

I think it could be enabled in in [lints] section of Cargo.toml. This should be more manageable than adding deny to each lib.rs.

@nnethercote
Copy link
Contributor Author

Something I didn't say is that when I enabled it universally I had to deal with several annoying cases, mostly in library/.

  • Some crates are only used in certain configurations.
  • Some crates are in Cargo.toml because of weird packaging reasons. E.g.
    # Make sure rustc_codegen_ssa ends up in the sysroot, because this
    # crate is intended to be used by codegen backends, which may not be in-tree.
    rustc_codegen_ssa = { path = "../rustc_codegen_ssa" }
    rustc_driver = { path = "../rustc_driver" }
    rustc_driver_impl = { path = "../rustc_driver_impl" }
    # Make sure rustc_smir ends up in the sysroot, because this
    # crate is intended to be used by stable MIR consumers, which are not in-tree.
    rustc_smir = { path = "../rustc_smir" }
    stable_mir = { path = "../stable_mir" }

These need use rustc_foo as _; items to avoid the warning, or some fiddling with conditional compilation stuff. Both of the cases you asked about above were leftovers for that kind of thing that I failed to removed.

So my conclusion is that this would be good to enable for almost every compiler crate (except the rustc crate), but not library/. There are also these false positive cases:

/// This lint is "allow" by default because it can provide false positives
/// depending on how the build system is configured. For example, when
/// using Cargo, a "package" consists of multiple crates (such as a
/// library and a binary), but the dependencies are defined for the
/// package as a whole. If there is a dependency that is only used in the
/// binary, but not the library, then the lint will be incorrectly issued
/// in the library.

So I think changing it to Deny in general is not a good idea.

@nnethercote
Copy link
Contributor Author

I think it could be enabled in in [lints] section of Cargo.toml. This should be more manageable than adding deny to each lib.rs.

I tried adding this to the top level Cargo.toml:

[workspace.lints.rust]
unused_crate_dependencies = "warn"

and it had no effect, even on a stage 2 build. I also tried without the workspace. but then I got a Cargo error. Any other suggestions? I'm no expert on Cargo and workspaces.

@mati865
Copy link
Contributor

mati865 commented Jun 7, 2024

When you add it to the workspace, you also have to enable lints inheritance in each member:

[lints]
workspace = true

https://doc.rust-lang.org/cargo/reference/workspaces.html#the-lints-table

@bors
Copy link
Contributor

bors commented Jun 7, 2024

☔ The latest upstream changes (presumably #126108) made this pull request unmergeable. Please resolve the merge conflicts.

I found these by setting the `unused_crate_dependencies` lint
temporarily to `Warn`.
@nnethercote
Copy link
Contributor Author

I rebased.

@jackh726
Copy link
Member

@bors r+

As much as I am wary about landing one-off changes like this without also linting to prevent similar PRs in the future, sounds like there are real reasons we can't lint (at the moment at least). So, I'm fine to land this.

@bors
Copy link
Contributor

bors commented Jun 10, 2024

📌 Commit 29629d0 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2024
@jackh726
Copy link
Member

(I would still recommend following up with linting for compiler crates if possible)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 10, 2024
…r=jackh726

Remove some unused crate dependencies.

I found these by setting the `unused_crate_dependencies` lint temporarily to `Warn`.

r? `@jackh726`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#126036 (Migrate `run-make/short-ice` to `rmake`)
 - rust-lang#126063 (Remove some unused crate dependencies.)
 - rust-lang#126115 (Fix ICE due to `unwrap` in `probe_for_name_many`)
 - rust-lang#126159 (ScalarInt: size mismatches are a bug, do not delay the panic)
 - rust-lang#126184 (interpret: do not ICE on padded non-pow2 SIMD vectors)
 - rust-lang#126191 (Fix `NonZero` doctest inconsistencies)
 - rust-lang#126211 (migrate tests/run-make/llvm-outputs to use rmake.rs)
 - rust-lang#126212 (fix: build on haiku)
 - rust-lang#126215 (Add explanatory note to async block type mismatch error)
 - rust-lang#126223 (run-make: add `run_in_tmpdir` self-test)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126063 (Remove some unused crate dependencies.)
 - rust-lang#126115 (Fix ICE due to `unwrap` in `probe_for_name_many`)
 - rust-lang#126159 (ScalarInt: size mismatches are a bug, do not delay the panic)
 - rust-lang#126184 (interpret: do not ICE on padded non-pow2 SIMD vectors)
 - rust-lang#126191 (Fix `NonZero` doctest inconsistencies)
 - rust-lang#126211 (migrate tests/run-make/llvm-outputs to use rmake.rs)
 - rust-lang#126212 (fix: build on haiku)
 - rust-lang#126215 (Add explanatory note to async block type mismatch error)
 - rust-lang#126223 (run-make: add `run_in_tmpdir` self-test)

r? `@ghost`
`@rustbot` modify labels: rollup
@nnethercote
Copy link
Contributor Author

When you add it to the workspace, you also have to enable lints inheritance in each member:

Would I have to make that change to the Cargo.toml file of every rustc_* crate under compiler/? If so, that's worse than adding a single #![deny(unused_crate_dependencies)] line to every lib.rs file.

@bors bors merged commit 1832ee0 into rust-lang:master Jun 10, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 10, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2024
Rollup merge of rust-lang#126063 - nnethercote:rm-unused-crate-deps, r=jackh726

Remove some unused crate dependencies.

I found these by setting the `unused_crate_dependencies` lint temporarily to `Warn`.

r? ``@jackh726``
@nnethercote nnethercote deleted the rm-unused-crate-deps branch June 11, 2024 00:02
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 14, 2024
…eps, r=jackh726

Remove some unnecessary crate dependencies.

A follow-up to rust-lang#126063.

r? `@jackh726`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 14, 2024
…eps, r=jackh726

Remove some unnecessary crate dependencies.

A follow-up to rust-lang#126063.

r? ``@jackh726``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 14, 2024
Rollup merge of rust-lang#126368 - nnethercote:rm-more-unused-crate-deps, r=jackh726

Remove some unnecessary crate dependencies.

A follow-up to rust-lang#126063.

r? ``@jackh726``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants