After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 660809 - document that if you fail a precondition check, documented guarantees do not apply
document that if you fail a precondition check, documented guarantees do not ...
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2011-10-03 18:17 UTC by Simon McVittie
Modified: 2014-02-11 15:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Be more clear that g_return_if_fail is undefined behaviour (4.43 KB, patch)
2013-06-02 20:58 UTC, Simon McVittie
reviewed Details | Review
Be more clear that g_return_if_fail is undefined behaviour (4.56 KB, patch)
2014-02-06 10:23 UTC, Simon McVittie
committed Details | Review

Description Simon McVittie 2011-10-03 18:17:37 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.
Comment 1 Simon McVittie 2011-10-04 10:13:30 UTC
See also Bug #580873.
Comment 2 Simon McVittie 2011-12-13 19:39:56 UTC
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.
Comment 3 Allison Karlitskaya (desrt) 2011-12-13 19:54:55 UTC
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.
Comment 4 Matthias Clasen 2011-12-13 23:10:46 UTC
> 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.
Comment 5 Simon McVittie 2013-06-02 20:58:50 UTC
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?
Comment 6 Colin Walters 2013-06-02 21:38:06 UTC
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.
Comment 7 Simon McVittie 2013-08-08 11:17:39 UTC
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.
Comment 8 Simon McVittie 2013-08-08 11:19:07 UTC
Ugh, sorry, I meant that comment to be on Bug #705629. *more coffee*
Comment 9 Simon McVittie 2014-02-06 10:23:33 UTC
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.
Comment 10 Emmanuele Bassi (:ebassi) 2014-02-06 11:20:56 UTC
Review of attachment 268271 [details] [review]:

looks good to me.
Comment 11 Simon McVittie 2014-02-11 15:38:24 UTC
Review of attachment 268271 [details] [review]:

> looks good to me.

commit a3cb5ce3, should be in 2.39.5