Implement inner deref for Option and Result #50267
Conversation
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/libcore/result.rs
Outdated
| @@ -909,6 +909,38 @@ impl<T: Default, E> Result<T, E> { | |||
| } | |||
| } | |||
|
|
|||
| #[unstable(feature = "inner_deref", reason = "newly added", issue = "50264")] | |||
| impl<T: Deref, E: Deref> Result<T, E> { | |||
There was a problem hiding this comment.
For maximum usability, these should probably be three impl blocks. One for T: Deref, one for E: Deref and one with both bounds. Otherwise you can't use deref_ok on a Result<String, u32> I think.
|
Can you add some more tests? Especially around starting out with non-reference types and checking whether String becomes str, Vec becomes [T], ... Maybe even compile-fail tests for the cases like Ok(42).deref_ok() and all other variants of it |
|
Yes, I can do that. A request: if you have other feedback, it helps to get it all at once (or to know that the review is incomplete, so I will know to wait), since context switching, diving back in, re-running tests, etc. has costs. I'll let this sit for a day or three, in case there is more feedback from you or from others, then I can address the feedback efficiently. |
src/libcore/option.rs
Outdated
| @@ -878,6 +878,17 @@ impl<T: Default> Option<T> { | |||
| } | |||
| } | |||
|
|
|||
| # [unstable(feature = "inner_deref", reason = "newly added", issue = "50264")] | |||
There was a problem hiding this comment.
There's a space between the # and the [
|
Other than squashing down (multiple commits are fine, merge commits are not), I have no further suggestions. |
|
Thanks, @oli-obk. |
|
I'm not so sure about naming the method |
|
Is having a function name equal to the trait name automatically bad? If so, why--if adhering to this guideline yields "such awkward names", is the cure as bad as the disease? Or is there a larger problem that this guideline addresses? (Honestly asking to learn.) If it is a problem, there were also other suggestions made (as you predicted ;)) on the IRC channel including |
U007D
force-pushed
the
master
branch
2 times, most recently
from
ea442e5
to
a279dc2
Compare
|
All actionable feedback provided to this point should now be addressed--please let me know if you have any comments or questions. Everyone, please take a look at src/libcore/tests/result.rs. As this is the first In particular, I am concerned about what I have labeled "Odd corner cases" starting on l.293: I'm wondering if, for example, calling |
|
IMO,
So it does! In that case (and because Result doesn't impl Deref, and because Deref specifically is an operator trait with special connotations anyway) i totally take back my objections. |
@ExpHP is it actually a crime though, or are you simply expressing that there is a precedent?
@QuietMisdreavus mmm--I do like that. The only wrinkle I see so far is that In our case, though, I know that you are aware of this, but for the record I'll state that there is precedent for this in the standard library--the simplest example I can think of is IMO, having "wrong variant" cases ideally no-op if (contractually) possible and panic if not seems like the right thing to do/expected behavior. In the |
|
@U007D It seems there's a bit of a misunderstanding here. I'd personally forgotten that the signature also took the reference of the "other" variant when making that statement, but i consider that just fine. I consider these methods as a sort of super-charged |
|
@U007D I was simply stating precedent. To be entirely honest, in my own projects I have more or less adopted it as a convention, and I often have inherent methods named |
|
@alexcrichton Great--I'll resolve the conflicts this weekend. Thank you! |
|
Ping from triage @U007D It's been a while since we heard from you, will you have time to work on this again? |
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors: r+ |
|
📌 Commit 56016cb has been approved by |
bors
added
S-waiting-on-bors
Mark-Simulacrum
changed the title
inner_deref PR tracking issue: rust-lang#50264
|
⌛ Testing commit 56016cb with merge a01fe877531f466aefbe9224f9d7c92ed06a51cd... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-bors
Implement inner deref for Option and Result tracking issue: #50264
|
☀️ Test successful - status-appveyor, status-travis |
tracking issue: #50264