Fix undefined behaviour.

[Diggsey]

Fixes #2

[RalfJung]

Isn't here any way in which you could depend on memoffset as opposed to copying it? Duplicating this kind of code is a real bad idea -- with any new compiler version, it might break (when "spec UB" turns into "real UB"), and I will maintain memoffset to work around such cases, but I won't do this for more than one crate.

Does FieldOffset::new have to be public? The examples I saw all just use the macro anyway, and then that could just internally call memoffset::offset_of! and turn the result into a FieldOffset.

[Diggsey]

Isn't here any way in which you could depend on memoffset as opposed to copying it? Duplicating this kind of code is a real bad idea -- with any new compiler version, it might break (when "spec UB" turns into "real UB"), and I will maintain memoffset to work around such cases, but I won't do this for more than one crate.

I'd rather not pull in another crate for this - it's supposed to be dependency-free so that there is little cost to using it.

In addition, this crate exposes a safe API whose safety is intricately linked with the acceptable inputs to the offset_of! macro, and I'd rather that contract didn't cross a crate boundary. (eg. let's say the memoffset crate expands on the way offset_of can be used to support calculating the offset of fields in enum variants).

[Diggsey]

Does FieldOffset::new have to be public? The examples I saw all just use the macro anyway, and then that could just internally call memoffset::offset_of! and turn the result into a FieldOffset.

It does have to be public, but it could be #[doc(hidden)]...

[RalfJung]

(eg. let's say the memoffset crate expands on the way offset_of can be used to support calculating the offset of fields in enum variants).

You could verify on your end that this is really a struct field (basically copying just this line).

[RalfJung]

I'd rather not pull in another crate for this - it's supposed to be dependency-free so that there is little cost to using it.

There's also a big cost to having to maintain more spots that exploit internal compiler knowledge. This code here, just like the code in memoffset, is wrong code. Centralizing all copies of the wrongness in one place is a big maintenance advantage.

I find it silly to count "cost" purely as "number of dependencies". The cost of this crate in terms of maintenance goes up if you copy-paste stuff.

[Diggsey]

I find it silly to count "cost" purely as "number of dependencies". The cost of this crate in terms of maintenance goes up if you copy-paste stuff.

I mean cost to users of the crate. There's a reason NPM gets a lot of hate.

Anyway, putting that aside, I've thought about this some more.

There are two pieces of "tricky unsafe code" to get right here:

1. Creating a valid pointer to an undef value.
2. Creating a raw pointer to part of an undef value addressed by a base pointer

When I wrote this crate I specifically wanted users to be able to create field offsets for more complex cases without users having to worry about (1). (For example, maybe a user want to get a field offset to an element of a fixed size array, something not possible with the macro). The intent was that they could just focus on being correct WRT (2) and (1) would be handled for them, and this is why that constructor was public.

Unfortunately it has turned out that (2) is actually harder than (1), but I think there is still value in only having to worry about (2) and in the future it will get easier.

If there was a crate which implemented this macro: raw_address_of!((*ptr_to_undef).bar) and which guaranteed in the future to compile down to the MIR operator your RFC introduced, whilst for the moment making a "best effort" attempt, then I would be happy to use that to implement (2) in this crate. For (1) I don't have to worry about it breaking because MaybeUninit is already stable, and the composite operation of (1+2) is not what I want to expose from this crate.

[RalfJung]

When I wrote this crate I specifically wanted users to be able to create field offsets for more complex cases without users having to worry about (1). (For example, maybe a user want to get a field offset to an element of a fixed size array, something not possible with the macro).

The issue here is Deref coercion protection, but I suppose you are putting that burden on the user? The macro used to support that but we removed support as we couldn't figure out a way to guard against Deref.

Unfortunately it has turned out that (2) is actually harder than (1), but I think there is still value in only having to worry about (2) and in the future it will get easier.

Fair.

If there was a crate which implemented this macro: raw_address_of!((*ptr_to_undef).bar)

Hm, that is an interesting idea. Though that's a micro-crate if I've ever seen one. ;) Do you think it would make sense to add that to memoffset or have a dedicated crate?

[Diggsey]

Eh, didn't mean to leave this sitting around so long. Going to merge this as I don't think further discussions will result in me changing the public API, even if I decide to change the implementation.

[RalfJung]

@Diggsey do you think it would make sense to yank old versions of this crate that predate this UB fix? That way, cargo audit will warn people that they depend on a yanked crate, making sure they upgrade.