borrowck: Prevent partial reinitialization of uninitialized structures #22079
Conversation
enumerations that implement the `Drop` trait.
This breaks code like:
struct Struct {
f: String,
g: String,
}
impl Drop for Struct { ... }
fn main() {
let x = Struct { ... };
drop(x);
x.f = ...;
}
Change this code to not create partially-initialized structures. For
example:
struct Struct {
f: String,
g: String,
}
impl Drop for Struct { ... }
fn main() {
let x = Struct { ... };
drop(x);
x = Struct {
f: ...,
g: ...,
}
}
Closes rust-lang#18571.
[breaking-change]
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
| 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 |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
The tests look good to me. @Ryman r=me after squashing (and I would like an answer to the question above if possible). Update: okay, my question has been answered. So r=me after squashing and adding the FIXME i asked for above. (If you do the refactoring I suggested, just ping me for a re-review.) |
|
(the bors queue is close to empty right now, so I might just steal this and make a fresh PR for it.) |
|
closing in favor of #22219 (which just does the squashing); thanks for the contribution, I made sure to preserve your authorship on the squash. :) |
|
@pnkfelix No worries, I didn't notice this before going ahead with a quick refactor, you can check Ryman@d57d5f3 to see if it warrants pulling in or not. (The alternative, taking a closure didn't really remove much duplication) |
update FIXME(rust-lang#6298) to point to open issue 15020 update FIXME(rust-lang#6268) to point to RFC 811 update FIXME(rust-lang#10520) to point to RFC 1751 remove FIXME for emscripten issue 4563 and include target in `test_estimate_scaling_factor` remove FIXME(rust-lang#18207) since node_id isn't used for `ref` pattern analysis remove FIXME(rust-lang#6308) since DST was implemented in rust-lang#12938 remove FIXME(rust-lang#2658) since it was decided to not reorganize module remove FIXME(rust-lang#20590) since it was decided to stay conservative with projection types remove FIXME(rust-lang#20297) since it was decided that solving the issue is unnecessary remove FIXME(rust-lang#27086) since closures do correspond to structs now remove FIXME(rust-lang#13846) and enable `function_sections` for windows remove mention of rust-lang#22079 in FIXME(rust-lang#22079) since this is a general FIXME remove FIXME(rust-lang#5074) since the restriction on borrow were lifted
This is a rebase of #18963
2nd commit is to accommodate @nikomatsakis's comments on the PR which handles his additional testcase.
Will squash post-review if it's adequate.
Fixes #18571.
cc @pcwalton @nikomatsakis