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

Adding Copy impl to type causes lifetime errors #24085

Closed
bkoropoff opened this issue Apr 5, 2015 · 7 comments · Fixed by #24527
Closed

Adding Copy impl to type causes lifetime errors #24085

bkoropoff opened this issue Apr 5, 2015 · 7 comments · Fixed by #24527
Labels
A-lifetimes Area: Lifetimes / regions

Comments

@bkoropoff
Copy link
Contributor

Code

Playpen link

#[derive(Clone)]
// Uncommenting the next line causes an error
//#[derive(Copy)]
struct Path<'a:'b, 'b> {
    x: &'a i32,
    tail: Option<&'b Path<'a, 'b>>
}

#[allow(dead_code, unconditional_recursion)]
fn foo<'a,'b,F>(p: Path<'a, 'b>, mut f: F)
                where F: for<'c> FnMut(Path<'a, 'c>) {
    foo(p, |x| f(x))
}

Error

<anon>:12:5: 12:8 error: cannot infer an appropriate lifetime for lifetime parameter `'a` due to conflicting requirements
<anon>:12     foo(p, |x| f(x))
              ^~~
<anon>:12:16: 12:20 note: first, the lifetime cannot outlive the lifetime 'c as defined on the block at 12:15...
<anon>:12     foo(p, |x| f(x))
                         ^~~~
<anon>:12:13: 12:14 note: ...so that trait type parameters matches those specified on the impl (expected `core::marker::Copy`, found `core::marker::Copy`)
<anon>:12     foo(p, |x| f(x))
                      ^
<anon>:11:54: 13:2 note: but, the lifetime must be valid for the lifetime 'a as defined on the block at 11:53...
<anon>:11                 where F: for<'c> FnMut(Path<'a, 'c>) {
<anon>:12     foo(p, |x| f(x))
<anon>:13 }
<anon>:12:5: 12:8 note: ...so that trait type parameters matches those specified on the impl (expected `core::ops::FnOnce<(Path<'_, 'c>,)>`, found `for<'c> core::ops::FnOnce<(Path<'a, 'c>,)>`)
<anon>:12     foo(p, |x| f(x))
              ^~~

Analysis

Notably, this does not occur with a trait like:

trait FakeCopy where Self: Clone {}

This smells like a bug to me, unless a language change introduced special treatment of Copy with regard to lifetimes.

@tomjakubowski
Copy link
Contributor

An interesting quirk is that if you replace |x| f(x) with just f, it compiles fine with the Copy impl.

@steveklabnik steveklabnik added the A-lifetimes Area: Lifetimes / regions label Apr 6, 2015
@bkoropoff
Copy link
Contributor Author

cc @nikomatsakis

@pnkfelix
Copy link
Member

pnkfelix commented Apr 8, 2015

@bkoropoff are you saying this used to work? Just curious what the original was.

@pnkfelix
Copy link
Member

pnkfelix commented Apr 8, 2015

(Also it seems difficult if not impossible to satisfy the combination of 'a:'b and F: for <'c> ... 'c substituted for 'b .... ..... well I guess 'a of static would, but I don't know if we can deduce that.

@bkoropoff
Copy link
Contributor Author

Yes, this used to work. This bug is affecting https://github.com/bkoropoff/rust-jlens

bkoropoff added a commit to bkoropoff/rust-jlens that referenced this issue Apr 11, 2015
This also changes selectors to pass JsonPath by reference
instead of value to work around rust-lang/rust#24085
@bkoropoff
Copy link
Contributor Author

It looks like this may have been introduced in 83ef304. Prior to this, checking for Copy bounds in expr_use_visitor was done via ty::type_moves_by_default, which creates a throw-away inference context before calling into trait selection. It now goes through the Typer; the impl of Typer for FnCtxt uses its own inference context. This causes region constraints introduced during trait selection for Copy to stick around, resulting in a seemingly inappropriate transitive constraint between the free 'c region within the closure body and 'a at the call site of foo.

bkoropoff added a commit to bkoropoff/rust that referenced this issue Apr 13, 2015
Perform trait selection for builtin traits with a throw-away
inference context, restoring the behavior prior to 83ef304.
This prevents unwanted region constraints that lead to later
errors.
bkoropoff added a commit to bkoropoff/rust that referenced this issue Apr 13, 2015
@nikomatsakis
Copy link
Contributor

@bkoropoff hmm good catch, sorry I let this slip through last week :( I'll try to pay it some attention now. The current code does not seem entirely wrong to me, though. I have to think this through a bit.

bkoropoff added a commit to bkoropoff/rust that referenced this issue Apr 14, 2015
Insert an inference variable before performing trait selection for
builtin traits.  This prevents overly-restrictive region constraints
that lead to later inference failures.
bkoropoff added a commit to bkoropoff/rust that referenced this issue Apr 14, 2015
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Apr 17, 2015
stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes rust-lang#24085.
bors added a commit that referenced this issue Apr 17, 2015
Expand the "givens" set to cover transitive relations.  The givens array
stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes #24085.

r? @pnkfelix 
cc @bkoropoff

*But* I am not sure whether this fix will have a compile-time hit. I'd like to push to try branch observe cycle times.
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Jun 19, 2015
stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes rust-lang#24085.
bors added a commit that referenced this issue Jun 19, 2015
Expand the "givens" set to cover transitive relations.  The givens array
stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes #24085.

r? @pnkfelix 
cc @bkoropoff

*But* I am not sure whether this fix will have a compile-time hit. I'd like to push to try branch observe cycle times.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants