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.

Sign up for GitHub

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

Jump to bottom

Add (unstable) FnBox trait as a nicer replacement for Thunk. #23939

Merged
merged 3 commits into from
Apr 2, 2015
Merged

Add (unstable) FnBox trait as a nicer replacement for Thunk. #23939

merged 3 commits into from
Apr 2, 2015

Conversation

nikomatsakis
Copy link
Contributor

Add (unstable) FnBox trait as a nicer replacement for Thunk. The doc comment includes a test that also shows how it can be used.

f? @aturon
f? @alexcrichton

@rust-highfive
Copy link
Collaborator

r? @brson

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

@nikomatsakis nikomatsakis changed the title [RFC] Add (unstable) FnBox trait as a nicer replacement for Thunk. [WIP] Add (unstable) FnBox trait as a nicer replacement for Thunk. Apr 1, 2015
@nikomatsakis nikomatsakis assigned alexcrichton and unassigned brson Apr 1, 2015
@nikomatsakis
Copy link
Contributor Author

r? @alexcrichton

@bors
Copy link
Contributor

bors commented Apr 1, 2015

☔ The latest upstream changes (presumably #23936) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor Author

Hmm, encountered an obstacle. Investigating now.

@nikomatsakis
Copy link
Contributor Author

OK, so, there is an obstacle that I'm not sure how best to rectify, though it's also not a strict blocker. Just make the FnBox story less good than I hoped it was. (Of course, FnBox has always been seen as a kind of temporary work-around.)

If you have a type like x: Box<FnBox(&mut Foo)> (note the bound region there), you can't call it with x() notation, you must write x.call_box((arg,)). The reason is that the impl is for Box<FnBox<A>> which is less polymorphic than Box<for<'a> FnBox<(&'a mut Foo,)>>. The compiler's normal "instantiate polymorphic region" logic doesn't help here, because the call is being redirected through an impl, which requires that Box<FnBox<A>> == Box<for<'a> FnBox<(&'a mut Foo,)>> for some A that is just not possible.

There are two workarounds. You can manually upcast:

let x: Box<FnBox<(&mut Foo,), Output=()>> = x;

which works because Box is covariant, or just use call_box directly.

(Interestingly, this would be a kind of use case for variance on traits (that I just opened a PR to remove) except that FnBox winds up being invariant due to associated types, kind of proving my point that it's not buying us as much as it should.)

alexcrichton
alexcrichton reviewed Apr 1, 2015
View reviewed changes
src/libtest/lib.rs
@@ -958,7 +959,7 @@ pub fn run_test(opts: &TestOpts,
}
DynMetricFn(f) => {
let mut mm = MetricMap::new();
f.invoke(&mut mm);
f.call_box((&mut mm,)); // TODO unfortunate
Copy link
Member

Choose a reason for hiding this comment

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

I think this may need to be "FIXME" to get past make tidy

@alexcrichton
Copy link
Member

Nice! r=me with a few nits

I'm ok with having to use unstable syntax to use an unstable feature :)

@nikomatsakis nikomatsakis force-pushed the fn-box branch 2 times, most recently from 1d599ca to aa4d829 Compare April 1, 2015 18:02
@aturon aturon mentioned this pull request Apr 1, 2015
Closed
91 tasks
@aturon
Copy link
Member

aturon commented Apr 1, 2015

LGTM as well. We can discuss the stabilization story at some point during the beta period.

@nikomatsakis
Add (unstable) FnBox trait as a nicer replacement for Thunk. The doc
ed63d32 
comment includes a test that also shows how it can be used.
@nikomatsakis
Remove Thunk struct and Invoke trait; change Thunk to be an alias
cade32a 
for `Box<FnBox()>`. I found the alias was still handy because it is
shorter than the fully written type.

This is a [breaking-change]: convert code using `Invoke` to use `FnBox`,
which is usually pretty straight-forward. Code using thunk mostly works
if you change `Thunk::new => Box::new` and `foo.invoke(arg)` to
`foo(arg)`.
@nikomatsakis nikomatsakis changed the title [WIP] Add (unstable) FnBox trait as a nicer replacement for Thunk. Add (unstable) FnBox trait as a nicer replacement for Thunk. Apr 1, 2015
@nikomatsakis
Copy link
Contributor Author

@bors r=alexcrichton cade32a

@nikomatsakis
Copy link
Contributor Author

@bors r=alexcrichton 8eed73f

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 1, 2015
@alexcrichton
rollup merge of rust-lang#23939: nikomatsakis/fn-box
9bb05fd 
Conflicts:
	src/liballoc/boxed.rs
@alexcrichton alexcrichton merged commit 8eed73f into rust-lang:master Apr 2, 2015
@nikomatsakis nikomatsakis deleted the fn-box branch March 30, 2016 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nikomatsakis @rust-highfive @bors @alexcrichton @aturon @brson