Allow underscore-prefixed renamed dependency in manifest

[Rustin170506]

close #3532

[rust-highfive]

r? @carols10cents

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

[Turbo87]

👋 @hi-rustin

the issue here is slightly more complicated. crate name generally are not allowed to start with an underscore, so adjusting valid_ident() to allow it could create some problems for us when people suddenly start publishing crates with a leading underscore.

#1487 implemented support for "renamed dependencies", but it looks like we did not test for the "name has a leading underscore" case. I think the first step in fixing this issue is to create a new test, roughly similar to the existing new_with_renamed_dependency() test, but with a leading underscore for the name. That should hopefully reproduce the issue that @jmagnuson is seeing in #3532.

[Rustin170506]

I think the first step in fixing this issue is to create a new test, roughly similar to the existing new_with_renamed_dependency() test, but with a leading underscore for the name.

@Turbo87 Thanks for the review! Added.

[Rustin170506]

The test case will fail when there is no such underscore change.

failed to decode: Error("missing field `crate`", line: 1, column: 249)
thread 'krate::publish::new_with_underscore_name_dependency' panicked at 'failed to decode: Error("missing field `crate`", line: 1, column: 249)', src/tests/util/response.rs:111:19
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9bc8c42bb2f19e745a63f3445f1ac248fb015e53/library/std/src/panicking.rs:493:5
   1: std::panicking::begin_panic_fmt
             at /rustc/9bc8c42bb2f19e745a63f3445f1ac248fb015e53/library/std/src/panicking.rs:435:5
   2: all::util::response::json
             at ./src/tests/util/response.rs:111:19
   3: all::util::response::Response<T>::good
             at ./src/tests/util/response.rs:26:9
   4: all::krate::publish::new_with_underscore_name_dependency
             at ./src/tests/krate/publish.rs:187:5
   5: all::krate::publish::new_with_underscore_name_dependency::{{closure}}
             at ./src/tests/krate/publish.rs:174:1
   6: core::ops::function::FnOnce::call_once
             at /Users/rustin/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/function.rs:227:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/9bc8c42bb2f19e745a63f3445f1ac248fb015e53/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

error: test failed, to rerun pass '-p cargo-registry --test all'

Process finished with exit code 101

[Rustin170506]

I did some local debugging and integration testing to make sure the logic was being tested.

[Turbo87]

looks good to me now, but I would like to get a second pair of eyes on this before we merge it.

@pietroalbini @jtgeibel @JohnTitor

[pietroalbini]

The code changes themselves look great, but I'd prefer for the internal API to be harder to misuse and misunderstand:

• The boolean Crate::valid_name("foo", true) or Crate::valid_name("bar" false) is a bit confusing if one doesn't know about this PR. I'd prefer to keep Crate::valid_name the same as before and adding a Crate::valid_dependency_name.
• EncodableExplicitName is also a bit confusing, it'd be better to call it EncodableDependencyName.

[Rustin170506]

The code changes themselves look great, but I'd prefer for the internal API to be harder to misuse and misunderstand:

* The boolean `Crate::valid_name("foo", true)` or `Crate::valid_name("bar" false)` is a bit confusing if one doesn't know about this PR. I'd prefer to keep `Crate::valid_name` the same as before and adding a `Crate::valid_dependency_name`.

* `EncodableExplicitName` is also a bit confusing, it'd be better to call it `EncodableDependencyName`.

@pietroalbini Addressed! Thanks for your review.

[pietroalbini]

Thanks!

[pietroalbini]

@bors r+

[bors]

📌 Commit 7dfc717 has been approved by pietroalbini

[bors]

⌛ Testing commit 7dfc717 with merge e5290b7...

[bors]

☀️ Test successful - checks-actions
Approved by: pietroalbini
Pushing e5290b7 to master...

[jmagnuson]

Thank you, @hi-rustin, and everyone who helped get this merged!