Ignore type of projections for upvar capturing #89648

nbdd0121 · 2021-10-07T21:10:24Z

Ignore type of projections for upvar capturing. Originally HashMap is used, and the hash/eq implementation of Place takes the type of projections into account. These types may differ by lifetime which causes #89606 to ICE.

I originally considered erasing regions but place.ty() is used when creating upvar tuple type, more than just serving as a key type, so I switched to a linear comparison with custom eq (compare_place_ignore_ty) instead.

r? @wesleywiser

@rustbot label +T-compiler

arora-aman · 2021-10-07T22:25:51Z

compiler/rustc_typeck/src/check/upvar.rs

+            .capture_information
+            .iter_mut()
+            .enumerate()
+            .find(|(_, (p, _))| compare_place_ignore_ty(p, place))


nit: you can just do

.find(|(_, (p, _))| compare_place_ignore_ty(p, place)) .map(|(idx, (p, _))| { .... }

Instead of the if let some

arora-aman · 2021-10-07T22:28:35Z

compiler/rustc_typeck/src/check/upvar.rs

-                capture_info
-            };
-            processed.insert(place, capture_info);
+            match processed.iter_mut().find(|(p, _)| compare_place_ignore_ty(p, &place)) {


I don't think this is really necessary. We can just add into the vector directly. When we process the captures it should be able to handle same place occuring in the list twice.

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

Lines 409 to 414 in d480cef

let capture_info = if let Some(existing) = processed.get(&place) {

determine_capture_info(*existing, capture_info)

} else {

capture_info

};

processed.insert(place, capture_info);

That said here it's going to be a perf vs memory trade off.

We can add this check self.demand_eqtype(usage_span, p.ty(), place.ty()); in the process_collected_capture_information function.

Actually never mind this won't handle the case since we want to compare ignoring types.

I think if there was higher precision capture this would work, eg: s.o.a and s.o were captured but regioons of s.o where different

Actually you pointed out another ICE that I haven't considered:

pub struct S<'a>((Option<&'a mut i32>,)); fn by_ref(s: &mut S<'_>) { (|| { let S((_o,)) = s; s.0 = (None, ); })(); }

The ancestry relation code needs to ignore types as well...

We can just add into the vector directly.

Are you saying that we should collect all consume/borrows in ExprUseVisitor, not bother to merge capture info, and have all the logic in process_collected_capture_information? That sounds like it should work as well. (but maybe we have to clone many times?)

My initial thoughts were it would just work out because it's possible that after truncations we same place again in that function. But I forgot about the part regarding regions in types 🤦‍♂️

But I think it would be easier if that logic was in once place.

Ideally we should either remove region (not sure if this is an option, give the use in regionchk), or maybe we implement PartialEq and Hash for Place that ignores the region.

maybe we implement PartialEq and Hash for Place that ignores the region.

IMO that would be kind of surprising for other uses of Place. However, we could create a newtype wrapper over Place that uses the compare_place_ignore_ty logic. I believe that would also allow us to continue using the FxIndexMap as we did originally.

I have considered that, but PlaceIgnoreTy cannot implement Borrow<Place> (because hash impl doesn't match), so we need transmute to get a &PlaceIgnoreTy from &Place and use that for HashMap lookup...

Also TBH I am not sure if demand_eqtype is needed here (I have it there just to be cautious, since I'm not a regionck expert), but it's very likely that it's not needed because regionck will link lifetimes together anyway.

arora-aman · 2021-10-07T22:31:23Z

compiler/rustc_typeck/src/check/upvar.rs

-
-        self.capture_information[&place_with_id.place] = updated_info;
+        let info = self.get_capture_info(&place_with_id.place).unwrap();
+        *info = determine_capture_info(*info, capture_info);


~~Same here~~

arora-aman · 2021-10-07T22:41:22Z

compiler/rustc_typeck/src/check/upvar.rs

@@ -1790,7 +1809,7 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
                capture_kind: ty::UpvarCapture::ByRef(new_upvar_borrow),
            };
            let updated_info = determine_capture_info(curr_capture_info, capture_info);


~~this can also be removed~~

nbdd0121 · 2021-10-08T03:04:09Z

New approach with just a single line change :)

Basically allow multiple occurrence of the same place initially, and let compute_min_captures remove it.

There are a few refactoring possibilities in upvar computing code, but I'll leave them as future work and keep this PR simple for backporting.

arora-aman

Can you run the code with the test I have supplied and also with the the two lines within the closure flipped?

arora-aman · 2021-10-08T04:55:46Z

compiler/rustc_typeck/src/check/upvar.rs

@@ -2278,7 +2278,7 @@ 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 {
        // First min(n, m) projections are the same


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.

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.

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?

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.

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.

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).

wesleywiser · 2021-10-09T16:32:08Z

This looks ok to me but I'd definitely like @nikomatsakis to take a look as well.

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

@arora-aman When you say "an incomplete fix", do you mean that this doesn't resolve all the diagnostic issues, or that you think there are other ICEs waiting to be found?

arora-aman · 2021-10-09T21:06:42Z

@arora-aman When you say "an incomplete fix", do you mean that this doesn't resolve all the diagnostic issues, or that you think there are other ICEs waiting to be found?

Yes, I meant that for diagnostics that I had initially pointed out. This PR LGTM and I'm fine merging it.

@nbdd0121 Thank you so much for looking into this!

nikomatsakis · 2021-10-11T20:58:59Z

@bors r+

bors · 2021-10-11T20:59:01Z

📌 Commit 7275cfa has been approved by nikomatsakis

wesleywiser · 2021-10-11T21:03:12Z

@bors p=1

Fixes stable-to-beta regression

wesleywiser · 2021-10-11T21:03:46Z

Nominating for backport as this resolves a stable-to-beta regression injected in 1.56.

bors · 2021-10-11T21:39:14Z

⌛ Testing commit 7275cfa with merge 7cc8c44...

bors · 2021-10-12T00:20:28Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 7cc8c44 to master...

rust-timer · 2021-10-12T02:29:44Z

Finished benchmarking commit (7cc8c44): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

wesleywiser · 2021-10-13T13:29:47Z

After discussion with @pnkfelix and @nikomatsakis, we've decided to approve the backport.

[beta] backports - 2229: Consume IfLet expr rust-lang#89282 - Wrapper for -Z gcc-ld=lld to invoke rust-lld with the correct flavor rust-lang#89288 - Fix unsound optimization with explicit variant discriminants rust-lang#89489 - Fix stabilization version for bindings_after_at rust-lang#89605 - Turn vtable_allocation() into a query rust-lang#89619 - Revert "Stabilize Iterator::intersperse()" rust-lang#89638 - Ignore type of projections for upvar capturing rust-lang#89648 - ~~Add Poll::ready and~~ revert stabilization of task::ready! rust-lang#89651 - CI: Use mirror for libisl downloads for more docker dist builds rust-lang#89661 - Use correct edition for panic in [debug_]assert!(). rust-lang#89622 - Switch to our own mirror of libisl plus ct-ng oldconfig fixes rust-lang#89599 - Emit item no type error even if type inference fails rust-lang#89585 - Revert enum discriminants rust-lang#89884

Closure capture cleanup & refactor Follow up of rust-lang#89648 Each commit is self-contained and the rationale/changes are documented in the commit message, so it's advisable to review commit by commit. The code is significantly cleaner (at least IMO), but that could have some perf implication, so I'd suggest a perf run. r? `@wesleywiser` cc `@arora-aman`

rust-highfive assigned wesleywiser Oct 7, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2021

arora-aman reviewed Oct 8, 2021

View reviewed changes

nbdd0121 added 2 commits October 8, 2021 04:00

Ignore projection type when determining upvar ancestory

5481201

Add regression test

d7f8a06

nbdd0121 force-pushed the issue-89606 branch from 352355b to d7f8a06 Compare October 8, 2021 03:01

arora-aman reviewed Oct 8, 2021

View reviewed changes

Explicit PlaceAncestryRelation::SamePlace and handle like Descendant

7275cfa

nikomatsakis approved these changes Oct 11, 2021

View reviewed changes

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 Oct 11, 2021

wesleywiser added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 11, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 12, 2021

bors merged commit 7cc8c44 into rust-lang:master Oct 12, 2021

rustbot added this to the 1.57.0 milestone Oct 12, 2021

nbdd0121 deleted the issue-89606 branch October 12, 2021 00:46

wesleywiser added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 13, 2021

cuviper mentioned this pull request Oct 13, 2021

[beta] backports #89854

Merged

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 13, 2021

cuviper modified the milestones: 1.57.0, 1.56.0 Oct 13, 2021

nbdd0121 mentioned this pull request Oct 13, 2021

Closure capture cleanup & refactor #89861

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore type of projections for upvar capturing #89648

Ignore type of projections for upvar capturing #89648

nbdd0121 commented Oct 7, 2021

arora-aman Oct 7, 2021

arora-aman Oct 7, 2021

arora-aman Oct 7, 2021

arora-aman Oct 8, 2021

nbdd0121 Oct 8, 2021

nbdd0121 Oct 8, 2021

arora-aman Oct 8, 2021

wesleywiser Oct 8, 2021

nbdd0121 Oct 8, 2021

nbdd0121 Oct 8, 2021

arora-aman Oct 7, 2021 •

edited

Loading

arora-aman Oct 7, 2021 •

edited

Loading

nbdd0121 commented Oct 8, 2021

arora-aman left a comment

arora-aman Oct 8, 2021

nbdd0121 Oct 8, 2021 •

edited

Loading

arora-aman Oct 8, 2021 •

edited

Loading

nbdd0121 Oct 8, 2021

nbdd0121 Oct 8, 2021

nbdd0121 Oct 9, 2021

wesleywiser commented Oct 9, 2021 •

edited

Loading

arora-aman commented Oct 9, 2021 •

edited

Loading

nikomatsakis commented Oct 11, 2021

bors commented Oct 11, 2021

wesleywiser commented Oct 11, 2021

wesleywiser commented Oct 11, 2021

bors commented Oct 11, 2021

bors commented Oct 12, 2021

rust-timer commented Oct 12, 2021

wesleywiser commented Oct 13, 2021

	let capture_info = if let Some(existing) = processed.get(&place) {
	determine_capture_info(*existing, capture_info)
	} else {
	capture_info
	};
	processed.insert(place, capture_info);

	/// 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.

Ignore type of projections for upvar capturing #89648

Ignore type of projections for upvar capturing #89648

Conversation

nbdd0121 commented Oct 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arora-aman Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

arora-aman Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

nbdd0121 commented Oct 8, 2021

arora-aman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

arora-aman Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesleywiser commented Oct 9, 2021 • edited Loading

arora-aman commented Oct 9, 2021 • edited Loading

nikomatsakis commented Oct 11, 2021

bors commented Oct 11, 2021

wesleywiser commented Oct 11, 2021

wesleywiser commented Oct 11, 2021

bors commented Oct 11, 2021

bors commented Oct 12, 2021

rust-timer commented Oct 12, 2021

wesleywiser commented Oct 13, 2021

arora-aman Oct 7, 2021 •

edited

Loading

arora-aman Oct 7, 2021 •

edited

Loading

nbdd0121 Oct 8, 2021 •

edited

Loading

arora-aman Oct 8, 2021 •

edited

Loading

wesleywiser commented Oct 9, 2021 •

edited

Loading

arora-aman commented Oct 9, 2021 •

edited

Loading