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

fixes move analysis #49392

Merged
merged 3 commits into from
Apr 7, 2018
Merged

fixes move analysis #49392

merged 3 commits into from
Apr 7, 2018

Conversation

hrvolapeter
Copy link
Member

Fixed compiler error by correct checking when dereferencing

Fix #48962

r? @nikomatsakis

librustc_mir/transform/elaborate_drops.rs — drop of untracked, uninitialized value

Fix #48962

r? @nikomatsakis
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2018
@shepmaster
Copy link
Member

Ping from triage, @nikomatsakis ! Will you have time to review this soon?

@@ -1454,8 +1492,6 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Place::Projection(ref proj) => {
let Projection { ref base, ref elem } = **proj;
match *elem {
ProjectionElem::Deref |
// assigning to *P requires `P` initialized.
ProjectionElem::Index(_/*operand*/) |
ProjectionElem::ConstantIndex { .. } |
// assigning to P[i] requires `P` initialized.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is wrong. Should be

// assigning to P[i] requires P to be valid

@@ -1465,6 +1501,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// FIXME: is this true even if P is a adt with a dtor?
{ }

ProjectionElem::Deref => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should have a comment

// assigning to (*P) requires P to be initialized

ProjectionElem::Deref => {
self.check_if_path_is_moved(
context, InitializationRequiringAction::Use,
(base, span), flow_state);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add a break here if you want - we already force the base to be initialized, so there is no need to further check.

Maybe in that case you would also not need the new error deduplication code.

@@ -1320,18 +1324,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename check_if_path_is_moved to check_if_full_path_is_moved?

@arielb1
Copy link
Contributor

arielb1 commented Mar 31, 2018

Sorry for the delay in reviewing. This is a bit of an embarrassing bug.

If adding a break in the check_if_assigned_path_is_moved loop fixes the error duplication, then pretty much r+ from me with the nits addressed.

@hrvolapeter
Copy link
Member Author

hrvolapeter commented Apr 1, 2018

@arielb1 Unfortunately adding a break won't fix duplicate errors. Let's have expression w[5] = 0; it translates to mir similar to

        _3 = Len((*_1));                 // bb0[3]: scope 1 at borrowck-use-in-index-lvalue.rs:16:5: 16:9
                                         | Live variables on entry to bb0[4]: [_1, _2, _3]
        _4 = Lt(_2, _3);                 // bb0[4]: scope 1 at borrowck-use-in-index-lvalue.rs:16:5: 16:9
                                         | Live variables on entry to bb0[5]: [_1, _2, _3, _4]
        assert(move _4, "index out of bounds: the len is {} but the index is {}", move _3, _2) -> [success: bb2, unwind: bb1]; // bb0[5]: scope 1 at borrowck-use-in-index-lvalue.rs:16:5: 16:9
    }

    | Live variables on entry to bb1: []
    bb1: {                               // cleanup
                                         | Live variables on entry to bb1[0]: []
        resume;                          // bb1[0]: scope 0 at borrowck-use-in-index-lvalue.rs:14:1: 22:2
    }

    | Live variables on entry to bb2: [_1, _2]
    bb2: {                              
                                         | Live variables on entry to bb2[0]: [_1, _2]
        (*_1)[_2] = const 0isize;  

the first error is printed from _3 = Len((*_1)); and the second (which we are trying to surpress) is from (*_1)[_2] = const 0isize;
We did not have second error till now due to incorrect deref checkin.

@arielb1
Copy link
Contributor

arielb1 commented Apr 1, 2018

@retep007

In that case, could you deduplicate based on (Place, Span) instead of only Place? That makes e.g. writing tests to much easier.

.contains(&root_place.clone())
{
debug!(
"report_use_of_moved_or_uninitialized place: {:?} errors was suppressed",
Copy link
Contributor

Choose a reason for hiding this comment

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

english fix: error about {:?} suppressed.

@arielb1
Copy link
Contributor

arielb1 commented Apr 1, 2018

I think this code is OK, but I didn't touch this code long enough that I would rather

r? @nikomatsakis

@hrvolapeter
Copy link
Member Author

@arielb1 _3 = Len((*_1)); and (*_1)[_2] = const 0isize; have also different spans from previous comment first one has span only of w[5] but second has w[5] = 0.

I also think that second error actually says better what's the problem, but It would require to first refactor how the errors are reported.
First:

w[5] = 0; //[ast]~ ERROR use of possibly uninitialized variable: `w` [E0381]
^^^^ use of possibly uninitialized `*w`

Second:

w[5] = 0; //[ast]~ ERROR use of possibly uninitialized variable: `w` [E0381]
^^^^^^^^ use of possibly uninitialized `w`

place_span: (&Place<'tcx>, Span),
flow_state: &Flows<'cx, 'gcx, 'tcx>,
) {
// FIXME: analogous code in check_loans first maps `place` to
Copy link
Contributor

Choose a reason for hiding this comment

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

what code are you referring to exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly same code let place = self.base_path(place_span.0); is in check_if_full_path_is_moved also with the comment. So If somebody decides to refactor this he will probably do both check_if_full_path_is_moved, check_if_path_or_subpath_is_moved

@nikomatsakis
Copy link
Contributor

This code looks good to me!

@hrvolapeter
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 3, 2018

@retep007: 🔑 Insufficient privileges: Not in reviewers

@hrvolapeter
Copy link
Member Author

@nikomatsakis Could you also accept it by bors?

@pnkfelix
Copy link
Member

pnkfelix commented Apr 3, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 3, 2018

📌 Commit 9056c7a has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2018
@kennytm
Copy link
Member

kennytm commented Apr 6, 2018

@bors p=6

@bors
Copy link
Contributor

bors commented Apr 6, 2018

⌛ Testing commit 9056c7a with merge 0e35ddd...

bors added a commit that referenced this pull request Apr 6, 2018
fixes move analysis

Fixed compiler error by correct checking when dereferencing

Fix #48962

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Apr 7, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 0e35ddd to master...

@bors bors merged commit 9056c7a into rust-lang:master Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants