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.

Sign up for GitHub

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

Jump to bottom

Add support for nounused --extern flag #96067

Merged
merged 1 commit into from
Apr 24, 2022
Merged

Add support for nounused --extern flag #96067

merged 1 commit into from
Apr 24, 2022

Conversation

jsgf
Copy link
Contributor

@jsgf jsgf commented Apr 15, 2022

This adds nounused to the set of extern flags:
--extern nounused:core=/path/to/core/libcore.rlib.

The effect of this flag is to suppress unused-crate-dependencies
warnings relating to the crate.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 15, 2022
@rust-highfive
Copy link
Collaborator

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2022
This was referenced Apr 15, 2022
Closed
Closed
@compiler-errors compiler-errors mentioned this pull request Apr 15, 2022
Merged
@jsgf
Copy link
Contributor Author

jsgf commented Apr 21, 2022

@compiler-errors friendlyping

compiler-errors
compiler-errors reviewed Apr 21, 2022
View reviewed changes
src/test/ui/extern-flag/nounused.rs
@@ -0,0 +1,7 @@
// check-pass
Copy link
Member

Choose a reason for hiding this comment

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

you may be able to use revisions to consolidate this into one test, but I guess I won't block this on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

compiler/rustc_metadata/src/creader.rs
@@ -908,6 +908,10 @@ impl<'a> CrateLoader<'a> {
// Don't worry about pathless `--extern foo` sysroot references
continue;
}
if entry.nounused_dep {
Copy link
Member

Choose a reason for hiding this comment

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

hmm... would something like ignore_unused be a better name for this? I know the convention in other compilers is X and noX for enabling or disabling X, but it's kinda hard to understand whether nounused is denying unused or allowing unused.

@compiler-errors
Copy link
Member

@jsgf: any strong thoughts about the name nounused? otherwise I think this is fine for r+, because it's behind an unstable options flag we have no guarantees for it.

@jsgf
Copy link
Contributor Author

jsgf commented Apr 22, 2022 •
edited
Loading

@compiler-errors > nounused

Yeah, in general I don't like negative options, but this seemed 1) in keeping with the other --extern flags (single compact word, eg noprelude) and 2) not too unclear. Something like allow_unused is probably clearer but pretty verbose.

Since this is an option that's almost entirely for other tooling to use - users never type --extern options, let alone flags - I think it just has to be descriptive enough for someone working at the tool level to be able to understand.

I'd expect Cargo, for example, to maybe support an allow_unused = True flag as part of Cargo.toml dependency spec, but I don't see a huge importance that it map directly to the underlying flag name. (Maybe @est31 has an opinion here, tho it's quite possible that Cargo wouldn't use --extern flags for this at all.)

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 22, 2022

📌 Commit 439c68c5a2cde5dec24a004c3752398302f348ad has been approved by compiler-errors

@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 Apr 22, 2022
@est31
Copy link
Member

est31 commented Apr 22, 2022

@jsgf what is the exact use case you want to achieve with that flag? Something like an allow_unused = true analog?

@bors
Copy link
Contributor

bors commented Apr 23, 2022

⌛ Testing commit 439c68c5a2cde5dec24a004c3752398302f348ad with merge 60a900f0b6e3c486ebc3074ddbcb21f459ddf9d7...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 23, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 23, 2022
@jsgf
Copy link
Contributor Author

jsgf commented Apr 24, 2022 •
edited
Loading

@est31 My specific use case is Buck builds that have all the dependencies explicitly specified, including the sysroot ones. So to do this:

  1. add --sysroot /some/empty/dir so that rustc can't pick up any implicit dependencies
  2. add explicit --extern std=/path/to/libstd.rlib etc (core, alloc, etc as well, though mostly as noprelude) by default to all crates since we don't want users to have to add all the things they'd expect to get by default

This all works pretty well. But since we also have -Wunused-crate-dependencies on by default, the sysroot dependencies generate lots of spurious unused dependency warnings (since not everyone uses core/alloc etc). So I added --extern nounused:std=... to avoid this.

But it seems like a generally useful feature at the same level as noprelude, hence this PR.

(The larger plan is to make Rust and C++ builds uniform, so we can have a C++ linker link Rust by having all the Rust dependencies fully represented to Buck so it can pass them to the linker.)

@est31
Copy link
Member

est31 commented Apr 24, 2022

Thanks for the explanation! The feature is useful in general, but it's always helpful to have a record of it, say 5 years later or so, when wondering "why was this added".

@jsgf jsgf force-pushed the extern-nounused branch from 439c68c to b19ee8d Compare April 24, 2022 04:49
@jsgf
Copy link
Contributor Author

jsgf commented Apr 24, 2022

(rebase to master to see if it clears the test failure)

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2022

📌 Commit b19ee8d6d5a7c17cc66754381b0df245fe5ada86 has been approved by compiler-errors

@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 Apr 24, 2022
@bors
Copy link
Contributor

bors commented Apr 24, 2022

⌛ Testing commit b19ee8d6d5a7c17cc66754381b0df245fe5ada86 with merge afc30b9cf01692fac85a277a6213cdeab46ca2b4...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 24, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 24, 2022
@jsgf
Add support for nounused --extern flag
9102edf 
This adds `nounused` to the set of extern flags:
`--extern nounused:core=/path/to/core/libcore.rlib`.

The effect of this flag is to suppress `unused-crate-dependencies`
warnings relating to the crate.
@jsgf
Copy link
Contributor Author

jsgf commented Apr 24, 2022

Updated failing test.

@jsgf jsgf force-pushed the extern-nounused branch from b19ee8d to 9102edf Compare April 24, 2022 06:32
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2022

📌 Commit 9102edf has been approved by compiler-errors

@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 Apr 24, 2022
@bors
Copy link
Contributor

bors commented Apr 24, 2022

⌛ Testing commit 9102edf with merge b759b22...

@bors
Copy link
Contributor

bors commented Apr 24, 2022

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing b759b22 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 24, 2022
@bors bors merged commit b759b22 into rust-lang:master Apr 24, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 24, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b759b22): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@mhammerly mhammerly mentioned this pull request May 4, 2022
Open
@ehuss ehuss mentioned this pull request Jun 22, 2022
Open
4 tasks
@ehuss
Copy link
Contributor

ehuss commented Jun 22, 2022

I posted #98400 to track the stabilization of this option.

This was referenced Mar 21, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

Assignees

@compiler-errors compiler-errors

Labels
merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
10 participants
@jsgf @rust-highfive @compiler-errors @bors @est31 @rust-log-analyzer @rust-timer @ehuss @cjgillot @rustbot