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

Lifetime handling doesn't handle function pointers properly #10501

Closed
LeoTestard opened this issue Nov 15, 2013 · 9 comments · Fixed by #18555
Closed

Lifetime handling doesn't handle function pointers properly #10501

LeoTestard opened this issue Nov 15, 2013 · 9 comments · Fixed by #18555
Labels
A-lifetimes Area: Lifetimes / regions

Comments

@LeoTestard
Copy link
Contributor

Let's say I've got a struct or enum containing functions (pure Rust functions, not closures, so passed by a simple pointer)

pub type Foo = fn(&int) -> ();

enum Baz {
    Bar(Foo)
}

The above code works. I can pass-around instances of Baz and call the function.
Now if I wan Baz to be copy-able, which should be possible since it only contains a pointer, and add the #[deriving(Clone)] attribute to it, I got a strange compiler error:

error: mismatched types: expected `fn(&int)` but found `fn(&int)` (expected concrete lifetime, but found bound lifetime parameter &)
test.rs:4 #[deriving(Clone)]

This worked perfectly well (including the deriving(Clone) attribute) while the function only took parameters by owned boxes or by-value. I only get this message since I added a borrowed parameter to the function. I don't see the problem here. If a function is simply passed by-pointer we should be able to copy this pointer.

I'm not sure this is the right title for this issue. Feel free to change it if it's miss chosen.

@huonw
Copy link
Member

huonw commented Nov 15, 2013

cc @nikomatsakis, possibly related to #10153?

@nikomatsakis
Copy link
Contributor

Interesting. Not related to #10153 I don't think but probably to the fix for #4846 -- this has something to do with the code that computes GLB/LUB of fn types, I imagine.

@nikomatsakis
Copy link
Contributor

Sorry, #10153 IS the fix for #4846. Anyway, yes, thanks @huonw

@nikomatsakis
Copy link
Contributor

Hmm. The error message is...horribly cryptic but the error seems legit. (Probably worth thinking, independently, on SOMETHING better to print out there, but I'm not sure what).

I was wrong, not related to #10153. This is kind of a flaw in the way that the "clone" operation is defined over fn types, but I'm not actually sure how it can be fixed without modifying the compiler to introduce some special bounds.

Let me elaborate a bit on the problem, as much for myself as anyone else (it's somewhat subtle). The definition of clone for fn pointers is roughly like:

impl<A,R> Clone for fn(A) -> R {
    fn clone(&self) -> fn(A) -> R { *self }
}

What happens here is that the type in question is (written out fully explicitly) <'a> fn(&'a int) -> (). In other words, it's a function that takes a borrowed int with any lifetime 'a, and that lifetime can be specified at call time. The problem is that the clone impl cannot express the notion of "a borrowed pointer with any lifetime". The type variable A here must be bound to a borrowed pointer with some specific lifetime. So the call to "f.clone()" winds up producing a fn with a type like fn(&'b int) -> () -- in other words, a function that takes a borrowed pointer with some lifetime 'b. This new type is not as general as the old one, though, because it only accepts a single lifetime, not any lifetime, and hence you wind up with a type error.

This is hard to fix. I'm not 100% sure what the "right" fix would even be, actually. But one possible fix is to add a builtin bound to the compiler, let's call it Simple, for want of a better name. I envision that Simple matches "simple" types. To avoid this bug, Simple must include fn pointers. It could also include integers, borrowed pointers, and *T pointers -- basically things that are "builtin types that are cloned just by copying their bits" but excluding compound types like structs and tuples. Otherwise this def'n is very close to implicitly copyable and sort of close to POD.

Given this bound, we might have an impl for clone like:

impl<T:Simple> Clone for T {
    fn clone(&self) -> T { *self } // compiler knows this is a copy, not move, due to Simple bound
}

this avoids the problem by not binding type parameters to the individual argument types. It's also more general.

@ghost ghost added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 30, 2014
@nikomatsakis
Copy link
Contributor

This was never fixed, but the code got buggy.

@nikomatsakis nikomatsakis reopened this Dec 2, 2014
@ghost ghost removed the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Dec 12, 2014
@steveklabnik
Copy link
Member

Today:

pub type Foo = fn(&i32) -> ();

#[derive(Copy, Clone)]
enum Baz {
    Bar(Foo)
}

fn main() {}

error:

error: the trait `core::clone::Clone` is not implemented for the type `fn(&i32)`

/cc @rust-lang/lang , I know that recently, we changed something related to fn and Clone, but I'm not sure if this is behavior we want today. The error message is way better now, at least.

@brson
Copy link
Contributor

brson commented Apr 11, 2017

Current error from @steveklabnik's code:



rustc 1.16.0 (30cf806ef 2017-03-10)
error[E0277]: the trait bound `fn(&i32): std::clone::Clone` is not satisfied
 --> <anon>:5:9
  |
5 |     Bar(Foo)
  |         ^^^^ the trait `std::clone::Clone` is not implemented for `fn(&i32)`
  |
  = help: the following implementations were found:
            <fn(A) -> Ret as std::clone::Clone>
            <extern "C" fn(A) -> Ret as std::clone::Clone>
            <extern "C" fn(A, ...) -> Ret as std::clone::Clone>
            <unsafe fn(A) -> Ret as std::clone::Clone>
          and 2 others
  = note: required by `std::clone::AssertParamIsClone`

@brson
Copy link
Contributor

brson commented Apr 11, 2017

Seems to work as designed.

@brson brson closed this as completed Apr 11, 2017
@eddyb
Copy link
Member

eddyb commented Apr 11, 2017

@nikomatsakis You may want to reopen this if you feel like there is a path to making it work.
(Either by making the impls apply through some form of hopefully-sound HRTB capture or implementing Clone automatically for all Copy types, possibly through a future extension of specialization)

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.

6 participants