Skip to content

Make *const (), *mut () okay for FFI #84267

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Oct 3, 2021
Merged

Make *const (), *mut () okay for FFI #84267

merged 2 commits into from
Oct 3, 2021

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Apr 17, 2021

Pointer-to-() is used occasionally in the standard library to mean "pointer to none-of-your-business". Examples:

I believe it's useful for the same purpose in FFI signatures, even while () itself is not FFI safe. The following should be allowed:

extern "C" {
    fn demo(pc: *const (), pm: *mut ());
}

Prior to this PR, those pointers were not considered okay for an extern signature.

warning: `extern` block uses type `()`, which is not FFI-safe
 --> src/main.rs:2:17
  |
2 |     fn demo(pc: *const (), pm: *mut ());
  |                 ^^^^^^^^^ not FFI-safe
  |
  = note: `#[warn(improper_ctypes)]` on by default
  = help: consider using a struct instead
  = note: tuples have unspecified layout

warning: `extern` block uses type `()`, which is not FFI-safe
 --> src/main.rs:2:32
  |
2 |     fn demo(pc: *const (), pm: *mut ());
  |                                ^^^^^^^ not FFI-safe
  |
  = help: consider using a struct instead
  = note: tuples have unspecified layout

@rust-highfive
Copy link
Contributor

r? @matthewjasper

(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 Apr 17, 2021
@tspiteri
Copy link
Contributor

Would there be any difference between *const () and *const c_void in FFI?

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2021
@bors
Copy link
Collaborator

bors commented Jun 5, 2021

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

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@camelid camelid added A-FFI Area: Foreign function interface (FFI) A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Jul 29, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@wesleywiser
Copy link
Member

r? rust-lang/compiler-team

@rust-highfive rust-highfive assigned nagisa and unassigned matthewjasper Sep 2, 2021
@nagisa
Copy link
Member

nagisa commented Sep 5, 2021

I believe this is something @rust-lang/lang should take a look at before this is merged. Implementation-wise this LGTM.

@nagisa nagisa added T-lang Relevant to the language team I-nominated and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 5, 2021
@joshtriplett
Copy link
Member

While there's no fundamental reason this couldn't work, we already have c_void for this, and it's useful to unify around one such type rather than two. (Hence the "voidpocalypse" some time ago.)

For that reason, I don't think we should do this.

As an alternative, perhaps we could emit a structured suggestion if we see those types used in FFI, suggesting the use of c_void instead?

@dtolnay
Copy link
Member Author

dtolnay commented Sep 7, 2021

@joshtriplett in pure Rust to Rust FFI (where C is not involved), c_void doesn't seem appropriate. Rust code idiomatically uses ptr to () as the unspecified pointer type, as seen in RawWakerVTable and to_raw_parts in the standard library. I think that c_void should only come up when talking to C.

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/lang meeting.

I would still like to see a lint on extern "C" usage of pointers to () that points to c_void instead. And from a different perspective, several people (myself included) felt like it'd be fine to allow pointers to T in general.

But for now, this seems like a good change, and a conservative one:

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Sep 7, 2021

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 7, 2021
@nikomatsakis
Copy link
Contributor

@bors reviewed

This seems to be setting a kind of precedent that the "C ABI" should not warn about things that are reasonable but which may not be idiomatic (and that the latter would be a distinct lint). Does that seem correct?

@joshtriplett
Copy link
Member

"reasonable but not idiomatic" seems like exactly the case for which we should accept the code but emit a lint.

@nikomatsakis
Copy link
Contributor

@joshtriplett well, this is ultimately a lint no matter what, right? So the question is more like: what is the domain of this lint?

@scottmcm
Copy link
Member

Hmm, *const () is arguably just temporary while waiting for extern types (#43467), right? Is that something we can push on -- at least as a canonical one in core -- now that Thin exists? Or make some kind of #[repr(transparent)] struct OpaquePtr(*const ()); to suggest in this case?

As for this specifically, I dunno. I could see both "just use c_void, since it's in core and easy" or "well, if there's not the best option available yet then it shouldn't be linting for () until we can have a better suggestion".

@nagisa nagisa added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2021
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Sep 14, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 14, 2021

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 14, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 24, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Sep 24, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Sep 24, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 30, 2021
@nagisa
Copy link
Member

nagisa commented Oct 2, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 2, 2021

📌 Commit abfad74 has been approved by nagisa

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 2, 2021
@bors
Copy link
Collaborator

bors commented Oct 3, 2021

⌛ Testing commit abfad74 with merge c70b35e...

@bors
Copy link
Collaborator

bors commented Oct 3, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing c70b35e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 3, 2021
@bors bors merged commit c70b35e into rust-lang:master Oct 3, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 3, 2021
@dtolnay dtolnay deleted the ptrunit branch October 3, 2021 03:29
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c70b35e): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@dtolnay dtolnay removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. labels Jan 14, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-FFI Area: Foreign function interface (FFI) A-raw-pointers Area: raw pointers, MaybeUninit, NonNull disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. T-lang Relevant to the language team
Projects
None yet
Development

Successfully merging this pull request may close these issues.