Fixes the is_null method in PtrExt for fat pointers.

[SergioBenitez]

The current implementation of is_null attempts to cast 0 to the
pointer type of *const T or *mut T for any T. This is incorrect when the
pointer type is not the size of some integer, as is the case for *const [T]
and *mut [T], and leads to an ICE.

This commit fixes this issue by looking at the first pointer size integer of
the pointer itself which in fat pointers is a pointer to the location of the data
and in regular pointers is the pointer itself.

The following program can be used to verify the ICE and the fix:

fn main() {
    let s = &[1, 2, 3] as *const [u32];
    println!("{}", s.is_null());
}

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[alexcrichton]

This may not quite be the implementation strategy we'd like to take, for example this does not handle if the pointer/vtable fields are rearranged (e.g. it doesn't transmute to raw::TraitObject). I think that fixing the ICE in this case may be the best course of action.

[SergioBenitez]

I agree that fixing the ICE is ultimately the best course of action. and I agree that field rearrangement could cause issues. But, even if the fields were rearranged in a trait object pointer, I would expect that if either of the pointer fields were null, then the trait object pointer itself would be considered null. Thus, this strategy would still work.

Also, is there a good reason why the data pointer would not be the first field of a fat pointer struct? It makes a lot of sense given optimizations; not having the data pointer be the first field would mean that most data dereferences of a fat pointer would need to first perform an arithmetic operation to locate the data pointer field.

To my knowledge this patch is correct given the current implementation and near to far future plans. Progress on fixing the ICE has not been quick, and blocking this commit would mean that is_null remains broken.

[alexcrichton]

Yes I understand that the implementation today has been chosen and is probably the right choice, but we want to avoid as many mem::transmute calls (or equivalents) as much as possible. Assumptions like this lead to notoriously difficult-to-track-down bugs in the future which can be easily prevented by fixing it up at this stage instead of later.

Progress on fixing the ICE has not been quick, and blocking this commit would mean that is_null remains broken.

It has only recently becomes possible to call this method on a T: ?Sized, another possible fix would be to add T: Sized for now with a FIXME. We should always be able to relax the bound later on backwards compatibly.

[SergioBenitez]

How does changing the current implementation of {Mut}PtrExt to be defined only for T: Sized, and in addition, adding an implementation for *const/mut [T], sound?

[alexcrichton]

The change would break the method for *const [T], I am proposing something like:

fn is_null(&self) -> bool where T: Sized {
    *self == 0 as *const T
}

[SergioBenitez]

How would that break the method? I'm suggesting adding an implementation specifically for *const/mut [T]:

impl<T> PtrExt for *const [T]

[alexcrichton]

We discussed this a bit on IRC, but I don't think that we should be adding an impl of PtrExt for all unsized T. Other unsized types include str, all trait objects, Path, OsStr, etc, and we don't want to add an impl for all of them.

In the long run this will likely need some form of compiler intrinsic to extract the actual pointer from *const T regardless of whether T is sized or not.

[bluss]

This is "6.1.3.2.3 Behavior considered undefined" because we put a null pointer inside a &[T]. It's just a test case, so I guess it can do no harm. I don't know what I want to say more than this is UB and rustc doesn't have to compile it into a sane test case if it doesn't want to.

[bors]

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

[alexcrichton]

Closing due to inactivity, and in the meantime it looks like these methods have re-grown the where T: Sized bounds to cover up this ICE.