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.

Sign up for GitHub

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

Ignore type of projections for upvar capturing #89648

Merged
merged 3 commits into from
Oct 12, 2021
Merged

Ignore type of projections for upvar capturing #89648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5481201
Ignore projection type when determining upvar ancestory
nbdd0121 Oct 8, 2021
d7f8a06
Add regression test
nbdd0121 Oct 7, 2021
7275cfa
Explicit `PlaceAncestryRelation::SamePlace` and handle like `Descendant`
nbdd0121 Oct 9, 2021
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
15 changes: 9 additions & 6 deletions compiler/rustc_typeck/src/check/upvar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ use std::iter;
enum PlaceAncestryRelation {
Ancestor,
Descendant,
SamePlace,
Divergent,
}

Expand Down Expand Up @@ -564,7 +565,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
for possible_ancestor in min_cap_list.iter_mut() {
match determine_place_ancestry_relation(&place, &possible_ancestor.place) {
// current place is descendant of possible_ancestor
PlaceAncestryRelation::Descendant => {
PlaceAncestryRelation::Descendant | PlaceAncestryRelation::SamePlace => {
ancestor_found = true;
let backup_path_expr_id = possible_ancestor.info.path_expr_id;

Expand Down Expand Up @@ -2278,15 +2279,17 @@ fn determine_place_ancestry_relation(
let projections_b = &place_b.projections;

let same_initial_projections =
iter::zip(projections_a, projections_b).all(|(proj_a, proj_b)| proj_a == proj_b);
iter::zip(projections_a, projections_b).all(|(proj_a, proj_b)| proj_a.kind == proj_b.kind);

if same_initial_projections {
use std::cmp::Ordering;

// First min(n, m) projections are the same
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we should refactor this to return something like PlaceAncestryRelation::Same and handle it explicity in the min capture code.

I have a feeling that this would cause issues with borrow checker error reporting a bit.

let mut t = (vec![], 0);

let mut c = || {
     println!("{:#?}, t.0); // Place 1
     t.0.push(1);  // Place 2
};

println!("{:#?}, t.0); // borrow conflict

c();

Now the code thinks that Place1 is ancestor of Place2 and therefore Place1 is the reason it captures the path t.0 but we need to capture via a mut borrow because of Place 2.

Now in the borrow conflict it would say something like t.0 is captured mutably because of use at Place2 and t.0 is captured because of use at Place1. i.e. it will try reason for 2 different spans present in CaptureInfo which can be confusing for the user.

What it really should do is just point at Place2 saying that this is the reason we captured this via mut borrow.

Copy link
Contributor Author

@nbdd0121 nbdd0121 Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This works fine, because the lifetime is the same so no multiple occurrence of the same place will happen. I tried this example and it does show what you suggest:

struct S<'a>(Option<&'a mut i32>);

fn by_value(mut s: S<'_>) {
    let mut c = || {
        s.0 = None;
        let S(ref _o) = s;
    };

    let _ = &s.0;
    c();
}

gives

error[E0502]: cannot borrow `s.0` as immutable because it is also borrowed as mutable
  --> ice.rs:9:13
   |
4  |     let mut c = || {
   |                 -- mutable borrow occurs here
5  |         s.0 = None;
   |         --- capture is mutable because of use here
6  |         let S(ref _o) = s;
   |                         - first borrow occurs due to use of `s.0` in closure
...
9  |     let _ = &s.0;
   |             ^^^^ immutable borrow occurs here
10 |     c();
   |     - mutable borrow later used here

Fixing this would require us to keep the order of use, which currently is lost due to use of HashMap.

Given that this diagnostics confusion only happens for minority of cases (where we currently ICE), I think the best way forward is to just merge and backport as it is, and open an issue to track this case. I can author a follow-up PR later.

Copy link
Member

@arora-aman arora-aman Oct 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We use an IndexMap which maintains the order of within the closure, the reason we are seeing two lines is because ancestor code thinks that there is an actual diference in the paths being used and CaptureInfo has a different path_expr_id and capture_kind_expr_id .

rust/compiler/rustc_typeck/src/check/upvar.rs

Lines 2132 to 2139 in d7f8a06

/// Helper function to determine if we need to escalate CaptureKind from
/// CaptureInfo A to B and returns the escalated CaptureInfo.
/// (Note: CaptureInfo contains CaptureKind and an expression that led to capture it in that way)
///
/// If both `CaptureKind`s are considered equivalent, then the CaptureInfo is selected based
/// on the `CaptureInfo` containing an associated `capture_kind_expr_id`.
///
/// It is the caller's duty to figure out which path_expr_id to use.

I'm fine with there being a second PR that fixed the diagnostics but I don't know if we should backport an incomplete fix. @nikomatsakis what are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indexed map doesn't help Immutable(lifetime A), Mutable(lifetime B), Mutable (lifetime A) case, and diagnostics would point to the 3rd use instead of the 2nd, because the place with lifetime A is inserted first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem you mentioned is visible today with unions:

union A {
    y: u32,
    x: (),
}

fn main() {
    let mut a = A { y: 1 };
    let mut c = || {
        let _ = unsafe { &a.y };
        let _ = &mut a; // <- should point to here
        let _ = unsafe { &mut a.y }; // <- but it points here
    };
    a.y = 1;
    c();
}

reason is the same, we relied on indexed map and disregard the actual order.

So I think I would prefer to address these together in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented your suggestion of PlaceAncestryRelation::SamePlace. By handling it like descendant instead of ancestor it will address the particular case you talked about, but it wouldn't solve the more generic case that I mentioned (I have started working on that already, but it seems too much change to backport).

// Select Ancestor/Descendant
if projections_b.len() >= projections_a.len() {
PlaceAncestryRelation::Ancestor
} else {
PlaceAncestryRelation::Descendant
match projections_b.len().cmp(&projections_a.len()) {
Ordering::Greater => PlaceAncestryRelation::Ancestor,
Ordering::Equal => PlaceAncestryRelation::SamePlace,
Ordering::Less => PlaceAncestryRelation::Descendant,
}
} else {
PlaceAncestryRelation::Divergent
Expand Down
40 changes: 40 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/issue-89606.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Regression test for #89606. Used to ICE.
//
// check-pass
// revisions: twenty_eighteen twenty_twentyone
// [twenty_eighteen]compile-flags: --edition 2018
// [twenty_twentyone]compile-flags: --edition 2021

struct S<'a>(Option<&'a mut i32>);

fn by_ref(s: &mut S<'_>) {
(|| {
let S(_o) = s;
s.0 = None;
})();
}

fn by_value(s: S<'_>) {
(|| {
let S(ref _o) = s;
let _g = s.0;
})();
}

struct V<'a>((Option<&'a mut i32>,));

fn nested(v: &mut V<'_>) {
(|| {
let V((_o,)) = v;
v.0 = (None, );
})();
}

fn main() {
let mut s = S(None);
by_ref(&mut s);
by_value(s);

let mut v = V((None, ));
nested(&mut v);
}