Change how handles are constructed, so that we can ensure non-null handles

[madsmtm]

A lot of consumers of raw-window-handle assume that the pointers are non-null, which makes those libraries unsound. Since the traits are fallible nowadays, we should try to use the type-system to ensure that the handles are always completely valid, instead of expecting user to check it themselves.

This could be done by using Option<NonNull<c_void>>, but that still needs the user to unwrap what should always be set anyhow, so I opted for renaming the empty methods to new, and taking parameters to these where it makes sense. Note that the structs are still left open to non-breaking extensions, as they are still marked #[non_exhaustive] - with the caveat that those extensions must be optional / have a sensible default.

See #33 and #55 for a bit of previous discussion on this.

[kchibisov]

I understand the motivation behind that and I agree that it's sort of useful to do so. Certain data is mandatory for a handle to be valid and useful in the first place.

I'm just not sure how fragile it could be in the wild. Some pointers could be nullable, like display on X11, because users could try to get some sort of default.

[madsmtm]

Yeah, I definitely need everybody's guidance on where a null pointer does or does not make sense (e.g. a null pointer NSView is definitely nonsense, there's nothing you can feasibly fall back to, but for others it may).

[notgull]

By the way, it's generally a bad idea to just "open" a new X11 display if you can't get a current one. wgpu just encountered a bug because of that, though I forget the issue number. (Edit: found it, gfx-rs/wgpu#3813)

[kchibisov]

By the way, it's generally a bad idea to just "open" a new X11 display if you can't get a current one. wgpu just encountered a bug because of that, though I forget the issue number.

You can create a glx display without a present display, just saying, the same with EGL, you can use EGL_DEFAULT_DISPLAY and it's a valid thing to do on X11 and sort of works(at least that's how it was initially iirc).

I think it's a better to use Option<NonNull> and similar instead of raw values or Option<NonZero> the null checks are mandatory in the clients anyway, so not sure why we have them implicit here.

@madsmtm you can set NonNull for all the wayland stuff.

[madsmtm]

I think it's a better to use Option<NonNull> and similar instead of raw values

Agreed, that's also what I've set out to do.

set NonNull for all the wayland stuff.

Done.

You can create a glx display without a present display

So XlibDisplayHandle.display should be Option<NonNull<c_void>>? What about XcbDisplayHandle.connection, should that also be nullable?

[madsmtm]

Tagging @Lokathor, you may remember the previous discussion on this topic better than I

[notgull]

So XlibDisplayHandle.display should be Option<NonNull<c_void>>? What about XcbDisplayHandle.connection, should that also be nullable?

I would personally be against this, for the reasoning above. This API affords itself to be misused.

[madsmtm]

I would personally be against this, for the reasoning above. This API affords itself to be misused.

I am not experienced enough in X11, I'll let you two discuss what the best way forward is.

In any case, Option<NonNull<c_void>> would still be a better choice, so if agreement can't be reached, maybe we can do the conservative option first, and then figure it out in another issue (ideally before 0.6)?

[Lokathor]

I am mildly against this because NonNull is more of a pain in the butt to work with. Using Option<NonNull<T>> has worse ergonomics than just *mut T much of the time, particularly because actual FFI calls are generally bound as using the raw pointer and so you get nn.get() all over the place, because there's no coercion.

However, if everyone else really want to use NonNull I won't block the change.

[kchibisov]

But Option<NonNull> is FFI friendly though, as in you can pass it as is? The same with other Option<NonZero> types.

[kchibisov]

The downstream already must validate before using data, but with nonnull they can pass data as is, because it's just a pointer. And they could even pass pointer, but for example in glutin we check every input we have.

Though, I understand why we have *mut T, it's just easier to work with in some cases.

[Lokathor]

The actual function bindings almost never accept Option

[kchibisov]

Yeah, that's fair, and the cast won't work because it's not trivial, so you have only an option to transmute, I guess.

[Lokathor]

opt.map(NonNull::get).unwrap_or(null()) is quite wordy. You can make a fn for it of course, but still.

And I have been advised that, in general, transmute and pointers should not be anywhere near each other, because in some cases that look fine the provenance can become messed up, and it's just much safer to never mix them.

[kchibisov]

Yeah, I think raw pointers is fine.

Do you have any opinion wrt plain NotNull for an essential data which makes RawWindowHandle and having a way to build a handle with the proposed changes?

[notgull]

opt.map(NonNull::get).unwrap_or(null()) is quite wordy. You can make a fn for it of course, but still.

My opinion is that ensuring greater correctness throughout the raw-window-handle ecosystem would take precedence over the ease of writing FFI. In fact, the wordiness here can be easily avoided with an extension trait.

trait PointerExt<T: ?Sized> {
    fn as_ptr(self) -> *mut T;
}

impl<T: ?Sized> PointerExt<T> for Option<NonNull<T>> {
    fn as_ptr(self) -> *mut T { self.map(NonNull::get).unwrap_or(null()) }
}

I am still in favor of the PR in its current form, as it prevents a footgun that is very prevalent throughout the users of raw-window-handle.

[kchibisov]

Do we want to do anything with XID on X11, zero is not a valid xid if it matters.

[notgull]

Yes, I think we should make XID types non-zero as well, at least for XCB, since the Xlib one relies on std::os::raw.

[madsmtm]

I've moved the NonZero* debate to #137, so all that's remaining here is figuring out whether we should wrap XlibDisplayHandle.display and XcbDisplayHandle.connection in an Option or not.

[madsmtm]

I went with making the handles nullable, since it's the conservative choice, and then we can discuss that in detail in #141 later.

I'll consider this PR ready now, and will merge it in a few days unless someone brings up objections.

[MarijnS95]

Also for posterity: the type was changed from pointer to isize here to match the representation in Windows-rs: microsoft/windows-rs#1643

[MarijnS95]

FYI, win32metadata is changing these back to void pointers it seems:

microsoft/win32metadata#1924
microsoft/win32metadata@b4dfd2f

See also my local regeneration: microsoft/windows-rs@96b9a27

CC @kennykerr, @riverar, @mikebattista.