lint on core::ffi::c_void as a return type

[euclio]

View all comments

Fixes #100972.

This PR introduces a new deny-by-default warn-by-default lint c_void_returns that fires on usage of core::ffi::c_void as a return type. This is never correct, and is a potential stumbling block for users coming from C.

[rustbot]

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 18 candidates

[Kivooeo]

why is deny by default? what about things like bindgen that generates bindgings from c code?

[asquared31415]

You're never meant to have a value of type c_void, it's only meant to be used in raw pointers, as an equivalent to void*. () is the equivalent to returning void.

in fact I expect that UB arises from the internal implementation details if you have a void meow() and call it as fn meow() -> c_void because the rust c_void type actually is not zero sized for Internal Reasons.

[Kivooeo]

I'd suggest warn rather than deny by default for a few reasons:

• Using c_void as a return type in extern blocks is extremely common pattern in existing FFI code and bindgen output
• Even outside extern blocks, there are plenty cases show real-world code that would break, and while the pattern is incorrect, making it a hard error seems like a high bar for ecosystem disruption
• warn would still surface the issue without being a breaking change. In a future edition it could be escalated to deny, but only for functions outside extern blocks, the cascading wrapper case shows that blindly denying it everywhere creates unnecessary churn in FFI-heavy codebases

[asquared31415]

I believe that every single one of those cases that are defining a -> c_void { ... } function are relying on internal details and so it's not currently UB, but could be in the future.

All the cases that are declaring a -> c_void; function are currently UB (and will almost certainly always remain UB) to link to a -> () (or C -> void), and on top of that, this UB cannot be detected in most of those cases, because miri won't support the typical use cases. I believe that a deny by default lint here is more than warranted.

Additionally, the c_void docs explicitly say "this is not the same as C’s void return type, which is Rust’s () type."

here is an example of a case where miri can detect the UB, but this is rarely possible.

[Kivooeo]

As I noted earlier, I agree this pattern is incorrect. My concern is primarily about ecosystem impact - a GitHub search returns over 1,000 instances of this pattern, including in production codebases from organizations like Microsoft and Azure.

Making this deny by default would break these codebases on the next compiler update with no warning period. warn by default would still surface the issue while giving the ecosystem time to adapt.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[joshtriplett]

Looks reasonable. We'd like to start with warn at first, and ramp up to deny after a couple of releases.

@rfcbot merge lang

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Thanks @euclio.

@rfcbot reviewed

[tmandry]

@rfcbot reviewed

[rust-rfcbot]

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

[rust-rfcbot]

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.

[theemathas]

@rustbot reroll

[mati865]

Thanks, looks good but needs a rebase.

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[euclio]

@mati865 Rebased.

[mati865]

@bors r+

[rust-bors]

📌 Commit b268dee has been approved by mati865

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.