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

Implement Fn traits for Rc and Arc #71570

Closed
wants to merge 2 commits into from
Closed

Conversation

upsuper
Copy link
Contributor

@upsuper upsuper commented Apr 26, 2020

Given that Box implements Fn traits, I think it makes sense for Rc and Arc to do so as well. But since inner of Rc and Arc is shared, apparently they can only be invoked when Fn is satisfied.

WDYT?

I can create a tracking issue for it if the team is fine with this new impl.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2020
@jonas-schievink jonas-schievink added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 26, 2020
@jonas-schievink jonas-schievink added this to the 1.45 milestone Apr 26, 2020
src/liballoc/rc.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

r? @dtolnay for libs-team acceptance of stable surface area expansion

cc @eddyb -- do we need to do something special here to not run into the problems encountered here: #68304?

I would also like to see some tests to make sure this is working. Regardless we'll only be able to support Fn for Arc, not FnOnce/FnMut as we never have guarantees through the type system of unique access to the stored value.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a compelling use case for this? I would prefer not to do this "just because".

Note that function calls do deref coercion so Rc and Arc are already callable as if they had a Fn impl.

fn main() {
    std::rc::Rc::new(|| {})();
}

@upsuper
Copy link
Contributor Author

upsuper commented Apr 27, 2020

Is there a compelling use case for this? I would prefer not to do this "just because".

The use case is when you want to pass an Rc-ed closure to a function which accepts an Fn. Without implementing this, you would need to wrap it in another closure.

The initial reason motivated me was that I have like three widgets in my UI (using gtk-rs) sharing the same closure, and that closure bundles a bunch things so I figured it makes more sense to have the closure itself Rc-ed rather than cloning it every time. But gtk-rs's connect_* functions only accepts Fn, so I have code like

let update_something = Rc::new(move || {
    // ...
});
toggle1.connect_toggled({
    let update_something = update_something.clone();
    move |_| update_something()
});
toggle2.connect_toggled({
    let update_something = update_something.clone();
    move |_| update_something()
});
toggle3.connect_toggled({
    let update_something = update_something.clone();
    move |_| update_something()
});

however, if Rc implements Fn as well, it can be simplified to something like

let update_something = Rc::new(move |_| {
    // ...
});
toggle1.connect_toggled(update_something.clone());
toggle2.connect_toggled(update_something.clone());
toggle3.connect_toggled(update_something.clone());

@upsuper upsuper requested a review from dtolnay April 27, 2020 07:05
@upsuper
Copy link
Contributor Author

upsuper commented Apr 27, 2020

I would also like to see some tests to make sure this is working. Regardless we'll only be able to support Fn for Arc, not FnOnce/FnMut as we never have guarantees through the type system of unique access to the stored value.

Fn types must also implement FnOnce and FnMut. The restriction here is that we can only implement them when the inner type satisfies Fn rather than only FnMut or even FnOnce. It's just like that &T implements all the three traits but only when T: Fn.

@eddyb
Copy link
Member

eddyb commented Apr 27, 2020

@Mark-Simulacrum this is harmless because you're not moving potentially-unsized values.

@upsuper
Copy link
Contributor Author

upsuper commented May 2, 2020

@dtolnay WDYT about #71570 (comment)?

If you and the lib team would be happy with moving forward on this, I can add some tests for it.

@dtolnay dtolnay added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label May 9, 2020
@dtolnay
Copy link
Member

dtolnay commented May 9, 2020

I am on board with adding the impls. Could you add some basic test coverage? It's too easy for this kind of impl to become accidentally infinite recursive in a refactor.

@upsuper
Copy link
Contributor Author

upsuper commented May 10, 2020

I am on board with adding the impls. Could you add some basic test coverage? It's too easy for this kind of impl to become accidentally infinite recursive in a refactor.

Yep, done! Hopefully it's enough to catch this kind of issues.

I actually tried to make one of the impl recursive with #[allow(unconditional_recursion)] (otherwise the compiler errors directly), and the test mysteriously doesn't fall into infinite recursion, but does fail with an incorrect result.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks good.

@dtolnay
Copy link
Member

dtolnay commented May 10, 2020

@rust-lang/libs:
@rfcbot fcp merge

impl<A, F: Fn<A> + ?Sized> FnOnce<A> for Rc<F> {...}
impl<A, F: Fn<A> + ?Sized> FnMut<A> for Rc<F> {...}
impl<A, F: Fn<A> + ?Sized> Fn<A> for Rc<F> {...}

impl<A, F: Fn<A> + ?Sized> FnOnce<A> for Arc<F> {...}
impl<A, F: Fn<A> + ?Sized> FnMut<A> for Arc<F> {...}
impl<A, F: Fn<A> + ?Sized> Fn<A> for Arc<F> {...}

The trait bounds on the above new impls match the impls for &F that have existed since 1.0.0:

impl<A, F: Fn<A> + ?Sized> FnOnce<A> for &F {...}
impl<A, F: Fn<A> + ?Sized> FnMut<A> for &F {...}
impl<A, F: Fn<A> + ?Sized> Fn<A> for &F {...}

@rfcbot
Copy link

rfcbot commented May 10, 2020

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 10, 2020
@sfackler
Copy link
Member

The Fn traits are #[fundamental] - adding these impls is in theory a breaking change: https://doc.rust-lang.org/src/core/ops/function.rs.html#67

@cuviper
Copy link
Member

cuviper commented May 10, 2020

@sfackler but the methods are still unstable, so I think there can't be any stable users with conflicting impls. wrong, see below.

@nbdd0121
Copy link
Contributor

@sfackler but the methods are still unstable, so I think there can't be any stable users with conflicting impls.

use std::rc::Rc;
trait LocalTrait {}
impl<F> LocalTrait for Rc<F> where F: Fn() {}
impl<F> LocalTrait for F where F: Fn() {}

@cuviper
Copy link
Member

cuviper commented May 10, 2020

@nbdd0121 Ah, indeed, I didn't think about that angle.

@dtolnay
Copy link
Member

dtolnay commented May 15, 2020

@rfcbot concern This is a breaking change, and definitely not an "allowed" breaking change

@dtolnay
Copy link
Member

dtolnay commented May 15, 2020

That's a pretty strong reason not to do this.

Is this a case where we would trust Crater on whether this kind of "impl for { F, Rc<F> } where F: Fn" or anything similar is a thing people do, or better to abandon?

@LukasKalbertodt
Copy link
Member

I think I prefer not merging this due to the breaking change. I don't think the advantages of these impls are great enough to violate the promise we explicitly (by annotating #[fundamental]) stated.

@upsuper
Copy link
Contributor Author

upsuper commented May 23, 2020

Fn traits have been implemented for several std pointer-like types including &, &mut, and Box, so not having it implemented for Rc and Arc itself feels like an inconsistency.

IIUC, the reason #[fundamental] attached to Fn traits was purely for regex to be able to rely on &str: !Fn (as the comment on the attribute shows), which is not being violated anyway. Unless Crater shows otherwise, there isn't evidence that it's actually utilized widely.

Because of the inconsistency, I think this can be considered an oversight when implementing Fn traits and marking them #[fundamental].

@SimonSapin
Copy link
Contributor

Crater can never test all Rust code in the world. It only tests a sample, with significant selection bias. So even with a perfectly green Crater run, knowingly landing a breaking change has non-zero risk.

For breaking changes considered "minor", that risk is to be balanced against the benefits brought by the change. Here the use case (#71570 (comment)) feels rather niche to me. The work-around is not that terrible, and its repetition can be reduced:

let update_something = Rc::new(move || {
    // ...
});
let clone_update_something = || {
    let update_something = update_something.clone();
    move |_| update_something()
};
toggle1.connect_toggled(clone_update_something());
toggle2.connect_toggled(clone_update_something());
toggle3.connect_toggled(clone_update_something());

However even if we judged the benefit worth the estimated risk, we have an accepted RFC that categorizes this kind of change as "major": https://rust-lang.github.io/rfcs/1105-api-evolution.html#major-change-implementing-any-fundamental-trait , and states that a major breaking change (that cannot be made edition-specific) requires incrementing the major component of the version number. (Personally I’d prefer Rust 2.0 to never happen.)

@dtolnay
Copy link
Member

dtolnay commented May 28, 2020

I agree with that.

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented May 28, 2020

@dtolnay proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels May 28, 2020
@dtolnay
Copy link
Member

dtolnay commented May 28, 2020

Thanks anyway @upsuper!

@dtolnay dtolnay closed this May 28, 2020
@upsuper upsuper deleted the rc-fn branch May 28, 2020 02:35
@crlf0710 crlf0710 removed the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 8, 2020
@crlf0710 crlf0710 removed this from the 1.45 milestone Jul 8, 2020
@marmeladema marmeladema mentioned this pull request Oct 11, 2021
bsodmike added a commit to bsodmike/rust that referenced this pull request Oct 12, 2021
@cuviper
Copy link
Member

cuviper commented Aug 18, 2022

Having forgotten about this PR, I was playing with such impls again and found this real-world conflict:

error[E0119]: conflicting implementations of trait `fmt::writer::MakeWriter<'_>` for type `std::sync::Arc<_>`
   --> /home/jistone/.cargo/registry/src/github.com-1ecc6299db9ec823/tracing-subscriber-0.3.3/src/fmt/writer.rs:686:1
    |
674 | impl<'a, F, W> MakeWriter<'a> for F
    | ----------------------------------- first implementation here
...
686 | impl<'a, W> MakeWriter<'a> for Arc<W>
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `std::sync::Arc<_>`

For more information about this error, try `rustc --explain E0119`.
error: could not compile `tracing-subscriber` due to previous error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.