Require opting in to repr-less types?

[joshlf]

Some of our derives support types without reprs because they're not required for soundness. This can be useful if you don't care about layout stability (and perhaps you care about allowing the compiler to make layout optimizations), but it's a big footgun: it makes it easy to accidentally write code which depends upon type layout without realizing it.

Perhaps we should require users who wish to not use reprs to opt-in to this behavior; something like:

#[derive(FromZeroes, FromBytes)]
#[allow(zerocopy::missing_repr)] // Is this legal? Maybe we need the syntax below instead.
struct Foo;

#[derive(FromZeroes, FromBytes)]
#[zerocopy(allow(missing_repr))]
struct Bar;

This would be a semver-breaking change, but a minor one, and our error message could suggest the allow to steer users in the right direction.

Note one important subtlety: On its own, this isn't sufficient: if a struct contains another type with a non-stable representation, then even if the outer struct is, e.g., repr(C), it isn't sufficient to guarantee the stability of the outer type's layout.

It seems unlikely, but perhaps we could also have zerocopy-derive emit a warning that users can allow using rustc's built-in machinery. According to this documentation, warnings are not currently supported except on nightly, so this may be a non-starter.

[jswrenn]

The goal of this feature is to ensure that customers using zerocopy to serialize and deserialize values are doing so consistently. But to achieve this, a StableRepr trait would need to be remarkably pedantic — the alignments of Rust's numeric primitives are unspecified, and changes to their alignments can easily impact their offsets in compound types.

I'm on the fence about whether this ergonomic tax is justified by its benefits.

[joshlf]

I wonder whether we can just do a more limited analysis - we warn or error if you derive on a type with no repr, and we provide an attribute to silence the warning/error. That's it. No recursive analysis or extra trait. You're still relying on your nested types to do the right thing. It's more like a Clippy lint - "are you sure you meant to do this? Doing this is often just an oversight on the part of the programmer." We'd need to figure out how to design the API to make it clear that this is just a lint and not a guarantee, but I think that'd be a good middle ground.

[jswrenn]

If we're aiming for a lint, perhaps a lint would be best! We could either:

1. propose a clippy lint
2. ship a rust-marker lint of our own

The benefit of a lint is that the diagnostic could be much better and actually explain things like platform-specific alignment.

[joshlf]

Would Clippy accept a lint that depends on knowing about zerocopy?

[joshlf]

Also, FWIW, those approaches would not be particularly discoverable. The advantage of doing it ourselves is that we could warn by default. It's exactly the users who wouldn't think to enable such a lint manually who are likely to make this sort of mistake.

[jswrenn]

Would Clippy accept a lint that depends on knowing about zerocopy?

Specifically for zerocopy? No, but this is a general problem. Even bytemuck doesn't account for layout fluctuations caused by platform-specific alignment.

Also, FWIW, those approaches would not be particularly discoverable.

Our derives can check for the explicit presence/absence of #[allow(<lint name>)]/#[deny(<lint name>)] and require that one of those directives be present.

[joshlf]

Would Clippy accept a lint that depends on knowing about zerocopy?

Specifically for zerocopy? No, but this is a general problem. Even bytemuck doesn't account for layout fluctuations caused by platform-specific alignment.

My concern isn't about #[repr(C)] type which contains a u16 (although that's also something that's possibly worth linting about), but about a type without any repr that derives one of our traits. Clippy would need to know what #[derive(FromBytes)] means in order to know that that's bad.

[jswrenn]

Clippy would need to know what #[derive(FromBytes)] means in order to know that that's bad.

It wouldn't; clippy would only need a general missing_repr lint that's allow-by-default. We can either then hard-code our derives to look for #[allow(clippy::missing_repr)]/#[deny(clippy::missing_repr)], or we can change our expansion to turn the lint on in a local context; something like having this:

#[derive(FromBytes)]
struct Foo(Bar, Baz);

additionally emit:

#[deny(clippy::missing_repr)]
const _: () = {
    struct Foo(Bar, Baz);
};

...which would trigger the clippy lint.

[joshlf]

Oh very clever...

But maybe, instead, we could have it be a rustc lint (also allow-by-default)? If it's a Clippy lint, it would only trigger for users running Clippy.

[jswrenn]

I've started a zulip topic to discuss. A rustc lint would be ideal, but the bar there is higher for getting it totally right on our first go.