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

Pattern guard can consume value that is being matched #31287

Closed
ghost opened this issue Jan 29, 2016 · 27 comments
Closed

Pattern guard can consume value that is being matched #31287

ghost opened this issue Jan 29, 2016 · 27 comments

Comments

@ghost
Copy link

@ghost ghost commented Jan 29, 2016

Following compiles, and crashes when trying to print moved from value:

fn main() {
    let a = Some("...".to_owned());
    let b = match a {
        Some(_) if { drop(a); false } => None,
        x                             => x,
    };
    println!("{:?}", b);
}
@apasel422
Copy link
Contributor

@apasel422 apasel422 commented Jan 29, 2016

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jan 29, 2016

triage: T-lang I-nominated

I thought we fixed this :(

@Aatch
Copy link
Contributor

@Aatch Aatch commented Jan 30, 2016

@alexcrichton unfortunately no. I fixed a very similar issue involving multiple guards, but not this case.

I did try to fix it, but it essentially requires SEME to work. This is because preventing this case introduces edges between successive patterns in the CFG and consequentially causes code like this:

match foo {
  (ref a, 0) => (),
  (ref a, 1) => ()
  _ => ()
}

to be rejected because the borrow in the first pattern overlaps the borrow in the second pattern. The reason why this has to be this way is because both move checking and borrow checking are done using the CFG, right now the CFG for a match "visits" all the arms in parallel, with pattern guards chaining into each other where appropriate. Improving it further requires chaining from the guard to the next pattern instead, but also requires indicating that the borrow in the pattern ends at both the "else" branch of the guard and the end of the block for the arm. In other words, we need regions with multiple exits aka SEME.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jan 31, 2016

Can't we just borrow the discriminant in all guards separately (cc @pnkfelix)? Actually, that might cause conflicts with ref mut borrows, but that conflict can be ignored (actually, we may want to mark patterns as aliasable in guards).

This issue won't be really fixed with MIR if we are talking about bypassing-exhaustiveness rather than

@Aatch
Copy link
Contributor

@Aatch Aatch commented Feb 1, 2016

@arielb1 did you accidentally comment on the wrong issue? I'm not sure how discriminants and exhaustiveness come in here.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Feb 18, 2016

triage: P-high

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Apr 7, 2016

I think this is one of those "fixed by moving region/borrowck to MIR"

@brson
Copy link
Contributor

@brson brson commented Jun 23, 2016

@pnkfelix Is this still on your radar? It's been assigned a while.

@brson
Copy link
Contributor

@brson brson commented Jul 14, 2016

@rust-lang/lang nominated for retriage. Still looks bad. Can we make progress now?

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 14, 2016

As an update, still segfaults both on normal and MIR trans (example above)

@aturon aturon added T-compiler and removed T-lang labels Jul 14, 2016
@aturon
Copy link
Member

@aturon aturon commented Jul 14, 2016

Moving to compiler team; we believe this is blocked on MIR borrowck.

@pnkfelix pnkfelix removed their assignment Jul 21, 2016
@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jul 21, 2016

triage: P-medium

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 1, 2016

After some discussion in @rust-lang/compiler, the consensus was that we should increase the priority of this issue. The preferred fix is still MIR borrowck. Assigning to @pnkfelix as he has been doing work in that direction.

triage: P-high

@nikomatsakis nikomatsakis added this to the NLL Future Compat Warnings milestone Jan 11, 2018
@nikomatsakis nikomatsakis removed this from the NLL run-pass without ICEs milestone Jan 19, 2018
@nikomatsakis nikomatsakis added this to the NLL soundness violations milestone Jan 19, 2018
@matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented Apr 24, 2018

Discussed in the weekly NLL triage.
All of the examples here fail to compile with NLL. This also has a test added from #47549. So we think this can be closed.

@landaire
Copy link

@landaire landaire commented May 21, 2019

A friend encountered this case which is not considered a hard error on the latest stable or nightly, but is a clear UAF (check the Miri interpreter):

fn main() {
    let x = String::from("sadsadsadsadsa");

    let x = match 0 {
        0 if { y(x) } => unreachable!(),
        _ => x,
    };

    println!("Hello, world!");
}

fn y(p: String) -> bool {
    false
}

Playground link

Relevant warning:

warning[E0382]: use of moved value: `x`
 --> src/main.rs:6:14
  |
2 |     let x = String::from("sadsadsadsadsa");
  |         - move occurs because `x` has type `std::string::String`, which does not implement the `Copy` trait
...
5 |         0 if { y(x) } => unreachable!(),
  |                  - value moved here
6 |         _ => x,
  |              ^ value used here after move
  |
  = warning: this error has been downgraded to a warning for backwards compatibility with previous releases
  = warning: this represents potential undefined behavior in your code and this warning will become a hard error in the future

This only becomes a hard error caught by NLL if you explicitly add #![feature(nll)]: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b87d208ad05b96a6f3f2552c2b948ea0

I'm not sure if this was intended to be considered a hard error now with NLL, but if not, I think it's worth revisiting.

@matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented May 21, 2019

I think this was closed before we decided to not close fixed by NLL issues until they're hard errors.

Reopening.

@matthewjasper matthewjasper reopened this May 21, 2019
@eddyb
Copy link
Member

@eddyb eddyb commented May 22, 2019

I'm a bit surprised about #![feature(nll)] having any effect on Rust 2018, is that intentional?
If so, where can I read more about that decision?

@landaire
Copy link

@landaire landaire commented May 22, 2019

I think the downgrade is performed here:

if tcx.migrate_borrowck() {
// When borrowck=migrate, check if AST-borrowck would
// error on the given code.
// rust-lang/rust#55492, rust-lang/rust#58776 check the base def id
// for errors. AST borrowck is responsible for aggregating
// `signalled_any_error` from all of the nested closures here.
let base_def_id = tcx.closure_base_def_id(def_id);
match tcx.borrowck(base_def_id).signalled_any_error {
SignalledError::NoErrorsSeen => {
// if AST-borrowck signalled no errors, then
// downgrade all the buffered MIR-borrowck errors
// to warnings.
for err in mbcx.errors_buffer.iter_mut() {
downgrade_if_error(err);
}

And references these issues related to the downgrade #55492 #58776

For the explicit #![feature(nll)], it looks like this has to do with if tcx.migrate_borrowck(), which going down the call graph leads us to:

/// What mode(s) of borrowck should we run? AST? MIR? both?
/// (Also considers the `#![feature(nll)]` setting.)
pub fn borrowck_mode(&self) -> BorrowckMode {
// Here are the main constraints we need to deal with:
//
// 1. An opts.borrowck_mode of `BorrowckMode::Migrate` is
// synonymous with no `-Z borrowck=...` flag at all.
//
// 2. We want to allow developers on the Nightly channel
// to opt back into the "hard error" mode for NLL,
// (which they can do via specifying `#![feature(nll)]`
// explicitly in their crate).
//
// So, this precedence list is how pnkfelix chose to work with
// the above constraints:
//
// * `#![feature(nll)]` *always* means use NLL with hard
// errors. (To simplify the code here, it now even overrides
// a user's attempt to specify `-Z borrowck=compare`, which
// we arguably do not need anymore and should remove.)
//
// * Otherwise, if no `-Z borrowck=...` then use migrate mode
//
// * Otherwise, use the behavior requested via `-Z borrowck=...`
if self.features().nll { return BorrowckMode::Mir; }
self.sess.opts.borrowck_mode
}

Can't seem to find the relevant discussion for the decision here though.

@eddyb
Copy link
Member

@eddyb eddyb commented May 23, 2019

@landaire Yeah but shouldn't Rust 2018 turn on the NLL borrowck in its full mode?
Unless BorrowckMode::Mir was never on stable and the Compare mode was enabled for 2015 recently?
(after having been the mode on the 2018 edition all this time)

@matthewjasper
Copy link
Contributor

@matthewjasper matthewjasper commented May 23, 2019

Unless BorrowckMode::Mir was never on stable

it wasn't.

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented May 24, 2019

@eddyb wrote:

I'm a bit surprised about #![feature(nll)] having any effect on Rust 2018, is that intentional?
If so, where can I read more about that decision?

and also wrote:

@landaire Yeah but shouldn't Rust 2018 turn on the NLL borrowck in its full mode?
Unless BorrowckMode::Mir was never on stable and the Compare mode was enabled for 2015 recently?
(after having been the mode on the 2018 edition all this time)

See also #58781.

In particular is mention there of making full NLL mode the default for the 2018 edition on its own (vs doing that for all editions en masse).

p.s. the default is BorrowckMode::Migrate, not Compare. We should kill off Compare mode in the short term, as its not serving a real purpose anymore.

Centril added a commit to Centril/rust that referenced this issue Jul 28, 2019
Centril added a commit to Centril/rust that referenced this issue Jul 29, 2019
Centril added a commit to Centril/rust that referenced this issue Jul 29, 2019
…ewjasper

Make `#![feature(bind_by_move_pattern_guards)]` sound without `#[feature(nll)]`

Implements rust-lang#15287 (comment).

Fixes rust-lang#31287
Fixes rust-lang#27282

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Jul 30, 2019
…ewjasper

Make `#![feature(bind_by_move_pattern_guards)]` sound without `#[feature(nll)]`

Implements rust-lang#15287 (comment).

Fixes rust-lang#31287
Fixes rust-lang#27282

r? @matthewjasper
Centril added a commit to Centril/rust that referenced this issue Jul 30, 2019
Centril added a commit to Centril/rust that referenced this issue Aug 1, 2019
…ewjasper

Make `#![feature(bind_by_move_pattern_guards)]` sound without `#[feature(nll)]`

Implements rust-lang#15287 (comment) making `#![feature(bind_by_move_pattern_guards)]]` sound without also having `#![feature(nll)]`. The logic here is that if we see a `match` guard, we will refuse to downgrade NLL errors to warnings. This is in preparation for hopefully stabilizing the former feature in rust-lang#63118.

As fall out from the implementation we also:
Fixes rust-lang#31287
Fixes rust-lang#27282

r? @matthewjasper
bors added a commit that referenced this issue Aug 3, 2019
Make `#![feature(bind_by_move_pattern_guards)]` sound without `#[feature(nll)]`

Implements #15287 (comment) making `#![feature(bind_by_move_pattern_guards)]]` sound without also having `#![feature(nll)]`. The logic here is that if we see a `match` guard, we will refuse to downgrade NLL errors to warnings. This is in preparation for hopefully stabilizing the former feature in #63118.

As fall out from the implementation we also:
Fixes #31287
Fixes #27282

r? @matthewjasper
bors added a commit that referenced this issue Aug 3, 2019
Make `#![feature(bind_by_move_pattern_guards)]` sound without `#[feature(nll)]`

Implements #15287 (comment) making `#![feature(bind_by_move_pattern_guards)]]` sound without also having `#![feature(nll)]`. The logic here is that if we see a `match` guard, we will refuse to downgrade NLL errors to warnings. This is in preparation for hopefully stabilizing the former feature in #63118.

As fall out from the implementation we also:
Fixes #31287
Fixes #27282

r? @matthewjasper
bors added a commit that referenced this issue Aug 3, 2019
Make `#![feature(bind_by_move_pattern_guards)]` sound without `#[feature(nll)]`

Implements #15287 (comment) making `#![feature(bind_by_move_pattern_guards)]]` sound without also having `#![feature(nll)]`. The logic here is that if we see a `match` guard, we will refuse to downgrade NLL errors to warnings. This is in preparation for hopefully stabilizing the former feature in #63118.

As fall out from the implementation we also:
Fixes #31287
Fixes #27282

r? @matthewjasper
@bors bors closed this in #63059 Aug 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment