Improve null pointer handling about gtk3-rs HOT 16 CLOSED

I don't agree, a lot of gtk objects returns NULL even if it's not a fatal error. For example, if you want to open an inexistent file, you'll have a NULL pointer returned. And there are a lot of cases like this. If the object allocation function returns None, it's up to the user to see what and why it didn't work.

from gtk3-rs.

When an Option is None it doesn't imply any sort of error, just that the value requested is unavailable. Having Option everywhere is just noise, IMO.

In cases of the former kind you mentioned, it's pragmatic in Rust to return Result. Even if the return types for these methods were changed to Result<_, ()> until a more sane error type came forward, it would be a massive increase in ergonomics because it would work with the try!() macro, and it would make it obvious that the function can fail.

For many pointer-returning functions, though, the Gnome docs don't say if and/or when they could return null, which I would assume means a null pointer coming from these functions is exceptionally rare, such as an allocation failure. Forcing the user to treat these pointers as nullable might improve safety but at a massive cost to ergonomics and code clarity. And panicking on a null pointer will tell them then and there that something went wrong.

from gtk3-rs.

Basically, we had a Result returning stuff if I remember well. Why did we change ? I'd need to speak about that with @jeremyletang. Personally, I prefer the Option over the Result, but that's just my opinion...

I'm against panicking in libraries, except for the library initialization function (and yet, and I don't really like that). Why ? Because you prevent the developer to handle the library error and doing its own stuff to recover from it. For example, he uses gtk for something but can load dynamically another library to do the same thing if the first one fails. If the first llibrary panicks, he can't do anything.

from gtk3-rs.

Rust is getting limited support for panic handling with thread::catch_panic, and it's already encapsulated with threads. The standard library itself has many instances where panicking is used to catch bugs and halt undefined behavior, such as multiple mutable borrows of RefCell, or joining a scoped thread that panicked, or attempting to index an array out of its bounds, or failure to allocate memory.

Anecdotally, how many times has GTK returned a null pointer from a function that didn't document a case for null pointers? If it's less than, say, 10% of the time, then having the user explicitly handle the error case is not worth the code bloat.

from gtk3-rs.

This discussion lacks specific examples of the offending practice. If you're talking about constructors, it's been established null checking (aside from asserts) isn't necessary and should be removed. There are cases where Option is legitimate. On the other hand, functions that can fail don't all return pointers.

from gtk3-rs.

@gkoz That's what I'm saying. Work towards more logical and informative return types.

In gdk::widgets::Pixbuf:

• new() - returns Option on a null-pointer check, but the docs for gtk_pixbuf_new say nothing about returning null pointers.
• new_from_subpixbuf() - same thing
• get_pixels_with_length() - same thing

No functions for GdkPixbuf state explicitly that they can return null pointers except gdk_pixbuf_get_option(), where NULL makes sense to be converted to Option.

I can find several more examples in gdk::widgets where Option is returned but the underlying function does not document a nullable returned pointer (obviously not exhaustive):

• Cursor::new(), new_for_display()
• Device::get_name()
• Display::get_name()
• Screen::get_system_visual(), get_root_window(), get_display(), make_display_name()
• Visual::get_system(), get_best(), get_screen()

Panicking on a null pointer in these functions would emphasize that Something Exceptionally Bad happened, because they're not supposed to get a null pointer. Expecting users to handle exceptional conditions is not pragmatic to Rust.

from gtk3-rs.

Constructors sure, but converting them wasn't a priority because we're likely to rehash them soon when altering the wrapper architecture.

Other functions - if they aren't expected to return NULL, you're welcome to fix them (in case of char*, you'd simply use FromGlibPtrNotNull::borrow()).

Now there's a question of functions that actually can return NULL, like gtk_button_get_label. Do you find returning Option here incorrect? I guess one can make a case for returning String::new() there instead of None.

Some functions should indeed return Result (and this isn't limited to pointers). I'm not sure that using () for error is the best decision, that can be explored further.

from gtk3-rs.

@gkoz As I've emphasized repeatedly, returning Option for a function that has a documented case of returning null pointers is perfectly fine! That's what Option is there for. Only functions that shouldn't return null pointers should panic.

Using () as an error type would just be a temporary stopgap, basically making it an Option but informing the user that an absence of a value is an error state, instead of implying that it's simply unavailable. Later on, the crate can grow into a sensible error type that reports useful information from GLib/GDK/GTK's error handling.

from gtk3-rs.

Yeah those that have a notion of failure should return Result.

from gtk3-rs.

So, I don't know if anyone has had any thoughts about this recently, but my $0.02 would be along the lines of:

should_not_return_NULL_but_might_because_ffi_craziness() -> T // with the possibility of a panic!
might_validly_return_NULL() -> Option<T>
might_return_GError() -> Result<T, Error>

We could try moving what we have in Error currently to GError or something like that, so we can have Error be a rust-gnome specific thing like:

enum Error {
    SomeRustGnomeError,
    SomeOtherRustGnomeError,
    GError(GError)
}

from gtk3-rs.

I didn't work a lot on Glib so I can't give you a good feedback on that... @gkoz, what do you think of his idea ?

from gtk3-rs.

enum Error {
    SomeRustGnomeError,
    SomeOtherRustGnomeError,
    GError(GError)
}

I'm not sure there'd be other rust-gnome errors beside Unkonwn when the lib doesn't give us any specifics...

There is some structure to the GError types:

Error domains and codes are conventionally named as follows:

• The error domain is called <NAMESPACE>_<MODULE>_ERROR, for example G_SPAWN_ERROR or G_THREAD_ERROR:
  ...
• The error codes are in an enumeration called <Namespace><Module>Error; for example, GThreadError or GSpawnError.
• Members of the error code enumeration are called <NAMESPACE>_<MODULE>_ERROR_<CODE>, for example G_SPAWN_ERROR_FORK or G_THREAD_ERROR_AGAIN.
• If there's a "generic" or "unknown" error code for unrecoverable errors it doesn't make sense to distinguish with specific codes, it should be called <NAMESPACE>_<MODULE>_ERROR_FAILED, for example G_SPAWN_ERROR_FAILED. In the case of error code enumerations that may be extended in future releases, you should generally not handle this error code explicitly, but should instead treat any unrecognized error code as equivalent to FAILED.

So we most likely want to convert GErrors into something more Rustic akin to std::io::Error but there's a complication that other libs and consequently crates want to define their own errors e.g. GdkPixbufError.

from gtk3-rs.

@jashephe aside from the question of how to represent GError best, your suggestion seems alright.

from gtk3-rs.

This may be relevant for error handling design: rust-lang/rust#24793

from gtk3-rs.

I think this has mostly been settled and as far as gdk-pixbuf is concerned the missing piece is adding a PixbufError binding.

from gtk3-rs.

This is solved by gtk-rs/gir#970 to some degree. GLib already makes use of the nullable annotations correctly, GIO will after this MR is in GIO and @sophie-h was looking at the situation in GTK. It just needs to be done one after another.

from gtk3-rs.