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

Unable to manually implement FnOnce #18835

Closed
nikomatsakis opened this issue Nov 10, 2014 · 4 comments · Fixed by #23282
Closed

Unable to manually implement FnOnce #18835

nikomatsakis opened this issue Nov 10, 2014 · 4 comments · Fixed by #23282
Labels
A-closures Area: Closures (`|…| { … }`)

Comments

@nikomatsakis
Copy link
Contributor

This reddit thread reported a few examples of being unable to implement FnOnce manually. I haven't investigated enough to produce a standalone test case, but I wanted to record the issue.

@aturon aturon mentioned this issue Nov 10, 2014
47 tasks
@pfalabella
Copy link
Contributor

standalone test case (could be reduced but I hope it's useful):

#![feature(overloaded_calls)]
// Implements http://rosettacode.org/wiki/Accumulator_factory
pub struct G<T, U> {
    n: T,
}

impl<T: Add<U, T> + Clone, U> FnMut<(U,), T> for G<T, U> {
    extern "rust-call" fn call_mut(&mut self, (i,):(U,)) -> T {
        self.n = self.n + i;
        self.n.clone()
    }
}

pub fn accum<T: Add<T, U> + Clone, U>(n: T) -> G<T, U> {
G { n: n }
}

pub fn main() {
    println!("{}", accumulate());
}

fn accumulate() -> f32 {
    let mut g = accum(1f32);
    g(5.);
    accum(3i32);
    g(2.3)
}

used to compile, but now it does not, with the following output:

$ rustc accum.rs
accum.rs:7:1: 12:2 error: conflicting implementations for trait `core::ops::FnMut` [E0119]
accum.rs:7 impl<T: Add<U, T> + Clone, U> FnMut<(U,), T> for G<T, U> {
accum.rs:8     extern "rust-call" fn call_mut(&mut self, (i,):(U,)) -> T {
accum.rs:9         self.n = self.n + i;
accum.rs:10         self.n.clone()
accum.rs:11     }
accum.rs:12 }
accum.rs:7:1: 12:2 note: conflicting implementation in crate `core`
accum.rs:7 impl<T: Add<U, T> + Clone, U> FnMut<(U,), T> for G<T, U> {
accum.rs:8     extern "rust-call" fn call_mut(&mut self, (i,):(U,)) -> T {
accum.rs:9         self.n = self.n + i;
accum.rs:10         self.n.clone()
accum.rs:11     }
accum.rs:12 }
error: aborting due to previous error

EDIT: reduced

pub struct G;

impl<T, U> FnMut<(U,), T> for G {
    extern "rust-call" fn call_mut(&mut self, (i,):(U,)) -> T {
        panic!()
    }
}

pub fn main() {} 

@netvl
Copy link
Contributor

netvl commented Nov 10, 2014

Very simple example:

#![feature(unboxed_closures, tuple_indexing)]

struct Chunk<T>(Box<FnOnce<(), T>+'static>);

impl<T> FnOnce<(), T> for Chunk<T> {
    extern "rust-call" fn call_once(self, arg: ()) -> T {
        self.0.call_once(arg)
    }
}

Compilation error:

lib.rs:5:1: 9:2 error: conflicting implementations for trait `core::ops::FnOnce` [E0119]
lib.rs:5 impl<T> FnOnce<(), T> for Chunk<T> {
lib.rs:6     extern "rust-call" fn call_once(self, arg: ()) -> T {
lib.rs:7         self.0.call_once(arg)
lib.rs:8     }
lib.rs:9 }
lib.rs:5:1: 9:2 note: conflicting implementation in crate `core`
lib.rs:5 impl<T> FnOnce<(), T> for Chunk<T> {
lib.rs:6     extern "rust-call" fn call_once(self, arg: ()) -> T {
lib.rs:7         self.0.call_once(arg)
lib.rs:8     }
lib.rs:9 }
error: aborting due to previous error

@nikomatsakis
Copy link
Contributor Author

Thanks guys for the reduced examples! I'll try to look into this this week.

@nikomatsakis
Copy link
Contributor Author

This is a special case of #19032.

bors added a commit that referenced this issue Jan 5, 2015
This PR removes boxed closures from the language, the closure type syntax (`let f: |int| -> bool = /* ... */`) has been obsoleted. Move all your uses of closures to the new unboxed closure system (i.e. `Fn*` traits).

[breaking-change] patterns

- `lef f = || {}`

This binding used to type check to a boxed closure. Now that boxed closures are gone, you need to annotate the "kind" of the unboxed closure, i.e. you need pick one of these: `|&:| {}`, `|&mut:| {}` or `|:| {}`.

In the (near) future we'll have closure "kind" inference, so the compiler will infer which `Fn*` trait to use based on how the closure is used. Once this inference machinery is in place, we'll be able to remove the kind annotation from most closures.

- `type Alias<'a> = |int|:'a -> bool`

Use a trait object: `type Alias<'a> = Box<FnMut(int) -> bool + 'a>`. Use the `Fn*` trait that makes sense for your use case.

- `fn foo(&self, f: |uint| -> bool)`

In this case you can use either a trait object or an unboxed closure:

``` rust
fn foo(&self, f: F) where F: FnMut(uint) -> bool;
// or
fn foo(&self, f: Box<FnMut(uint) -> bool>);
```

- `struct Struct<'a> { f: |uint|:'a -> bool }`

Again, you can use either a trait object or an unboxed closure:

``` rust
struct Struct<F> where F: FnMut(uint) -> bool { f: F }
// or
struct Struct<'a> { f: Box<FnMut(uint) -> bool + 'a> }
```

- Using `|x, y| f(x, y)` for closure "borrows"

This comes up in recursive functions, consider the following (contrived) example:

``` rust
fn foo(x: uint, f: |uint| -> bool) -> bool {
    //foo(x / 2, f) && f(x)  // can't use this because `f` gets moved away in the `foo` call
    foo(x / 2, |x| f(x)) && f(x)  // instead "borrow" `f` in the `foo` call
}
```

If you attempt to do the same with unboxed closures you'll hit ""error: reached the recursion limit during monomorphization" (see #19596):

``` rust
fn foo<F>(x: uint, mut f: F) -> bool where F: FnMut(uint) -> bool {
    foo(x / 2, |x| f(x)) && f(x)
    //~^ error: reached the recursion limit during monomorphization
}
```

Instead you *should* be able to write this:

``` rust
fn foo<F>(x: uint, mut f: F) -> bool where F: FnMut(uint) -> bool {
    foo(x / 2, &mut f) && f(x)
    //~^ error: the trait `FnMut` is not implemented for the type `&mut F`
}
```

But as you see above `&mut F` doesn't implement the `FnMut` trait. `&mut F` *should* implement the `FnMut` and the above code *should* work, but due to a bug (see #18835) it doesn't (for now).

You can work around the issue by rewriting the function to take `&mut F` instead of `F`:

``` rust
fn foo<F>(x: uint, f: &mut F) -> bool where F: FnMut(uint) -> bool {
    foo(x / 2, f) && (*f)(x)
}
```

This finally works! However writing `foo(0, &mut |x| x == 0)` is unergonomic. So you can use a private helper function to avoid this:

``` rust
// public API function
pub fn foo<F>(x: uint, mut f: F) -> bool where F: FnMut(uint) -> bool {
    foo_(x, &mut f)
}

// private helper function
fn foo_<F>(x: uint, f: &mut F) -> bool where F: FnMut(uint) -> bool {
    foo_(x / 2, f) && (*f)(x)
}
```

Closes #14798

---

There is more cleanup to do: like renaming functions/types from `unboxed_closure` to just `closure`, removing more dead code, simplify functions which now have unused arguments, update the documentation, etc. But that can be done in another PR.

r? @nikomatsakis @aturon (You probably want to focus on the deleted/modified tests.)
cc @eddyb
@steveklabnik steveklabnik added the A-closures Area: Closures (`|…| { … }`) label Jan 29, 2015
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 24, 2015
The primary motivation here is to sidestep rust-lang#19032 -- for a time, I thought that we should improve coherence or otherwise extend the language, but I now think that any such changes will require more time to bake. In the meantime, inheritance amongst the fn traits is both logically correct *and* a simple solution to that obstacle. This change introduces inheritance and modifies the compiler so that it can properly generate impls for closures and fns.

Things enabled by this PR (but not included in this PR):

1. An impl of `FnMut` for `&mut F` where `F : FnMut` (rust-lang#23015).
2. A better version of `Thunk` I've been calling `FnBox`.

I did not include either of these in the PR because:

1. Adding the impls in 1 currently induces a coherence conflict with the pattern trait. This is interesting and merits some discussion.
2. `FnBox` deserves to be a PR of its own.

The main downside to this design is (a) the need to write impls by hand; (b) the possibility of implementing `FnMut` with different semantics from `Fn`, etc. Point (a) is minor -- in particular, it does not affect normal closure usage -- and could be addressed in the future in many ways (better defaults; convenient macros; specialization; etc). Point (b) is unfortunate but "just a bug" from my POV, and certainly not unique to these traits (c.f. Copy/Clone, PartialEq/Eq, etc). (Until we lift the feature-gate on implementing the Fn traits, in any case, there is room to correct both of these if we find a nice way.)

Note that I believe this change is reversible in the future if we decide on another course of action, due to the feature gate on implementing the `Fn` traits, though I do not (currently) think we should reverse it.

Fixes rust-lang#18835.

r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants