Functional record update should respect privacy (or consume a non-Copy source)

[arielb1]

Tracking issue for: rust-lang/rfcs#736

Old Description:

Functional record update does not mark the source as consumed, even if does not implement Copy. This means that e.g. the source's destructor will eventually be run, so the following code prints ["Heap Spray"]:

pub fn free_and_use<T>(t: Vec<T>) -> Vec<T> {
    Vec { ..t }
}

fn main() {
    let v = vec!["Hello, World!"];
    let r = free_and_use(v);
    for _ in range(0, 65536u32) {
        vec!["Heap Spray"];
    }
    println!("{}", r);
}

[arielb1]

Note that, as @glaebhoerl discovered, you can also explicitly consume t, also allowing code like:

pub fn free_and_use<T>(t: Vec<T>) -> Vec<T> {
    let result = Vec { ..t };
    drop(t); result /* or drop(result); t */
}

[kmcallister]

💣

[bluss]

Is Vec { ..t } legal, even if all members are private? It's a bit surprising

[arielb1]

@bluss

It is. There's the potentially-useful case where some of the members are private, and FRU allows you to update the public ones. Of course it should consume the source even then.

[pnkfelix]

P-backcompat-lang, 1.0 beta

[nikomatsakis]

In my opinion the problem here is the privacy violation. The idea is that Vec { ..v } is expanded to Vec { f: v.f, g: v.g } and so forth. In this case, that does not trigger a move, because the types of those fields are Copy. I think that part is acting as expected. What is surprising is that one can do this from outside the vec module.

[glaebhoerl]

That was my opinion as well (but my favored solution isn't minor).

[pnkfelix]

When I initially saw this problem, I did not see it as a privacy violation, for the (obvious?) reason that since no private members of v had been mentioned in the code { ..v }, the client module is not breaking any abstraction introduced by privacy.

I think this line of thinking is analogous to @arielb1's outline of the useful case where FRU allows one to update the public members of a type and silently move the private ones as well, via Bar { x: new_x, y: new_y, ..old_b }. This is useful for the case where the author of Bar wants to reserve the right to add new private members to Bar while still allowing for clients to use the convenient FRU syntax.

---

I originally was going to try to explain my point of view via analogy with something from hygienic macros: If I were to define (and export) a macro within Bar's defining module:

mod foo {
    pub struct Bar { ... }
    #[macro_export_or_whatever]
    macro_rules! bar_update_xy {
        ($x:expr, $y:expr, $b:expr) =>
          { Bar { x: $x, y: $y,
            other_1: $b.other_1,
            [... manually list other fields here ...]
            other_k: $b.other_k } }
    }
}

let b1: Bar = ...;
let b2 : Bar = bar_update_xy!(new_x(), new_y(), b1));

then it should, in principle, not be a privacy violation to use bar_update_xy! from client code outside Bar's defining module. (That is, in principle, hygiene should allow macros to expose controlled access to private internals.)

Then I realized the crucial problem with that kind of analogy: bar_update_xy! is something that the designer of Bar has to choose to put into the module; i.e., it is opt-in. But currently in Rust, FRU is not opt-in; its not even something you can opt out of. Any struct automatically is given support for it, whether it be appropriate for that struct or not. And that seems wrong, at least given examples like the one provided here in #21407.

This latter point, that providing support for FRU should not be something that is silently attached to a type like Vec, seems somewhat in line with the thinking given in @glaebhoerl's draft RFC. But I am also pretty sure we cannot adopt anything of the scope given in that draft. (Still, maybe there's a hint of solution embedded within, in the sense of having some way of marking a type as data vs abstract; see final two bullet points below.)

---

So, as I see it, we can either:

1. Figure out a way to support the point-of-view outlined here in the description for Functional record update should respect privacy (or consume a non-Copy source) #21407, in that Bar { x: new_x, y: new_y, ..old_b } should somehow be treated as consuming old_b.

   Such a change seems like it would be quite subtle, since we would need to have some way of expressing that code like { x: old_b.x.f(), y: old_b.y.g(), ..old_b } is sound, even if the types of the fields x and y are consumed by their respective f and g methods.

   (In other words, ..old_b need to consume old_b, yet somehow not completely consume it. Maybe there's a way to think of it as consuming "whatever is left of the type", I do not know.)

   Advantages: This would address the bug as described by the issue author, and would not break any code that was not already broken.

   Drawbacks: It is not clear what the semantics are for this form (in other words: the phrase "would not break any code" above is only true if you first accept that this hypothesized semantics exists). Also, there may not be time to do this properly for 1.0.

2. Or, try to adopt a data/abstract-type distinction along the lines of the one in @glaebhoerl's draft RFC. (I'm not going to write Advantages/Drawbacks for this; I think we clearly do not have time to do it that way for 1.0)

3. Or, change FRU into a non-hygienic expansion into the record construction form with all fields listed, in the sense that Vec { ..v } is synonymous with Vec { ptr: v.ptr, len: v.len, cap: v.cap } and subject to privacy checks.

   Advantages: Seems really simple to implement, and is consistent with at least one core team member's mental model of FRU.

   Drawbacks: If we did this, then code like the Bar example above would break.

4. Or, let FRU keep its current hygienic (aka "privacy violating") semantics, but also make FRU something one must opt-in to support on a type.

   E.g. make a builtin FunUpdate trait that a struct must implement in order to be usable with FRU. (Or maybe its an attribute you attach to the struct item.)

   Advantages: Also seems really simple to implement (in the language). Accommodates users of FRU for the Bar example.

   Drawbacks: If we did this, it would impose a burden on all code today that makes use of FRU, since they would have to start implementing FunUpdate. Thus, not simple to implement for the libraries and the overall ecosystem.

5. Or, a hybrid between the previous two bullet-points: By default, treat FRU as non-hygienic expansion, but add a builtin HygienicFunUpdate trait that one can opt-into to get the hygienic (aka "privacy violating") semantics. As a bonus, perhaps implementing Copy on a struct would also imply implementing HygienicFunUpdate on that struct, which seems like it would basically give us something very similar to the semantics desired by @arielb1.

   (Or, again, maybe its an attribute you attach to the struct item. This detail does not particularly matter to me.)

   Advantage: While this is obviously more complicated than the previous two bullet-points, it has the advantage that it has a staged landing strategy: We could just implement the change to FRU to be non-hygienic expansion for 1.0 beta. We could add HygienicFunUpdate at an arbitrary point in the future; it would not have to be in the 1.0 release.

   Drawback: People have claimed that some of my past proposals were a bit ... baroque. I imagine this one is no exception. :) (In other words, this semantics is a little complicated to explain to a newcomer.)

[glaebhoerl]

I don't want to make a big fuss about it, but I do want to note, again, that our "deadlines" are entirely self-imposed, and that under these circumstances "we don't have time" is slightly odd, because we can make time, any time we want. The blanket refusal to even contemplate doing that - even in light of new information - and to weigh the tradeoffs on their own merits is something I still don't really understand.

That out of the way, an additional possibility, let's call it 6., for @pnkfelix's list:

• We could just make the "abstract type" vs. "data type" distinction based on "does or doesn't the type have a private field". This doesn't entirely fill me with satisfaction (as with "worse is better" solutions in general), with specific drawbacks being that it doesn't extend cleanly to the nullary case (no fields at all), and that in terms of UX a change of semantics based on the presence or absence of a private field may be unexpected, but on the plus side, it's simpler, and would, I think, work.

[pnkfelix]

@glaebhoerl Sure, hypothetical show-stopper issues could cause us to revisit our self-imposed schedule. I personally don't think that this issue, on its own at least, provides sufficient motivation for schedule revision. I admit that your draft RFC seems like it addresses a host of issues, so maybe all of them taken in unison would be a different story. (But then again, that's not even my call to make.)

[pnkfelix]

There were a couple more options that I should have put on the list, for completeness, but overlooked.

• 6 @glaebhoerl's note, the "has any private fields ==> abstract-type; all public fields ==> data-type", is one of them. This would (probably) be less work than adopting the draft RFC in full, but I am not sure what the other Advantages/Disadvantages are. I invite others to write an Advantages/Drawbacks for this approach (perhaps in an RFC PR).

• 7 We could add a way for a struct to opt-out of FRU support entirely. So in this approach, one would attach the appropriate #[no_fun_update] attribute to Vec, HashMap, etc.

  Advantages: won't break any existing code

  Drawbacks: Seems quite fragile; we may be good about auditing our own stdlib, but clients who are not aware of the attribute may inadvertently expose themselves to the same bug that currently plagues Vec<T>.