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

Add RFC: formalise reborrows #2364

Closed
wants to merge 2 commits into from
Closed

Conversation

dhardy
Copy link
Contributor

@dhardy dhardy commented Mar 19, 2018

Summary: Formalise the re-borrowing logic used on &T and &mut T.

Motivation: Solve #1403: Some way to simulate &mut reborrows in user code.

Rendered

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Mar 19, 2018
@dhardy
Copy link
Contributor Author

dhardy commented Mar 19, 2018

Related to this there is what could be considered a bug in this example:

trait Counter { ... }
impl<'a, C: Counter + ?Sized> Counter for &'a mut C { ... }
fn do_thing<C: Counter>(mut c: C) { ... }

let mut c: &mut Counter = ...;
do_thing(c); // does not reborrow
do_thing::<&mut Counter>(c); // identical except does reborrow

@ExpHP
Copy link

ExpHP commented Mar 19, 2018

The unexpressible type here can be expressed via generic associated types (tracking issue)


Edit: even that is less than ideal. What we reallly want here is to also constrain the output type to be the same as the input (up to some lifetime).

@eddyb
Copy link
Member

eddyb commented Mar 19, 2018

Since this is a coercion, you might want to take a look at CoerceUnsized, which could theoretically be generalized to allow any kind of coercion (and thus become simply Coerce).


When a parameter `x` is passed into a function `f` (e.g. `f(x)`),

- if `x` supports `Copy` [and `x` is later reused], then a copy of `x` will
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is necessary to qualify this with x is later reused. A copy is just a move that does not mark the value as uninitialized.


- if `x` supports `Copy` [and `x` is later reused], then a copy of `x` will
be passed into `f`
- if `x` supports `Reborrow` [and `x` is later reused], then a reborrow of `x`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here, though in this case it's an honest question from me; does it matter whether x is later used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would presume the optimiser can replace a reborrow with a move — maybe that should be mentioned explicitly.

@dhardy
Copy link
Contributor Author

dhardy commented Mar 20, 2018

The unexpressible type here can be expressed via generic associated types (tracking issue)

It's more complicated than that unfortunately, because we don't know how many lifetimes are needed (e.g. the type (&'a str, &'b str) uses two lifetimes), and should allow reborrowing of each independently.

@burdges
Copy link

burdges commented Mar 21, 2018

I think exploring CoerceUnsized sounds wise. We already have lifetime shrinking in

impl<'a, 'b, T, U> CoerceUnsized<&'a U> for &'b T where
    'b: 'a,
    T: Unsize<U> + ?Sized,
    U: ?Sized, 

Also, there are already several distinct impls of CoerceUnsized for both &T and &mut T, indicating that reborrowing might require several impls too.

I suppose associated type makes sense if you could really only reborrow in one way though:

trait Reborrow {
    type Reborrowed;  // ReborrowedSelf, ReSelf, etc.
    fn reborrow(&self) -> Reborrowed;
}

@dhardy
Copy link
Contributor Author

dhardy commented Mar 21, 2018

A reborrow is supposed to yield the same type except that all lifetimes are fresh, subsets of the corresponding lifetimes, and (when mutable) block the parent lifetimes until expiry. Thus there should only be one way to reborrow; anything else is a coercion I guess.

This is not merely a life-time shrinkage; I really don't think CoerceUnsized will solve this, nor do I think the return type is expressible (unless one restricts arbitrarily to supporting a single sub-lifetime only).

Personally I think the next move should be to try implementing this; despite the unexpressible lifetime I think it should be possible. I was going to try, but am busy with other things now, and don't have much experience with the compiler internals.

P.S. do we even have formal notation to express that one &mut T blocks another &mut T (i.e. disallows use of the parent) until after the child has expired? Or is that directly implied by 'a: 'b when using &mut references?

@burdges
Copy link

burdges commented Mar 21, 2018

I think your Option example makes sense as roughly

impl<'a, T, U> CoerceUnsized<Option<&'a mut U>> for Option<&'a mut T> where
    T: Unsize<U> + ?Sized,
    U: ?Sized, 
impl<'a, 'b, T, U> CoerceUnsized<Option<&'a U>> for Option<&'b mut T> where
    'b: 'a,
    T: Unsize<U> + ?Sized,
    U: ?Sized, 

You'll notice CoerceUnsized never shrinks mutable lifetimes. We have only

impl<'a, T, U> CoerceUnsized<&'a mut U> for &'a mut T where
    T: Unsize<U> + ?Sized,
    U: ?Sized, 

and

impl<'a, 'b, T, U> CoerceUnsized<&'a U> for &'b mut T where
    'b: 'a,
    T: Unsize<U> + ?Sized,
    U: ?Sized, 

It's frequently important the mutable lifetime cannot be shrunk inside say rent_to_own, which corresponds to &'a T being (co)variant over 'a and T, while &'a mut T is (co)variant over 'a but invariant over T. I suppose CoerceUnsized must happen inside other types T, so they must require this restriction to maintain invariance of T in &mut T.

Is this invariance restriction workable? If no then I suppose this explains your desire for a new Reborrow trait. In that case, CoerceUnsized is CoerceInside and Reborrow is CoerceOutside, which can strictly generalize CoerceInside. If yes then I do not quite understand the issue with CoerceUnsized.

As an aside, we presumably cannot generically CoerceUnsized for technical reasons, like internal NonZero optimizations, etc.

impl<TT, UU> CoerceUnsized<Option<UU>> for Option<TT> where
    TT: CoerceUnsized<UU>

@eddyb
Copy link
Member

eddyb commented Mar 22, 2018

Thus there should only be one way to reborrow; anything else is a coercion I guess.

There are no automatic reborrows in Rust when passing a value that aren't coercions.

As an aside, we presumably cannot generically CoerceUnsized for technical reasons, like internal NonZero optimizations, etc.

That's not the case - CoerceUnsized is the way it is because it's specified by an RFC.
There's nothing stopping us from generalizing it, and .

For the Option:

impl<T: Coerce<U>, U> Coerce<Option<U>> for Option<T> {}

For mutable reborrows: that's not in the library implementations of CoerceUnsized because we special-case it before invoking CoerceUnsized.

You can definitely shrink the 'a in &'a mut T, however, doing it by-value consumes the &mut, which is not usually what you want. You need the built-in non-consuming &mut T -> &mut T coercion which is equivalent to &mut *x. If we have a generalized Coerce, it'd work.

@eddyb
Copy link
Member

eddyb commented Mar 22, 2018

Personally I think the next move should be to try implementing this; despite the unexpressible lifetime I think it should be possible. I was going to try, but am busy with other things now, and don't have much experience with the compiler internals.

FWIW, as a @rust-lang/compiler member I'm opposed to adding anything that behaves like a coercion but introduces new machinery instead of generalizing the existing coercion machinery.

Not only is the proposed "inexpressible type" pretty much impossible to implement in the current compiler, it's also unsound (see the rules about unconstrained lifetimes in return types).
There's only one way I could think of, and that's special-casing the method in a very hacky way.

Also, the existing &'a mut T -> &'b mut T reborrow coercions we have today are driven by known source and target types, as opposed to starting with &'a mut T and getting &'b mut T.
This is why you can sometimes move &mut T by passing it to something more generic (e.g. id).

@nikomatsakis
Copy link
Contributor

@rfcbot fcp postpone

So I strongly agree with the motivation of this RFC, but I don't feel the timing is right. It seems to me that the changes here intersect three distinct ongoing tasks, all of which are higher priority. I'd rather wait to see those tasks play out before layering changes on top. The tasks are:

  • transition to MIR-based borrow check and NLL
    • after all, we have to consider the value borrowed
  • transition to Chalk
    • in particular, I'd like to handle coercions in a more principled way
  • custom self types (Custom self types #2362)
    • that RFC also will build on CoerceUnsized and likely require changes

(Generally speaking, I think I agree with @eddyb that something along the lines of CoerceUnsized might be right for this -- basically a way to "opt-in" to some specific coercions that the compiler can do for you, but without opening the flood gates to arbitrary code.)

In any case, for the reasons above, I move to postpone.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Mar 22, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 22, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Mar 25, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 25, 2018

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

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Mar 25, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 4, 2018

The final comment period is now complete.

@Centril
Copy link
Contributor

Centril commented Apr 4, 2018

Postponed per #2364 (comment) and completed FCP.

Thanks @dhardy!

@Centril Centril closed this Apr 4, 2018
@Centril Centril added the postponed RFCs that have been postponed and may be revisited at a later time. label Apr 4, 2018
@earthengine
Copy link

earthengine commented Aug 14, 2018

I have a internal post a few weeks ago, proposed a solution of this without lifetime shrinking or coercing. However it needs to adjust our mental model from "reborrowing" to "reviving".

The idea is, when call a function like f(a)

  • The value a is always move to function f
  • If the type of a is something we are going to support like &mut T, the value of a is revived afterwards. In other words, the variable a will be rebinded to a revived value of the moved value, so its lifetime is started right after the moved value ends.
  • If the type of a is Copy, the same thing happens. In addition, the value of a was copied before the function call, allowing you to send multiple copies to the function. But anyways, as the original value is always moved, value a will have a newly started lifetime after the use.

I assume the above should always indistinguishable in case of Copy. So it only enables some types to be "revival". I would also like to see the analogies of Clone: Revive trait as to Copy just like Resurrect as to Clone, so Revive is automatic marker trait, and Resurrect is manually implement trait.

@dhardy
Copy link
Contributor Author

dhardy commented Aug 14, 2018

@earthengine this doesn't solve the first example in the RFC:

allow copying of Option<&mut T>

@eddyb
Copy link
Member

eddyb commented Aug 14, 2018

@earthengine Note that types not invariant over lifetimes (so anything that only uses shared references) have subtyping (e.g. Option<&'a T> is a subtype of Option<&'b T>) so the reborrow of (types containing only) shared references is a noop, and doesn't need any new solutions.

I still prefer a generalized form of opt-in "coerce a type by coercing its fields" (#2364 (comment)).

@earthengine
Copy link

@dhardy

I understand Option<&mut T> is still not Copy; but why this thing should be copied? if it contains a Some variant, we will need to "revive" it after use rather than copying it to maintain type safety. When it is None it can be copied though but that's another story.

@earthengine
Copy link

earthengine commented Aug 15, 2018

@eddyb

I know reborrow is no-op like copy. But like copy, revive is also no-op (the binary representation of &mut T is still there after move, revive only need to make sure that the moved value is dead to ensure we can mark it as valid again), as like Copy don't have methods, Revive don't have as well. On the other hand, Resurrect is like Clone, it is implemented and to be used explicitely, for cases you need to do something extra to "resurrect" the value.

And the proposal is to use revive instead of “reborrow", making it happen in all types that is applicable. So for example, (i32,&mut v) will be able to revive automatically after use (we can be conservitive on this to require user defined structs that have #[derive(Revive,Resurrect)] annotation).

@dhardy
Copy link
Contributor Author

dhardy commented Aug 15, 2018

@earthengine the point isn't to "copy" but to "borrow", however having to deal with &mut Option<&mut T> (or, in case there is another path where None can occur, Option<&mut Option<&mut T>>) is clumsy and not always easy to work into existing code.

@earthengine
Copy link

earthengine commented Aug 16, 2018

@dhardy

The idea of revive is allow you to reuse the value after the value have been moved. So the following will compile:

let mut i = 42;
let mut v = Some(&mut i);
match v {
    Some(v) => *v+=1,
    None => {}
}
match v {
    Some(v) => *v+=1,
    None => {}
}

if Option<&mut T> implements Revive. This is because, after the v value being moved in the first match block, once the moved value dead the original v revives, so this justify the second match block.

Because Revive is derived automatically, &mut Option<&mut T> and Option<&mut Option<&mut T>> will all being able to Revive.

In general, Copy enables f(v,v) and Revive enables f(v);f(v);.

Is this answered your question?

@dhardy
Copy link
Contributor Author

dhardy commented Aug 16, 2018

Revive would need to be usable as a trait bound to work with examples like this, but that part is easy enough.

The hard part in your proposal is making "revive" work with lifetime analysis. My proposal here is that you call f(v.reborrow()); f(v.reborrow()); such that each reborrowed item has a partially independent lifetime, restricted to be shorter than that of v. Your approach instead pushes the same lifetime into f — does this make lifetime analysis harder?

Further, Revive is fundamentally incompatible with Drop, though this may not matter since it only concerns references.

But I don't really know why you're trying to convince me. I only wrote an RFC...

@earthengine
Copy link

@dhardy

But I don't really know why you're trying to convince me. I only wrote an RFC...

Emm. Ok, So if you are interested maybe we can discuss in my post?

@earthengine
Copy link

As something implied in my "Revive" proposal, something is also apply to this RFC.

We should consider making closures implement Reborrow when all captured variables are Reborrow, and if so, FnMut is implemented in terms of Reborrow, because Reborrow means you can obtain Self within a smaller lifetime, and so you can pass this reborrowed Self to call_once.

This is similar to Copy: any closures implements Copy will be FnMut and Fn as both call_mut and call can be called on a copy of Self, and this is what was already happening today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. postponed RFCs that have been postponed and may be revisited at a later time. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants