[no_mangle_with_rust_abi]: Check for statics with a non #[repr(Rust)] type

[Centri3]

Fixes #11219

changelog: [no_mangle_with_rust_abi]: Check for statics with an unstable ABI

[rustbot]

r? @Alexendoo

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

[Alexendoo]

We could add a check for (non unit) tuples as well as ADTs

[Centri3]

iirc slices (&[T], not [T]) also have an undefined layout too, so we can maybe lint that as well? (Unless that's incorrect, I'm not sure)

[bors]

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

[asquared31415]

I think that the current implementation is correct given a conservative approach to the current unsafe guarantees, but I believe that there are wishes to make more of repr(Rust) defined. If that happens, this lint may need to be updated to be less restrictive.

[Alexendoo]

Yeah sounds good to me, we can adapt this if future guarantees are ever added

r=me with rebase/squash

[Alexendoo]

Oh @asquared31415's comment reminded me of something, Option<&T> and such

[Centri3]

iirc Option<&T> still doesn't have a stable ABI, even if the underlying type is (which, in this case, has a stable ABI). The discriminant can be whatever it wants so I think it should still lint it.

(Unless you meant something other than not linting it, not sure)

[Centri3]

Will rebase later today

[Alexendoo]

https://doc.rust-lang.org/std/primitive.reference.html says

In fact, Option<&T> has the same memory representation as a nullable but aligned pointer, and can be passed across FFI boundaries as such.

[asquared31415]

This is a complete list of the types that have well defined layouts inside Option: https://doc.rust-lang.org/std/option/index.html#representation

[Centri3]

doc.rust-lang.org/std/primitive.reference.html says

In fact, Option<&T> has the same memory representation as a nullable but aligned pointer, and can be passed across FFI boundaries as such.

Odd, the type layout section of the reference should really mention that

While Option<&T> is the same as &T, the underlying type must be repr(C) for it to be FFI safe. I mean, you can pass it around, but you can't easily give it to an external function expecting it. So we'll need to probe a little deeper into Option types to see if T is repr(Rust)

[asquared31415]

This does not need to do any inspection of the type behind one of the pointer-like types because Option<&T> is as FFI-safe as a raw pointer. We are not be linting on *const NotFFISafe, because the inner type of a pointer (when it's Sized) does not affect the ABI, So none of the pointer-like Options need inspection besides "is an Option of one of these pointer types"
(for T: Sized):

• Option<Box<T>>
• Option<&T>
• Option<&mut T>
• Option<fn> (with or without an extern ABI)
• Option<ptr::NonNull<T>>

The only remaining case that has guaranteed layout is the Option<NonZeroX> types, which are represented as plain integers (and don't have types to inspect further, like references).

[Centri3]

Option<&T> is as FFI-safe as a raw pointer.

Yes, the Option<&...> part is safe, but the T is absolutely not. I was not referring to Option<&...> in specific; that's FFI-safe. The underlying type isn't however, and because of that it's basically useless.

You can pass it around safely, but accessing the inner data on its own won't work. Unless all fields are private doing that is a terrible idea.

We are not be linting on *const NotFFISafe

And we probably should with #[no_mangle].

As I said, you can pass the pointer/reference around care-free, but you can't read from it in an external function. You can pass it to a function defined in the crate that defines NotFFISafe, that returns a mutable reference to that field. So it's better than nothing, but not as useful as a repr(C) type.

I don't see any reason to assume the author has gone through that effort if there are public fields. They're likely to just do a null check, then read it and cause a multitude of issues.

(If there are public fields, that's a major footgun anyway for rust code in a cdylib.)

Code in an external language though would be weird because the concept of visibility doesn't carry over, and would need to redefine the type. They're 100% free to read the fields then anyway, even though this wouldn't lint

[asquared31415]

In my experience the "opaque pointer" pattern is fairly common. And while we could say "use a FFI-safe type", that won't satisfy most people. In fact we even have an exception for *const T such that it does not trigger the FFI safety lint in rustc for similar reasons. Especially *const () which is suggested as a opaque pointer type.

[asquared31415]

example of what does and does not lint: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3f5774c16df2d4ef9c564d97fbf2138d

Notice that pointers never lint, even when their pointee does.

[Centri3]

There isn't any issue with using repr(C) on an opaque type. The nomicon even suggests it in fact (is it Rustonomicon or the nomicon?), I'd say you should since it immediately represents it comes from/is sent to C. It's definitely pedantic, since it's never constructed (only theoretically pointed to by a pointer, but it's casted before it's read anyway), but clippy can be more pedantic than rustc by default.

I think it makes sense to not lint this stuff in rustc. It can be correct, but it most of the time isn't (or could be written better). imo, in clippy it'd make sense to lint this since either the code is incorrect or the purpose of the struct can be stated better (both ways are solved by adding repr(C).

Especially *const () which is suggested as a opaque pointer type.

We already don't lint *const () or ().

[Alexendoo]

I think aligning with improper_ctypes would be the best place to start, the pointer doesn't have to be opaque on the rust side to be opaque to C

[Centri3]

For now, yes. I'll update this soon to do so. However, in the future we should have an off-by-default configuration option to make it a bit more pedantic.

[xFrednet]

Hey this is triage, I'm closing this due to inactivity. Currently, @Centri3 sadly doesn't have the time to continue this implementation. If anyone is interested in continuing this PR, you're more than welcome to create a new PR and push it over the finish line. :D

Thank you to @Centri3 and the reviewers for the time, that you already put into this!

@rustbot label +S-inactive-closed -S-waiting-on-author -S-waiting-on-review

[Centri3]

Triaging my old PRs in case someone wants to pick them up, this needs a tiny bit of work. Mostly around opaque pointers, but also I left a TODO for when repr(Rust) is explicitly allowed - it now is, so we should handle it.

In terms of opaque pointers, tbh I don't think *const T will show up enough for it to matter. By that I mean, we can just ignore it like the other lint does.

I feel like another lint is still in order for this case since opaque pointers feel... weird, considering you can always define another unit struct. Generally rust prefers explicit over implicit, does it not? So I think that makes sense.

I will also finish this at some point if nobody else does, since I do quite like this lint.