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

Tracking issue for FnBox() #28796

Open
abonander opened this Issue Oct 1, 2015 · 83 comments

Comments

Projects
None yet
@abonander
Copy link
Contributor

abonander commented Oct 1, 2015

This is a tracking issue for the fn_box feature and FnBox trait, which allows a Box<FnOnce(...) -> _> closure to be invoked via the "rust-call" sugar.

I'm not sure what the primary concerns are. All I know is it'd be really, really nice if we could invoke a Box<FnOnce()> somehow in safe Rust. This may or may not be the best approach.

@eddyb feel free to CC anyone who might want to comment or edit this issue to add any pertinent information.

@eefriedman

This comment has been minimized.

Copy link
Contributor

eefriedman commented Oct 1, 2015

The reason we don't want to stabilize FnBox is because we would prefer to make Box<FnOnce()> just work.

Making Box<FnOnce()> work isn't exactly controversial (there's only one obvious way to handle the construct), but it's sort of tied up with rust-lang/rfcs#997, and more generally the issue that we want to remove the special cases for Box from the compiler.

@charlesetc

This comment has been minimized.

Copy link

charlesetc commented Oct 2, 2015

So in the stable version of rust it's impossible to call Box<FnOnce()> and it's impossible to return a closure that's not in a box, judging from the book, because it doesn't have a known size. So how does one return a closure and call it?

@abonander

This comment has been minimized.

Copy link
Contributor Author

abonander commented Oct 2, 2015

@charlesetc Box<Fn()> and Box<FnMut()> can both be called without a problem, because they don't require destructuring the box they're in; they only need &self or &mut self, respectively, which totally works with the current deref coercions. Which of the closure traits, Fn(), FnMut(), or FnOnce(), is implemented for a specific closure is determined by inference.

Box<FnOnce()> is a challenge because it takes self by-value, which means it needs to be moved out of the Box and onto the stack to be executed, but this doesn't work in current Rust because there's no stable way to move out of (destructure) a Box. The DerefMove trait proposed in the RFC @eefriedman linked would make this work, but the design of it is still being worked on.

The key here is that Box<FnOnce()> does have a known size, but it's hidden behind the Box pointer so that anything implementing FnOnce, regardless of its size, can fit in the same spot. The v-table in the trait object directs the processor to the concrete implementation of the trait (in this case, the closure body) for that specific object. All the details about the object, its size, fields, etc., are coded into that implementation.

@charlesetc

This comment has been minimized.

Copy link

charlesetc commented Oct 2, 2015

Oh that makes more sense, thanks!

@kosta

This comment has been minimized.

Copy link

kosta commented Oct 2, 2015

I don't want to be pushy, but I think this is really a major missing piece in Rusts "Closures" story - and not really mentioned on in the rust book.

I understand that you want to remove special-cases for Box in the compiler and I agree that that's a worthy goal. But when will that land?

If we can have Box<FnOnce> right now for the price of "a bit more Box magic" until the no-special-case-for-Box story is complete, then is that maybe the right price to pay?
(I couldn't say, I would just profit from it as a Rust user)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 4, 2015

Nominating for discussion in 1.6

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Nov 4, 2015

cc me

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Nov 5, 2015

🔔 This issue is now entering its cycle-long final comment period 🔔

The libs team discussed this and we have two possible routes for this right now:

  1. The in-tree copy could be stabilized with the intention of immediately deprecating once by-value DST works. The in-tree copy has the benefit of "being variadic" and having the paren sugar in bounds.
  2. The in-tree copy could be deprecated and a crates.io crate could provide a suite of traits like FnBox1, FnBox2, etc. This has the benefit of not polluting the standard library but is less ergonomic to use.

We're a little undecided on which route to take, so some comments would be welcome!

@abonander

This comment has been minimized.

Copy link
Contributor Author

abonander commented Nov 5, 2015

I'd vote for the former. We have enough micro-crates in the ecosystem and this is a common enough pattern that it's worth supporting in the stdlib.

Edit: I want to add that I do have a use for this. I need it for one-off callbacks in wingui. Right now I'm having to wrap FnOnce callbacks in an Option and move it into an FnMut closure so I can call it from a box. It's not very ergonomic.

I don't want to have to add another crate to my build because that seems like a very heavyweight solution just to get a trait or two.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Nov 5, 2015

@cybergeek94 you can do a bit better than that with some boilerplate: https://github.com/sfackler/r2d2/blob/master/src/thunk.rs but it's a bit of a pain.

@abonander

This comment has been minimized.

Copy link
Contributor Author

abonander commented Nov 5, 2015

It's clunky any way you spin it.

@bstrie

This comment has been minimized.

Copy link
Contributor

bstrie commented Nov 5, 2015

Given that we all understand that FnBox is a will-inevitably-be-deprecated-hopefully-sooner-rather-than-later hack (albeit a useful one), I would like commitment from the Rust devs on some kind of timeframe for fixing the underlying issues before letting this be stabilized. Being able to say "this will be deprecated within the next year" is much more reassuring than "this will be deprecated, uhm, iunno, sometime".

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Nov 5, 2015

What's wrong with "this will be deprecated, uhm, iunno, sometime"? If we don't have a timeline for by-value trait objects, why would we not want to have this bridge?

@dlight

This comment has been minimized.

Copy link

dlight commented Nov 8, 2015

I don't want to have to add another crate to my build because that seems like a very heavyweight solution just to get a trait or two.

Well, the "it lives on crates.io until it's stabilized" pattern has been applied to other cases, it's a stopgap solution that's cleaner than introducing something with the intent of deprecating it.

@sinclairzx81

This comment has been minimized.

Copy link

sinclairzx81 commented Nov 20, 2015

Hi,

With the current implementation of FnBox, I note that drop() is not implicitly called for captured members at the end of a boxed closure, which may (or may not) be a unintended source of leaking memory if relying solely on Rust's move semantics. I have produced a gist to demonstrate what i believe to be going on.

https://play.rust-lang.org/?gist=aa96e9e0fe78a09af2eb&version=nightly
https://gist.github.com/aa96e9e0fe78a09af2eb

I am not sure of the future of FnBox, (certainly would love to see a full blown Box<FnOnce> implementation if under review for a 1.6 release), but in the interim, I wonder if its possible to address this specifically to bring things inline.

Thanks all

@eefriedman

This comment has been minimized.

Copy link
Contributor

eefriedman commented Nov 20, 2015

@sinclairzx81 Filed #29946.

@sinclairzx81

This comment has been minimized.

Copy link

sinclairzx81 commented Nov 20, 2015

@eefriedman Brilliant, thank you.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 1, 2015

I think I would stabilize this, personally. My reasoning is:

  1. Anything we can do to make things more ergonomic here is good, until such time as Box<FnOnce()> works properly.
  2. The original idea of FnBox was that it would serve as a place-holder for where you would use Box<FnOnce> such that, in the glorious future, you can basically s/FnBox/FnOnce/ and Everything Just Works. I think this is mostly true now. This will be rather untrue if we move things to a crates.io crate. Obviously I don't really expect anyone to run sed, but I think having FnBox work as much like FnOnce as we can makes it easier to explain to newcomers: "ah, Box<FnOnce> doesn't quite work yet, but you can do Box<FnBox> for now, and it mostly works the same except that it requires a box".

Basically, our story here is not great, but I don't see any reason to make it worse, just to avoid a deprecated item in the standard library.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 3, 2015

The libs team discussed this during triage yesterday and the decision was to stabilize. @bluss brought up a good point, however, that Box<FnMut<A>> doesn't implement FnMut<A> for coherence reasons related to FnBox which may be good to sort out, however. Concretely, an impl like

impl<F: ?Sized, A> FnMut<A> for Box<F> where F: FnMut<A> {
    ...
}

does not work. An impl of FnMut requires an impl of Fn which in turn requires an impl of FnOnce:

impl<F: ?Sized, A> FnOnce<A> for Box<F> where F: FnOnce<A> {
    ...
}

Unfortunately this impl can't be written for two reasons:

  1. Unsized types by value don't work, so we can't use .call_once or * to move out of the box if F: ?Sized.
  2. This impl is a coherence violation with FnOnce<A> for Box<FnBox<A>>.
@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 3, 2015

@nikomatsakis What's blocking Box<FnOnce()> from "just working"?
FnBox seems like a lot of work for a band-aid.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 3, 2015

Ok, so I'm wary to stabilize FnBox if it prevents us from later having the impls for Box<FnMut()> to also implement FnMut(), and to drill down a bit I've come up with this small sample:

#![crate_type = "lib"]
#![feature(fundamental)]

#[fundamental]
trait FnBox<A> {}
#[fundamental]
trait FnOnce<A> {}

struct Box<T: ?Sized>(T);

impl<A> FnOnce<A> for Box<FnBox<A>> {}

impl<F: FnOnce<A> + ?Sized, A> FnOnce<A> for Box<F> {}

This represents basically the traits that are in play here, but doesn't worry about the unsized types by value problem. Currently this code yields a coherence violation:

<anon>:11:1: 11:39 error: conflicting implementations for trait `FnOnce` [E0119]
<anon>:11 impl<A> FnOnce<A> for Box<FnBox<A>> {}
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:11:1: 11:39 help: see the detailed explanation for E0119
<anon>:13:1: 13:55 note: note conflicting implementation here
<anon>:13 impl<F: FnOnce<A> + ?Sized, A> FnOnce<A> for Box<F> {}
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

Yet I would think that because of the fundamental attribute the compiler should be able to know that the type FnBox<A> does not implement FnOnce<A> so the two impls can never overlap. @nikomatsakis mentioned that #19032 may come into play here.

For now I'm not going to stabilize FnBox while we sort this out, but it's certainly a candidate for backporting quickly!

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 3, 2015

@eddyb DST by value

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Dec 3, 2015

"DST by value" or its explicit / first-class formulation, &move.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Dec 3, 2015

@glaebhoerl they are different:

  • fn foo(&move DST) -> SST doesn't copy bits into its stack frame
  • fn foo(DST) -> SST does copy bits.
  • fn foo(SST) -> &move DST better be refining a move reference as it's stack frame doesn't live long enough.
  • fn foo(SST) -> DST copies bits into receiver.

Now for making a realistic API &move and &fillMeIn may be used under the hood, just as they are for certain return values today, but that is an implementation detail. Out pointers are not very ergonomic.

@earthengine

This comment has been minimized.

Copy link

earthengine commented Nov 9, 2018

The same code I posted above compiles (and runs), but when you run the miri check it gives ICE in the playground.

   Compiling playground v0.0.1 (/playground)
thread 'main' panicked at 'assertion failed: self.meta.is_none()', librustc_mir/interpret/place.rs:142:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.32.0-nightly (13dab66a6 2018-11-05) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z always-encode-mir -Z mir-emit-retag -Z mir-opt-level=0 -C codegen-units=1 -C debuginfo=2 --crate-type bin

note: some of the compiler flags provided by cargo are hidden

error: Could not compile `playground`.

To learn more, run the command again with --verbose.

Where should I report issue for the miri check?

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Nov 9, 2018

@earthengine Report it as a separate issue (referencing this one) on this repo.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Nov 9, 2018

Perhaps somebody should add a test for that case before we FCP?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 9, 2018

I view FCP as an opportunity for anyone to object to a proposal, or to some details of it. At this point I believe there is strong consensus that we can and will make Box<dyn FnOnce()> work eventually, it’s "only" a matter of implementation effort.

So, while it’d probably be premature to deprecate FnBox right now, I think FCP to decide we want to do it once the replacement works is fine.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 9, 2018

that we can and will make Box<dyn FnOnce()> work eventually, it’s "only" a matter of implementation effort.

This seems confusing to me. It's already fully implemented, isn't it?
(ignoring the miri bug that @earthengine found - which is not relevant to Rust itself)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 10, 2018

I meant "implementation" as opposed to design decisions, so fixing bugs is part of it.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Nov 10, 2018

Ah, sure, I'm only trying to point out that the only bug we're aware of relates to miri supporting this, which shouldn't block anything, it's not relevant to the language itself.

@Centril Centril added the T-lang label Nov 14, 2018

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 18, 2018

The final comment period, with a disposition to close, as per the review above, is now complete.

scottlamb added a commit to scottlamb/moonfire-nvr that referenced this issue Jan 5, 2019

fix #64 (extraneous flushes)
Now each syncer has a binary heap of the times it plans to do a flush.
When one of those times arrives, it rechecks if there's something to do.
Seems more straightforward than rechecking each stream's first
uncommitted recording, especially with the logic to retry failed flushes
every minute.

Also improved the info! log for each flush to see the actual recordings
being flushed for better debuggability.

No new tests right now. :-( They're tricky to write. One problem is that
it's hard to get the timing right: a different flush has to happen
after Syncer::save's database operations and before Syncer::run calls
SimulatedClocks::recv_timeout with an empty channel[*], advancing the
time. I've thought of a few ways of doing this:

   * adding a new SyncerCommand to run something, but it's messy (have
     to add it from the mock of one of the actions done by the save),
     and Box<dyn FnOnce() + 'static> not working (see
     rust-lang/rust#28796) makes it especially annoying.

   * replacing SimulatedClocks with something more like MockClocks.
     Lots of boilerplate. Maybe I need to find a good general-purpose
     Rust mock library. (mockers sounds good but I want something that
     works on stable Rust.)

   * bypassing the Syncer::run loop, instead manually running iterations
     from the test.

Maybe the last way is the best for now. I'm likely to try it soon.

[*] actually, it's calling Receiver::recv_timeout directly;
Clocks::recv_timeout is dead code now? oops.

bors added a commit that referenced this issue Feb 11, 2019

Auto merge of #55431 - qnighy:boxed-closure-impls, r=cramertj
Unsized rvalues: implement boxed closure impls.

This pull request contains **`boxed_closure_impls`** that provides long-hoped three impls:

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

This has been blocked by several reasons; see `FnBox` #28796 for details.

Now that #48055 is (partly) implemented, we're ready to introduce the above impls, replacing existing `FnBox` workarounds.

There are two major concerns, however.

## Major concern 1: instability

I think these impls should be introduced as an unstable feature first, mainly because it relies on unsized rvalues. However, `impl` itself can't stand as unstable; all existing unstable features are tied with functions, methods, or types. I tried putting `#[unstable]` on the impls but that didn't work. <del>**I'll mark this PR as [WIP] until it is resolved**</del>. I'll just remove `[WIP]` and let the compiler team decide how to deal with the instability.

## Major concern 2: compatibility with `FnBox`

I'm not really sure, but `FnBox` may be widely used in the nightly world. Although unstable features may regress, it's good if there's a path for gradual migration. It can be done in the following way:

- Make `FnBox` a subtrait of `FnOnce`. This ensures that `dyn FnBox` implements `FnOnce`.
- Make use of specialization to avoid overlap between `FnOnce` impls for `Box<impl FnOnce>` and `Box<dyn FnBox>`.

I believe that this minimizes breakage of crates that use `FnBox`.

## Minor concern: feature name and tracking issue

I currently assign `boxed_closure_impls` as the name and #48055 as the tracking issue. I'll prepare a separate tracking issue when the Major Concern 1 is resolved.

bors added a commit that referenced this issue Feb 12, 2019

Auto merge of #55431 - qnighy:boxed-closure-impls, r=cramertj
Unsized rvalues: implement boxed closure impls.

This pull request contains **`boxed_closure_impls`** that provides long-hoped three impls:

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

This has been blocked by several reasons; see `FnBox` #28796 for details.

Now that #48055 is (partly) implemented, we're ready to introduce the above impls, replacing existing `FnBox` workarounds.

There are two major concerns, however.

## Major concern 1: instability

I think these impls should be introduced as an unstable feature first, mainly because it relies on unsized rvalues. However, `impl` itself can't stand as unstable; all existing unstable features are tied with functions, methods, or types. I tried putting `#[unstable]` on the impls but that didn't work. <del>**I'll mark this PR as [WIP] until it is resolved**</del>. I'll just remove `[WIP]` and let the compiler team decide how to deal with the instability.

## Major concern 2: compatibility with `FnBox`

I'm not really sure, but `FnBox` may be widely used in the nightly world. Although unstable features may regress, it's good if there's a path for gradual migration. It can be done in the following way:

- Make `FnBox` a subtrait of `FnOnce`. This ensures that `dyn FnBox` implements `FnOnce`.
- Make use of specialization to avoid overlap between `FnOnce` impls for `Box<impl FnOnce>` and `Box<dyn FnBox>`.

I believe that this minimizes breakage of crates that use `FnBox`.

## Minor concern: feature name and tracking issue

I currently assign `boxed_closure_impls` as the name and #48055 as the tracking issue. I'll prepare a separate tracking issue when the Major Concern 1 is resolved.

bors added a commit that referenced this issue Feb 12, 2019

Auto merge of #55431 - qnighy:boxed-closure-impls, r=cramertj
Unsized rvalues: implement boxed closure impls.

This pull request contains **`boxed_closure_impls`** that provides long-hoped three impls:

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

This has been blocked by several reasons; see `FnBox` #28796 for details.

Now that #48055 is (partly) implemented, we're ready to introduce the above impls, replacing existing `FnBox` workarounds.

There are two major concerns, however.

## Major concern 1: instability

I think these impls should be introduced as an unstable feature first, mainly because it relies on unsized rvalues. However, `impl` itself can't stand as unstable; all existing unstable features are tied with functions, methods, or types. I tried putting `#[unstable]` on the impls but that didn't work. <del>**I'll mark this PR as [WIP] until it is resolved**</del>. I'll just remove `[WIP]` and let the compiler team decide how to deal with the instability.

## Major concern 2: compatibility with `FnBox`

I'm not really sure, but `FnBox` may be widely used in the nightly world. Although unstable features may regress, it's good if there's a path for gradual migration. It can be done in the following way:

- Make `FnBox` a subtrait of `FnOnce`. This ensures that `dyn FnBox` implements `FnOnce`.
- Make use of specialization to avoid overlap between `FnOnce` impls for `Box<impl FnOnce>` and `Box<dyn FnBox>`.

I believe that this minimizes breakage of crates that use `FnBox`.

## Minor concern: feature name and tracking issue

I currently assign `boxed_closure_impls` as the name and #48055 as the tracking issue. I'll prepare a separate tracking issue when the Major Concern 1 is resolved.
@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Mar 20, 2019

This issue (fixing Box<dyn FnOnce>) is the only hard blocker preventing the libtest crate from working on stable Rust.

Playground:

fn foo(x: Box<dyn FnOnce()>) { x(); }

errors with:

error[E0161]: cannot move a value of type dyn std::ops::FnOnce(): the size of dyn std::ops::FnOnce() cannot be statically determined
 --> src/lib.rs:2:5
  |
2 |     x();
  |     ^

error: aborting due to previous error
@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 20, 2019

That is not a hard blocker. You can easily build a one-off trait to cover the boxed once closure use case.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Mar 20, 2019

Do you have an example or is that documented somewhere?

This is the closest thing I know (playground):

trait MyFnBox {
    fn call_box(self: Box<Self>) -> ();
}

impl<T> MyFnBox for T where T: FnOnce() -> () {
    fn call_box(self: Box<Self>) -> () {
        (*self)()
    }
}

fn foo(b: Box<dyn FnOnce()>) {
    b.call_box()
}

But it doesn't work (dyn FnOnce(): ?Sized, and I don't know how to call that in a trait impl since one can't move that out of the box).

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Mar 20, 2019

Does this API specifically require taking exactly the type Box<dyn FnOnce()>?

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Mar 20, 2019

@sfackler I haven't spent too much time trying to use Box<dyn FnMut()> but that was the first thing I attempted and it does requires significant refactorings - it requires changing many APIs that rely on being able to move the environment into the closure to take references to the environment, which must be kept alive somehow else. I didn't see them through because I thought these refactorings were too invasive, but I'll take a PR that does them if it gets the library running on stable.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Mar 20, 2019

@sfackler so I performed a lot of refactorings since I tried that, and I just tried it again, and it all worked with minimal changes - sorry for the noise =/

@earthengine

This comment has been minimized.

Copy link

earthengine commented Mar 20, 2019

@gnzlbg I think @sfackler means you can change the API to use Box<dyn MyFnBox> before Box<dyn FnOnce> can be used in stable.

@crlf0710

This comment has been minimized.

Copy link
Contributor

crlf0710 commented Mar 20, 2019

Really wish #55431 can be revived and pushed forward.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Mar 21, 2019

@gnzlbg Where is this requirement for libtest tracked, or where in the source is it needed? I'm not sure but I would suspect you might want your own trait anyway, that is implemented for closures but also allows manual implementations.

Alternatively, you could potentially have a wrapper that exposes only a constructor with an F: FnOnce(...) -> ... bound, and internally uses a private trait that works like FnBox.

@crlf0710

This comment has been minimized.

Copy link
Contributor

crlf0710 commented Apr 10, 2019

impl FnOnce for Box<FnOnce()> will be stable in 1.35.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.