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

Introduce {Ref, RefMut}::try_map for optional projections in RefCell #78455

Merged
merged 3 commits into from
Jan 16, 2021
Merged

Introduce {Ref, RefMut}::try_map for optional projections in RefCell #78455

merged 3 commits into from
Jan 16, 2021

Conversation

udoprog
Copy link
Contributor

@udoprog udoprog commented Oct 27, 2020

This fills a usability gap of RefCell I've personally encountered to perform optional projections, mostly into collections such as RefCell<Vec<T>> or RefCell<HashMap<U, T>>:

This kind of API was briefly featured under Open questions in #10514 back in 2013 (!)

let values = RefCell::new(vec![1, 2, 3, 4]);
let b = Ref::opt_map(values.borrow(), |vec| vec.get(2));

It primarily avoids this alternative approach to accomplish the same kind of projection which is both rather noisy and panicky:

let values = RefCell::new(vec![1, 2, 3, 4]);

let b = if values.get(2).is_some() {
    Some(Ref::map(values.borrow(), |vec| vec.get(2).unwrap()))
} else {
    None
};

Open questions

The naming opt_map is preliminary. I'm not aware of prior art in std to lean on here, but this name should probably be improved if this functionality is desirable.

Since opt_map consumes the guard, and alternative syntax might be more appropriate which instead tries to perform the projection, allowing the original borrow to be recovered in case it fails:

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

This would be more in line with the try_map method provided by parking lot.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2020
@udoprog udoprog changed the title Introduce {Ref, Mut}::opt_map for optional component access Introduce {Ref, Mut}::opt_map for optional component access in RefCell Oct 27, 2020
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 13, 2020
@Dylan-DPC-zz
Copy link

r? @KodrAus

@rust-highfive rust-highfive assigned KodrAus and unassigned sfackler Dec 8, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Dec 16, 2020

This seems like a reasonable addition to me. We could consider a method like try_map, but in that case I think it should return a impl Try rather than an Option:

fn try_map<F, U, R>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, R::Error>
where
    F: FnOnce(&T) -> R,
    R: Try<Ok = &U>

What do you think?

@udoprog
Copy link
Contributor Author

udoprog commented Dec 16, 2020

This seems like a reasonable addition to me. We could consider a method like try_map, but in that case I think it should return a impl Try rather than an Option:

fn try_map<F, U, R>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, R::Error>
where
    F: FnOnce(&T) -> R,
    R: Try<Ok = &U>

What do you think?

Thanks for taking a look. This seems good to me. I'll take it for a spin!

@udoprog udoprog changed the title Introduce {Ref, Mut}::opt_map for optional component access in RefCell Introduce {Ref, RefMut}::try_map for optional projections in RefCell Dec 16, 2020
@udoprog
Copy link
Contributor Author

udoprog commented Dec 16, 2020

Updated and seems to work 🎉

@RustyYato
Copy link
Contributor

@udoprog your current implementation is unsound! You cannot name the lifetimes like so:

fn try_map<F, U, R>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, R::Error>
where
    F: FnOnce(&'b T) -> R,
    R: Try<Ok = &'b U>

Because this allows you to extract the value out of the Ref (playground). Note: the playground link uses a dummy implementation of RefCell and Ref, but the problem will persist in the real implementation.

Here's one (clunky and really bad) solution that works playground. But it doesn't play with closures well, so you'll need to use functions playground. Or we could not generalize to Try in the first place, and be satisfied with Result or Option (my preferred option).

@udoprog
Copy link
Contributor Author

udoprog commented Dec 16, 2020

@RustyYato doh, I had a feeling something was gonna go wrong with removing the HRTB's. Thanks for showcasing it!

FWIW, returning Result<Ref<'b, T>, Self> would be my preferred approach as well.

@udoprog
Copy link
Contributor Author

udoprog commented Dec 17, 2020

For now I've at least pushed an impl that returns Option<..> which should be sound.

While working on it I noticed that a variant that returns Result<..., Self> would require a raw pointer since the error variant would need to reconstruct the guard from the mutable reference being passed in. I'll be pushing that on a separate branch, cause I'm not entirely sure it's a sound approach.

Here it is: udoprog@fc60063#diff-75a679ddd1aae61c8b60e45cea1fb213a086caee3600993c68af9f7a09785e9eR1444

@RustyYato
Copy link
Contributor

You can simplify the match a bit

match f(unsafe { &mut *value }) {
    Some(value) => Ok(RefMut { value, borrow }),
    None => Err(RefMut { value: unsafe { &mut *value }, borrow }),
}

But yes, this is sound (exactly because of HRTB 😁)

@KodrAus
Copy link
Contributor

KodrAus commented Jan 14, 2021

Hmm, I don't think I'd want to stabilize try_map without Try, but if we wanted to punt on it and use Option then we could just call it filter_map?

@udoprog
Copy link
Contributor Author

udoprog commented Jan 14, 2021

Hmm, I don't think I'd want to stabilize try_map without Try, but if we wanted to punt on it and use Option then we could just call it filter_map?

I'm fine either way. I do prefer one that produces Result<Ref<'b, U>, Self> because it allows reconstruction of the original value if the projection fails but have no idea what to call it if not try_map (see: parking_lot).

But I'll defer to other people's judgement. Tell me what to do and I'll do it.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 15, 2021

I do prefer one that produces Result<Ref<'b, U>, Self> because it allows reconstruction of the original value if the projection fails

Ah that's an interesting point 🤔 That might be a better way to go then since this is already a niche API. I'm still thinking Ref::filter_map is a more appropriate name than try_map, because we're not fallible inside the closure, it's just that what we're projecting might be None. That would align with a Ref::map method that can always project individual fields, and a Ref::try_map method (that we can't currently write generically) that's fallible.

So what do we think of:

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

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 

The use of Result allows for making use of a reconstructed original value on failed
projections.
@udoprog
Copy link
Contributor Author

udoprog commented Jan 15, 2021

Using filter_map for the name now, which returns a Result.

@KodrAus KodrAus mentioned this pull request Jan 16, 2021
3 tasks
@KodrAus
Copy link
Contributor

KodrAus commented Jan 16, 2021

Thanks @udoprog! I've opened #81061 as a tracking issue and updated the #[unstable] attributes to reflect that.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2021

📌 Commit c625b97 has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 16, 2021
Introduce {Ref, RefMut}::try_map for optional projections in RefCell

This fills a usability gap of `RefCell` I've personally encountered to perform optional projections, mostly into collections such as `RefCell<Vec<T>>` or `RefCell<HashMap<U, T>>`:

> This kind of API was briefly featured under Open questions in rust-lang#10514 back in 2013 (!)

```rust
let values = RefCell::new(vec![1, 2, 3, 4]);
let b = Ref::opt_map(values.borrow(), |vec| vec.get(2));
```

It primarily avoids this alternative approach to accomplish the same kind of projection which is both rather noisy and panicky:
```rust
let values = RefCell::new(vec![1, 2, 3, 4]);

let b = if values.get(2).is_some() {
    Some(Ref::map(values.borrow(), |vec| vec.get(2).unwrap()))
} else {
    None
};
```

### Open questions

The naming `opt_map` is preliminary. I'm not aware of prior art in std to lean on here, but this name should probably be improved if this functionality is desirable.

Since `opt_map` consumes the guard, and alternative syntax might be more appropriate which instead *tries* to perform the projection, allowing the original borrow to be recovered in case it fails:

```rust
pub fn try_map<U: ?Sized, F>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, Self>
where
    F: FnOnce(&T) -> Option<&U>;
```

This would be more in line with the `try_map` method [provided by parking lot](https://docs.rs/lock_api/0/lock_api/struct.RwLockWriteGuard.html#method.try_map).
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 16, 2021
Introduce {Ref, RefMut}::try_map for optional projections in RefCell

This fills a usability gap of `RefCell` I've personally encountered to perform optional projections, mostly into collections such as `RefCell<Vec<T>>` or `RefCell<HashMap<U, T>>`:

> This kind of API was briefly featured under Open questions in rust-lang#10514 back in 2013 (!)

```rust
let values = RefCell::new(vec![1, 2, 3, 4]);
let b = Ref::opt_map(values.borrow(), |vec| vec.get(2));
```

It primarily avoids this alternative approach to accomplish the same kind of projection which is both rather noisy and panicky:
```rust
let values = RefCell::new(vec![1, 2, 3, 4]);

let b = if values.get(2).is_some() {
    Some(Ref::map(values.borrow(), |vec| vec.get(2).unwrap()))
} else {
    None
};
```

### Open questions

The naming `opt_map` is preliminary. I'm not aware of prior art in std to lean on here, but this name should probably be improved if this functionality is desirable.

Since `opt_map` consumes the guard, and alternative syntax might be more appropriate which instead *tries* to perform the projection, allowing the original borrow to be recovered in case it fails:

```rust
pub fn try_map<U: ?Sized, F>(orig: Ref<'b, T>, f: F) -> Result<Ref<'b, U>, Self>
where
    F: FnOnce(&T) -> Option<&U>;
```

This would be more in line with the `try_map` method [provided by parking lot](https://docs.rs/lock_api/0/lock_api/struct.RwLockWriteGuard.html#method.try_map).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2021
Rollup of 17 pull requests

Successful merges:

 - rust-lang#78455 (Introduce {Ref, RefMut}::try_map for optional projections in RefCell)
 - rust-lang#80144 (Remove giant badge in README)
 - rust-lang#80614 (Explain why borrows can't be held across yield point in async blocks)
 - rust-lang#80670 (TrustedRandomAaccess specialization composes incorrectly for nested iter::Zips)
 - rust-lang#80681 (Clarify what the effects of a 'logic error' are)
 - rust-lang#80764 (Re-stabilize Weak::as_ptr and friends for unsized T)
 - rust-lang#80901 (Make `x.py --color always` apply to logging too)
 - rust-lang#80902 (Add a regression test for rust-lang#76281)
 - rust-lang#80941 (Do not suggest invalid code in pattern with loop)
 - rust-lang#80968 (Stabilize the poll_map feature)
 - rust-lang#80971 (Put all feature gate tests under `feature-gates/`)
 - rust-lang#81021 (Remove doctree::Import)
 - rust-lang#81040 (doctest: Reset errors before dropping the parse session)
 - rust-lang#81060 (Add a regression test for rust-lang#50041)
 - rust-lang#81065 (codegen_cranelift: Fix redundant semicolon warn)
 - rust-lang#81069 (Add sample code for Rc::new_cyclic)
 - rust-lang#81081 (Add test for rust-lang#34792)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@scottmcm
Copy link
Member

-> Result<Ref<'b, U>, Self>

With a return type like that, I think it makes sense to do this in a way that doesn't use Try -- always using Self as the error type makes this materially different, to me, from something like Iterator::try_find.

So 👍 to phrasing this as filter_map and just supporting Option.

@bors bors merged commit 6bb06f4 into rust-lang:master Jan 16, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.