GNOME Bugzilla – Bug 660809
document that if you fail a precondition check, documented guarantees do not apply
Last modified: 2014-02-11 15:38:43 UTC
The GError docs say: > By convention, if you return a boolean value indicating success > then TRUE means success and FALSE means failure. If FALSE is > returned, the error *must* be set to a non-NULL value. Developers not familiar with GLib conventions sometimes interpret this as meaning that this common pattern is buggy: > gboolean > my_object_do_stuff (MyObject *self, > const gchar *who_to_blame, > GError **error) > { > g_return_val_if_fail (MY_IS_OBJECT (self), FALSE); > g_return_val_if_fail (g_dbus_is_unique_name (who_to_blame)); > > return my_object_do_stuff_full (self, who_to_blame, 42, error); > } because in the two error cases, it returns FALSE without filling @error. (In some C environments, like CPython (the reference Python implementation), you never do this: you always raise an exception when returning (PyObject *) NULL, even if it's definitely C programmer error.) The GError documentation does also say this: > First and foremost: GError should only be used to report > recoverable runtime errors, never to report programming errors. > If the programmer has screwed up, then you should use g_warning(), > g_return_if_fail(), g_assert(), g_error(), or some similar facility. I've seen this explained quite well (possibly by Havoc), but unfortunately not in the GLib docs themselves. As far as I can remember, the explanation went something like this: --- If you do what the documentation says, the function behaves as documented. If you break the preconditions of the function, then it has undefined behaviour - it could coincidentally work, it could fail, it could segfault, it could make demons fly out of your nose - and it doesn't matter what the documentation says it does, because by breaking the preconditions, you've made the documented behaviour irrelevant. If checks are enabled during compilation, then to be more programmer-friendly, GLib libraries make a "best effort" to have certain bits of undefined behaviour manifest as an assertion message; if checks are disabled, the same things probably manifest as a segfault. Either way, the right fix for the library-user code isn't to catch a recoverable error and try again, it's to avoid calling the function wrong in the first place. Preconditions that are often checked via g_return_if_fail include "pointer isn't NULL", "string is syntactically valid according to some rule", "GObject is of type Foo"; preconditions that aren't feasible for a library to check, and lead to equally undefined results if they fail, include "pointer points into valid memory" and "string is correctly 0-terminated". This has the corollary that if the message in the g_return_val_if_fail() is about the object's internal state and isn't something that the library user could reasonably check for themselves, then the library is probably wrong: for instance, I think it's a bug (albeit a WONTFIX'd one at this point) that dbus-glib criticals when you call methods on a DBusGProxy that has emitted ::destroy, because there is no dbus_g_proxy_is_destroyed() method that the caller could reasonably have used.
See also Bug #580873.
If a GLib maintainer will "officially" confirm that my interpretation in Comment #0 and Bug #580873 is right, I'll try to write a documentation patch based on Comment #0.
Your comments are pretty much exactly equal to the 'official' story, indeed. The one exception is that you should make it clear that returns-FALSE-implies-GError-set is not universally true, even in non-programmer-error cases. We have some (broken) APIs that return FALSE without setting the error. g_key_file_has_key() is a good example.
> The one exception is that you should make it clear that > returns-FALSE-implies-GError-set is not universally true, even in > non-programmer-error cases. We have some (broken) APIs that return FALSE > without setting the error. g_key_file_has_key() is a good example. In general, that is the pattern we want to follow though. Any counterexamples are considered broken and should be explicitly documented as breaking this pattern.
Created attachment 245879 [details] [review] Be more clear that g_return_if_fail is undefined behaviour In particular, it is not incorrect to g_return_if_fail (..., FALSE) in a function returning a "success" gboolean and a GError: "failure to meet the preconditions is an error" takes precedence over the GError documentation's guarantee that the error will be set on failure. --- How's this?
Review of attachment 245879 [details] [review]: ::: glib/gerror.c @@ +337,3 @@ + * Functions that return a boolean value that <emphasis>does not</emphasis> + * indicate success, and also have a #GError ** parameter, are considered + * to be a bad idea; if such functions exist for compatibility reasons, This is an awkward sentence due to the double "that...that" and only finally telling us at the end of the sentence that it's a bad idea. Also we could tell them what to do instead. How about: Avoid creating functions which have a boolean return value and a GError parameter, but where the boolean does something other than signal whether the GError is set. Among other problems, it requires C callers to allocate a temporary error. Instead, provide a "gboolean *" out parameter. There are functions in GLib itself such as g_key_file_has_key() that are deprecated because of this.
Indeed, g_key_file_has_key also says: > Note > > This function does not follow the rules for GError strictly; the > return value both carries meaning and signals an error. To use this > function, you must pass a GError pointer in error, and check whether > it is not NULL to see if an error occurred. That wording might be better than what I suggested.
Ugh, sorry, I meant that comment to be on Bug #705629. *more coffee*
Created attachment 268271 [details] [review] Be more clear that g_return_if_fail is undefined behaviour In particular, it is not incorrect to g_return_if_fail (..., FALSE) in a function returning a "success" gboolean and a GError: "failure to meet the preconditions is an error" takes precedence over the GError documentation's guarantee that the error will be set on failure. --- Rebased onto current master; using Colin's suggested wording for the bit he didn't like in my previous patch.
Review of attachment 268271 [details] [review]: looks good to me.
Review of attachment 268271 [details] [review]: > looks good to me. commit a3cb5ce3, should be in 2.39.5