Fix ref-to-ptr coercions not working with NLL in certain cases #47873
Conversation
Implicit coercions from references to pointers were lowered to slightly different Mir than explicit casts (e.g. 'foo as *mut T'). This resulted in certain uses of self-referential structs compiling correctly when an explicit cast was used, but not when the implicit coercion was used. To fix this, this commit adds an outer 'Use' expr when applying a raw-ptr-borrow adjustment. This makes the lowered Mir for coercions identical to that of explicit coercions, allowing the original code to compile regardless of how the raw ptr cast occurs. Fixes rust-lang#47722
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
kennytm
added
S-waiting-on-review
|
Thanks for the PR! We’ll periodically check in on it to make sure that @michaelwoerister or someone else from the team reviews it soon. |
There was a problem hiding this comment.
Hmm. This is a really interesting case. Some part of me is tempted to call this a flaw in borrowck -- i.e., it should accept the original MIR -- but it's the kind of edge-case where I think it would be useful to get a bit more clarity.
Basically the problem is that we generate MIR like this:
_2 = &mut (*_1); // S1
((*_1).0: *mut SelfReference) = move _2 as *mut SelfReference (Misc); // S2
The borrow of _1 extends to {S1, S2}, since it is live at both of those entry-points. But then S2 checks for borrows in scope at S2 that conflict with (*_1).0, and hence reports an error. It seems like we could check for borrows in scope at S3, since the assignment is the last thing that happens in the statement, or something like that.
(I've been hoping to avoid "before/after" points, and I'd like to keep doing so, but this gets annoying at terminators.)
That said, in other contexts -- notably function calls -- I think we wanted to ensure that the destination in X = FOO() is basically "dead scratch space" on entry to the function, and this fits the current check. Maybe there is a similar desire for other rvalues?
So, for now it seems ok to make this change and get the code working. I wonder if we'll revisit this question though with other examples.
|
cc @pnkfelix -- see the above comment, it's interesting |
|
@bors r+ |
|
📌 Commit b5f8cd5 has been approved by |
kennytm
added
S-waiting-on-bors
Fix ref-to-ptr coercions not working with NLL in certain cases Implicit coercions from references to pointers were lowered to slightly different Mir than explicit casts (e.g. 'foo as *mut T'). This resulted in certain uses of self-referential structs compiling correctly when an explicit cast was used, but not when the implicit coercion was used. To fix this, this commit adds an outer 'Use' expr when applying a raw-ptr-borrow adjustment. This makes the lowered Mir for coercions identical to that of explicit coercions, allowing the original code to compile regardless of how the raw ptr cast occurs. Fixes #47722
|
☀️ Test successful - status-appveyor, status-travis |
Implicit coercions from references to pointers were lowered to slightly
different Mir than explicit casts (e.g. 'foo as *mut T'). This resulted
in certain uses of self-referential structs compiling correctly when an
explicit cast was used, but not when the implicit coercion was used.
To fix this, this commit adds an outer 'Use' expr when applying a
raw-ptr-borrow adjustment. This makes the lowered Mir for coercions
identical to that of explicit coercions, allowing the original code to
compile regardless of how the raw ptr cast occurs.
Fixes #47722