Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Option::as_deref_inner: Option<&T> -> Option<&T::Target> where T: Deref #407

Closed
tesuji opened this issue Jul 8, 2024 · 7 comments
Closed
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@tesuji
Copy link

tesuji commented Jul 8, 2024

Proposal

Problem statement

Currently, we have Option::as_deref to transform &Option<T> to Option<&T::Target>. But because there is impl Deref for &T with Target = T. as_deref(opt: &Option<&T>) where T: Deref always returns Option<&T>, not what we look for, Option<&T::Target>.

Motivating examples or use cases

Given this code:

pub fn get(index: &HashMap<usize, Vec<i32>>, id: usize) -> &[i32] {
     if let Some(names) = index.get(&id) { names } else { &[] }
}

User could try to rewrite it by using combinators:

index.get(&id).unwrap_or(&[]) // compile errors: wrong type
// they try to fix the above error
index.get(&id).as_deref().unwrap_or(&[]) // the error remains: wrong type
// okay, that's unexpected, now try again
index.get(&id).map(|v| &**v).unwrap_or(&[]) // finally!

That was frustrating because as_deref promises that &Option<T> -> Option<&T::Target>. But they don't see that when T=&U, it will become &Option<&U> -> Option<&U>, which is not always what they want.

Solution sketch

Add this implementation

impl<T: Deref> Option<&T> {
    fn as_deref_inner(self) -> Option<&T::Target>; // may require a better name when stabilizing
    // mut version maybe
}
// maybe impl for Result

Or add a lint to war about this pitfall if the build passes. If the build fails, guide people to use map(|v| &**v) instead.

Alternatives

Links and related work

Zulip: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Make.20.60Option.3C.26T.3E.3A.3Aas_deref.28.29.20returns.20Option.3C.26T.3A.3ATarget.3E.60

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@tesuji tesuji added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jul 8, 2024
@tgross35
Copy link

tgross35 commented Jul 8, 2024

Original tracking issue and stabilization for as_deref, might be worth some time digging to figure out what the motivation for the current design were.

Any chance we could update the existing as_deref to be more flexible in a backward-compatible way, rather than adding a new method?

@tesuji
Copy link
Author

tesuji commented Jul 8, 2024

I don't think we could because &T implements Deref: &T -> T. So this holds true for the signature of as_deref:

impl<T> Option<T> {
    fn as_deref(&self) -> Option<&T::Target> where T: Deref;
}
// replace T = &U
impl<U> Option<&U> {
    fn as_deref(&self) -> Option<&U>;
}

@programmerjake
Copy link
Member

can we add a mut version at the same time for consistency?

@tesuji tesuji changed the title New API to change Option<&T> to Option<&T::Target> where T: Deref Add Option::as_deref_inner: Option<&T> -> Option<&T::Target> where T: Deref Jul 9, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Jul 9, 2024

We discussed this in the libs-api meeting. We don't think this method outweighs the cost in readability: it's relatively uncommon and it's not immediately clear what it does when encountering this in code, and it doesn't seem to provide enough value over the alternative over map with &**.

@m-ou-se m-ou-se closed this as completed Jul 9, 2024
@dtolnay dtolnay closed this as not planned Won't fix, can't repro, duplicate, stale Jul 9, 2024
@tesuji
Copy link
Author

tesuji commented Jul 9, 2024

Ty for letting me know that. Is improving compile errors when the build fail with as_deref also considered?

@dtolnay
Copy link
Member

dtolnay commented Jul 9, 2024

Improving compile errors would be welcome.

Current error, for the motivating use case above:

error[E0308]: mismatched types
   --> src/lib.rs:5:30
    |
5   |     index.get(&id).unwrap_or(&[])
    |                    --------- ^^^ expected `&Vec<i32>`, found `&[_; 0]`
    |                    |
    |                    arguments to this method are incorrect
    |
    = note: expected reference `&Vec<i32>`
               found reference `&[_; 0]`
help: the return type of this call is `&[_; 0]` due to the type of the argument passed
   --> src/lib.rs:5:5
    |
5   |     index.get(&id).unwrap_or(&[])
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^---^
    |                              |
    |                              this argument influences the return type of `unwrap_or`

It should be possible for rustc to suggest replacing this specific instantiation of unwrap_or with map_or(&[], Vec::as_slice). Maybe similarly for some other common cases: map_or("", String::as_str).

@Skgland
Copy link

Skgland commented Jul 10, 2024

I could have also used this several times recently, the name as_deref_inner is a bit of a mouth full, and my suggestion would have been as_reref as it acts similar to &* where one dereferences and the re-references a reference. (play-ground)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

6 participants