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

ACP: Add {Ref,RefMut}::try_map method #341

Open
GrigorenkoPV opened this issue Feb 21, 2024 · 2 comments
Open

ACP: Add {Ref,RefMut}::try_map method #341

GrigorenkoPV opened this issue Feb 21, 2024 · 2 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@GrigorenkoPV
Copy link

Proposal: Add {Ref,RefMut}::try_map method

Problem statement

The filter_map method introduced in 1.63 extended the existing map method by allowing the conversion to be fallible. However, in case of a failure, the conversion doesn't have a straight-forward way to return any value.

Motivating examples or use cases

  1. Consider this piece of code from the Rust compiler:
RefMut::filter_map(
    self.result.borrow_mut(),
    |r| r.get_or_insert_with(|| f().map(Steal::new)).as_mut().ok(),
)
.map_err(|r| *r.as_ref().unwrap().as_ref().map(|_| ()).unwrap_err())
.map(QueryResult)

Using try_map it can be rewritten as follows:

RefMut::try_map(
    self.result.borrow_mut(),
    |r| r.get_or_insert_with(|| f().map(Steal::new)).as_mut()
)
.map_err(|(_, &mut e)| e)
.map(QueryResult)

This removes the need to use 2 unwraps.

  1. The use-case where I personally encountered the need for such API:
use anyhow::{anyhow, Error};
use std::collections::{hash_map::Entry as HashMapEntry, HashMap};

pub fn start_new_segment(
    segments: RefMut<'_, HashMap<Key, Vec<Value>>>,
    key: Key,
) -> Result<RefMut<'_, Vec<Value>>, Error> {
    RefMut::try_map(segments, |map| match map.entry(key) {
        HashMapEntry::Occupied(e) => Err(anyhow!("{} already exits: {:?}", e.key(), e.get())),
        HashMapEntry::Vacant(e) => Ok(e.insert(vec![])),
    })
    .map_err(|(_, e)| e)
}

Solution sketch

https://github.com/rust-lang/rust/pull/118087/files

Design considerations

  1. Do we need to return the original RefMut in case of failure:
    • Pros:
      • More general, potentially covers more use-cases.
      • Consistent with filter_map.
    • Cons:
      • You have to .map_err(|(_, e)| e) to use ?
      • Need to return 2 values in Result::Err.
  2. What do we put in Resut:Err in return value?
    1. A struct (pros: field order doesn't matter, cons: Occam's razor)
    2. A tuple (pros: more ergonomic, cons: have to decided on the order of values)
  3. Can we generalize this using Try / Residual?

    I have considered generalizing this to use the Try & Residual just like #79711 does for array::try_map, but it does not seem to be possible: we want to essentially .map_err(|e| (orig, e)) but this does not seem to be supported with Try. (Plus I am not even sure if it is possible to express the fact that &U in Try::Output would have to have the same lifetime as the &T input of F.)
    [src]

Alternatives

External implementation using Option and unwrap

pub fn try_map<'a, T, U, E>(
    r: RefMut<'a, T>,
    f: impl FnOnce(&mut T) -> Result<&mut U, E>,
) -> Result<RefMut<'a, U>, (RefMut<'a, T>, E)>
where
    T: ?Sized + 'a,
    U: ?Sized + 'a,
{
    let mut error = None;
    RefMut::filter_map(r, |t| match f(t) {
        Ok(u) => Some(u),
        Err(e) => {
            error = Some(e);
            None
        }
    })
    .map_err(|r| (r, error.unwrap()))
}

External implementation using unsafe

pub fn try_map<'a, T, U, E>(
    mut r: RefMut<'a, T>,
    f: impl FnOnce(&mut T) -> Result<&mut U, E>,
) -> Result<RefMut<'a, U>, (RefMut<'a, T>, E)>
where
    T: ?Sized + 'a,
    U: ?Sized + 'a,
{
    let t: *mut T = r.deref_mut();
    let u: *mut U = match f(unsafe { &mut *t }) {
        Ok(u) => u,
        Err(e) => return Err((r, e)),
    };
    Ok(RefMut::<'a, T>::map(r, |_| unsafe { &mut *u }))
}

(Might be incorrect)

Links and related work

rust-lang/rust#118087

@GrigorenkoPV GrigorenkoPV added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Feb 21, 2024
@Slixe
Copy link

Slixe commented Sep 10, 2024

I also have a need of this feature which would greatly help with easier error handling when dealing with Ref/RefMut.

@GrigorenkoPV
Copy link
Author

I also have a need of this feature which would greatly help with easier error handling when dealing with Ref/RefMut.

Well, this will probably take ages before it's accepted (and even more before it's stabilized), just as everything that has to do with libs-api, but meanwhile you can use one of the workaround from the OP.

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

2 participants