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

Add Fn impl to Arc #89771

Closed
wants to merge 5 commits into from
Closed

Add Fn impl to Arc #89771

wants to merge 5 commits into from

Conversation

bsodmike
Copy link

This commit refers to the discussion in #89727

Please let me know if any further improvements are needed :)

Thanks!

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2021
@marmeladema
Copy link
Contributor

I think this would be a valuable addition to rust, however it has been tried and rejected in the past: #71570

But maybe the situation changed since?

@the8472 the8472 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 11, 2021
@bsodmike
Copy link
Author

Just checking if the test in #71570 works for my changes. Feel free to close this PR due to historical/breaking changes.

@Mark-Simulacrum
Copy link
Member

The previous PR was closed because the presented motivation did not feel strong enough to override the desire to avoid breaking changes, particularly ones judged major by accepted policy. Do you have a motivating example for this? If this is just motivated by a desire to clean up and inconsistency, then I think we can just close this PR, that is insufficient IMO to motivate overriding the breaking change concern.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2021
@marmeladema
Copy link
Contributor

My personal use case is that i'd like to have a trait with an associated types that must implement Fn and I want the implementors of the trait to be able to return an Arc<dyn Fn> or a Rc<dyn Fn> depending on if they need thread safety or not etc without requiring boxing a new closure each time.

This is a contrived example of what I would to be able to do:

use std::sync::Arc;

trait GenFn {
    type Output: Fn() -> String;
    
    fn gen(&mut self) -> Self::Output;
}

struct FnBuilder;

impl GenFn for FnBuilder {
    type Output = Arc<dyn Fn() -> String>;
    
    fn gen(&mut self) -> Self::Output {
        Arc::new(|| "Hello World!".to_string())
    }
}

Play link: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=bdf2d5e5e1526542219cc29927f1970a

@ollpu
Copy link

ollpu commented Oct 26, 2021

Was there discussion about adding the corresponding impls for Box being a breaking change (merged in #59500) by the same token? Of course, the Box impls are arguably more useful, but I wonder if it was overlooked back then or what the arguments were. Unfortunate situation in any case.

@JohnCSimon
Copy link
Member

Ping from triage:
@bsodmike - Can you post your status on this PR?

@Others
Copy link
Contributor

Others commented Dec 6, 2021

I think these missing implementations are a minor language wart, and we should fix them somehow.

Is it possible to get a crater run for the PR to see if this theoretical breakage exists in practice?

I'm happy to rebase this if the original owner is unavailable.

@Mark-Simulacrum
Copy link
Member

Following up on this (I missed #89771 (comment) in my notifications) now -- it is somewhat unfortunate these impls do not exist, but I don't think the presented motivation is sufficiently strong to override the breaking change concern (which crater cannot really resolve).

The specific pattern mentioned is fairly rare (bounding an associated type with Fn) and substituting a user-defined trait, perhaps one deref'ing to Fn, should work I think.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2021
@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2022
@Dylan-DPC
Copy link
Member

Closing this based on #89771 (comment)

@Dylan-DPC Dylan-DPC closed this Feb 22, 2022
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants