Add suggestion to use &*var when &str: From<String> is expected
#59268
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Wo that looks nice! I'd just like confirmation from the @rust-lang/docs team but otherwise looks good to me! r=me if everything's fine for someone else. |
There was a problem hiding this comment.
Woah, this is nice! It would be cool if we could get this kind of message for any Deref relation, but this should take care of one of the most common cases. (Maybe one for Vec and &[T] would get the other most common Deref relation? Just thinking out loud, since we're already making changes here.)
src/libcore/convert.rs
Outdated
| #[rustc_on_unimplemented( | ||
| on( | ||
| all(_Self="&str", T="std::string::String"), | ||
| note="you can coerce a `{T}` into a `{Self}` by writing `&*variable`" |
There was a problem hiding this comment.
Is the use of "variable" in error messages something we've already done? It seems odd to have that specific default name.
There was a problem hiding this comment.
We haven't done it like this but we do have some messages that use placeholders when the snippet is not available, but they are rarely seen because we have access to the snippet most of the time. Because this is an on_unimplemented message we don't have access to the snippet at all. I'm not too happy with the copy so wording changes are totally welcome.
There was a problem hiding this comment.
I skimmed through the uses of rustc_on_unimplemented to see what some other messages do, and i liked this one message in the Iterator trait:
rust/src/libcore/iter/traits/iterator.rs
Lines 78 to 79 in 89573b3
To make it totally fit in, i would write something like "to coerce a `{T}` into a `{Self}`, use `&*`", but IMO your existing one can work as-is by using "foo" instead of "variable".
There was a problem hiding this comment.
Changed to "to coerce a {T} into a {Self}, use &* as a prefix" as just suggesting &* can confuse newcomers as to where it should go.
For arrays we already have a way of targeting any type of array (using |
Ah, in that case i won't hold up this PR any more. Thanks for doing this! @bors r+ |
|
📌 Commit e929d19 has been approved by |
bors
added
S-waiting-on-bors
…avus Add suggestion to use `&*var` when `&str: From<String>` is expected Fix rust-lang#53879.
…avus Add suggestion to use `&*var` when `&str: From<String>` is expected Fix rust-lang#53879.
…avus Add suggestion to use `&*var` when `&str: From<String>` is expected Fix rust-lang#53879.
Rollup of 18 pull requests Successful merges: - #57293 (Make some lints incremental) - #57565 (syntax: Remove warning for unnecessary path disambiguators) - #58253 (librustc_driver => 2018) - #58837 (librustc_interface => 2018) - #59268 (Add suggestion to use `&*var` when `&str: From<String>` is expected) - #59283 (Make ASCII case conversions more than 4× faster) - #59284 (adjust MaybeUninit API to discussions) - #59372 (add rustfix-able suggestions to trim_{left,right} deprecations) - #59390 (Make `ptr::eq` documentation mention fat-pointer behavior) - #59393 (Refactor tuple comparison tests) - #59420 ([CI] record docker image info for reuse) - #59421 (Reject integer suffix when tuple indexing) - #59430 (Renames `EvalContext` to `InterpretCx`) - #59439 (Generalize diagnostic for `x = y` where `bool` is the expected type) - #59449 (fix: Make incremental artifact deletion more robust) - #59451 (Add `Default` to `std::alloc::System`) - #59459 (Add some tests) - #59460 (Include id in Thread's Debug implementation) Failed merges: r? @ghost
Fix #53879.