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

Allow closure to extern fn pointer coercion #61528

Closed
wants to merge 3 commits into from
Closed

Allow closure to extern fn pointer coercion #61528

wants to merge 3 commits into from

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Jun 4, 2019 •
edited
Loading

Analogous to #59580, resolves #44291.

I'm still quite new to rustc and don't know how legit it was to basically just redo the same thing the PR for closure to unsafe fn pointer coercion did with abi instead of unsafety.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2019
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 4, 2019
@jonas-schievink jonas-schievink added this to the 1.37 milestone Jun 4, 2019
@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2019
@Centril
Copy link
Contributor

Centril commented Jun 4, 2019

This is not analogous to #59580 because in that case we had a coercion A => B (closure -> fn) and B => C (fn -> unsafe fn) and so the PR was just the transitive coercion implemented directly. In this case it is a non-transitive coercion from closure -> extern "C" fn.

@cramertj
Copy link
Member

cramertj commented Jun 6, 2019

It's unclear to me exactly what this would do: you might have a closure that is called once in rust and then passed into a function or place which expected a C ABI. Is a C ABI used for both? Are two functions generated? One function and a shim?

Also, how does this interact with the improper C-types lint?

These questions feel pretty easily resolvable to me, but there is design work here-- this isn't an entirely obvious/self-explanatory addition IMO. I'd like to see a (short) RFC for this so that we can more formally specify the behavior of this feature.

@Centril
Copy link
Contributor

Centril commented Jun 6, 2019

r? @cramertj

@rust-highfive rust-highfive assigned cramertj and unassigned oli-obk Jun 6, 2019
@jplatte
Copy link
Contributor Author

jplatte commented Jun 6, 2019

Also, how does this interact with the improper C-types lint?

I actually wanted to add a test case for improper_ctypes but then noticed that extern function definitions don't trigger it, only functions in an extern block do (see #19834). So for now nothing needs to be done to be consistent with free-standing extern functions.

I'd like to see a (short) RFC for this so that we can more formally specify the behavior of this feature.

Okay. I might have time in the next few days to write one, but I'm not sure.

@Centril Centril added needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. and removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Jun 6, 2019
@Centril
Copy link
Contributor

Centril commented Jun 7, 2019 •
edited
Loading

We discussed this on the language team meeting yesterday where we had some questions along the lines outlined by @cramertj in #61528 (comment). We felt that while we can land this PR separately and under a feature gate with an associated tracking issue, it would indeed be good to follow up with an RFC to flesh out the details.

To the end of feature gating, I think it would be a good idea to move all tests introduced and modified into a separate folder under src/test/ui/closure-to-extern-fnptr-coercion/ where the feature gate tests can also reside. That way it is easier to see what we have and what modifications are necessary after an RFC.

@Centril Centril removed this from the 1.37 milestone Jun 7, 2019
@Centril Centril removed the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 7, 2019
@bors
Copy link
Contributor

bors commented Jun 10, 2019

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

@jplatte
Copy link
Contributor Author

jplatte commented Jun 11, 2019

Rebased and moved the tests. I didn't try feature-gating this yet, will do so when I find the time.

@Centril Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 13, 2019
@jplatte
closure_to_fn_coercion: Define feature gate
f1716a6 
and add a corresponding test. It still fails because the feature gate is
not actually implemented.

[ci skip]
@bors
Copy link
Contributor

bors commented Jun 25, 2019

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

eddyb
eddyb reviewed Jun 26, 2019
View reviewed changes
src/librustc_codegen_ssa/mir/rvalue.rs
@@ -192,7 +192,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
}
}
mir::CastKind::Pointer(PointerCast::ClosureFnPointer(_)) => {
mir::CastKind::Pointer(PointerCast::ClosureFnPointer(_, _)) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, it needs a shim to work correctly. I think we should consider having a coercion from a Rust function item (not a fn pointer) to an extern "..." fn pointer.

Then the closure coercion would still be "like coercing <typeof(|| ...) as FnOnce<_>>::call_once, modulo first argument", and they would both have the extra capability of switching ABI.

@jplatte
Copy link
Contributor Author

jplatte commented Jun 26, 2019 •
edited
Loading

Given @eddyb's review, this looks like it is not as straight-forward as I had hoped. This feature is more of a nice-to-have for me rather than something I'd like to invest significant time into, so I'd be happy if somebody else took over. Otherwise, I might still come back to this some time in the future. This PR could probably be tagged E-help-wanted.

@joelpalmer
Copy link

Hi @jplatte this is a ping from Triage. We will go ahead and close this due to inactivity. Please re-open before pushing changes. Thanks for the PR.

@rustbot modify labels to +S-inactive-closed, -S-waiting-on-author

@rustbot rustbot added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 22, 2019
@Dylan-DPC-zz
Copy link

@jplatte thanks for the comment but we don't tag PRs with "E-help-wanted". Closing this, and will leave a comment on the main issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb left review comments

Assignees

@cramertj cramertj

Labels
needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support coercing non-capturing closures to extern function pointers