lint on more incorrect usages of core::ffi::c_void

[Jules-Bertholet]

View all comments

core::ffi::c_void has size 1 and a highly non-trivial validity invariant. It should only ever be used behind a raw pointer. However, people mess this up all the time.

• c_void_references: Lint against & or &mut references to c_void. This is a widespread mistake, e.g. here in Miri, here across GitHub.
• c_void_statics: Lint against statics and consts of type c_void. Examples of this mistake across GitHub
• c_void_parameters: Lint against usage of c_void as a function parameter type. Examples of this mistake across GitHub
• c_void_values: Lint against usage of c_void by value in MIR.

Also fixes false negatives in the pre-exisiting c_void_returns. Like that lint, these new lints are warn-by-default, but should eventually be upgraded to deny.

See also #159935

@rustbot label T-lang needs-fcp A-FFI

[rustbot]

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
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 74 candidates
• Random selection from 17 candidates

[RalfJung]

The lint should probably also complain about statics of type c_void.

[Jules-Bertholet]

The lint should probably also complain about statics of type c_void.

Added another lint for this

[Jules-Bertholet]

Added a third lint, against c_void function parameters.

[theemathas]

Idea (not sure if this is feasible): If we do anything that asks for the size of c_void, emit a warning unless it's for a pointer offset operation

[mejrs]

Idea (not sure if this is feasible): If we do anything that asks for the size of c_void, emit a warning unless it's for a pointer offset operation

We can emit a warning regardless, with a note to just use byte_add for offset operations. (Is that what people are using size_of for..?)

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[Jules-Bertholet]

Added a fourth lint to check for incorrect usage of c_void in MIR.

[tgross35]

Perhaps also:

let x: c_void;

View changes since the review

[tgross35]

Apologies if missed, but do any of these catch patterns like struct S(core::ffi::c_void);? This is rejected by C compilers.

[Jules-Bertholet]

Apologies if missed, but do any of these catch patterns like struct S(core::ffi::c_void);? This is rejected by C compilers.

No. For something like a #[repr(transparent)] pub struct Foo(c_void); that is only used behind a raw pointer, I'm not confident we want to lint, so I decided to start with the most clearly wrong stuff first. But if the lang team is fine with it, sure we can lint on that too

[tgross35]

Assuming this needs lang to take a look and that it is about ready for it:

@rustbot label +I-lang-nominated

[Jules-Bertholet]

One thing I would like lang's input on: should we merge the lints? We could have a single lint for all incorrect c_void usage, or one lint for by-ref and one for by-value. Note that the pre-existing c_void_returns hits stable in 3 weeks.

[tgross35]

My 2cents: all of these (including c_void_returns) should be a single improper_c_void lint. A lot of the value of splitting lints is giving users the ability to toggle a single lint on or off without affecting the others; however, since there aren't any valid uses of the code this lints against, I don't think there's any point. (Assuming no single lint is more prone to false positives than the others.)

One counterpoint is that maybe we'd want to deny-by-default everything except c_void_returns in the near future, deferring that one since it probably has the most use. I still think we could do without, though.

[mejrs]

And all of them are basically different variants of "this c_void isn't behind a raw pointer". it makes most sense to me to just have a single lint for that.

[asquared31415]

I could see a world in which for "practical reasons" someone might want to ignore the -> c_void case specifically, but it's a weak argument. I think that a general invalid_c_void lint would likely suffice.

[rust-bors]

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