Fix [multiple_crate_versions] to correctly normalize package names to avoid missing the local one
#12146
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
| @@ -16,7 +16,7 @@ pub(super) fn check(cx: &LateContext<'_>, metadata: &Metadata) { | |||
|
|
|||
| if let Some(resolve) = &metadata.resolve | |||
| && let Some(local_id) = packages.iter().find_map(|p| { | |||
| if p.name == local_name.as_str() { | |||
| if p.name.replace('-', "_") == local_name.as_str() { | |||
There was a problem hiding this comment.
I wonder if we can avoid the allocation by using iteration here
May be a premature optimization
There was a problem hiding this comment.
I've replaced it by iterating over the chars. While this allocates a char on each end per char compared at least it stops once they're not equal.
A further (and nastier) optimization would be to go through it as bytes to avoid the u8 -> char bloat but then we're introducing a (hidden) dependency on the fact that the name never is anything else than ASCII.
This feels like a better tradeoff.
Thanks for the review, it made me read up on a lot of stuff.
There was a problem hiding this comment.
Reading more: crates.io only allows a subset of ASCII:
https://github.com/rust-lang/crates.io/blob/825cce6c189fcf1174da4b7d31855bae430ed97c/src/models/keyword.rs#L56-L63
Cargo itself only allows ASCII:
https://github.com/rust-lang/cargo/blob/92395d90106b3b61bcb68bcf2069052c93771764/src/cargo/ops/cargo_new.rs#L260C32-L266
But I still believe char is better.
There was a problem hiding this comment.
While this allocates a char on each end per char compared at least it stops once they're not equal.
it's a stack allocation, not usually what people mean when talking about allocations.
There was a problem hiding this comment.
but yeah why not make the change to use .as_bytes() and normalize b'-' to b'_'
There was a problem hiding this comment.
While this allocates a char on each end per char compared at least it stops once they're not equal.
it's a stack allocation, not usually what people mean when talking about allocations.
Thanks for that. I didn't think of that. It's all new for me.
There was a problem hiding this comment.
but yeah why not make the change to use
.as_bytes()and normalizeb'-'tob'_'
Let me do that.
There was a problem hiding this comment.
@Manishearth Done. Thanks for the mentoring.
|
@rustbot review |
|
@rustbot review |
|
@bors r+ thanks! |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #12145
changelog: [
multiple_crate_versions]: correctly normalize package name