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 RFC 1558: Allow coercing non-capturing closures to function pointers #39817

Closed
2 tasks done
aturon opened this issue Feb 14, 2017 · 29 comments
Closed
2 tasks done
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Feb 14, 2017

RFC

Summary:

A closure that does not move, borrow, or otherwise access (capture) local variables should be coercable to a function pointer (fn).

Outstanding issues:

@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 14, 2017
@est31
Copy link
Member

est31 commented Feb 14, 2017

I'd like to implement this. Which steps are required?

@aturon
Copy link
Member Author

aturon commented Feb 14, 2017

@est31 Awesome, thank you!

@eddyb or @nikomatsakis, can one of you give some pointers?

@eddyb
Copy link
Member

eddyb commented Feb 14, 2017

I'd reuse the existing "reify to fn pointer" coercion (see the code that applies it).
The difference for a TyClosure is you have to check that tcx.with_freevars gives you an empty vector.

Then HIR+Ty->HAIR lowering of the reify coercion has to generate something like... wait, no 😢
So this is the "hard part":
I want to suggest <closure as FnOnce(...)>::call_once as fn(...) -> _ which would work at the lowest level, because we ignore that first argument, but it doesn't type-check (the MIR, that is).
So you'd have to keep around a reify coercion, and only in trans get a FnOnce::call_once DefId and pass the closure and the tupled arguments as type parameters (in a Substs).
But with those you should be able to use Callee::def(ccx, def_id, substs).reify(ccx) to get the fn pointer.

@est31
Copy link
Member

est31 commented Feb 14, 2017

Hmm seems its more than I thought. As I'm rather tight with time this week, is it a problem if I implement it only next week?

@aturon
Copy link
Member Author

aturon commented Feb 14, 2017

@est31 Absolutely! I don't know of anyone chomping at the bit to do this implementation work.

@nikomatsakis
Copy link
Contributor

@eddyb I am not so sure how I feel about using this coercion. In particular, as we discussed in the RFC thread itself, it would imply that the closure from which we coerce is a "zero-capture" closure -- well, I guess we can actually check that eagerly easily enough, since the list of upvars that a closure uses is something we analyze very early. (i.e., the freevars list is available quite early.)

@eddyb
Copy link
Member

eddyb commented Feb 16, 2017

@nikomatsakis I've assumed we use that early check all along ;).

@nikomatsakis
Copy link
Contributor

@eddyb well as usual you're two steps ahead of me. =) Have pity on an old man.

That said, do we want to allow said coercion is you only capture zero-sized types? I guess I'm happy not to permit that for now. =)

eddyb added a commit to eddyb/rust that referenced this issue Feb 25, 2017
Implement non-capturing closure to fn coercion

Implements non capturing closure coercion ([RFC 1558](https://github.com/rust-lang/rfcs/blob/master/text/1558-closure-to-fn-coercion.md)).

cc tracking issue rust-lang#39817
eddyb added a commit to eddyb/rust that referenced this issue Feb 25, 2017
Implement non-capturing closure to fn coercion

Implements non capturing closure coercion ([RFC 1558](https://github.com/rust-lang/rfcs/blob/master/text/1558-closure-to-fn-coercion.md)).

cc tracking issue rust-lang#39817
@brson brson added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Mar 1, 2017
@brson
Copy link
Contributor

brson commented May 3, 2017

cc @rust-lang/lang Is this ready for FCP? Can it get in for 1.19?

@withoutboats
Copy link
Contributor

It seems to be fully implemented, but I don't know if anyone has actually had any experience using it. Any users who have, it would be great to hear from you.

I don't have any objection to stabilizing in principle, don't know about anyone else. I'd like to know that someone has used it and not had any ICEs or anything though.

@leoyvens
Copy link
Contributor

leoyvens commented May 3, 2017

Use case I found for this:
Implementing a trait for any T: Fn() + 'static can cause coherence issues, it's sufficient for my use case to implement it for fn(). However that means it's not implemented for closures which sucks for ergonomics. Haven't had much hands on experience with the feature though.

@est31
Copy link
Member

est31 commented May 3, 2017

I don't know if anyone has actually had any experience using it

this gives a list of people using the feature on github. Maybe ask them?

I personally was (positively) surprised that the Rust ABI for having () as the type of an argument is the same ABI as not having that argument. We are relying on that fact in the implementation, but most likely when that changes it will be noticed soon enough.

@archshift
Copy link
Contributor

archshift commented May 3, 2017

@withoutboats #40204 (which affects this feature) is yet unresolved.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 4, 2017

One problem I had trying to use this feature was the type inference for the expected type of arguments doesn't work. For example, this code does not compile https://is.gd/FNoLWg:

#![feature(closure_to_fn_coercion)]

fn foo(f: fn(Vec<u32>) -> usize) { }

fn main() {
    foo(|x| x.len())
}

UPDATE: Opened an issue (#41755) with some mentoring instructions.

@briansmith
Copy link
Contributor

briansmith commented May 6, 2017

Any users who have, it would be great to hear from you.

I tried to use thing in ring here: briansmith/ring@44170d4.

However, I got the error:

  --> src\ec\suite_b\ops\p384.rs:69:20
   |
69 |     elem_sqr_mont: |r, a| GFp_p384_elem_mul_mont(r, a, a),
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected unsafe fn, found normal fn
   |
   = note: expected type `unsafe extern "C" fn(*mut u64, *const u64)`
              found type `[closure@src\ec\suite_b\ops\p384.rs:69:20: 69:58]`

The type of elem_sqr_mont is: elem_sqr_mont: unsafe extern fn(r: *mut Limb, a: *const Limb).

The problem is that the implementation doesn't allow closures where unsafe extern "C" fn() is expected.

@est31
Copy link
Member

est31 commented May 6, 2017

Yeah, converting to extern "C" is not really possible right now. Implementing that to a satisfying degree is a bit bigger than the initial implementation. First the ABI of the closure needs to be inferred, and then the code that creates the actual closure function needs to be changed, to use the inferred type. Also, one needs to create an error when you put the closure into a local variable and use it in both extern "C" and normal extern "rust" contexts.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 8, 2017

@est31

First the ABI of the closure needs to be inferred, and then the code that creates the actual closure function needs to be changed, to use the inferred type

I would think we would generate a wrapper for the closure's (internal) function. (Though I guess ideally we'd do this more uniformly or not at all, probably.)

@aturon
Copy link
Member Author

aturon commented May 11, 2017

Now that the outstanding bugs have been resolved, this feature is ready at least for consideration for stabilization.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented May 11, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

@brson
Copy link
Contributor

brson commented May 22, 2017

2 weeks to get this in for 1.19

@est31
Copy link
Member

est31 commented May 22, 2017

2 weeks to get this in for 1.19

If we want it in until then, this can become tight.

In the optimal case, people can already prepare PRs to the respective repositories for the neccessary steps required for stabilisation, like documentation.

Also, as the time is pressing, it would be nice to have contributors for this who can devote enough time to get the PRs through review in a timely manner. All of this of course only if we actually want this feature in 1.19.

@est31
Copy link
Member

est31 commented May 23, 2017

So I've opened PRs to document and stabilize the feature, to hopefully speed up the process around those.

@steveklabnik do you think the book requires documentation of this feature before stabilization?

@rfcbot
Copy link

rfcbot commented May 23, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 23, 2017
@steveklabnik
Copy link
Member

@est31 I think this is a bit in-the-weeds for the book, so I'm leaning towards no. @carols10cents ?

@carols10cents
Copy link
Member

@est31 I think this is a bit in-the-weeds for the book, so I'm leaning towards no. @carols10cents ?

Agreed, I think the reference and the API docs (if those are relevant?) is more appropriate.

@est31
Copy link
Member

est31 commented May 26, 2017

API docs (if those are relevant?)

I'm not aware of any examples in the documentation that could be written more concisely with this feature present, nor do I know of an easy way to spot them if there are any. When replacing the try! macro with ?, the move was much easier as you could simply grep for it.

There is also (to my knowledge) no api page for the fn pointer type (like there is for f32, i16, etc), so I can't document that either.

So I'd say API docs would require no updating.

@briansmith
Copy link
Contributor

@est31

First the ABI of the closure needs to be inferred, and then the code that creates the actual closure function needs to be changed, to use the inferred type

I would think we would generate a wrapper for the closure's (internal) function. (Though I guess ideally we'd do this more uniformly or not at all, probably.)

IIUC, that would not "zero cost" and so it would break the language design principles. It would be zero-cost if the closure's internal function were always inlined into the wrapper, I guess. But then that's the same as not having a wrapper, right?

@eddyb
Copy link
Member

eddyb commented May 27, 2017

@briansmith The ABI could be part of monomorphization such that the closure isn't translated until it's either used with a Fn trait or coerced to an fn pointer and the latter would choose the ABI of the closure body itself.

@michaelwoerister would be able to say if the trans item collector could reasonably do this.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue May 27, 2017
Stabilize non capturing closure to fn coercion

Stabilisation PR for non capturing closure to fn coercion.

closes rust-lang#39817
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue May 27, 2017
Stabilize non capturing closure to fn coercion

Stabilisation PR for non capturing closure to fn coercion.

closes rust-lang#39817
@bors bors closed this as completed in 5d2512e May 28, 2017
@joshtriplett
Copy link
Member

I filed #44291 for the issue of coercing to extern fn. cc @est31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.