2229: Handle patterns within closures correctly when `capture_disjoint_fields` is enabled #82536

roxelo · 2021-02-25T23:26:38Z

This PR fixes several issues related to handling patterns within closures when capture_disjoint_fields is enabled.

Matching is always considered a use of the place, even with _ patterns
Compiler ICE when capturing fields in closures through let assignments

To do so, we

Introduced new Fake Reads
Delayed use of Place in favor of PlaceBuilder
Ensured that PlaceBuilder can be resolved before attempting to extract Place in any of the pattern matching code

Closes rust-lang/project-rfc-2229/issues/27
Closes rust-lang/project-rfc-2229/issues/24
r? @nikomatsakis

arora-aman · 2021-02-26T06:50:00Z

Can you add a test file that prints out the capture analysis for the following cases:

This should capture:

match some_multi_variant_enum {
    Some(_) => {}
    _ => {}
}

This shouldn't capture:

match some_multi_variant_enum {
    _ => {}
}

I think the following might capture, and we don't want to (playground outside closures)

match single_variant_enum {
    SingleVariant::Point(_) => {}
}

This shouldn't capture:

match arr {
    [_, _, _] => {}
}

This should capture:

enum X { A, B, C }; // no values associated

let x: X;

match x {
    X::A => {}
     _ => {}
}

arora-aman

I'm wondering if we want tests that dump out the MIR so that we can see the fake reads added before the closure is constructed. I think I have seen some similar tests, but I'm not sure, I'll try look them up tomorrow.

src/test/ui/closures/2229_closure_analysis/run_pass/struct_update_syntax.rs

src/test/ui/closures/2229_closure_analysis/run_pass/no_capture_with_wildcard_match.rs

src/test/ui/closures/2229_closure_analysis/run_pass/lit-pattern-matching-with-methods.rs

src/test/ui/closures/2229_closure_analysis/run_pass/destructure_patterns.rs

...est/ui/closures/2229_closure_analysis/run_pass/destructure-pattern-closure-within-closure.rs

compiler/rustc_typeck/src/expr_use_visitor.rs

compiler/rustc_mir_build/src/build/matches/test.rs

lqd · 2021-02-27T21:13:41Z

@bors try @rust-timer queue

rust-timer · 2021-02-27T21:13:42Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-02-27T21:13:50Z

⌛ Trying commit d566cb4d2a4681c744880151b114618cbdd9df9a with merge 569f22e3240a3d7862bb8b35ab64479e839d35b5...

compiler/rustc_typeck/src/expr_use_visitor.rs

src/test/ui/closures/2229_closure_analysis/patterns-capture-analysis.rs

arora-aman · 2021-02-27T21:02:34Z

src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern.stderr

+LL |           let (ref _x0, _x1, _) = tup;
+   |                                   --- variable moved due to use in closure
+LL |       };
+LL |       let c2 = || {


Hmm this is pointing to the wrong message. We might want to add something like this in this PR and then use it

@roxelo This is still needs to be fixed, I should probably spend sometime and see where this is coming from

I built this branch with the part that adds the fake reads onto the cfg commented out, and the error was reported as expected.

...est/ui/closures/2229_closure_analysis/run_pass/tuple-struct-pattern-matching-with-methods.rs

src/test/ui/closures/2229_closure_analysis/run_pass/struct-pattern-matching-with-methods.rs

compiler/rustc_mir_build/src/thir/cx/expr.rs

compiler/rustc_mir_build/src/build/expr/as_place.rs

bors · 2021-02-27T22:03:01Z

☀️ Try build successful - checks-actions
Build commit: 569f22e3240a3d7862bb8b35ab64479e839d35b5 (569f22e3240a3d7862bb8b35ab64479e839d35b5)

rust-timer · 2021-02-27T22:03:03Z

Queued 569f22e3240a3d7862bb8b35ab64479e839d35b5 with parent 94736c4, future comparison URL.

rust-timer · 2021-02-28T00:57:34Z

Finished benchmarking try commit (569f22e3240a3d7862bb8b35ab64479e839d35b5): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

nikomatsakis

First round of comments.

compiler/rustc_mir_build/src/build/expr/as_place.rs

nikomatsakis · 2021-02-27T14:33:37Z

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

@@ -214,6 +215,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                        }
                    })
                    .collect();
+
+                for thir_place in fake_reads.into_iter() {


I think this code is right, but it could use a comment. In particular, I think it's important that the fake reads get inserted after we build all the captures for the closure (and before the closure itself is created). My reasoning for this that it might be possible to have a fake read that conflicts with one of the borrows-- actually, maybe that's not possible in Rust surface syntax, because if they referred to the same path, we would coalesce them into a capture?

I guess I'm imagining an example like:

let mut x = 10; || { match x { _ => () } // this counts as a fake read, right? x += 1; // and this a borrow? }

Now that I think about it, i'm actually unsure if the reads are in the right place! What happens with this example? I'll have to read your PR more and find out!

Yes, this is what should happen with your example

EDIT: I fixed the ICE issue

I am now thinking that you are maybe right and fake_reads are added at the wrong time. I was trying to add more tests and I found a test that ICE when attempting to resolved a PlaceBuilder here

#![feature(capture_disjoint_fields)] //~^ WARNING: the feature `capture_disjoint_fields` is incomplete //~| `#[warn(incomplete_features)]` on by default //~| see issue #53488 <https://github.com/rust-lang/rust/issues/53488> #![feature(never_type)] fn main() { let x: !; let c2 = || match x { _ => () }; //~^ ERROR: use of possibly-uninitialized variable: `x` }

compiler/rustc_mir_build/src/build/matches/mod.rs

nikomatsakis · 2021-03-01T18:50:07Z

compiler/rustc_mir_build/src/build/matches/mod.rs

        set_match_place: bool,
    ) -> BlockAnd<()> {
+        let place = initializer.clone().into_place(self.hir.tcx(), self.hir.typeck_results());


Taking ownership of initializer and then immediately cloning it definitely seems wrong :)

I agree, it seems wrong, but I don't see how I could make it work with a PlaceBuilder reference instead.
If I attempt to use a reference of PlaceBuilder, I need to update both Candidate and MatchPair so they hold references of PlaceBuilder instead. The issues arrises with the MatchPair code, where we do changes to the PlaceBuilder but we don't have anyone to take ownership of the resultant PlaceBuilder for long enough.
For example, here someone needs to own the the PlaceBuilder that was downcasted: https://github.com/rust-lang/rust/blob/75f10f34c3e43e40492d194d3a3731ba7287f298/compiler/rustc_mir_build/src/build/matches/test.rs#L745-L750
Let me know if there is something I didn't consider, but at this time I don't see how it's feasible...

compiler/rustc_typeck/src/expr_use_visitor.rs

compiler/rustc_mir_build/src/build/matches/mod.rs

compiler/rustc_mir_build/src/thir/cx/expr.rs

nikomatsakis · 2021-03-01T20:38:25Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                            }
+                        } else if let PatKind::TupleStruct(qpath, _, _) = &pat.kind {
+                            // If a TupleStruct has a Some PathSegment, we should read the discr_place
+                            // regardless if it contains a Wild pattern later


I think I would find this comment hard to understand if I lacked context. Can you add a code example?

I think it would be great to e.g. showing matching on an Option and explain that this does require reading the discriminant, even if we don't load any data.

nikomatsakis · 2021-03-01T20:42:58Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

@@ -305,15 +305,15 @@ impl<'tcx> PlaceBuilder<'tcx> {
        to_upvars_resolved_place_builder(self, tcx, typeck_results).unwrap()
    }

+    /// try_upvars_resolved will attempt to resolve the PlaceBuilder.
+    /// On success, it will return the resolved PlaceBuilder
+    /// On failure, it will return itself


maybe good to explain why it might fail

nikomatsakis · 2021-03-03T21:29:15Z

Example test that I think will fail with the code as is:

let mut x = 1;
let c = || {
    drop(&mut x);
    match x { _ => () }
};

arora-aman · 2021-03-05T06:20:53Z

One thing that we discussed during the 2229 sync on wednesday was that instead of adding a test that dumps MIR it might be easier to add tests that fail in the case the fake reads were missing.

eg: Match on uninitialized never type

#![feature(never_type)]
fn main() {
    let x: !;
    let c = || match x { }; // ERROR: borrow of possibly-uninitialized variable: `x`
}

nikomatsakis · 2021-03-05T11:34:34Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

+                // For single variants, enums are not captured completely.
+                // We keep track of VariantIdx so we can use this information
+                // if the next ProjectionElem is a Field.
+                variant = Some(*idx);


So-- I'm a bit confused by this change. Are we trying to truncate for multi-variant enums? Do we expect multi-variant enums to show up in this code path?

If we capture a multi-variant enum, we will do it in entierty in typchk, i.e. they will be trunacted by the time we get here

Though i think the comment should be something like, "We capture multi-varirant enums completely, but we might see a downcast projection in case of single variant enums, so we need to account for it here."

We just happen to be accounting for it more generally. Since we search for ancestors in the list of captures of the place returned by this function, I think its fine to do what we are doing here.

I think the "we capture multi-variant enums completely" is actually sort of confusingly phrased. I would say something like:

We don't expect to see multi-variant enums here, as earlier phases will have truncated them already. However, there can still be downcasts, thanks to single-variant enums.

If this is correct, can we add an assertion that idx is 0?

I dont think that is quite right. We are going from a MIR place -> HIR place. For all the HIR places we have in our capture set, we would stop at the multivairant enum and not capture anything on top of it. However the MIR place might be for something precise, eg:

struct Foo(String, String) x: Option<Foo> let c = || match x { Some(Foo(, y)) => // Upvar(x), [downcast(0), field(0), field(1)] // ...

The ideal thing to do here would really be to look at the type before a downcast projection and see if it's a multi-variant enum and break, but that is a little hard to do here -- because we don't know the actual place since we don't know the capture index, and hence can't get the type. We can make it work by checking if we know any anscetors (in our capture set) of the partially converted place. This sort of makes the code hard to follow and somewhat cyclic in logic where to find a valid ancestor we need to convert the mir place into some hir place and to do that we need to find a valid ancestor.
EDIT: Though this is like base case of a recursion.

On the otherhand, the field projection in HirPlace is defined as Field(field_idx, variant_idx) which as I understand is Downcast to variant_idx and then read the field with index field_idx. Which is cleaner to express, somewhat more precise and not an incorrect expression of what is happening here. We can then make the function that compares two places to check for the ancestor relation to handle this case properly, and since our capture rules don't really capture multivariant enums it doesn't have to do much for it.
EDIT: Not sure if this is any better to follow for a person who is not aware about these portions of the codebase

Another thing to note here is that a multivariant enum has a variant with index 0, so checking for 0 is not an "if and only if" like check.

If we agree on this approach to thinking about the problem, we would need to update the comments to reflect this

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

nikomatsakis · 2021-03-08T10:56:25Z

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -237,12 +246,35 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                let arm_scope = (arm.scope, arm_source_info);
                self.in_scope(arm_scope, arm.lint_level, |this| {
                    let body = this.hir.mirror(arm.body.clone());
+
+                    // `try_upvars_resolved` may fail if it is unable to resolved the given


Suggested change

// `try_upvars_resolved` may fail if it is unable to resolved the given

// `try_upvars_resolved` may fail if it is unable to resolve the given

nikomatsakis · 2021-03-08T10:59:03Z

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -479,7 +509,29 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                        VarBindingForm { opt_match_place: Some((ref mut match_place, _)), .. },
                    )))) = self.local_decls[local].local_info
                    {
-                        *match_place = Some(initializer);
+                        // `try_upvars_resolved` may fail if it is unable to resolved the given


Suggested change

// `try_upvars_resolved` may fail if it is unable to resolved the given

// `try_upvars_resolved` may fail if it is unable to resolve the given

nikomatsakis · 2021-03-08T10:59:18Z

compiler/rustc_mir_build/src/build/matches/mod.rs

+                        // `PlaceBuilder` inside a closure. In this case, we don't want to include
+                        // a scrutinee place. `scrutinee_place_builder` will fail for destructured
+                        // assignments. This is because a closure only captures the precise places
+                        // that will read and as a result a closure may not capture the entire tuple


Suggested change

// that will read and as a result a closure may not capture the entire tuple

// that it will read and as a result a closure may not capture the entire

nikomatsakis · 2021-03-08T11:04:39Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                                    }
+                                }
+                            }
+                            PatKind::Tuple(..) | PatKind::Lit(_) => {


I'm not sure why this would be the case for a tuple... a tuple seems equivalent to a struct with a single variant.

compiler/rustc_typeck/src/expr_use_visitor.rs

nikomatsakis · 2021-03-08T11:13:50Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                            //       let (_, t2) = v2;
+                            //    }
+                            // }
+                            // ```


This code example looks right to me, but I can't quite tell what the purpose of this check is -- I would sort of expect this to just "fall out' from other bits of the code working correctly. That is, I would expect that the closure_fake_reads field on the typeck results for c would only have v1, and the same field for e would have v1 and v2. Am I wrong about this?

It might help if you clarified whether you expect this continue to fire when visiting the outermost body here (that contains let c = ...) or when visiting the body of c (which contains let e = ...).

The fake read of v1 in c only happens here in the code. It's similar idea to how captures from the nest closures are added to the outermost closures here: https://github.com/rust-lang/rust/blob/bfcecbd0dc429014395062ab0df4d2306cc76766/compiler/rustc_typeck/src/expr_use_visitor.rs#L700

Side note that I personally find map_or to be very hard to read. I tend to prefer foo.map(...).unwrap_or(...) -- map-or requires me to read the fallback case before the function, out of order with what happens at runtime, and it's confusing :/

OK, so, I think this is what is going on:

We read the upvar list of the "inner" closure.

For each upvar, we check if it is also an upvar for the current closure.

If so, we generate a fake read?

I think I find this confusing also for the other code down below. I think what I expect is:

We go through the fake reads from the inner closure, each of which is a fake read of a captured path P.

I think those captured paths P are expressed in terms of the closure creator (i.e., us), so no translation is needed, we just do a delegate.fake_read(captured_path)

But if translation is needed, then maybe I would expect a helper that says "is this upvar a local variable here?", and then we could just use that helper here and below.

Does that make sense? Am I missing something? (cc @arora-aman, I think the code that @roxelo cited is code you wrote? Or maybe I'm misremembering)

That is kind of what's happening. There is a code sample but it is at function-doc level

https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/expr_use_visitor.rs#L589-L595
Maybe we should bring it within the function.

I think those captured paths P are expressed in terms of the closure creator (i.e., us), so no translation is needed, we just do a delegate.fake_read(captured_path)

I don't think this is correct, the fake reads that are stored within an inner/nested closure are (and should be) from the prespective of that nested closure. When we take this and convert it from the prespective of the enclosing/outer closure we'd have to convert the Place to say so.
For reference see: https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/expr_use_visitor.rs#L624-L631

I like the idea behind the having a helper because this was confusing earlier aswell.

nikomatsakis · 2021-03-08T11:15:17Z

Hi @roxelo! Left some more comments. This is looking really close. :) I am a bit confused about a few things still, though.

nikomatsakis

Looking great! I would like to resolve this nested closure case. I feel like the code feels more complicated than it has to feel.

nikomatsakis · 2021-03-09T10:04:02Z

compiler/rustc_middle/src/ty/context.rs

+    /// }
+    /// ```
+    /// In the first example, we don't want to read/borrow `x` in `c` as the only match
+    /// arm contains only a wildcard.


I think it might be useful to extend the first example to be:

let x: u8; let c = || match x { _ => () };

then you can say "in this example, we don't need to actually read/borrow x in c, and so we don't want to capture it. However, we do still want an error here, because x should have to be initialized at the point where c is created. Therefore, we add a "fake read" instead."

Actually, I know we have tests for cases where x has type !, but do we have a test for a case like this? I imagine we want that.

nikomatsakis · 2021-03-09T10:09:56Z

compiler/rustc_mir_build/src/build/expr/as_place.rs

+                // For single variants, enums are not captured completely.
+                // We keep track of VariantIdx so we can use this information
+                // if the next ProjectionElem is a Field.
+                variant = Some(*idx);


I think the "we capture multi-variant enums completely" is actually sort of confusingly phrased. I would say something like:

We don't expect to see multi-variant enums here, as earlier phases will have truncated them already. However, there can still be downcasts, thanks to single-variant enums.

If this is correct, can we add an assertion that idx is 0?

nikomatsakis · 2021-03-09T10:16:51Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                            //       let (_, t2) = v2;
+                            //    }
+                            // }
+                            // ```


Side note that I personally find map_or to be very hard to read. I tend to prefer foo.map(...).unwrap_or(...) -- map-or requires me to read the fallback case before the function, out of order with what happens at runtime, and it's confusing :/

nikomatsakis · 2021-03-09T10:28:09Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                            //       let (_, t2) = v2;
+                            //    }
+                            // }
+                            // ```


OK, so, I think this is what is going on:

We read the upvar list of the "inner" closure.

For each upvar, we check if it is also an upvar for the current closure.

If so, we generate a fake read?

I think I find this confusing also for the other code down below. I think what I expect is:

We go through the fake reads from the inner closure, each of which is a fake read of a captured path P.

I think those captured paths P are expressed in terms of the closure creator (i.e., us), so no translation is needed, we just do a delegate.fake_read(captured_path)

But if translation is needed, then maybe I would expect a helper that says "is this upvar a local variable here?", and then we could just use that helper here and below.

Does that make sense? Am I missing something? (cc @arora-aman, I think the code that @roxelo cited is code you wrote? Or maybe I'm misremembering)

arora-aman · 2021-03-09T19:36:01Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                            !upvars.contains_key(&upvar_id.var_path.hir_id)
+                        }) {
+                            // The nested closure might be fake reading the current (enclosing) closure's local variables.
+                            // We only want to fake read the fake read present in the nested closure that are not part of


Maybe "The only places we want to fake read before creating the parent closure are the ones that are not local to it/ defined by it."

arora-aman · 2021-03-09T19:48:39Z

compiler/rustc_typeck/src/expr_use_visitor.rs

+                            //       let (_, t2) = v2;
+                            //    }
+                            // }
+                            // ```


That is kind of what's happening. There is a code sample but it is at function-doc level

https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/expr_use_visitor.rs#L589-L595
Maybe we should bring it within the function.

I think those captured paths P are expressed in terms of the closure creator (i.e., us), so no translation is needed, we just do a delegate.fake_read(captured_path)

I don't think this is correct, the fake reads that are stored within an inner/nested closure are (and should be) from the prespective of that nested closure. When we take this and convert it from the prespective of the enclosing/outer closure we'd have to convert the Place to say so.
For reference see: https://github.com/rust-lang/rust/blob/master/compiler/rustc_typeck/src/expr_use_visitor.rs#L624-L631

I like the idea behind the having a helper because this was confusing earlier aswell.

arora-aman · 2021-03-10T10:12:19Z

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

+                        place_builder.try_upvars_resolved(this.hir.tcx(), this.hir.typeck_results())
+                    {
+                        let mir_place = place_builder_resolved
+                            .clone()


Do we need to clone here?

arora-aman · 2021-03-10T10:25:07Z

src/test/ui/pattern/move-ref-patterns/borrowck-move-ref-pattern.stderr

+LL |           let (ref _x0, _x1, _) = tup;
+   |                                   --- variable moved due to use in closure
+LL |       };
+LL |       let c2 = || {


I built this branch with the part that adds the fake reads onto the cfg commented out, and the error was reported as expected.

arora-aman · 2021-03-10T10:33:27Z

compiler/rustc_mir_build/src/build/expr/as_rvalue.rs

+                        let mir_place = place_builder_resolved
+                            .clone()
+                            .into_place(this.hir.tcx(), this.hir.typeck_results());
+                        this.cfg.push_fake_read(block, source_info, cause, mir_place);


@roxelo so the source_info here is the span of the expression currently being processed, which in this case is the closure itself.

In the test case where the diagnostics are currently affected, the fake reads conflicts with the place that was moved earlier and for diagnostics we are providing the entire closure.

To get proper diagnostics we want to have the span of the place being fake read as the source_span, so maybe we add a diag_expr similar to other ExprUseVisitor::Delegate methods and store that in typeck results and then use it here.

There is sort of another question at the back of my mind, which is should the fake reads have caused this conflict?

On the surface I think it makes sense for them to do so, because we shouldn't be able to even fake read a place that has already been moved, but would be nice if we can confirm this is valid/acceptable behavior

for eg:

fn closure() { let mut tup = (U, U, U); // 8 let c1 = || { // 9 let (ref _x0, _x1, _) = tup; // 10 }; // 11 let c2 = || { // 12 //~^ ERROR use of moved value let (ref mut _x0, _, _x2) = tup; }; // 15 drop(c1); }

mir is:

FakeRead(ForLet, _1); // scope 0 at borrowck-move-ref-pattern.rs:8:9: 8:16 StorageLive(_5); // scope 1 at borrowck-move-ref-pattern.rs:9:9: 9:11 FakeRead(ForLet, _1); // scope 1 at borrowck-move-ref-pattern.rs:9:14: 11:6 _5 = [closure@borrowck-move-ref-pattern.rs:9:14: 11:6] { tup: move _1 }; // scope 1 at borrowck-move-ref-pattern.rs:9:14: 11:6 // closure // + def_id: DefId(0:7 ~ borrowck_move_ref_pattern[317d]::closure::{closure#0}) // + substs: [ // i32, // extern "rust-call" fn(()), // ((U, U, U),), // ] FakeRead(ForLet, _5); // scope 1 at borrowck-move-ref-pattern.rs:9:9: 9:11 StorageLive(_6); // scope 2 at borrowck-move-ref-pattern.rs:12:9: 12:11 FakeRead(ForLet, _1); // scope 2 at borrowck-move-ref-pattern.rs:12:15: 15:6 _6 = [closure@borrowck-move-ref-pattern.rs:12:15: 15:6] { tup: move _1 }; // scope 2 at borrowck-move-ref-pattern.rs:12:15: 15:6

roxelo · 2021-03-16T16:32:36Z

r=nikomatsakis

roxelo · 2021-03-16T16:33:49Z

@bors r=nikomatsakis

bors · 2021-03-16T16:33:50Z

📌 Commit 189d206 has been approved by nikomatsakis

bors · 2021-03-16T19:19:11Z

⌛ Testing commit 189d206 with merge f5d8117...

bors · 2021-03-16T22:05:38Z

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

…=nikomatsakis 2229: Handle patterns within closures correctly when `capture_disjoint_fields` is enabled This PR fixes several issues related to handling patterns within closures when `capture_disjoint_fields` is enabled. 1. Matching is always considered a use of the place, even with `_` patterns 2. Compiler ICE when capturing fields in closures through `let` assignments To do so, we - Introduced new Fake Reads - Delayed use of `Place` in favor of `PlaceBuilder` - Ensured that `PlaceBuilder` can be resolved before attempting to extract `Place` in any of the pattern matching code Closes rust-lang/project-rfc-2229/issues/27 Closes rust-lang/project-rfc-2229/issues/24 r? `@nikomatsakis`

…=nikomatsakis 2229: Fix diagnostic issue when using FakeReads in closures This PR fixes a diagnostic issue caused by rust-lang#82536. A temporary work around was used in this merged PR which involved feature gating the addition of FakeReads introduced as a result of pattern matching in closures. The fix involves adding an optional closure DefId to ForLet and ForMatchedPlace FakeReadCauses. This DefId will only be added if a closure pattern matches a Place starting with an Upvar. r? `@nikomatsakis`

…=nikomatsakis 2229: Fix diagnostic issue when using FakeReads in closures This PR fixes a diagnostic issue caused by rust-lang#82536. A temporary work around was used in this merged PR which involved feature gating the addition of FakeReads introduced as a result of pattern matching in closures. The fix involves adding an optional closure DefId to ForLet and ForMatchedPlace FakeReadCauses. This DefId will only be added if a closure pattern matches a Place starting with an Upvar. r? ``@nikomatsakis``

…=nikomatsakis 2229: Fix diagnostic issue when using FakeReads in closures This PR fixes a diagnostic issue caused by rust-lang#82536. A temporary work around was used in this merged PR which involved feature gating the addition of FakeReads introduced as a result of pattern matching in closures. The fix involves adding an optional closure DefId to ForLet and ForMatchedPlace FakeReadCauses. This DefId will only be added if a closure pattern matches a Place starting with an Upvar. r? `@nikomatsakis`

…=nikomatsakis 2229: Fix diagnostic issue when using FakeReads in closures This PR fixes a diagnostic issue caused by rust-lang#82536. A temporary work around was used in this merged PR which involved feature gating the addition of FakeReads introduced as a result of pattern matching in closures. The fix involves adding an optional closure DefId to ForLet and ForMatchedPlace FakeReadCauses. This DefId will only be added if a closure pattern matches a Place starting with an Upvar. r? ``@nikomatsakis``

…=nikomatsakis 2229: Fix diagnostic issue when using FakeReads in closures This PR fixes a diagnostic issue caused by rust-lang#82536. A temporary work around was used in this merged PR which involved feature gating the addition of FakeReads introduced as a result of pattern matching in closures. The fix involves adding an optional closure DefId to ForLet and ForMatchedPlace FakeReadCauses. This DefId will only be added if a closure pattern matches a Place starting with an Upvar. r? ```@nikomatsakis```

pnkfelix · 2021-05-21T19:30:58Z

FYI this appears to have injected issue #85561

rust-highfive assigned nikomatsakis Feb 25, 2021

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

roxelo marked this pull request as ready for review February 25, 2021 23:31

roxelo mentioned this pull request Feb 25, 2021

Handle patterns within closures sexxi-goose/rust#45

Merged

arora-aman reviewed Feb 26, 2021

View reviewed changes

roxelo force-pushed the handle-patterns-take-2 branch 2 times, most recently from ea9060a to d566cb4 Compare February 26, 2021 20:53

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 27, 2021

arora-aman reviewed Feb 27, 2021

View reviewed changes

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 28, 2021

roxelo force-pushed the handle-patterns-take-2 branch from d566cb4 to 9aca998 Compare March 1, 2021 02:11

nikomatsakis reviewed Mar 1, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

roxelo force-pushed the handle-patterns-take-2 branch from a1d3cb5 to bfcecbd Compare March 6, 2021 02:38

nikomatsakis approved these changes Mar 8, 2021

View reviewed changes

nikomatsakis reviewed Mar 9, 2021

View reviewed changes

arora-aman reviewed Mar 9, 2021

View reviewed changes

arora-aman reviewed Mar 10, 2021

View reviewed changes

roxelo added 3 commits March 14, 2021 19:28

Use the correct FakeReadCause

685a4c6

Only borrow place for matching under specific conditions

74fc643

Add fake_read() to clippy

fb3b77a

roxelo force-pushed the handle-patterns-take-2 branch from 459ebf8 to 88de9c2 Compare March 15, 2021 04:36

This comment has been minimized.

Sign in to view

roxelo added 2 commits March 15, 2021 13:16

Add comments with examples and tests

22eaffe

Fix error after rebase

189d206

roxelo force-pushed the handle-patterns-take-2 branch from 88de9c2 to 189d206 Compare March 15, 2021 17:23

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 Mar 16, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 16, 2021

bors merged commit f5d8117 into rust-lang:master Mar 16, 2021

rustbot added this to the 1.52.0 milestone Mar 16, 2021

roxelo mentioned this pull request Mar 26, 2021

2229: Fix diagnostic issue when using FakeReads in closures #83521

Merged

ldm0 mentioned this pull request Aug 6, 2021

Closure captures immutable reference when mutable reference is necessary #87814

Closed

	// `try_upvars_resolved` may fail if it is unable to resolved the given
	// `try_upvars_resolved` may fail if it is unable to resolve the given

	// that will read and as a result a closure may not capture the entire tuple
	// that it will read and as a result a closure may not capture the entire

2229: Handle patterns within closures correctly when capture_disjoint_fields is enabled #82536

2229: Handle patterns within closures correctly when capture_disjoint_fields is enabled #82536

Conversation

roxelo commented Feb 25, 2021 • edited Loading

arora-aman commented Feb 26, 2021 • edited Loading

arora-aman left a comment

Choose a reason for hiding this comment

lqd commented Feb 27, 2021

rust-timer commented Feb 27, 2021

bors commented Feb 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 27, 2021

rust-timer commented Feb 27, 2021

rust-timer commented Feb 28, 2021

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roxelo Mar 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Mar 3, 2021

arora-aman commented Mar 5, 2021 • edited Loading

This comment has been minimized.

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 Mar 9, 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

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 Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Mar 8, 2021

nikomatsakis left a 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 Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arora-aman Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

roxelo commented Mar 16, 2021

roxelo commented Mar 16, 2021

bors commented Mar 16, 2021

bors commented Mar 16, 2021

bors commented Mar 16, 2021

pnkfelix commented May 21, 2021

2229: Handle patterns within closures correctly when `capture_disjoint_fields` is enabled #82536

2229: Handle patterns within closures correctly when `capture_disjoint_fields` is enabled #82536

roxelo commented Feb 25, 2021 •

edited

Loading

arora-aman commented Feb 26, 2021 •

edited

Loading

roxelo Mar 5, 2021 •

edited

Loading

arora-aman commented Mar 5, 2021 •

edited

Loading

arora-aman Mar 9, 2021 •

edited

Loading

arora-aman Mar 9, 2021 •

edited

Loading

arora-aman Mar 9, 2021 •

edited

Loading

arora-aman Mar 10, 2021 •

edited

Loading