Better handling for unwrapping reference to Result (or Option)

[ghost]

Having such simplified code:

fn main() {
    let result: Result<String, String> = Ok("ok".to_string());
    let ref_result = &result;
    let x = ref_result.unwrap();
}

gives:

error[E0507]: cannot move out of borrowed content
 --> src/main.rs:4:13
  |
4 |     let x = ref_result.unwrap();
  |             ^^^^^^^^^^ cannot move out of borrowed content

error: aborting due to previous error

error: Could not compile `playground`.

Thanks to irc Kimundi & Amarnath I got the solution & expanation:

// use:
let x = ref_result.as_ref().unwrap();

Explanation:

• unwrap requires full ownership because it moves values
  • see self in signature?
• ref_result is only borrowing, (ref_result is a reference, a reference means borrowing)

Using .as_ref() works because:
It creates a brand new Result with references to inner value, like:
from initial:

`&Result<T, V>`

to brand new one:

`Result<&T, &V>`

See implementation of Result.as_ref here

If i got this correctly, using unwrap on &Result will always be corrupted.
My question is, could this be handled internally somehow by Rust?
And/Or better compilation error would be awesome for noobs like me :)
I'm not sure what the message should look like but the current one is a bit general.

[ExpHP]

If i got this correctly, using unwrap on &Result will always be corrupted.

You are correct, though there is one exception for the case where both T and E impl Copy. In this case, Result<T, E> impls Copy, and thus calling a self function on a &Result<T, E> becomes allowed.

My question is, could this be handled internally somehow by Rust?

This is no trivial matter, and is something that would probably require an RFC and require lots of discussion (and probably additions to the language) to make happen. Consider Option, which is in a similar boat:

impl<T> Option<T> {
    pub fn as_ref(&self) -> Option<&T> {
        match *self {
            Option::Some(ref x) => Option::Some(x),
            Option::None => Option::None,
        }
    }
    
    pub fn unwrap(self) -> T {
        match self {
            Option::Some(x) => x,
            Option::None => panic!("unwrap on None"),
        }
    }
    
    pub fn unwrap_or_default(self) -> T
    where T: Default, {
        match self {
            Option::Some(x) => x,
            Option::None => Default::default(),
        }
    }
}

Both unwrap() and unwrap_or_default() are functions with the signature fn(Option<T>) -> T, but only one of them can used together with with as_ref() to behave like a fn(&Option<T>) -> &T. Hence there would need to be something else in the signature to reflect this difference in capability; some way to indicate that unwrap() can be made polymorphic over ownership versus references.

(well, okay; technically the where T: Default bound is a difference between the signatures, and is in fact the key thing that makes .as_ref().unwrap_as_default() fail to compile; but attempting to apply this information in any sort of general fashion seems nontrivial, and I fear it could pose problems for specialization)

---

One final thought: It is unfortunate that default match binding modes will further blur the line between &Result<T, E> and Result<&T, &E>, potentially making issues like this feel even more subtle to beginners. But this was already anticipated...

[ghost]

but only one of them can used together with with as_ref() to behave like a fn(&Option) -> &T. Hence there would need to be something else in the signature to reflect this difference in capability; some way to indicate that unwrap() can be made polymorphic over ownership versus references.

(well, okay; technically the where T: Default bound is a difference between the signatures, and is in fact the key thing that makes .as_ref().unwrap_as_default() fail to compile; but attempting to apply this information in any sort of general fashion seems nontrivial, and I fear it could pose problems for specialization)

I don't get it. How the code for that would look like?

Anyway, If it's really non trivial case then a better error message will be good enough I think.
What do you @ExpHP think?
Maybe I'm even able to fix it. I hope I don't overestimate my skills ;)

[ExpHP]

Anyway, If it's really non trivial case then a better error message will be good enough I think.
What do you @ExpHP think?

Seems possible. The question is, what is the scope of the lint? Just Option and Result? Does the compiler try to "guess" for the presence of methods like as_ref() based on their signature? Should it also suggest clone, which is also frequently often the right solution for Option?

I will cc @estebank who has been prolific with diagnostic hints and may have better ideas.

[ExpHP]

I don't get it. How the code for that would look like?

I guess I was thinking of something like

impl<T> Option<T> {
    // The `&?` would be a new syntax that indicates the function
    // is polymorphic over `self`, `&self`, and `&mut self`
    fn unwrap(&? self) -> &? T {
        // I envision `match` as the language primitive which would make it
        // possible to implement a function with such a signature, by extending
        // the upcoming "match default binding modes" feature to support `&?`.
        match self {
            Some(x) => x,
            None => panic!("unwrap on None value"),
        }
    }

    // The above function would behave as though these three functions existed.
    //
    // I'm including the lifetimes explicitly to show that `unwrap` would have
    // a variable number of lifetime parameters based on how it is called;
    // this is something not yet possible to express in rust, and I imagine
    // it would be a huge barrier to adoption and implementation.
    fn unwrap(self) -> T;
    fn unwrap<'a>(&'a self) -> &'a T;
    fn unwrap<'a>(&'a mut self) -> &'a mut T;

    // `unwrap_or_default` would keep its old signature because it can not
    // be made polymorphic over references
    fn unwrap_or_default(self) -> T where T: Default;
}

The new &? syntax would make the signatures of unwrap and unwrap_or_default different enough to clearly demonstrate why one can be called on references while the other cannot.

However, an idea like this would need to go through the RFC process, where I imagine it would be quite contentious, and where it would be competing against other ideas that solve similar problems.

[ghost]

Now I got it, thanks for explanation.
Anyway kill me, but I'm not sure I like it yet. :) It's a bit parallel (a few meanings of &?) and implicit? I don't know.. .
However I would happily see better compilation error, maybe I'm able even to contribute it?

[estebank]

There're a couple of tickets open to turn the error into something along the lines of

error[E0507]: cannot move out of borrowed content
 --> src/main.rs:4:13
  |
4 |     let x = ref_result.unwrap();
  |             ^^^^^^^^^^ -------- `unwrap` consumes `ref_result`
  |             |
  |             cannot move out of borrowed content

This wouldn't be helpful enough, but it is incremental improvement.

It could be possible that the machinery to suggest implementing traits that have a given method could be used to suggest as_ref() here, but I would tackle both action items separately.

[ghost]

Maybe it could be also added to https://doc.rust-lang.org/error-index.html ?

[asquared31415]

Currently in 1.62.0 stable the output is now

error[[E0507]](https://doc.rust-lang.org/stable/error-index.html#E0507): cannot move out of `*ref_result` which is behind a shared reference
 --> src/main.rs:4:13
  |
4 |     let x = ref_result.unwrap();
  |             ^^^^^^^^^^^^^^^^^^^ move occurs because `*ref_result` has type `Result<String, String>`, which does not implement the `Copy` trait
  |
help: consider borrowing the `Result`'s content
  |
4 |     let x = ref_result.unwrap().as_ref();
  |                                +++++++++

However that suggestion is still wrong, and applying it will cause an error with regards to type inference which will lead you down a path that won't work. The ideal suggestion is to put the .as_ref() before the .unwrap().

[estebank]

CC #90286

[estebank]

Triage: current output in beta is correct:

error[E0507]: cannot move out of `*ref_result` which is behind a shared reference
 --> src/main.rs:4:13
  |
4 |     let x = ref_result.unwrap();
  |             ^^^^^^^^^^^--------
  |             |          |
  |             |          `*ref_result` moved due to this method call
  |             move occurs because `*ref_result` has type `Result<String, String>`, which does not implement the `Copy` trait
  |
note: this function takes ownership of the receiver `self`, which moves `*ref_result`
help: consider calling `.as_ref()` to borrow the type's contents
  |
4 |     let x = ref_result.as_ref().unwrap();
  |                        +++++++++

[Dylan-DPC]

Current error:

error[[E0507]](https://doc.rust-lang.org/stable/error_codes/E0507.html): cannot move out of `*ref_result` which is behind a shared reference
 --> src/main.rs:4:13
  |
4 |     let x = ref_result.unwrap();
  |             ^^^^^^^^^^^--------
  |             |          |
  |             |          `*ref_result` moved due to this method call
  |             help: consider calling `.as_ref()` or `.as_mut()` to borrow the type's contents
  |             move occurs because `*ref_result` has type `Result<String, String>`, which does not implement the `Copy` trait
  |
note: `Result::<T, E>::unwrap` takes ownership of the receiver `self`, which moves `*ref_result`
 --> /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/core/src/result.rs:1083:19
help: you can `clone` the value and consume it, but this might not be your desired behavior
  |
4 |     let x = ref_result.clone().unwrap();
  |                        ++++++++

For more information about this error, try `rustc --explain E0507`.

[estebank]

I believe that there won't be any changes to the stdlib here, and the diagnostic is already "good enough". I'd like to fix the consider calling as_ref or as_mut to be a structured suggestion, but at least is there. I believe it is safe to close this ticket at this time.