Tracking issue for RFC 1558: Allow coercing non-capturing closures to function pointers

[aturon]

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:

• [x] https://github.com/rust-lang/rust/issues/41755 -- inference shortcomings
• [x] https://github.com/rust-lang/rust/issues/40204 -- ICE

[est31]

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

[aturon]

@est31 Awesome, thank you!

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

[eddyb]

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]

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]

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

[nikomatsakis]

@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]

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

[nikomatsakis]

@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. =)

[brson]

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

[withoutboats]

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]

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]

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]

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

[nikomatsakis]

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 (https://github.com/rust-lang/rust/issues/41755) with some mentoring instructions.

[briansmith]

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

I tried to use thing in ring here: https://github.com/briansmith/ring/commit/44170d4936b477d2de6135e1ab668a881d8d6943.

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]

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]

@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]

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

@rfcbot fcp merge

[rfcbot]

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

• [x] @aturon
• [x] @eddyb
• [x] @nikomatsakis
• [x] @nrc
• [x] @pnkfelix
• [x] @withoutboats

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]

2 weeks to get this in for 1.19

[est31]

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]

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]

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

[steveklabnik]

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

[carols10cents]

@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]

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]

@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]

@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.

[joshtriplett]

I filed https://github.com/rust-lang/rust/issues/44291 for the issue of coercing to extern fn. cc @est31