Tracking issue for cell_extras stabilization

[aturon]

The Ref type connected to RefCell supports some methods for changing the type of the reference, such as map and filter_map. While map is clearly fully general, filter_map is somewhat of a special case hooked into Option. To make it fully general would likely require HKT. We need an overall plan before stabilization.

[aturon]

cc @SimonSapin

[SimonSapin]

This is the kind of thing (unlike say ref_slice #27774) that is impossible to do reasonably outside of std. (Where “unreasonable” is guessing struct layout to access private fields.)

[SimonSapin]

(Ok, thinking a bit more about it, this may not be true. Something like https://github.com/Kimundi/owning-ref-rs/ could, using (Ref<'a, T>, *const U) and unsafe code to dereference the raw pointer, have equivalent functionality with a different API (separate type) and 3 words instead of 2 per reference.)

[mbrt]

What about making RefCell::map stable? It has good use cases and it is clearly usable as-is.

[SimonSapin]

I’d like to nominate at least Ref::map and RefMut::map for stabilization.

filter_map’s functionality can be safely built on top of it with a performance cost (testing the presence of the mapped thing twice). For example:

fn borrow_get<'a>(hashmap: &'a RefCell<HashMap<String, String>>, key: &str) 
                  -> Option<Ref<'a String>> {
    let hashmap: = hashmap.borrow();
    if hashmap:.contains_key(key) {  // Duplicated hash table lookup.
        Some(Ref::map(|hashmap| {
            hashmap[key]  // panic!() for missing key unlikely to be optimized away
        }))
    } else {
        None
    }
}

It’s probably possible to do it unsafely without the run-time cost with raw pointers. (But still less risky than guessing the memory layout of Ref or RefMut.)

[SimonSapin]

For future reference, here is fully generic filter_map built on top of map using raw pointers. I’m fairly confident that it’s safe.

use std::cell::{Ref, RefMut};

pub fn ref_filter_map<
    T: ?Sized,
    U: ?Sized,
    F: FnOnce(&T) -> Option<&U>
>(orig: Ref<T>, f: F) -> Option<Ref<U>> {
    f(&orig)
        .map(|new| new as *const U)
        .map(|raw| Ref::map(orig, |_| unsafe { &*raw }))
}

pub fn ref_mut_filter_map<
    T: ?Sized,
    U: ?Sized,
    F: FnOnce(&mut T) -> Option<&mut U>
>(mut orig: RefMut<T>, f: F) -> Option<RefMut<U>> {
    f(&mut orig)
        .map(|new| new as *mut U)
        .map(|raw| RefMut::map(orig, |_| unsafe { &mut *raw }))
}

[alexcrichton]

🔔 This issue is now entering its cycle final comment period to be handled in 1.8 🔔

We're specifically considering stabilizing map and deprecating filter_map. We're also willing to stabilize map on the lock-related guards as well so long as they land soon.

[reem]

As just noted in #30834:

I've discovered a safety hole in MutexGuard::map - it is not safe in combination with Condvar, since the "inner" borrow can be invalidated by another thread taking the lock during a Condvar::wait see: http://is.gd/RM038X.

This is not a problem with the RwLock guard methods, since the RwLock guards cannot be used with a Condvar.

It would be possible to provide a similar, but safe, API by defining a separate type for "mapped" mutex guards that can't be used with a Condvar.

[alexcrichton]

With MutexGuard::map being removed in #31428, we may wish to also remove the RwLock*Guard::map functions even though they're not used with CondVar at this time. Windows actually has the ability to wait on condvars with rwlocks via SleepConditionVariableSRW, which we also use internally. That seems like it would be a viable platform-specific extension, and perhaps even an extension we could support via other trickery. (I think C++ has something like condition_variable_any?)

[aturon]

@alexcrichton Certainly we shouldn't stabilize them, and I tend to agree that we should probably remove them and do this in a more considered way.

[ghost]

If you decide to keep these functions (or at least the one on RwLock I guess), the lifetimes should be removed from the closure like in Ref::map, or at least not made the same as the underlying container. By using the same lifetime as the container, the reference can escape from the closure. See: #25747 (comment)

http://is.gd/vUbM4D (the example in the link above modified for RwLock currently results in use-after-free).
http://is.gd/f0AtWL (using a function with the signature changed fails to compile)

[reem]

I need a RwLock that can be waited on for other reasons, so I implemented an equivalent to C++17s std::shared_mutex which is a fair reader-writer lock implemented using a mutex and two condvars.

I went ahead and also demoed a (I believe) safe API for map/option_map/result_map in the same crate based on having a different type for mappable and waitable guards. I think we could apply the same strategy to both RwLock and Mutex in std.

Code: https://github.com/reem/rust-shared-mutex
Mappable Guards: https://github.com/reem/rust-shared-mutex/blob/6dca082c864df3d46fa952ff89f7c813aa7b2012/src/lib.rs#L379-L504

EDIT: Also submitted #31440 to fix the safety hole noted by @whataloadofwhat and update the unstable reason to include a note about a possible bad interaction with Condvar.

[alexcrichton]

The libs team discussed this during triage yesterday and the decision was to:

• Stabilize Ref::map and RefMut::map
• Deprecate everything else (if it still exists), including filter_map

[alexcrichton]

Reopening as apparently Ref::clone is still under this umbrella.

[alexcrichton]

Also cc #33880, possible unsoundness with that method.

[alexcrichton]

@rfcbot fcp merge

Sounds like Ref::clone is ready for stabilizing

[rfcbot]

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, 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]

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

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

[rfcbot]

The final comment period is now complete.