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

Can mutate in match-arm using a closure #27282

Open
arielb1 opened this issue Jul 25, 2015 · 23 comments

Comments

Projects
None yet
7 participants
@arielb1
Copy link
Contributor

commented Jul 25, 2015

Update from pnkfelix: Note that this is now fixed by NLL (see #50783). This bug just remains open because @pnkfelix thinks our policy is to keep NLL-fixed-by-NLL bugs open until we make NLL the default for rustc.


STR

fn main() {
    match Some(&4) {
        None => {},
        ref mut foo
            if {
                (|| { let bar = foo; bar.take() })();
                false
            } => {},
        Some(s) => println!("{}", *s)
    }
}

Actual Results

playpen: application terminated abnormally with signal 4 (Illegal instruction)
@eefriedman

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2015

Ideally, the type of "foo" in the pattern guard should be &Option<&i32> rather than &mut Option<&i32>... then we could loosen the overly restrictive mutation rules in pattern guards.

We might be able to specifically check for this case at

fn check_for_mutation_in_guard<'a, 'tcx>(cx: &'a MatchCheckCtxt<'a, 'tcx>,
; the problem there is that it could end up spuriously detecting a mutable borrow.

Another possibility would be to make it impossible to move out of variables of type &mut _; this is probably something we want to do anyway to eliminate the distinction between let bar = foo; (which moves) and let foo : &mut _ = foo (which reborrows).

@steveklabnik

This comment has been minimized.

Copy link
Member

commented Feb 7, 2017

This compiles with no error today. Nominating.

@arielb1

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2017

Fix by MIR borrowck. Forgot to tag as I-unsound myself.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Dec 20, 2017

Oddly this seems to be on the road to be fixed (as in flagged as error) by -Z borrowck=mir, but then when you add -Z nll to the mix, the error is again missed.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2017

The NLL analysis not wrong exactly, but there is a kind of implicit borrow that's not manifest in the MIR. In particular, the NLL analysis determines that the ref mut foo is only used in one particular basic block, and hence there are no overlapping uses of the value being matched.

That is, the MIR is sort of doing piecemeal tests that read the value of the option, but assuming that the option will not change in the meantime. We can guarantee that it won't by taking a borrow, but we don't do that. So effectively we get this:

let mut v = Some(4);

if v.is_none() { ... }

let mut p = &mut v;
(|| { p.take(); () })(); // clears `v`

let s = v.unwrap(); // panics here, but UB in MIR

except that the final unwrap() would be UB.

At least that's how I read it now.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

for now I'm labelled this with the NLL labels. We may not be able to include a fix for this in the initial release, but it would be nice to try...

@nikomatsakis nikomatsakis added this to the NLL Future Compat Warnings milestone Jan 4, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2018

Seems like we have to fix this. I'm not sure just what the best way is -- but I think we have to represent the "implicit borrow" in the MIR.

@pnkfelix pnkfelix self-assigned this Jan 10, 2018

@pnkfelix pnkfelix added P-high and removed P-medium labels Jan 10, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jan 10, 2018

Assigning to self and up'ing priority to P-high to reflect severity for deploying NLL.

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

I am currently exploring extending MIR with a way to express "borrow all the discriminants reachable from place" so that we can freeze the variant alone over the course of all patterns+guards for a given match. Hopefully that can address this bug.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

@pnkfelix I've been having second thoughts on that approach. For example, consider this:

let mut b = true;
match b {
    true => ...
    false => ...
}

no "discriminant" is touched, but we want to know that no writes occur.

I think we should think again about idea of shared borrow of the entire value being matched and then having guards access their bindings through a &T (where T is the user-visible type of the binding). To handle ref mut bindings, we can consider creating (for the guard) a ref binding and using a cast to coerce the &'a &'a T to a &'a &'a mut T (which I believe are identical in power anyhow).

That last part is a hack, no question, but overall the approach is pretty elegant, and seems like it would cover all cases.

Thoughts?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

Seems like my suggestion (from our private conversation) of an explicit EndBorrow would also handle the case you mention

But I’m willing to try your suggestion. Either direction has some hackery to it

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 25, 2018

rust-lang#27282: emit `ReadForMatch` on each match arm.
Also, turn on ReadForMatch emission by default (when using NLL).

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 25, 2018

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 28, 2018

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 28, 2018

rust-lang#27282: Add `StatementKind::ReadForMatch` to MIR.
(This is just the data structure changes and some boilerplate match
code that followed from it; the actual emission of these statements
comes in a follow-up commit.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 28, 2018

rust-lang#27282: emit `ReadForMatch` on each match arm.
Also, turn on ReadForMatch emission by default (when using NLL).

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 28, 2018

bors added a commit that referenced this issue May 29, 2018

Auto merge of #50783 - pnkfelix:issue-27282-match-borrows-its-input-t…
…ake-three, r=nikomatsakis

every match arm reads the match's borrowed input

This PR changes the `match` codegen under NLL (and just NLL, at least for now) to make the following adjustments:
 * It adds a `-Z disable-ast-check-for-mutation-in-guard` which, as described, turns off the naive (conservative but also not 100% sound) check for mutation in guards of match arms.
 * We now borrow the match input at the outset and emit a special `ReadForMatch` statement (that, according to the *static* semantics, reads that borrowed match input) at the start of each match arm. The intent here is to catch cases where the match guard mutates the match input, either via an independent borrow or via `ref mut` borrows in that arm's pattern.
 * In order to ensure that `ref mut` borrows do not actually conflict with the emitted `ReadForMatch` statements, I expanded the two-phase-borrow system slightly, and also changed the MIR code gen so that under NLL, when there is a guard on a match arm, then each pattern variable ends up having *three* temporaries associated with it:
   1. The first temporary will hold the substructure being matched; this is what we will move the (substructural) value into *if* the guard succeeds.
   2. The second temporary also corresponds to the same value as the first, but we are just constructing this temporarily for use during the scope of the guard; it is unaliased and its sole referrer is the third temporary.
   3. The third temporary is a reference to the second temporary.
   * (This sounds complicated, I know, but its actually *simpler* than what I was doing before and had checked into the repo, which was to sometimes construct the final value and then take a reference to it before evaluating the guard. See also PR #49870.)

Fix #27282

This also provides a path towards resolving #24535 aka rust-lang/rfcs#1006, at least once the `-Z disable-ast-check-for-mutation-in-guard` is just turned on by default (under NLL, that is. It is not sound under AST-borrowck).
 * But I did not want to make `#![feature(nll)]` imply that as part of this PR; that seemed like too drastic a change to me.

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 29, 2018

rust-lang#27282: Add `StatementKind::ReadForMatch` to MIR.
(This is just the data structure changes and some boilerplate match
code that followed from it; the actual emission of these statements
comes in a follow-up commit.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 29, 2018

rust-lang#27282: emit `ReadForMatch` on each match arm.
Also, turn on ReadForMatch emission by default (when using NLL).

pnkfelix added a commit to pnkfelix/rust that referenced this issue May 29, 2018

bors added a commit that referenced this issue May 30, 2018

Auto merge of #50783 - pnkfelix:issue-27282-match-borrows-its-input-t…
…ake-three, r=nikomatsakis

every match arm reads the match's borrowed input

This PR changes the `match` codegen under NLL (and just NLL, at least for now) to make the following adjustments:
 * It adds a `-Z disable-ast-check-for-mutation-in-guard` which, as described, turns off the naive (conservative but also not 100% sound) check for mutation in guards of match arms.
 * We now borrow the match input at the outset and emit a special `ReadForMatch` statement (that, according to the *static* semantics, reads that borrowed match input) at the start of each match arm. The intent here is to catch cases where the match guard mutates the match input, either via an independent borrow or via `ref mut` borrows in that arm's pattern.
 * In order to ensure that `ref mut` borrows do not actually conflict with the emitted `ReadForMatch` statements, I expanded the two-phase-borrow system slightly, and also changed the MIR code gen so that under NLL, when there is a guard on a match arm, then each pattern variable ends up having *three* temporaries associated with it:
   1. The first temporary will hold the substructure being matched; this is what we will move the (substructural) value into *if* the guard succeeds.
   2. The second temporary also corresponds to the same value as the first, but we are just constructing this temporarily for use during the scope of the guard; it is unaliased and its sole referrer is the third temporary.
   3. The third temporary is a reference to the second temporary.
   * (This sounds complicated, I know, but its actually *simpler* than what I was doing before and had checked into the repo, which was to sometimes construct the final value and then take a reference to it before evaluating the guard. See also PR #49870.)

Fix #27282

This also provides a path towards resolving #24535 aka rust-lang/rfcs#1006, at least once the `-Z disable-ast-check-for-mutation-in-guard` is just turned on by default (under NLL, that is. It is not sound under AST-borrowck).
 * But I did not want to make `#![feature(nll)]` imply that as part of this PR; that seemed like too drastic a change to me.

@bors bors closed this in #50783 May 30, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

commented May 30, 2018

Hmm actually for consistency with other bugs I suppose I should reopen this and tag it as NLL-fixed-by-NLL

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

(but I am going to remove it from the "NLL invalid code does not compile" milestone because it is fixed by NLL... Update: ah niko is too quick for me.)

pnkfelix added a commit to pnkfelix/rust that referenced this issue Jul 24, 2018

Add scary warnings to errors-downgraded-to-warnings in borrowck=migrate.
Also convert an ICE that became reachable code under borrowck=migrate
into a normally reported error (which is then downgraded to a
warning). This actually has a nice side benefit of providing a
somewhat more useful error message, at least in the particular case of
the example from issue rust-lang#27282.

pnkfelix added a commit to pnkfelix/rust that referenced this issue Jul 26, 2018

Add scary warnings to errors-downgraded-to-warnings in borrowck=migrate.
Also convert an ICE that became reachable code under borrowck=migrate
into a normally reported error (which is then downgraded to a
warning). This actually has a nice side benefit of providing a
somewhat more useful error message, at least in the particular case of
the example from issue rust-lang#27282.

bors added a commit that referenced this issue Sep 24, 2018

Auto merge of #53438 - matthewjasper:permissive-match-access, r=pnkfelix
[NLL] Be more permissive when checking access due to Match

Partially addresses #53114. notably, we should now have parity with AST borrowck. Matching on uninitialized values is still forbidden.

* ~~Give fake borrows for match their own `BorrowKind`~~
* ~~Allow borrows with this kind to happen on values that are already mutably borrowed.~~
* ~~Track borrows with this type even behind shared reference dereferences and consider all accesses to be deep when checking for conflicts with this borrow type. See [src/test/ui/issues/issue-27282-mutate-before-diverging-arm-3.rs](cb5c989#diff-a2126cd3263a1f5342e2ecd5e699fbc6) for an example soundness issue this fixes (a case of #27282 that wasn't handled correctly).~~
* Create a new `BorrowKind`: `Shallow` (name can be bike-shed)
* `Shallow` borrows differ from shared borrows in that
  * When we check for access we treat them as a `Shallow(Some(_))` read
  * When we check for conflicts with them, if the borrow place is a strict prefix of the access place then we don't consider that a conflict.
    * For example, a `Shallow` borrow of `x` does not conflict with any access or borrow of `x.0` or `*x`
* Remove the current fake borrow in matches.
* When building matches, we take a `Shallow` borrow of any `Place` that we switch on or bind in a match, and any prefix of those places. (There are some optimizations where we do fewer borrows, but this shouldn't change semantics)
  * `match x { &Some(1) => (),  _ => (), }` would `Shallow` borrow `x`, `*x` and `(*x as Some).0` (the `*x` borrow is unnecessary, but I'm not sure how easy it would be to remove.)
* Replace the fake discriminant read with a `ReadForMatch`.
* Change ReadForMatch to only check for initializedness (to prevent `let x: !; match x {}`), but not conflicting borrows. It is still considered a use for liveness and `unsafe` checking.
* Give special cased error messages for this kind of borrow.

Table from the above issue after this PR

| Thing | AST | MIR | Want | Example |
| --- | --- | --- | --- |---|
| `let _ = <unsafe-field>` | 💚  | 💚  |  |  [playground](https://play.rust-lang.org/?gist=bb7843e42fa5318c1043d04bd72abfe4&version=nightly&mode=debug&edition=2015) |
| `match <unsafe_field> { _ => () }` |   |  |  | [playground](https://play.rust-lang.org/?gist=3e3af05fbf1fae28fab2aaf9412fb2ea&version=nightly&mode=debug&edition=2015) |
| `let _ = <moved>` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=91a6efde8288558e584aaeee0a50558b&version=nightly&mode=debug&edition=2015) |
| `match <moved> { _ => () }` |  |   | 💚 | [playground](https://play.rust-lang.org/?gist=804f8185040b2fe131f2c4a64b3048ca&version=nightly&mode=debug&edition=2015) |
| `let _ = <borrowed>` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |
| `match <borrowed> { _ => () }` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |

r? @nikomatsakis

bors added a commit that referenced this issue Sep 25, 2018

Auto merge of #53438 - matthewjasper:permissive-match-access, r=pnkfelix
[NLL] Be more permissive when checking access due to Match

Partially addresses #53114. notably, we should now have parity with AST borrowck. Matching on uninitialized values is still forbidden.

* ~~Give fake borrows for match their own `BorrowKind`~~
* ~~Allow borrows with this kind to happen on values that are already mutably borrowed.~~
* ~~Track borrows with this type even behind shared reference dereferences and consider all accesses to be deep when checking for conflicts with this borrow type. See [src/test/ui/issues/issue-27282-mutate-before-diverging-arm-3.rs](cb5c989#diff-a2126cd3263a1f5342e2ecd5e699fbc6) for an example soundness issue this fixes (a case of #27282 that wasn't handled correctly).~~
* Create a new `BorrowKind`: `Shallow` (name can be bike-shed)
* `Shallow` borrows differ from shared borrows in that
  * When we check for access we treat them as a `Shallow(Some(_))` read
  * When we check for conflicts with them, if the borrow place is a strict prefix of the access place then we don't consider that a conflict.
    * For example, a `Shallow` borrow of `x` does not conflict with any access or borrow of `x.0` or `*x`
* Remove the current fake borrow in matches.
* When building matches, we take a `Shallow` borrow of any `Place` that we switch on or bind in a match, and any prefix of those places. (There are some optimizations where we do fewer borrows, but this shouldn't change semantics)
  * `match x { &Some(1) => (),  _ => (), }` would `Shallow` borrow `x`, `*x` and `(*x as Some).0` (the `*x` borrow is unnecessary, but I'm not sure how easy it would be to remove.)
* Replace the fake discriminant read with a `ReadForMatch`.
* Change ReadForMatch to only check for initializedness (to prevent `let x: !; match x {}`), but not conflicting borrows. It is still considered a use for liveness and `unsafe` checking.
* Give special cased error messages for this kind of borrow.

Table from the above issue after this PR

| Thing | AST | MIR | Want | Example |
| --- | --- | --- | --- |---|
| `let _ = <unsafe-field>` | 💚  | 💚  |  |  [playground](https://play.rust-lang.org/?gist=bb7843e42fa5318c1043d04bd72abfe4&version=nightly&mode=debug&edition=2015) |
| `match <unsafe_field> { _ => () }` |   |  |  | [playground](https://play.rust-lang.org/?gist=3e3af05fbf1fae28fab2aaf9412fb2ea&version=nightly&mode=debug&edition=2015) |
| `let _ = <moved>` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=91a6efde8288558e584aaeee0a50558b&version=nightly&mode=debug&edition=2015) |
| `match <moved> { _ => () }` |  |   | 💚 | [playground](https://play.rust-lang.org/?gist=804f8185040b2fe131f2c4a64b3048ca&version=nightly&mode=debug&edition=2015) |
| `let _ = <borrowed>` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |
| `match <borrowed> { _ => () }` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |

r? @nikomatsakis

@pnkfelix pnkfelix removed their assignment Nov 9, 2018

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

NLL (migrate mode) is enabled in all editions as of PR #59114.

The only policy question is that, under migrate mode, we only emit a warning on this (unsound) test case. Therefore, I am not 100% sure whether we should close this until that warning has been turned into a hard-error under our (still in development) future-compatibility lint policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.