Unsized rvalues: implement boxed closure impls.

[qnighy]

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

impl<A,F:FnOnce<A> + ?Sized>FnOnce<A>forBox<F>{ .. }impl<A,F:FnMut<A> + ?Sized>FnMut<A>forBox<F>{ .. }impl<A,F:Fn<A> + ?Sized>Fn<A>forBox<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. I'll mark this PR as [WIP] until it is resolved. 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.

[rust-highfive]

r? @frewsxcv

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

[rust-highfive]

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:0f20aec0:start=1540737014979086364,finish=1540737068428273499,duration=53449187135
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout
Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
---
[00:51:15] .................................................................................................... 2200/4972
[00:51:19] .................................................................................................... 2300/4972
[00:51:23] .................................................................................................... 2400/4972
[00:51:27] .................................................................................................... 2500/4972
[00:51:30] ............................................................iiiiiiiii............................... 2600/4972
[00:51:37] ..........ii........................................................................................ 2800/4972
[00:51:39] .................................................................................................... 2900/4972
[00:51:43] .................................................................................................... 3000/4972
[00:51:46] .....i.............................................................................................. 3100/4972
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:04:41] [01:04:41] running 112 tests
[01:04:44] i..ii...iii.......i...i.........i..iii...........i.....i.....ii...i..i.ii..............i...ii..ii.i. 100/112
[01:04:44] test result: ok. 82 passed; 0 failed; 30 ignored; 0 measured; 0 filtered out
[01:04:44] [01:04:44] finished in 3.451
[01:04:44] travis_fold:end:test_codegen
---
[01:39:47] [01:39:47] [01:39:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:39:47] Build completed unsuccessfully in 0:53:20
[01:39:47] make: *** [check] Error 1
[01:39:47] Makefile:58: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:03b33118
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:133bb880:start=1540743076301033206,finish=1540743076305694415,duration=4661209
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0beb0618
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then pr

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[bjorn3]

@TimNN ^ not very helpful

[alexreg]

FnBox was only ever a stop-gap solution, and highly unstable. I really wouldn't worry about migration, and would just obsolete it immediately in favour of this, if it works just as well/better in all circumstances.

[Mark-Simulacrum]

I've re-assigned this to @nikomatsakis since I think it'll probably want lang/compiler team discussion or review.

[XAMPPRocky]

Triage; @nikomatsakis Hello, have you been able to get back to this PR?

[Dylan-DPC-zz]

CI fails with:

[01:39:47] failures:
[01:39:47] [01:39:47] ---- /checkout/src/doc/unstable-book/src/library-features/boxed-closure-impls.md - boxed_closure_impls::Usage (line 27) stdout ----
[01:39:47] error[E0057]: this function takes 1 parameter but 0 parameters were supplied
[01:39:47] --> /checkout/src/doc/unstable-book/src/library-features/boxed-closure-impls.md:40:5
[01:39:47] |
[01:39:47] 14 | f();
[01:39:47] | ^^^ expected 1 parameter
[01:39:47] [01:39:47] thread '/checkout/src/doc/unstable-book/src/library-features/boxed-closure-impls.md - boxed_closure_impls::Usage (line 27)' panicked at 'couldn't compile the test', librustdoc/test.rs:332:13
[01:39:47] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:39:47] [01:39:47] [01:39:47] failures:
[01:39:47] /checkout/src/doc/unstable-book/src/library-features/boxed-closure-impls.md - boxed_closure_impls::Usage (line 27)
[01:39:47] [01:39:47] test result: FAILED. 0 passed; 1 failed; 4 ignored; 0 measured; 0 filtered out
[01:39:47] [01:39:47] [01:39:47] stderr ----
[01:39:47] [01:39:47] [01:39:47] [01:39:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:39:47] Build completed unsuccessfully in 0:53:20
[01:39:47] make: *** [check] Error 1
[01:39:47] Makefile:58: recipe for target 'check' failed

[Dylan-DPC-zz]

Pinging from triage @rust-lang/compiler can anyone review this?

[eddyb]

This is not really a compiler change. Maybe @rust-lang/lang + @rust-lang/libs?

[cramertj]

@eddyb I think it is compiler-relevant since one of the core questions in the PR description is how to make use of the new impls unstable:

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. I'll mark this PR as [WIP] until it is resolved.

This isn't possible today, and it's hard for me to understand how it could become possible: we'd have to somehow gate all coercions, where clauses, transitive uses (e.g. trait MyTrait {} impl<T> MyTrait for T where T: FnOnce() { ... }) etc. I'm personally fine exposing this from the lang side as it seems obvious that these impls should exist somehow. The only hold-up I would have is if we accidentally make it backwards-incompatible to change the impl of FnOnce for Box<T> to avoid alloca by passing self by-pointer and dropping the allocated memory after the call. I can't think of many reasonable ways that a user could be relying on the alloca + move-to-stack occurring, though, so I think this is fine and I don't see a reason not to land these impls now that we finally can.

[eddyb]

@cramertj I think that's a lang team discussion topic - that is, whether we should just stabilize now, given that feature-gating is hard (or even impossible).
Maybe we should start an FCP merge on this PR?

[cramertj]

@rfcbot fcp merge

This change introduces the following (insta-stable) trait impls:

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

Defining these impls requires the use of the unsized_locals feature and we'd ideally like to gate these impls since they rely on unstable features in order to make it possible to implement them (i.e. it's not just this particular impl that is unstable as with e.g. most of std, but implementing them at all is impossible without the unstable feature or some extra-clever and probably UB hacks).

However, we don't have the machinery (and possibly won't ever have the machinery) to make trait impls unstable. As such, merging this PR makes Box<FnOnce()> instantly call-able on stable.

These implementations have been desired for a long time but have been impossible to add until now. I'm not sure of any other reasonable ways to implement them, or any ways that we might want to change the implementations going forward in a user-observable way.

[rfcbot]

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

• @Centril
• @Kimundi
• @SimonSapin
• @alexcrichton
• @aturon
• @cramertj
• @dtolnay
• @eddyb
• @joshtriplett
• @nikomatsakis
• @nrc
• @pnkfelix
• @scottmcm
• @sfackler
• @withoutboats

Concerns:

• i-need-to-think-about-this resolved by Unsized rvalues: implement boxed closure impls. #55431 (comment)
• risky-stabilization-order resolved by Unsized rvalues: implement boxed closure impls. #55431 (comment)
• unimplementable-if-unsized-does-not-pan-out resolved by Unsized rvalues: implement boxed closure impls. #55431 (comment)

Once a majority of reviewers approve (and none object), 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.

[Centril]

Is there a particular reason to stabilize these impls now rather than to wait until unsized_locals is stable?

[alexreg]

@Centril The surface area is much smaller for these three impls, I guess, and they cover a large portion of the urgent demand for unsized locals at the same time.

[alexcrichton]

@rfcbot concern unimplementable-if-unsized-does-not-pan-out

Historically we've avoiding expanding the stable API surface area of the standard library with unstable features. One of the best examples here is specialization where we've exclusively used it so far for performance improvements and we haven't started to use it for actually expanding trait impls yet. The rationale for this is that we can probably recover performance via other means if absolutely necessary if the unstable feature (specialization) doesn't pan out.

Do we have a similar recourse for this? What if the unstable feature doesn't pan out? Are we confident enough that we can provide this implementation for all of time?

[cramertj]

@alexcrichton So I'd argue this is implementable even if the current unsized-rvalues-using-alloca proposal doesn't shake out-- we can still pass self by-pointer into the method and clean up the memory for the Box after the fact, or add a vtable shim to FnOnce specifically that takes Box<Self>. I think we definitely want self-by-value to be object-safe for all the benefits it gives, so I'd argue we want something that can provide this functionality regardless of whether the current feature as-implemented shakes out.

Compared with specialization, where exactly which implementations are allowed and when overlaps may occur is still open to soundness-related debate, the likelihood of the API surface needing to change here is far smaller. Basically the only thing that's being promised here is "there is some way to call FnOnce::call_once on a Box<dyn FnOnce()>". I think there are enough alternatives (pass-by-pointer, a special vtable shim which takes Box<Self>, etc.) that we can feel safe providing this API.

[cramertj]

@Centril

Is there a particular reason to stabilize these impls now rather than to wait until unsized_locals is stable?

Box<dyn FnOnce(..) -> ...> working would be a massive improvement to the ergonomics of lots of code which currently relies on hacky custom traits for every function signature needed. As I said above in my response to @alexcrichton, I feel like the API surface being provided here is high value, small, and low-risk enough that it needn't be blocked by the much larger and more complex unsized_locals feature.

[alexcrichton]

@rfcbot resolve unimplementable-if-unsized-does-not-pan-out

Sounds solid to me, thanks for the explanation!

[kennytm]

Sounds like #45417

[cramertj]

@kennytm Sorry, I see you marked this as waiting-on-author-- is there something that needs to be changed here, or is CI just flaking?

[kennytm]

@cramertj the error is #55431 (comment) on the gnux32 target, and it doesn't look like an existing spurious error. Probably hit an LLVM bug. So this is waiting for the author (or anybody else) to debug why is this happening and how to fix/workaround it.

[qnighy]

Easily reproduced in my machine by setting target = ["x86_64-unknown-linux-gnu", "x86_64-unknown-linux-gnux32"]. No further progress yet.

[Dylan-DPC-zz]

ping from triage @qnighy any updates?

[Dylan-DPC-zz]

ping frm triage @qnighy
Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again! Thanks for contributing.

[cramertj]

Procedurally it seems a bit odd to me to close a perfectly fine PR due to it being unmergable as a result of happening to trigger a suspected LLVM bug. To be clear, there's no requested change to the code in this PR-- only a change to LLVM to successfully compile it.

[Centril]

@cramertj This is entirely by the book:

If the author’s been unresponsive for more than 14 days, close the PR due to inactivity and ask the author to reopen when they have a chance to make the necessary changes. Make sure to thank the author for the changes. Also tag the PR with S-inactive-closed.

If there has been no meaningful updates after two triage updates, with no meaningful being defined as no commits or no status updates that show progress (i.e. “Soon TM”). The PR should be closed due to prolonged inactivity and ask the author to reopen when they have a chance to make the necessary changes. Make sure to thank the author for the changes, and warn them not to push to the PR while it is closed as GitHub will prevent the PR from being reopened. Make sure to also tag the PR with S-inactive-closed.

Once the LLVM issues can be fixed the PR should be reopened.

[cramertj]

ask the author to reopen when they have a chance to make the necessary changes

This sounds like we're asking the author for changes. There are no changes being requested here.

[Centril]

Or generally actions (e.g. "fix LLVM") that unblocks the PR. Perhaps this should be labeled as S-blocked-closed.

[earthengine]

Are there any investigations have been done on LLVM? Can anyone find a workaround (maybe not optimal; we need it to be workable)?

[alexreg]

Has this bug been reported to LLVM? Does anyone have a link?

[kennytm]

@alexreg Please follow #59674 for the LLVM bug.

[alexreg]

So, it looks like this PR has effectively been implemented and merged in #59500. Great!