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

Unsound: Borrowck missing that closure needs to be "move" #35139

Closed
diwic opened this issue Jul 31, 2016 · 3 comments · Fixed by #35143
Closed

Unsound: Borrowck missing that closure needs to be "move" #35139

diwic opened this issue Jul 31, 2016 · 3 comments · Fixed by #35143

Comments

@diwic
Copy link
Contributor

diwic commented Jul 31, 2016

Here's as far as I've come in minifying the problem. It gives bad result on both Stable, Beta and Nightly according to playground.

pub trait MethodType: Sized + Default {
    type GetProp: ?Sized;
}

#[derive(Default, Debug, Copy, Clone)]
pub struct MTFn;

impl<'a> MethodType for MTFn {
    type GetProp = Fn(&mut String) -> Result<(), ()> + 'a;
}

pub struct Property<M: MethodType> {
    get_cb: Option<Box<M::GetProp>>,
}

impl<'a> Property<MTFn> {
    pub fn on_get<H>(mut self, handler: H) -> Property<MTFn>
        where H: Fn(&mut String) -> Result<(), ()> + 'a {
        self.get_cb = Some(Box::new(handler) as Box<_>);
        self
    }
}

fn main() {
    let z: Option<Property<MTFn>>;
    {
        let mut v = format!("ABC DEF GHI JKL");
        let p = Property { get_cb: None };

        // This line should give an error indicating we need a "move" closure
        z = Some(p.on_get(|s| { *s = v.clone(); Ok(()) }));

        // Destroy v to show the error
        v = format!("MNO PQR");
    }
    let mut q = format!("STU");
    let _ = (z.unwrap().get_cb.unwrap())(&mut q);
    println!("{}", q); // Prints "STU PQR" !
}
@eefriedman
Copy link
Contributor

The problem has to do with type GetProp = Fn(&mut String) -> Result<(), ()> + 'a;. It probably should be triggering E0207 ("the lifetime parameter 'a is not constrained by the impl trait, self type, or predicates").

@diwic
Copy link
Contributor Author

diwic commented Jul 31, 2016

@eefriedman, hmm, but this would be a bad solution in my real code. I want to indicate that the GetProp function can borrow from its environment (as opposed to + 'static, which is the default and does not allow borrows at all. I believe?).

@eefriedman
Copy link
Contributor

You can change MTFn to pub struct MTFn<'a>(PhantomData<&'a u8>);, then write impl<'a> MethodType for MTFn<'a> {, and everything should work.

arielb1 added a commit to arielb1/rust that referenced this issue Jul 31, 2016
bors added a commit that referenced this issue Jul 31, 2016
typeck: use a TypeVisitor in ctp

Use a TypeVisitor in ctp instead of `ty::walk`

This fixes a few cases where a region could be projected out of a trait while not being constrained by the type parameters, violating rust-lang/rfcs#447 and breaking soundness. As such, this is a [breaking-change].

Fixes #35139

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants