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

Copy not implemented on returned closures #68307

Closed
zakcutner opened this issue Jan 17, 2020 · 21 comments
Closed

Copy not implemented on returned closures #68307

zakcutner opened this issue Jan 17, 2020 · 21 comments
Labels
A-closures Area: closures (`|args| { .. }`) A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@zakcutner
Copy link

I am exploring how the Copy trait is implicitly added to closures as in this RFC. As a simple example, let's say I create a function foo which takes a Copy closure.

fn foo(f: impl Fn(i32) + Copy) {
    f(10);
}

Although in this case f does not need to be Copy, this is just to simplify the example, let's pretend that it really does. Now I create a function bar which returns the same type of closure that foo accepts.

fn bar() -> impl Fn(i32) {
    |x: i32| {
        println!("{}", x);
    }
}

From my reading of the RFC, the returned closure should implicitly implement Copy. However, if I try to call foo with the result from bar, the compiler gives me an error because I do not explicitly implement Copy on the closure returned by bar.

foo(bar());
    ^^^^^ the trait `std::marker::Copy` is not implemented for `impl std::ops::Fn<(i32,)>`

Whilst I could change the signature of bar to return impl Fn(i32) + Copy let's assume that I do not have control over this function as it is defined in another crate. Interestingly, if I wrap the whole thing in another (and pointless!) closure, it works 👀

foo(|x: i32| bar()(x));

I have seen this exact pattern come up when using combinator libraries (nom for instance), and the second working solution is pretty frustrating as the outer closure has no purpose.

  1. Is this the intended behaviour of the Rust compiler? Surely this is a bug that it can only infer Copy on local closures and not those returned by functions?

  2. Is there something simple I'm missing; maybe I can write this in another way to avoid this issue? For this, please assume that I cannot edit foo or bar as this is based on real examples where combinators I use are defined by other crates.

Thank you! 😄

@zakcutner
Copy link
Author

If you would like to try this out for yourself, I have created this example on the playground

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Jan 17, 2020

You have to write -> impl Fn(i32) + Copy,otherwise the returned closure is allowed to not be Copy,and you wouldn't be able to use the fact that it's Copy outside the fn.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a88cce55fb8b6b2a12f653281aa56581

fn foo(f: impl Fn(i32) + Copy) {
    f(10);
}

fn bar() -> impl Fn(i32) + Copy {
    |x: i32| {
        println!("{}", x);
    }
}

fn main() {
    foo(bar());
}

@zakcutner
Copy link
Author

zakcutner commented Jan 17, 2020

Whilst I could change the signature of bar to return impl Fn(i32) + Copy let's assume that I do not have control over this (bar) function as it is defined in another crate.

@rodrimati1992 Thanks for your reply! I am aware of this however please see my question above for why I can't do this... I have encountered this exact problem! My issue is that the compiler seems able to infer implicit Copy when the closure is local, why doesn't it do the same for returned closures?

@rodrimati1992
Copy link
Contributor

That's how impl Trait return types work,you can only use the traits that are part of their type.

@zakcutner
Copy link
Author

zakcutner commented Jan 17, 2020

That's how impl Trait return types work,you can only use the traits that are part of their type.

Right, but the Rust compiler can figure out the Copy implicitly when it's local... I appreciate that it doesn't currently support this (whether this is intended or a bug?), however this is a common problem when using combinators and the foo(|x: i32| bar()(x)) alternative in my question is a really annoying pattern! Surely there should be a better way to achieve this?

@Centril
Copy link
Contributor

Centril commented Jan 17, 2020

  1. Is this the intended behaviour of the Rust compiler?

Yes, this is intended behavior. Only auto traits leak out from impl Trait and Copy is not such a trait.

@Centril Centril closed this as completed Jan 17, 2020
@zakcutner
Copy link
Author

zakcutner commented Jan 17, 2020

Yes, this is intended behavior. Only auto traits leak out from impl Trait and Copy is not such a trait.

@Centril Okay, would there be a possible change to the compiler/language that could fix this? Or alternatively, is there another way that currently exists to achieve what I'm trying to do in a nicer way?

@Centril
Copy link
Contributor

Centril commented Jan 17, 2020

Possible? Yes. Desirable? Unlikely, as it would require special casing Copy. The very point of -> impl Trait is to hide unpromised internal details from the caller, and so exposing Copy implicitly, beyond being an ad-hoc special case, would also weaken such guarantees.

I'm not aware of other ways at this time.

@zakcutner
Copy link
Author

@Centril Thanks, that's really helpful! I have a few questions about this...

  1. Is Copy not already treated as a special case by the compiler in order for the original RFC to work or do I misunderstand?

  2. To clarify, are you saying that the foo(|x: i32| bar()(x)) solution is currently the best one you are aware of?

@Centril
Copy link
Contributor

Centril commented Jan 17, 2020

  1. Is Copy not already treated as a special case by the compiler in order for the original RFC to work or do I misunderstand?

That's the wrong RFC (you want the -> impl Trait one). :) The compiler treats Copy specially in some circumstances, but not here.

  1. To clarify, are you saying that the foo(|x: i32| bar()(x)) solution is currently the best one you are aware of?

I don't know enough about your use case to say I'm afraid.

@zakcutner
Copy link
Author

@Centril Okay, thanks I understand. For me, the solution of foo(|x: i32| bar()(x)) is unsatisfactory as, even if the compiler can optimise it away, the additional closure is pretty ugly and in larger examples could make the code kinda confusing.

Therefore, although there may not currently be an easy way to resolve this, since the underlying issue has not been solved, would it be possible to re-open it as a feature request / request for suggestions as others may have further ideas on how this can be improved? Alternatively, is this better suited to an RFC and/or is there additional information I can provide to help clarify where the problem occurs?

@Centril Centril added A-closures Area: closures (`|args| { .. }`) A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 17, 2020
@Centril
Copy link
Contributor

Centril commented Jan 17, 2020

I suppose it can't hurt, though I don't think it will be actionable in the foreseeable future.

@Centril Centril reopened this Jan 17, 2020
@nagisa
Copy link
Member

nagisa commented Jan 17, 2020

I was surprised adding another layer of closure "helps", but it turns out to be a red herring. The reason

let closure: !Copy = bar()
(|x: i32| closure(x)) : Copy

is because closure is captured by reference and &T are always Copy. If the outer closure is replaced to capture closure by value instead:

let closure: !Copy = bar()
(move |x: i32| closure(x)) : !Copy

you get the expected error of closure not implementing Copy.


Inferring Copy on the impl Trait return type should not happen, because it is a compatibility hazard in API design. (I’m surprised we opted to do that for auto-traits…).

Consider a crate that consciously avoids adding the + Copy bound anticipating a future change where closure could become !Copy. Automatically inferring it then opens the door for unintended breaking changes and semver violations and the crate won't know until somebody hits it in practice, because compiler will just silently not infer Copy anymore.

@zakcutner
Copy link
Author

zakcutner commented Jan 17, 2020

If the outer closure is replaced to capture bar by value instead:

(move |x: i32| bar()(x))

you get the expected error of closure not implementing Copy.

@nagisa I'm not sure this is true, take a look at this playground example.

The reason [...] is because bar is captured by reference and &T are always Copy

As bar is a static function rather than a function pointer I think it is always referenced in the same way regardless of whether it is called in a move closure or not although I'm not 100% sure about this...

I was surprised adding another layer of closure "helps"

So the reason (again, I think) this works is because Rust is able to automatically infer the type of the outer closure as it is declared inline and make it implicitly Copy, as per this RFC. However, it is unable to do the same with the closure returned from bar because I explicitly specify the type (as you must do with all return values!) and I don't include Copy, although this makes for a frustrating solution.

@nagisa
Copy link
Member

nagisa commented Jan 17, 2020

My bad, I cut some corners in my explanation. Please see the updated code samples in my previous comments and this playground. This should be sufficient to mis-prove your current intuition.

EDIT: in the original examples we were indeed capturing bar rather than the closure it returns and fn() -> _ { bar } is Copy too, regardless of what it returns.

@zakcutner
Copy link
Author

@nagisa Hmmm... yeah you're right that's really interesting! Any ideas of how to solve this or perhaps nicer ways to tell the compiler that the closure is Copy without wrapping it?

@nagisa
Copy link
Member

nagisa commented Jan 17, 2020

How about passing in a reference to the closure? Note, however that this only works for Fn closures. Calling an FnMut closure requires a &mut which is not copy. And FnOnce wants ownership for the call, so you cannot pass in by-reference either.

@rodrimati1992
Copy link
Contributor

rodrimati1992 commented Jan 18, 2020

@nagisa Nevermind the following paragraphs,I don't think I've read you correctly.

Passing a type that implements Fn to a function expenting FnMut or FnOnce works though.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=e21f7f657b6cffa83391497be8c1d596
What you can't do is pass a type that only implements FnOnce to a function that expects an FnMut(or Fn),or a type that only implements FnMut to a function that expects an Fn.

@zakcutner
Copy link
Author

How about passing in a reference to the closure? Note, however that this only works for Fn closures. Calling an FnMut closure requires a &mut which is not copy. And FnOnce wants ownership for the call, so you cannot pass in by-reference either.

@nagisa Thanks, that's really nice actually! Am I right that this works because the &Fn is Copy (as all non-mutable references are) but the Fn is not? Is it also true that &Fn implements Fn and that's why it can be accepted by foo. If so, surely this is almost "cheating" as we can make any (Fn) closure Copy simply by referencing it, perhaps even when the underlying closure is really not Copy?

@nagisa
Copy link
Member

nagisa commented Jan 19, 2020

Am I right that this works because the &Fn is Copy (as all non-mutable references are)

Yes.

Is it also true that &Fn implements Fn

Yes.

If so, surely this is almost "cheating"

The previous two points are perfectly sound and logical in isolation, so I’m not sure why it would be cheating to combine them to achieve something else. In fact the "wrapping in a closure" trick abuses exactly the same rules, just not as obviously.

Anyway, closing this as there is nothing actionable to do here. For further support, consider posting your questions on users.rlo, stackoverflow, reddit, or discussing it on one of the chat platforms: discord or zulip.

@nagisa nagisa closed this as completed Jan 19, 2020
@zakcutner
Copy link
Author

Fair enough, I think this is now a great solution and I hope the discussion will be helpful to anyone else with the same issue. Thanks for all the help 😍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants