fix(compile): Where possible, hint about misplaced deps #16940
fix(compile): Where possible, hint about misplaced deps #16940epage wants to merge 10 commits intorust-lang:masterfrom
Conversation
The one side effect is we won't list used export in traces.
Through the re-ordering of things, the check has become redundant and can be removed.
Sometimes a normal dependency is actually a dev-dependency. For any of the dev-dependencies we build, we can let users know that an unused dep might be a misplaced dep.
rustbot
added
A-build-execution
|
r? @ehuss rustbot has assigned @ehuss. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
The bump check is a non-blocking job currently broken by GitoxideLabs/gitoxide#2541 because cargo-semver-checks isn't able to respect lockfiles fully. |
|
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. |
| r#" | ||
| #[test] | ||
| fn foo { | ||
| fn foo() { |
| state.seen_units.push(unit.clone()); | ||
| if let Some(existing) = state.unused_externs.as_mut() { | ||
| existing.retain(|ext| unused_externs.contains(ext)); | ||
| } else { | ||
| state.unused_externs = Some(unused_externs); | ||
| } |
There was a problem hiding this comment.
It is a bit weird that the actual core part of this PR is this commit not the fix commit, which people may easily overlook.
The commit message around also doesn't say anything more obvious than "track as we go". It is indeed a refactor commit, though I think it might be worthy adding more context about why this is needed and how this data model unblock the later behavior change.
| } | ||
| if used_in_dev { | ||
| let help = Group::with_title(Level::HELP.secondary_title(format!( | ||
| "to still use for development builds, move to `dev-dependencies`" |
There was a problem hiding this comment.
nit: format! is not necessary.
| /// Expected [`Self::seen_units`] entries to know we've received them all | ||
| /// | ||
| /// To avoid warning in cases where we didn't, | ||
| /// e.g. if a [`Unit`] errored and didn't report unused externs. | ||
| needed_units: Option<usize>, | ||
| needed_units: usize, | ||
| /// As reported by rustc | ||
| seen_units: Vec<Unit>, | ||
| /// As reported by rustc | ||
| unused_externs: IndexMap<Unit, Vec<InternedString>>, | ||
| unused_externs: Option<BTreeSet<InternedString>>, |
There was a problem hiding this comment.
I understand it's implementation detail, though is still confusing to see two fields saying as reported by rustc whereas they are actually not the same and unused_externs is not directly reported by rustc.
Probably something like:
/// Units that have reported their unused externs
seen_units: Vec<Unit>,
/// Intersection of unused externs across all [`Self::seen_units`]
unused_externs: Option<BTreeSet<InternedString>>,4 participants
What does this PR try to resolve?
Sometimes a normal dependency is actually a dev-dependency.
For any of the dev-dependencies we build, we can let users know that an
unused dep might be a misplaced dep. This check came from #8437 but I originally punted on it due to the complexity of having the right information. Through the refactors done in this, I found it became easy to report this. The help could be improved to show the insertion of the dev-dependency with a removal of the normal dependency but that is being left to a future exercise to limit the scope of this.
This also improves the quality of the logged messages for people who are ok with the false positives that we can't report.
How to test and review this PR?