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

regionck fails to link lifetime of ref bindings in certain situations #16994

Closed
nikomatsakis opened this issue Sep 4, 2014 · 4 comments
Closed
Labels
A-typesystem Area: The type system E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.

Comments

@nikomatsakis
Copy link
Contributor

The recent attempts to improve hashmap seem to have exposed a bug in regionck where it fails to link the lifetimes of ref bindings in some situations.

A mostly minimized test case is here:

fn cb<'a,T>(x: |(&'a uint, &'a (Vec<&'static uint>, bool))| -> T) -> T {fail!()}

fn main() {
    cb(|(k, &(ref v, b))| (*k, v.clone(), b));
}

This test fails to compile but (I believe) should succeed. The problem is that the lifetime of the ref v binding inside the callback from map is not "linked" (in regionck terminology) to the lifetime 'a, and hence region inference infers a lifetime for v that is too small. It was not immediately obvious what the best way is to fix this -- the code around handling ref bindings seems a bit ... off in regionck, so I need to think about it some.

Perturbing the code in various ways can workaround the issue, which is bad for making minimized test cases but good for landing the hasmap patch in the meantime!

@sfackler
Copy link
Member

sfackler commented Sep 4, 2014

I've run into (I think) another manifestation of this issue that forces the destructuring of a reference to a tuple from the arguments to the closure body: https://github.com/sfackler/rust-phf/blob/master/phf/src/lib.rs#L63

This works, but

        self.get_entry(key, |k| key == k).map(|e| {
            let &(_, ref v) = e;
            v
        })

this does not

        self.get_entry(key, |k| key == k).map(|&(_, ref v)| v)

@steveklabnik steveklabnik added the A-typesystem Area: The type system label Jan 27, 2015
@steveklabnik
Copy link
Member

I think this is the original problem, updated to current syntax:

fn cb<'a,T>(x: Box<Fn((&'a i32, &'a (Vec<&'static i32>, bool))) -> T>) -> T {panic!()}

fn main() {
    cb(|(k, &(ref v, b))| (*k, v.clone(), b));
}

error:

hello.rs:4:28: 4:30 error: the type of this value must be known in this context
hello.rs:4     cb(|(k, &(ref v, b))| (*k, v.clone(), b));
                                      ^~
hello.rs:4:34: 4:39 error: the type of this value must be known in this context
hello.rs:4     cb(|(k, &(ref v, b))| (*k, v.clone(), b));
                                            ^~~~~
hello.rs:4:8: 4:45 error: mismatched types:
 expected `Box<core::ops::Fn((&i32, &(collections::vec::Vec<&'static i32>, bool))) -> _ + 'static>`,
    found `[closure@hello.rs:4:8: 4:45]`
(expected box,
    found closure) [E0308]
hello.rs:4     cb(|(k, &(ref v, b))| (*k, v.clone(), b));
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hello.rs:4:8: 4:45 help: run `rustc --explain E0308` to see a detailed explanation
error: aborting due to 3 previous errors

Actually, maybe this should be using something other than Box... ugh old closures.

@arielb1
Copy link
Contributor

arielb1 commented Feb 2, 2016

If you use a boxed closure, remember to box it:

fn cb<'a,T>(x: Box<Fn((&'a i32, &'a (Vec<&'static i32>, bool))) -> T>) -> T {panic!()}

fn main() {
    cb(Box::new(|(k, &(ref v, b))| (*k, v.clone(), b)));
}

This runs on modern rustc.

@arielb1 arielb1 added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Feb 2, 2016
@steveklabnik
Copy link
Member

Ah duh. Thanks!

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Apr 30, 2017
bors added a commit that referenced this issue May 1, 2017
Add test for issue #16994.

Fixes #16994.

Please check that this is the correct way to write this test.

r? @arielb1 (author of test case)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

No branches or pull requests

4 participants