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

Include fn take_mut<T, F: FnOnce(T) -> T>(&mut T, F) in libstd #1508

Open
Sgeo opened this issue Feb 23, 2016 · 10 comments
Open

Include fn take_mut<T, F: FnOnce(T) -> T>(&mut T, F) in libstd #1508

Sgeo opened this issue Feb 23, 2016 · 10 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@Sgeo
Copy link

Sgeo commented Feb 23, 2016

It is possible to write a function to temporarily take, by value, a T out of a &mut T, replacing it by the end of the closure. Panics can be handled by killing the process. Crate to do this: https://github.com/Sgeo/take_mut/tree/v0.1.3 , take_mut::take.

This might be reasonable to include in std::mem, possibly under a different name. It's a function that effectively extends what it's possible to do with a &mut T in safe code, similarly to std::mem::swap and std::mem::replace.

Drawbacks include that we might not want to encourage use of functions that, in the worst case, do things more extreme than panicking (exiting or aborting the process).

More bikeshedding opportunities on whether it should exit or abort on panic,

@oli-obk
Copy link
Contributor

oli-obk commented Feb 23, 2016

How about automatically implementing a DoesntPanic trait for all functions that are guaranteed not to panic? The analysis that figures out whether a function can panic can be done by MIR once the MIR-passes are powerful enough (optimize and run DCE often enough and no calls to panic or to panicking functions should be left for a properly written function). This would be somewhat like transmute guarantees that the types have the same size.

@bluss
Copy link
Member

bluss commented Feb 23, 2016

I'll mention a concern that was raised: This API allows a free T value to exist even though an API only ever exposed it as &mut T and no constructors. I can't immediately see if that's ok or not.

@Stebalien
Copy link
Contributor

@oli-obk

How about automatically implementing a DoesntPanic trait for all functions that are guaranteed not to panic?

As long as there's some #[unsafe_no_panic] escape hatch.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 23, 2016

As long as there's some #[unsafe_no_panic] escape hatch.

you can always manually implement the unsafe DoesntPanic marker trait for your function. But for closures we'd need something else.

@Stebalien
Copy link
Contributor

@oli-obk Unfortunately, functions aren't currently types.

@glaebhoerl
Copy link
Contributor

The problem with ideas like DoesntPanic is trait methods and indirect calls. Handling them adds a ton of complexity (now it has to be some kind of first-class aspect of the type system), not handling them makes the effectiveness very limited. In practice I don't think it would carry its weight.

Another option alongside "just abort" that could be offered in case the function panics is to replace the stolen T with a backup value. One way is to pass a T directly:

fn take_mut_backup<T, F: FnOnce(T) -> T>(from: &mut T, backup: T, body: F) -> T

This type signature may be surprising at first. How it works is, if body panics, it replaces the contents of from with backup, and then continues unwinding. If it doesn't panic, then it returns the backup value back to you. (I.e., you might expect that it should return an Option<T>, but this is why it doesn't. In the case where it uses up the backup value and would return a None, it re-panics instead, and so doesn't return anything.) This signature avoids any possibility of aborts.

Another option is to produce the backup value only on demand:

fn take_mut_lazy_backup<T, F: FnOnce(T) -> T, B: FnOnce() -> T>(from: &mut T, backup: B, body: F)

Now in this case, the backup closure itself could panic. So it first calls body; if body doesn't panic, great; if body does panic, it calls backup; if backup doesn't panic, it replaces from with the produced value, and continues unwinding; and if backup panics too, then it aborts.

And there could be a convenience take_mut_default_backup which just uses Default::default as the backup closure.

(The reason all of these propagate the panic from body is that otherwise we would have created recover, which is a cough problematic function.)

So for anyone not keeping track, that's 4 different variants on this functionality it potentially makes sense to provide.

(I did not put any thought into naming.)

@glaebhoerl
Copy link
Contributor

I'm not sure if the never-aborting take_mut_backup offers any expressiveness gain over just using mem::replace (since both of them require that you already have a T, which is the fundamental restriction that take_mut lifts).

@durka
Copy link
Contributor

durka commented Feb 24, 2016

With the current formulation of drop flags, a convenient backup value is the drop flag. If you overwrite the T with the drop flag by calling ptr::read_and_drop, you can continue unwinding and the drop machinery is tricked into not dropping it again. This worked when I tried it in take_mut but I'm not 100% sure it's safe. And when drop flags are moved to the stack... I have no idea if a similar functionality will be possible (mem::drop_flag_for(&T) -> &mut bool??).

@oli-obk
Copy link
Contributor

oli-obk commented Feb 24, 2016

While this doesn't call Drop twice, if it's in a struct-field, the outer struct's Drop impl might access it.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Aug 18, 2016
@ticki
Copy link
Contributor

ticki commented Sep 1, 2016

#1736.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

8 participants