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

borrowck: Prevent partial reinitialization of uninitialized structures #22079

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/librustc_borrowck/borrowck/check_loans.rs
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,26 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
use_kind, lp_base);
}
LpExtend(ref lp_base, _, LpInterior(InteriorField(_))) => {
match lp_base.to_type().sty {
ty::ty_struct(def_id, _) | ty::ty_enum(def_id, _) => {
if ty::has_dtor(self.tcx(), def_id) {
// In the case where the owner implements drop, then
// the path must be initialized to prevent a case of
// partial reinitialization
Copy link
Member

Choose a reason for hiding this comment

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

This code is an improvement on the prior PR that you rebased, thanks!

However, I do wonder whether we could simplify further and just call self.check_if_path_is_moved(id, span, use_kind, lp_base); after doing the ty::has_dtor check, the same way that the other cases below do.

Does that produce an error message that is too obscure? (Or, is there some technical reason we cannot do that?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Off the top of my head, it was possible but the error was a more generic 'use of moved value foo.bar'.

I reasoned that the improved error message from the original changeset might be preferable but I'm willing to change it for simpler code.

Copy link
Member

Choose a reason for hiding this comment

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

How about this: Keep your code the way it is, but throw in a FIXME here and above fn check_if_path_is_moved noting that the body of check_if_path_is_moved is largely the same as the code you put into this function. That way, if someone does this same thing again, maybe we will then be inspired to refactor the code, parameterizing over what error message to report (since that is AFAICT the only thing that is changing between the two fragements of code).

(That, or you could add in such a refactoring yourself, as a followup commit post squash.)

That way, we keep a nice precision in the error messages, and we slightly reduce the risk that this code fragment will be cut-and-pasted into other contexts without an attempt to pull it all together.

let loan_path = owned_ptr_base_path_rc(lp_base);
self.move_data.each_move_of(id, &loan_path, |_, _| {
self.bccx
.report_partial_reinitialization_of_uninitialized_structure(
span,
&*loan_path);
false
});
return;
}
},
_ => {},
}

// assigning to `P.f` is ok if assigning to `P` is ok
self.check_if_assigned_path_is_moved(id, span,
use_kind, lp_base);
Expand Down Expand Up @@ -775,10 +795,12 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
mark_variable_as_used_mut(self, assignee_cmt);
}
}

return;
}

// Initializations are OK.
// Initializations are OK if and only if they aren't partial
// reinitialization of a partially-uninitialized structure.
if mode == euv::Init {
return
}
Expand Down
12 changes: 12 additions & 0 deletions src/librustc_borrowck/borrowck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,18 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}
}

pub fn report_partial_reinitialization_of_uninitialized_structure(
&self,
span: Span,
lp: &LoanPath<'tcx>) {
self.tcx
.sess
.span_err(span,
(format!("partial reinitialization of uninitialized \
structure `{}`",
self.loan_path_to_string(lp))).as_slice());
}

pub fn report_reassigned_immutable_variable(&self,
span: Span,
lp: &LoanPath<'tcx>,
Expand Down
49 changes: 49 additions & 0 deletions src/test/compile-fail/borrowck-partial-reinit-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright 2014-2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct Test;

struct Test2 {
b: Option<Test>,
}

struct Test3(Option<Test>);

impl Drop for Test {
fn drop(&mut self) {
println!("dropping!");
}
}

impl Drop for Test2 {
fn drop(&mut self) {}
}

impl Drop for Test3 {
fn drop(&mut self) {}
}

fn stuff() {
let mut t = Test2 { b: None };
let u = Test;
drop(t);
t.b = Some(u);
//~^ ERROR partial reinitialization of uninitialized structure `t`

let mut t = Test3(None);
let u = Test;
drop(t);
t.0 = Some(u);
//~^ ERROR partial reinitialization of uninitialized structure `t`
}

fn main() {
stuff()
}
34 changes: 34 additions & 0 deletions src/test/compile-fail/borrowck-partial-reinit-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright 2014-2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct Test {
a: isize,
b: Option<Box<Test>>,
}

impl Drop for Test {
fn drop(&mut self) {
println!("Dropping {}", self.a);
}
}

fn stuff() {
let mut t = Test { a: 1, b: None};
let mut u = Test { a: 2, b: Some(Box::new(t))};
t.b = Some(Box::new(u));
//~^ ERROR partial reinitialization of uninitialized structure `t`
println!("done");
}

fn main() {
stuff();
println!("Hello, world!")
}

22 changes: 22 additions & 0 deletions src/test/compile-fail/borrowck-partial-reinit-3.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
use std::mem;

struct Test { f: usize }
impl Drop for Test {
fn drop(&mut self) {}
}

fn main() {
let mut x = (Test { f: 2 }, Test { f: 4 });
mem::drop(x.0);
x.0.f = 3;
//~^ ERROR partial reinitialization of uninitialized structure `x.0`
}
33 changes: 33 additions & 0 deletions src/test/compile-fail/borrowck-partial-reinit-4.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct Test;

struct Test2(Option<Test>);

impl Drop for Test {
fn drop(&mut self) {
println!("dropping!");
}
}

impl Drop for Test2 {
fn drop(&mut self) {}
}

fn stuff() {
let mut x : (Test2, Test2);
(x.0).0 = Some(Test);
//~^ ERROR partial reinitialization of uninitialized structure `x.0`
}

fn main() {
stuff()
}