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 NLL: error from URL crate #47917

Merged
merged 6 commits into from Feb 17, 2018

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link
Member

davidtwco commented Feb 1, 2018

Fixes #47703.

r? @nikomatsakis

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Feb 1, 2018

A quick note about this initial solution, I'm not all that confident that I should be using a Shallow access for the individual fields, so happy to find another solution to the issues that fixed if it causes an problem I didn't anticipate.

// When this function is called as a result of an `access_terminator` call attempting
// to drop a struct, if that struct does not have a destructor, then we need to check
// each of the fields in the struct. See #47703.
let (access, places) = if let ContextKind::Drop = context.kind {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 1, 2018

Contributor

Hmm, I had intended for us think to put this code here:

self.access_place(
ContextKind::Drop.new(loc),
(drop_place, span),
(Deep, Write(WriteKind::StorageDeadOrDrop)),
LocalMutationIsAllowed::Yes,
flow_state,
);

That is, we would invoke access_place more than once. We might want to make the context carry the original path being dropped though, for error message purposes, not sure.

Also, I think this needs to be recursive -- that is, once we construct the place foo.f or the field f, we need to recursively check if that should be broken down. In other words, if we alter the original test to add one more layer of struct surrounding the MyStruct, it should still pass:

struct MyMyStruct<'a> {
    field: MyStruct<'a>
}

This comment has been minimized.

@davidtwco

davidtwco Feb 1, 2018

Author Member

This should be resolved now.

};
let place = Place::Projection(Box::new(proj));

match field_ty.sty {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 1, 2018

Contributor

instead of this match, we could invoke:

self.visit_terminator_drop(self, loc, term, flow_state, &place, span);

This comment has been minimized.

@davidtwco

davidtwco Feb 1, 2018

Author Member

I've changed this locally, will push with other changes.

self.access_place(
ContextKind::Drop.new(loc),
(&place, span),
(Shallow(None), Write(WriteKind::StorageDeadOrDrop)),

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 1, 2018

Contributor

In particular, this Shallow(None) is wrong -- we should fall through to the Deep case below.

Here is why: imagine this is a struct with a destructor. Then we will be running random user code, it could touch anything within!

This comment has been minimized.

@davidtwco

davidtwco Feb 1, 2018

Author Member

I wasn't sure about the Shallow(None), but when using the place for just the field, we still get an error, so figured it must have been necessary:

DEBUG:<unknown>: MirBorrowckCtxt::process_terminator(bb0[4], Terminator { source_info: SourceInfo { span: src/test/run-pass/issue-47703.rs:25:50: 25:50, scope: scope[0] }, kind: drop(_1) -> [return: bb2, unwind: bb1] }): borrows in effect: [&mut (*(_1.0: &mut ())), &mut (*(_1.0: &mut ()))@active] borrows generated: [] inits: [_0, _1] uninits: [_2] move_out: [] ever_init: [mp1@src/test/run-pass/issue-47703.rs:25:16: 25:20 (Deep), mp0@src/test/run-pass/issue-47703.rs:25:36: 25:50 (Deep)]
DEBUG:<unknown>: check_access_permissions((_1.0: &mut ()), Write(StorageDeadOrDrop), Yes)
DEBUG:<unknown>: places_conflict((*(_1.0: &mut ())),(_1.0: &mut ()),Deep)
DEBUG:<unknown>: places_conflict: components [_1, (_1.0: &mut ()), (*(_1.0: &mut ()))] / [_1, (_1.0: &mut ())]
DEBUG:<unknown>: places_conflict: Some(_1) vs. Some(_1)
DEBUG:<unknown>: place_element_conflict: DISJOINT-OR-EQ-LOCAL
DEBUG:<unknown>: places_conflict: Some((_1.0: &mut ())) vs. Some((_1.0: &mut ()))
DEBUG:<unknown>: place_element_conflict: DISJOINT-OR-EQ-FIELD
DEBUG:<unknown>: places_conflict: Some((*(_1.0: &mut ()))) vs. None
DEBUG:<unknown>: places_conflict: None vs. None
DEBUG:<unknown>: places_conflict: full borrow, CONFLICT

This comment has been minimized.

@davidtwco

davidtwco Feb 1, 2018

Author Member

I'm able to change this so that the access_place ends up matching the borrow_place, ie. (*(_1.0: &mut ())) on both sides. This also does not fix the error.

DEBUG:<unknown>: MirBorrowckCtxt::process_terminator(bb0[4], Terminator { source_info: SourceInfo { span: src/test/run-pass/issue-47703.rs:25:50: 25:50, scope: scope[0] }, kind: drop(_1) -> [return: bb2, unwind: bb1] }): borrows in effect: [&mut (*(_1.0: &mut ())), &mut (*(_1.0: &mut ()))@active] borrows generated: [] inits: [_0, _1] uninits: [_2] move_out: [] ever_init: [mp1@src/test/run-pass/issue-47703.rs:25:16: 25:20 (Deep), mp0@src/test/run-pass/issue-47703.rs:25:36: 25:50 (Deep)]
DEBUG:<unknown>: check_access_permissions((*(_1.0: &mut ())), Write(StorageDeadOrDrop), Yes)
DEBUG:<unknown>: places_conflict((*(_1.0: &mut ())),(*(_1.0: &mut ())),Deep)
DEBUG:<unknown>: places_conflict: components [_1, (_1.0: &mut ()), (*(_1.0: &mut ()))] / [_1, (_1.0: &mut ()), (*(_1.0: &mut ()))]
DEBUG:<unknown>: places_conflict: Some(_1) vs. Some(_1)
DEBUG:<unknown>: place_element_conflict: DISJOINT-OR-EQ-LOCAL
DEBUG:<unknown>: places_conflict: Some((_1.0: &mut ())) vs. Some((_1.0: &mut ()))
DEBUG:<unknown>: place_element_conflict: DISJOINT-OR-EQ-FIELD
DEBUG:<unknown>: places_conflict: Some((*(_1.0: &mut ()))) vs. Some((*(_1.0: &mut ())))
DEBUG:<unknown>: place_element_conflict: DISJOINT-OR-EQ-DEREF
DEBUG:<unknown>: places_conflict: None vs. None
DEBUG:<unknown>: places_conflict: full borrow, CONFLICT

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 2, 2018

Contributor

I'm a bit confused by what you are showing me -- you are saying that if you don't use the "shallow" case, you get an error on the original test case?

This comment has been minimized.

@davidtwco

davidtwco Feb 2, 2018

Author Member

Yes. If I change it to Deep (or let it fall through to that case), then I get the error on the original test case. That's why I had it as Shallow(None) initially.

// See #47703.
ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => {
for (index, field) in def.all_fields().enumerate() {
let field_ty = field.ty(self.tcx, substs);

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 1, 2018

Contributor

Sadly, before landing we're going to have to normalize associated types here. I have to dig a bit to see what's the most convenient way to do that. Probably we can erase regions, actually, which might simplify our lives a bit.

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Feb 5, 2018

I've pushed what changes I made locally as requested by @nikomatsakis. It will fail the tests because the previous version used Shallow(None) instead of Deep. It also doesn't normalize associated types.

Also, the line below is included because it was something I tried out to see if it would help, I don't think it's what we should have:

https://github.com/davidtwco/rust/blob/f9de0484ce401b9407dc05c8eb6291cdc2ef5269/src/librustc_mir/borrow_check/mod.rs#L724

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 5, 2018

OK, I pushed a few tweaks, but we still need to solve the normalization problem.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 6, 2018

Marking as S-blocked for now -- I have an in-progress branch that may be very helpful here.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 9, 2018

☔️ The latest upstream changes (presumably #47489) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

OK, I laid out how I think we can make the transformation. If we do this, it's probably worth adding some comments to access_place that the Place<'tcx> may have a region-erased type in some cases. But I don't think that should matter, those bits of code ought not to be testing the region. Certainly my searches through the code suggest that this is the case. (All instances of TyRef ignore the region, for example.)

loc: Location,
term: &Terminator<'tcx>,
flow_state: &Flows<'cx, 'gcx, 'tcx>,
drop_place: &Place<'tcx>,

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

This will take an extra argument:

erased_drop_place_ty: Ty<'gcx>,

Note that it belongs in the global arena ('gcx) -- this is possible because its regions have been erased.

LocalMutationIsAllowed::Yes,
flow_state,
);
self.visit_terminator_drop(loc, term, flow_state, drop_place, span);

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

Here, we will start by erasing the type of the drop_place, roughly like so:

let gcx = self.tcx.global_tcx();

// Compute the type with accurate region information.
let drop_place_ty = drop_place.ty(self.mir, self.tcx);

// Erase the regions.
let drop_place_ty = self.tcx.erase_regions(&drop_place_ty);

// "Lift" into the gcx -- once the regions are erased, this type should be in the global arenas; this "lift" operation basically just asserts that this is true, but that's useful later.
let drop_place_ty = gcx.lift(&drop_place_ty).unwrap();

(Note that I am intentionally shadowing drop_place_ty here, because we don't want to accidentally access the intermediate values.)

Then we'll add this drop_place_ty as an argument to visit_terminator_drop

drop_place: &Place<'tcx>,
span: Span,
) {
let ty = drop_place.ty(self.mir, self.tcx).to_ty(self.tcx);

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

remove this line

span: Span,
) {
let ty = drop_place.ty(self.mir, self.tcx).to_ty(self.tcx);
match ty.sty {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

becomes

match erased_drop_place_ty.sty {
...
}
ty::TyAdt(def, substs) if def.is_struct() && !def.has_dtor(self.tcx) => {
for (index, field) in def.all_fields().enumerate() {
let place = drop_place.clone();
let place = place.field(Field::new(index), field.ty(self.tcx, substs));

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

Here, we can normalize like so:

let gcx = self.tcx.global_tcx();
let field_ty = field.ty(gcx, substs);
let field_ty = gcx.normalize_associated_types_in_env(&field_ty, self.param_env);
let place = place.field(Field::new(index), field_ty);
// "needs drop". If so, we assume that the destructor
// may access any data it likes (i.e., a Deep Write).
let gcx = self.tcx.global_tcx();
let erased_ty = gcx.lift(&self.tcx.erase_regions(&ty)).unwrap();

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

Here, the regions etc are already erased, so we can just remove this code

// may access any data it likes (i.e., a Deep Write).
let gcx = self.tcx.global_tcx();
let erased_ty = gcx.lift(&self.tcx.erase_regions(&ty)).unwrap();
if erased_ty.needs_drop(gcx, self.param_env) {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

Here we would use if erased_drop_place_ty.needs_drop(...) {

davidtwco and others added some commits Jan 27, 2018

check that types "need drop" before we access them
Also, add some comments and remove extra deref.

@davidtwco davidtwco force-pushed the davidtwco:issue-47703 branch from babf202 to 159b742 Feb 14, 2018

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Feb 14, 2018

@nikomatsakis pushed up rebased changes that include associated type normalization.

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

Left a few nits, but seems good.

let drop_place_ty = self.tcx.erase_regions(&drop_place_ty).to_ty(self.tcx);

// "Lift" into the gcx -- once regions are erased, this type should be in the
// global areans; this "lift" operation basically just asserts that is true, but

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

Nit: arenas, not "areans"

This comment has been minimized.

@davidtwco

davidtwco Feb 14, 2018

Author Member

Oops, fixed.

term: &Terminator<'tcx>,
flow_state: &Flows<'cx, 'gcx, 'tcx>,
drop_place: &Place<'tcx>,
erased_drop_place_ty: &'gcx ty::TyS<'gcx>,

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

This is always written Ty<'gcx>. We never reference TyS outside of the ty module if we can help it. =)

This comment has been minimized.

@davidtwco

davidtwco Feb 14, 2018

Author Member

Fixed.

@@ -2093,7 +2159,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
while let Some(i) = elems_incoming.next() {
let borrowed = &data[i.borrow_index()];

if self.places_conflict(&borrowed.borrowed_place, place, access) {
if self.places_conflict(&borrowed.borrowed_place, &place, access) {

This comment has been minimized.

@nikomatsakis

nikomatsakis Feb 14, 2018

Contributor

Why this diff?

This comment has been minimized.

@davidtwco

davidtwco Feb 14, 2018

Author Member

I have no idea what that came from. My bad.

@davidtwco davidtwco force-pushed the davidtwco:issue-47703 branch from 159b742 to 98904c2 Feb 14, 2018

@davidtwco

This comment has been minimized.

Copy link
Member Author

davidtwco commented Feb 14, 2018

@nikomatsakis fixed those nits.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 14, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2018

📌 Commit 98904c2 has been approved by nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 17, 2018

⌛️ Testing commit 98904c2 with merge 507a46a...

bors added a commit that referenced this pull request Feb 17, 2018

Auto merge of #47917 - davidtwco:issue-47703, r=nikomatsakis
Fixes NLL: error from URL crate

Fixes #47703.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 17, 2018

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

@bors bors merged commit 98904c2 into rust-lang:master Feb 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-47703 branch Feb 25, 2018

bors added a commit that referenced this pull request Apr 27, 2018

Auto merge of #49822 - matthewjasper:dropck-closures, r=nikomatsakis
Access individual fields of tuples, closures and generators on drop.

Fixes #48623, by extending the change in #47917 to closures. Also does this for tuples and generators for consistency.

Enums are unchanged because there is now way to borrow `*enum.field` without borrowing `enum.field` at the moment, so any error would be reported when the enum goes out of scope. Unions aren't changed because unions they don't automatically drop their fields.

r? @nikomatsakis

bors added a commit that referenced this pull request Apr 27, 2018

Auto merge of #49822 - matthewjasper:dropck-closures, r=nikomatsakis
Access individual fields of tuples, closures and generators on drop.

Fixes #48623, by extending the change in #47917 to closures. Also does this for tuples and generators for consistency.

Enums are unchanged because there is now way to borrow `*enum.field` without borrowing `enum.field` at the moment, so any error would be reported when the enum goes out of scope. Unions aren't changed because unions they don't automatically drop their fields.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment