-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
"error: reached the recursion limit during monomorphization" with unboxed closures #19596
Comments
This seems like expected behavior to me. Passing a closure to either of the The only only way that I could see This should work as a replacement: fn method_ok_unboxed<F>(&self, f: F) -> bool where F: Fn(()) -> bool {
self.method_ok_unboxed(f);
} Additionally, it should be faster, as it won't be going through the useless indirection of |
@gereeter Sure, that works in the reduced case, but in the |
@japaric Why not have it just take the closure by reference, then? |
Changing the signature to always take the closure by reference is a loss in ergonomics: trie_map.each_reverse(&mut |a, b| /* body */);
// vs
trie_map.each_reverse(|a, b| /* body */); What we probably want to do is add the following impl<'a, Args, Result> FnMut<Args, Result> for &'a mut F where F: FnMut<Args, Result> { /* */ } That way we can pass the closure either by value or by reference. However, due to #18835/#19032 we can't do that at the moment. |
The the last argument of the `ItemDecorator::expand` method has changed to `Box<FnMut>`. Syntax extensions will break. [breaking-change] --- This PR removes pretty much all the remaining uses of boxed closures from the libraries. There are still boxed closures under the `test` directory, but I think those should be removed or replaced with unboxed closures at the same time we remove boxed closures from the language. In a few places I had to do some contortions (see the first commit for an example) to work around issue #19596. I have marked those workarounds with FIXMEs. In the future when `&mut F where F: FnMut` implements the `FnMut` trait, we should be able to remove those workarounds. I've take care to avoid placing the workaround functions in the public API. Since `let f = || {}` always gets type checked as a boxed closure, I have explictly annotated those closures (with e.g. `|&:| {}`) to force the compiler to type check them as unboxed closures. Instead of removing the type aliases (like `GetCrateDataCb`), I could have replaced them with newtypes. But this seemed like overcomplicating things for little to no gain. I think we should be able to remove the boxed closures from the languge after this PR lands. (I'm being optimistic here) r? @alexcrichton or @aturon cc @nikomatsakis
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
I agree, but I think this is so subtle and hard to debug (the infinite chain of unique types is unexpected, since they all look the same in the source) that this is at least worth special error messages. |
The compiler never returns when compiling this code. If you remove the predicate closure then you get "error: reached the recursion limit during monomorphization". But with both closures included it just loops and never returns. I would assume that the "recursion limit" test in the error message is failing with two closures. rustc 1.0.0-nightly (3d0d9bb 2015-01-12 22:56:20 +0000) #![crate_name = "compiler_bug"] #![crate_type = "bin"] #![allow(unused_variables)] fn main() { pub fn recursive< P: Fn() -> bool, F: Fn()>(predicate : P , function : F ) { I read that the error message may be expected behaviour. But this endless loop behaviour may influence your design choice or encourage a error to be added. Can someone private message me a version of the included code that will compile. |
We now have a better message. Updated code:
errors:
|
this doesn't look fixed to me, there's still various
|
…=cjgillot Remove old FIXMEs referring to rust-lang#19596 Having an inner function that accepts a mutable reference seems to be the only way this can be expressed. Taking a mutable reference would call the same function with a new type &mut F which then causes the infinite recursion error in rust-lang#19596.
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#108241 (Fix handling of reexported macro in doc hidden items) - rust-lang#108254 (Refine error span for trait error into borrowed expression) - rust-lang#108255 (Remove old FIXMEs referring to rust-lang#19596) - rust-lang#108257 (Remove old FIXME that no longer applies) - rust-lang#108276 (small `opaque_type_origin` cleanup) - rust-lang#108279 (Use named arguments for `{,u}int_impls` macro) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Came up with this issue while "unboxing" the
collections::trie::map::InternalNode::each_reverse
method.Reduced test case
Output
Version
cc @nikomatsakis (you already know, but I want to keep track of the issue)
The text was updated successfully, but these errors were encountered: