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

Tracking Issue for cell_filter_map #81061

Closed
2 of 3 tasks
KodrAus opened this issue Jan 16, 2021 · 8 comments · Fixed by #97308
Closed
2 of 3 tasks

Tracking Issue for cell_filter_map #81061

KodrAus opened this issue Jan 16, 2021 · 8 comments · Fixed by #97308
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@KodrAus
Copy link
Contributor

KodrAus commented Jan 16, 2021

Feature gate: #![feature(cell_filter_map)]

This is a tracking issue for Ref::filter_map and RefMut::filter_map, which lets you optionally project data inside a Ref or RefMut, wrapped in a Ref or RefMut. If the projection returns None then these methods return a Err(Self) so that you can still get the original Ref or RefMut back out.

Public API

impl<'b, T: ?Sized> Ref<'b, T> {
    pub fn filter_map<U: ?Sized, F>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, Self>
    where
        F: FnOnce(&T) -> Option<&U> {}
}

impl<'b, T: ?Sized> RefMut<'b, T> {
    pub fn filter_map<U: ?Sized, F>(orig: RefMut<'b, T>, f: F) -> Result<RefMut<'b, U>, Self>
    where
        F: FnOnce(&mut T) -> Option<&mut U> {}
}

Steps / History

Unresolved Questions

  • Naming and signature: We wanted to call this method try_map and accept a closure like FnOnce(&T) -> R where R: Try<Ok = &U>, but that's not sound with bound lifetimes. This method is only ok when the mapping closure uses HRTB. So we opted to call it filter_map instead, with the closure returning a Option<&U>.
@KodrAus KodrAus added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Jan 16, 2021
@Robbepop
Copy link
Contributor

Robbepop commented Apr 18, 2021

I am in need of this API for my library in order to be able to have an efficient implementation that avoids unsafe Rust code.
I wonder, why is the goal to have try_map instead of filter_map since I can see a great value in having both APIs available.
So if filter_map is already good to go we could stabilize that one and later add try_map as soon as all open questions for it are resolved.

@hniksic
Copy link
Contributor

hniksic commented Apr 24, 2021

The name filter_map() seems unusual for something that returns a Result - for example, Iterator::filter_map() returns an iterator with filtering built in. The equivalent method already exists in the Mutex implementations of tokio and parking_lot, under the name try_map(). In both cases the closure it accepts returns Option<&U> and not the more general impl Try. So maybe the name try_map should be reconsidered in that light.

@lucassardois
Copy link

I'm also interested in the stabilization of this API for the same reason as @Robbepop. However, I also agree that filter_map() should be named try_map() since it returns a Result. As an API user it makes more sense to me.

@tony612
Copy link

tony612 commented Sep 13, 2021

I wonder if we can have a generic filter & map for any inner enum instead of just Option.

For example in this code

pub enum Foo<T> {
    A(),
    B(T),
}

pub struct Bar {}

pub struct User {
    pub data: RefCell<Foo<Bar>>,
}

impl User {
    pub fn get_data(&self) -> std::result::Result<Ref<'_, Bar>, String> {
        let v = self.data.borrow();
        if let Foo::B(_) = *v {
            Ok(Ref::map(v, |x| match x {
                Foo::B(b) => b,
                _ => unreachable!(),
            }))
        } else {
            Err(String::from("error"))
        }
    }
}

As the code show, data ref is compared twice to get the result. Is there any way to make it simpler?

@m-ou-se m-ou-se added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Mar 9, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Mar 16, 2022

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 16, 2022

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 16, 2022
@rfcbot
Copy link

rfcbot commented Mar 19, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 29, 2022
@rfcbot
Copy link

rfcbot commented Mar 29, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Mar 29, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 31, 2022
@m-ou-se m-ou-se removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Apr 6, 2022
@bors bors closed this as completed in af15e45 May 24, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
…Simulacrum

Stabilize `cell_filter_map`

FCP has been completed: rust-lang/rust#81061 (comment)
Closes #81061
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants