Rust: borrow-checker allows partial reinit of struct that has been moved away, but no use of it.

[pnkfelix]

Here is some sample code:

#[derive(Show)]
struct Pair { lft: u8, rgt: u8, }

fn main() {
    let mut p = Pair { lft: 1, rgt: 2 };
    let mut f = move |&mut:| { p.lft = 3; p.rgt };
    p.rgt = 4;
    println!("f: {:?}", f());
    p.lft = 5;
    // println!("p: {:?}", p);
}

This compiles without an error; running it prints f: 2u8.

If you uncomment the last line, you get an error saying "error: use of moved value: p". Likewise if you just attempt to print individual fields.

While I do see the logic being used here ("we are not overwriting the value within the closure itself; we are writing into the uninitialized memory that resulted from moving p into the closure"), it seems potentially confusing.

Is there any reason that we allowing these writes, which cannot be subsequently read? It seems like a situation analogous to moving into a moved-away array (see https://github.com/rust-lang/rfcs/pull/533 )

[pnkfelix]

(however, unlike rust-lang/rfcs#533, it would not be all that terrible for non-zeroing dynamic-drop if we did not get around to making this illegal; we're already going to have to support representing structure fragments to implement non-zeroing dynamic drop, and initializations like the one in this ticket are just another case of that. Plus we might in the future add the ability to track such partial initializations and allow reads from initialized fields in partially-initialized structs.)

[mbrubeck]

If this doesn't become an error, it should at least be a warning. Code like this is almost certainly a logic error of some kind, but currently compiles without error or warning:

struct Foo { x: i32 }
let mut f = Foo { x: 0 };
drop(f);
f.x = 1;

[pnkfelix]

triage: P-backcompat-lang, 1.0 beta

(but do see comments above noting that its not the end of the world if we did not fix this.)

[pnkfelix]

triage: P-backcompat-lang (1.0 beta)

just trying to accommodate highfive...

[nikomatsakis]

Interesting, I see the bug being that you get an error at all -- that is, I think that once you have fully "reinitialized" the structure, you should be able to read it again. (Rather than the ability to partially reinit the struct as being an error.)

[nikomatsakis]

I'm going to tag this as I-Needs-Decision.

[mbrubeck]

Note that this _is_ currently an error if the moved value has a destructor. For example, if you add impl Drop for Foo {fn drop(&mut self) {}} to the previous example:

struct Foo { x: i32 }
impl Drop for Foo { fn drop(&mut self) {} }
let mut f = Foo { x: 0 };
drop(f);
f.x = 1;

then it is rejected with "error: partial reinitialization of uninitialized structure f."

[pnkfelix]

at this point I think I would be in favor saying that we can live with the current semantics as is today, and fix things in the future backwards-compatibly to either 1. track the initialized parts and allow reading from them, as I mentioned at the end of my comment, or 2. the slightly more limited (but perhaps more in line with our current compiler infrastructure) approach of tracking when the entire (non-Drop) structure is reinitialized and _then_ allow reads from it, as outlined in niko's comment.

So, bascially, I'm willing to reclassify this as a P-low, not 1.0 bug.

[pnkfelix]

triage: P-low ()

[pnkfelix]

(apparently empty brackets does not clears the milestone, though they may have at one point?)

[pnkfelix]

@mbrubeck also, if you wanted to try to develop a lint to warn about this case, I would not object (assuming it did not have too many false positives).

[nikomatsakis]

@mbrubeck yes, we are more restrictive about structs with destructors, because in that case it doesn't make sense to drop one part and not another.

[logannc]

Real quick question after seeing this on Reddit:

When we are "writing into the uninitialized memory that resulted from moving p into the closure", where does that written value go? Does the compiler actually emit write instructions? If so, could that lead to a vulnerability by overwriting an important field?

(I would check the LLVM output myself, but I wouldn't have a clue how to read it.)

[huonw]

It will generate writes to p's stack slot, but it shouldn't cause any problems: see also https://github.com/rust-lang/rust/issues/26665.

[gnzlbg]

@logannc If one omits the assignment to a:

fn main() {
    let mut a = A{val: 1};
    let a_ = a;
    /// a.val = 2;   // In the original example this line is uncommented
    println!("a_: {}", a_.val);
    /// println!("a: {}", a.val);  // error: use of moved value: `a.val` ?!?!
}

the optimizer should omit the stack slot of a since assigning to a and then moving into a_ should be optimized into just initializing a_. However, the code in the example is equivalent to just initializing a_ and having an uninitialized a variable that is initialized later:

fn main() {
    let mut a : A; // uninitialized
    let a_ = A{val: 1};
    a.val = 2;   // Initialize a here
    println!("a_: {}", a_.val);
    /// println!("a: {}", a.val);  // error: use of possibly uninitialized variable
}

However, I do not understand why using a reinitialized variable results in an error, e.g. by uncommenting the lines in the above examples.

It seems that writing to uninitialized memory:

• does not reinitialize the variable, and thus
• the value that was written cannot be used.

This makes no sense: either one cannot write to uninitialized memory, or doing so reinitializes the variable so that it can be used. Being able to write to uninitialized memory but not being able to use that value is weird.

[steveklabnik]

Triage: updated code

#[derive(Debug)]
struct Pair { lft: u8, rgt: u8, }

fn main() {
    let mut p = Pair { lft: 1, rgt: 2 };
    let mut f = move || { p.lft = 3; p.rgt };
    p.rgt = 4;
    println!("f: {:?}", f());
    p.lft = 5;
    println!("p: {:?}", p);
}

Same error happens.

[qthree]

Most confusing example

struct Point { x: u32, y: u32 }
fn main() {
    let mut point: Point = Point { x: 1, y: 2 };
    drop(point);
    point.x = 3;
    println!("{}", point.x); //use of moved value: `point.x`
}

It works like

struct Point { x: u32, y: u32 }
fn main() {
    let mut point: Point = Point { x: 1, y: 2 };
    drop(point);
    {
        let mut point: Point
        point.x = 3;
    }
    println!("{}", point.x); //use of moved value: `point.x`
}

when it should work like

struct Point { x: u32, y: u32 }
fn main() {
    let mut point: Point = Point { x: 1, y: 2 };
    drop(point);
    {
        let mut point: Point
        point.x = 3;
        println!("{}", point.x); //use of possibly uninitialized variable: `point.x`
    }
}

[Havvy]

Doesn't this mean that adding an impl for Drop to a type a backwards incompatible change?

[nikomatsakis]

@Havvy if the type has public fields, yes (this has always been the case, though)

[withoutboats]

I was trying to demonstrate to a newer Rust user how it won't allow you to create cyclic structures without shared ownership constructs. I wrote this code, to create a linked list loop:

struct Node {
    next: Option<Box<Node>>,
}

fn main() {
    let mut first = Node { next: None };
    let second = Node { next: Some(Box::new(first)) };
    first.next = Some(Box::new(second));
}

Of course, this currently compiles. This was surprising to me at first, and I modified it to examine the type to figure out what was going on:

#[derive(Debug)]
struct Node {
    next: Option<Box<Node>>,
}

fn main() {
    let mut first = Node { next: None };
    let second = Node { next: Some(Box::new(first)) };
    first.next = Some(Box::new(second));
    println!("{:?}", first);
}

This didn't compile, because the println was a use after move.

To a user who doesn't already know this quirk of Rust, this sequence of events would be completely baffling.

[FraGag]

Just wanted to mention that variables that are _never_ fully initialized also exhibit the same behavior: you can set individual fields but you can't read them, nor can you read the whole struct even if each field has been initialized separately.

#[derive(Debug)]
struct Foo {
    a: i32,
    b: i32,
}

fn main() {
    let mut foo: Foo; // `foo` is not initialized, we're just giving a type hint
    foo.a = 1; // no error
    foo.b = 2; // no error
    println!("{}", foo.a); // error[E0381]: use of possibly uninitialized variable: `foo.a`
    println!("{:?}", foo); // error[E0381]: use of possibly uninitialized variable: `foo`
}

[richardwhiuk]

What's the next step on this issue? Do we need an RFC about what the change is here?

Given https://github.com/rust-lang/rust/issues/21232#issuecomment-77457249, it feels like it would be fairly easy to add a warning here - which would make it more consistent with when Drop trait is implemented?

[nikomatsakis]

Note: if/when we do fix this, we have to be careful about things like unions:

union Foo { a: u32, b: u32 }

fn main() {
  let foo: Foo;
  foo.a = 22;
  foo.b = 44; // error
}

[abonander]

A new user on Reddit was surprised by this behavior when attempting to implement a singly linked list: https://old.reddit.com/r/rust/comments/98r22a/hey_rustaceans_got_an_easy_question_ask_here/e4pakxb/

[abonander]

However, it seems like there's two orthogonal issues here:

1. the borrow checker allows partial reinit but not reuse
2. partial reinit marks the value as used (or rather, doesn't mark it as unused) so no warnings are issued

I feel like we could spin #2 off into a simple lint issue while continuing to discuss how to fix #1.

[abonander]

@nikomatsakis opinion on the above?

[pnkfelix]

tagging with A-NLL because I think it makes a lot of sense for us to address any potential change in static semantics here as part of the overall NLL effort.

[pnkfelix]

tagging with NLL-complete under the assumption that the goal will be to start allowing one to read from the initialized fields of partially initialized structs.

[scuzzycheese]

tagging with NLL-complete under the assumption that the goal will be to start _allowing_ one to read from the initialized fields of partially initialized structs.

Sorry for the stupid question, but what would the purpose of this be?

[pnkfelix]

@scuzzycheese well, I think the long term goal would be to allow one to initialize structs piece meal, i.e.:

struct S { x: u32, y: u32 }

fn main() {
    let s: S;
    s.x = 10;
    s.y = 30;
    a_use_of(s);
}

Its a style of code that some people like.

and if you're going to allow the previous example, then why not:

fn main() {
    let s: S;
    s.x = 10;
    s.y = s.x + 20;
    a_use_of(s);
}

---

To be clear, this feature is not something that I'm personally wedded to, but when faced with the question of whether to reject or accept code like:

• fn main() { let s: S; s.x = 10;,
  I start thinking about variants like
• fn main() { let s: S; s.x = 10; a_use_of(s.x); },

and I guess I start thinking of cases like the above.

[pnkfelix]

Nominating for discussion at lang team meeting.

Namely, the summary is:

• Our current status is weird and confusing:
  
  
  
  • Under old and new borrowck, we accept fn main() { let mut s: S; s.x = 10; }
    
    
    
    
    
    • but we reject fn main() { let mut s: S; s.x = 10; s.y = 30; a_use_of(s); }
    
    
    
    • (and we also reject fn main() { let mut s: S; s.x = 10; a_use_of(s.x); })
    
    
    
  
  
• There's no strong reason that we're rejecting these cases, not in terms of soundness, at least.
  
  
  
  • (Note in particular that the struct S here does not implement Drop. You don't need to worry about ensuring it is completely initialized before a potential unwind of the stack.)
  
  
• We can either start rejecting code in 2018 edition, (and leave ourselves the freedom to choose whether to accept or reject later on)...
• Or we can keep the status quo, and still leave ourselves the freedom to start accepting more code. (But we wouldn't be able to start rejecting new cases, at least not until some further later edition...)

[scuzzycheese]

@scuzzycheese well, I think the long term goal would be to allow one to initialize structs piece meal, i.e.:

struct S { x: u32, y: u32 }

fn main() {
    let s: S;
    s.x = 10;
    s.y = 30;
    a_use_of(s);
}

Its a style of code that some people like.

and if you're going to allow the previous example, then why not:

fn main() {
    let s: S;
    s.x = 10;
    s.y = s.x + 20;
    a_use_of(s);
}

To be clear, this feature is not something that I'm personally wedded to, but when faced with the question of whether to reject or accept code like:

• fn main() { let s: S; s.x = 10;,
  I start thinking about variants like
• fn main() { let s: S; s.x = 10; a_use_of(s.x); },

and I guess I start thinking of cases like the above.

One of the things I'm particularly fond of in Rust, is the inability to have uninitialised variables. Would this break that idea, if I can go let s: S; ? Or would it only break if I try actually use the struct uninitialised? eg: let s: S; a_use_of(s);
Again, sorry if I'm asking silly questions, I'm still getting to grips with the whole of rust.

[pnkfelix]

You would not be able to do let s: S; a_use_of(s); under what is being proposed here.

[shepmaster]

if I can go let s: S;

To be clear, you can already do that:

struct S {
    x: u32,
    y: u32,
}

fn main() {
    let s: S;

    if true {
        s = S { x: 1, y: 2 };
    } else {
        s = S { x: 2, y: 1 };
    }
}

I wouldn't say it's idiomatic, but it's possible.

[scottmcm]

I think the one that's particularly bad is this one:

fn main() { let mut s: S; s.x = 10; use(s.x); }

I don't think it should ever be allowed to write something that you cannot then read.

I would be fine with either direction of a fix for that, whether denying the write or allowing the read.

Edit: I do think that, as long as the following is valid, there should be a lint about the assignment, which there currently isn't:

struct S { x:i32, _y:i32 }
fn main() { let mut s: S; s.x = 10; }

[pnkfelix]

Discussed at lang team meeting.

For reference, here are the three example programs we discused (assuming we're starting with e.g. struct S { x: u32, y: u32 } and S does not implement Drop):

• Accepted today: fn main() { let mut s: S; s.x = 10; }
• Rejected today: fn main() { let mut s: S; s.x = 10; s.y = 30; a_use_of(s); }
• Rejected today: fn main() { let mut s: S; s.x = 10; a_use_of(s.x); })

There was generally strong agreement that in the long term, it would be good to accept all three cases. I.e., we should allow building up a record in a piecemeal fashion, and it would also be good to allow reading a field after one has written to it, regardless of the original initialization state of the struct overall.

• And it was also agreed that this is suitable as a long term goal. We don't need to try to do it e.g. for the edition.

There was some lukewarm support that, in the short term, we should be rejecting all three cases in the name of consistency.

• This lukewarm support was based in part on the assumption that it would not be difficult to add to NLL
• And it was also coupled with the shared understanding that it be solely added to NLL (where the 2018 edition's use of borrowck=migrate will imply that even if this does inject a borrow_check-error into someone's existing code, if that code previously compiled with the 2015 edition, then that borrow_check-error will be downgraded to a warning by the migrate machinery.)

@nikomatsakis hypothesized during the meeting that implementing this would be perhaps a day's worth of thought and/or programming effort.

With that in mind, we have tentatively decided to attempt, for the short-term, to adopt the semantics that rejects all three cases. (I.e., it will become an error to write to a field of a struct if that whole struct is not already initialized)

[pnkfelix]

the effort here, i.e. the new check described in my previous comment, needs to happen soon if its going to happen at all.

So I'm retagging this as I-nominated to make it crystal clear that I now want this discussed at the NLL meeting, if we haven't already found a volunteer to implement this by that time.

[pnkfelix]

Assigning to @spastorino; they are going to drive the initial effort on making it an error, for the short-term, to write to a field of a struct if that whole struct has not be already initialized.

Once that is done, we should either leave this issue open and assign it to someone working on the longer term project for supporting partial-assignments of uninitialized records, or close this issue and open a fresh issue to track the longer term project.

[pnkfelix]

Putting on the RC2 milestone. If we're doing this change at all, its gotta land by then IMO.

[pnkfelix]

Its been getting hard in discussions to be clear about what "this fixes #21232" means when there are distinct short-term and long-term goals attached to that one issue number.

So I forked off #54986 and #54987 to cover the distinct short- and long-term goals.

[pnkfelix]

(We did talk about this at the last meeting; removing the I-nominated tag.)

[pnkfelix]

54986 has landed.

54987 covers the longer term goal of supporting partial initialization of structs.

So this issue (#21232) can be closed.