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

FnMut not implemented for mutable FnMut implementors #23015

Closed
alexcrichton opened this issue Mar 4, 2015 · 5 comments · Fixed by #23895
Closed

FnMut not implemented for mutable FnMut implementors #23015

alexcrichton opened this issue Mar 4, 2015 · 5 comments · Fixed by #23895
Labels
A-closures Area: Closures (`|…| { … }`) P-medium Medium priority

Comments

@alexcrichton
Copy link
Member

For example this program does not compile

fn foo<F: FnMut()>(_: F) {}

fn main() {
    let mut cb = || {};
    foo(&mut cb);
}
foo.rs:5:5: 5:8 error: the trait `core::ops::Fn<()>` is not implemented for the type `&mut [closure foo.rs:4:18: 4:23]` [E0277]
foo.rs:5     foo(&mut cb);
             ^~~
error: aborting due to previous error

The unfortunate error message is covered by another issue, but this seems like an unfortunate limitation. I tried to implement this but got coherence violations. I also implemented this "halfway" and got compile errors due to legitimate coherence violations which sprang up. I believe we have some hardwired impls of these traits in the compiler, but I'd like to make sure we have our story straight on these.

@laijs
Copy link

laijs commented Mar 4, 2015

These code can't work?:

impl<T:FnMut> FnMut for &mut T { ... }
impl<T:Fn> Fn for &T { ... }

I know this will leave another four types without implmenting any Fn*.
the left four types: &mut Fn, &FnMut, &FnOnce, &mut FnOnce

@theemathas
Copy link
Contributor

Already discussed in http://internals.rust-lang.org/t/implement-closure-traits-on-t-when-t-implements-them/1638

You can workaround withfoo(||cb()) in most cases.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 5, 2015

seems likely fixable in backwards compatible manner.

P-high, not 1.0.

@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Mar 5, 2015
@nikomatsakis
Copy link
Contributor

I'm working hard on this.

@nikomatsakis
Copy link
Contributor

So I've implemented this but I found two surprising interactions.

First, impls like FnMut for &mut F where F : FnMut conflict with the core/str/pattern impls. I fixed this by removing the "autoderef" Pattern impl and adding an impl of Pattern for &String.

Second, the autoderef loop for calls, currently, always autorefs. This is because it's based around the operator code and not the normal method lookup. This probably needs to be changed though because otherwise if we have x: &mut F where F:FnMut, you can't call x() -- you have to declare mut x: &mut F. The reason is that x() gets elaborated to <&mut F as FnMut()>::call_mut(&mut x) rather than <F as FnMut()>::call_mut(&mut *x). The latter is what we get today and probably what we want.

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
Manishearth added a commit to Manishearth/rust that referenced this issue Apr 1, 2015
…dd-impls, r=pnkfelix

The primary purpose of this PR is to add blanket impls for the `Fn` traits of the following (simplified) form:

    impl<F:Fn> Fn for &F
    impl<F:FnMut> FnMut for &mut F

However, this wound up requiring two changes:

1. A slight hack so that `x()` where `x: &mut F` is translated to `FnMut::call_mut(&mut *x, ())` vs `FnMut::call_mut(&mut x, ())`. This is achieved by just autoderef'ing one time when calling something whose type is `&F` or `&mut F`.
2. Making the infinite recursion test in trait matching a bit more tailored. This involves adding a notion of "matching" types that looks to see if types are potentially unifiable (it's an approximation).

The PR also includes various small refactorings to the inference code that are aimed at moving the unification and other code into a library (I've got that particular change in a branch, these changes just lead the way there by removing unnecessary dependencies between the compiler and the more general unification code). 

Note that per rust-lang/rfcs#1023, adding impls like these would be a breaking change in the future. 

cc @japaric
cc @alexcrichton 
cc @aturon 

Fixes rust-lang#23015.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) P-medium Medium priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants