Skip to content

/ rust Public

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

Jump to bottom

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

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
+69 −45

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
Collaborator

@rust-highfive rust-highfive commented Apr 17, 2021

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@tspiteri
Copy link
Contributor

@tspiteri tspiteri commented Apr 18, 2021

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

@bors
Copy link
Contributor

@bors bors commented Jun 5, 2021

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

dtolnay added 2 commits Jun 5, 2021
@dtolnay
Add improper ctypes test for ptr to unit, ptr to tuple
c17e3e2
@dtolnay
Make *const (), *mut () okay for FFI
abfad74
@wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Sep 2, 2021

r? rust-lang/compiler-team

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

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

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Sep 6, 2021

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 dtolnay commented Sep 7, 2021 •
edited

@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

@joshtriplett joshtriplett commented Sep 7, 2021

@dtolnay That's an interesting point. I was thinking of extern "C", where I think c_void should always be the right answer. But for opaque pointers within Rust, I can see the argument for ().

I'm wondering if it would be appropriate to provide a lint about it for extern "C" FFI, to steer people towards c_void. But I don't have any objection to using it for Rust FFI.

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Sep 7, 2021

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

@rfcbot rfcbot commented Sep 7, 2021 •
edited by cramertj

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.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 9, 2021

@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

@joshtriplett joshtriplett commented Sep 9, 2021

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

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 10, 2021

@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

@scottmcm scottmcm commented Sep 10, 2021

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

@rfcbot
Copy link

@rfcbot rfcbot commented Sep 14, 2021

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

@rfcbot
Copy link

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

@nagisa
Copy link
Member

@nagisa nagisa commented Oct 2, 2021

@bors r+

@bors
Copy link
Contributor

@bors bors commented Oct 2, 2021

📌 Commit abfad74 has been approved by nagisa

@bors
Copy link
Contributor

@bors bors commented Oct 3, 2021

⌛ Testing commit abfad74 with merge c70b35e...

@bors
Copy link
Contributor

@bors bors commented Oct 3, 2021

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

@bors bors merged commit c70b35e into rust-lang:master Oct 3, 2021
11 checks passed
@rustbot rustbot added this to the 1.57.0 milestone Oct 3, 2021
@dtolnay dtolnay deleted the ptrunit branch Oct 3, 2021
@rust-timer
Copy link
Collaborator

@rust-timer rust-timer commented Oct 3, 2021

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

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

@nagisa nagisa

Labels
A-ffi A-raw-pointers disposition-merge finished-final-comment-period merged-by-bors S-waiting-on-bors S-waiting-on-fcp T-lang
Projects
None yet
Milestone
1.57.0
Linked issues

Successfully merging this pull request may close these issues.

None yet