Repository navigation
borrow-checker allows partial reinit of struct that has been moved away, but no use of it. #21232
Description
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 rust-lang/rfcs#533 )
Reacted by Felix Dreissig, Yuchen Liu, Kornel and Adam Reichold
Activity
pnkfelix commentedon Jan 16, 2015
(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 commentedon Mar 3, 2015
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:
pnkfelix commentedon Mar 3, 2015
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 commentedon Mar 3, 2015
triage: P-backcompat-lang (1.0 beta)
just trying to accommodate highfive...
nikomatsakis commentedon Mar 5, 2015
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 commentedon Mar 5, 2015
I'm going to tag this as I-Needs-Decision.
mbrubeck commentedon Mar 5, 2015
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:then it is rejected with "error: partial reinitialization of uninitialized structure
f."pnkfelix commentedon Mar 5, 2015
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 commentedon Mar 5, 2015
triage: P-low ()
44 remaining items
pnkfelix commentedon Oct 4, 2018
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-nominatedto 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 commentedon Oct 8, 2018
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 commentedon Oct 9, 2018
Putting on the RC2 milestone. If we're doing this change at all, its gotta land by then IMO.
pnkfelix commentedon Oct 11, 2018
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.
Make us error consistently in issue rust-lang#21232, to fix rust-lang…
pnkfelix commentedon Oct 16, 2018
(We did talk about this at the last meeting; removing the I-nominated tag.)
Auto merge of #54941 - pnkfelix:issue-21232-reject-partial-reinit, r=…
pnkfelix commentedon Oct 17, 2018
#54986 has landed.
#54987 covers the longer term goal of supporting partial initialization of structs.
So this issue (#21232) can be closed.
Unions reinitialized after assignment into field.