GNOME Bugzilla – Bug 660879
Add (nullable) and (optional) annotations
Last modified: 2015-02-07 16:58:22 UTC
This was briefly discussed on IRC so now I am opening a bug in aim of broader discussion. My main motivation for having g-i support for NULL return values comes from using Vala. In Vala, you can declare a method like this: string? foo (); stating that foo might return NULL. This turns out to be helpful because if you then do: bar = foo (); the compiler will warn you about that. The end result is you'll (hopefully) be more concious about what is actually returned (or not) by a given method call. On the other hand, for dynamic languages there is no much use for loading GIR files with information about NULL return values besides (maybe?) documentation purposes. So, given that this feature would be targeted at a very specific audience (at least for the time being) I wanted to ask around if it still might be accepted.
vala "stole" nullable types from C#, so presumably gtk# could make use of it as well. uh, likewise the gtk haskell bindings :)
It's harmless to support I guess. The name is weird when used as a return value, but it also doesn't seem really compelling to add a new (nullable) annotation.
This annotation seems necessary if bindings are to be able to have a different type for conceptually optional values. This sort of type-safety would help prevent errors, as mentioned above. Some GTK functions have already started to use this annotation. As of 3.2.3, I can find _gtk_tree_menu_get_root gtk_font_chooser_get_font gtk_font_chooser_get_font_desc So is this annotation is 'official' now? The annotations description currently says it is "only valid for parameters": http://live.gnome.org/GObjectIntrospection/Annotations
Just noting that support for this exists already in GIRepository and at the typelib level: gitypelib-internal.h: guint16 may_return_null : 1; gicallableinfo.h: gboolean g_callable_info_may_return_null (GICallableInfo *info); Furthermore, creation of GIR files from typelibs should output the attribute allow-none="1" where required for return-value tags. See function write_callable_info in girwriter.c
Can we have that the other way around? Un-annotated functions should always be assumed to return NULL as a possible value. At least that's what my "safe by default" education tells me.
*** Bug 672612 has been marked as a duplicate of this bug. ***
Just my two cents, it would be interesting if support for this could be implemented, one way or another, before the next release. We are developing the next version of gitg extensively using vala and GIR and we can't release before support for this is available in g-i.
considering this and the other allow-none bug, maybe we want to deprecate allow-none and replace it with two new annotations, "nullable" (or "allow-null" or "maybe-null" or "null-ok") and "optional". ("allow-none" would then be translated to "optional" on (out) args and "nullable" on (in) args)
Created attachment 256839 [details] [review] giscanner: don't accept (allow-none) on Returns: The old annotationparser.py happily parsed this, but giscanner/girwriter.py never serialized an allow-none attribute to the .gir file and girepository/girparser.c never looked for an allow-none attribute either.
Putting this patch here for now; but I agree with the plan in comment 8.
Created attachment 274526 [details] [review] maintransformer: split up a complex conditional We assign node.allow_none in two separate cases: - if the (allow-none) annotation was given - for GCancellable and GAsyncReadyCallback, as special cases Split the two up. This will simplify future commits.
Created attachment 274527 [details] [review] giscanner: tweak GCancellable null special case We treat GCancellable and GAsyncReadyCallback as if they were always tagged (allow-none) because these parameters universally allow %NULL to be given as a value when they appear as in parameters on functions. Unfortunately, the meaning of (allow-none) is different on out parameters. There it means that you can give %NULL to ignore the return value. Limit this special case to only in parameters.
Created attachment 274528 [details] [review] giscanner: change some internal field logic Replace the 'allow_none' field on parameters with two separate fields: 'nullable' and 'optional'. Currently, we use 'nullable' to mean the same thing as 'allow-none' for normal (non-out) parameters. For out parameters, we use the 'optional' field instead. Note that the special case for GCancellable and GAsyncReadyCallback is already guarded by a check for being an in parameter, so we always use 'nullable' here. On the .gir writer side, we decide which variable to consult when writing the allow-none attribute depending on the parameter direction.
Created attachment 274529 [details] [review] giscanner: write nullable and optional attributes Record our internal 'nullable' and 'optional' attributes into the written .gir file. It is now theoretically possible to express the concept of an out parameter with a nullable type (although presently there is no way to do this). Modify our own internal parser (in the scanner) to understand the newly-written attributes. Update the expected output of the 'Regress-1.0.gir' test to account for the new attributes. Nothing else understands 'nullable' yet, but the girparser in the typelib compiler already understands 'optional' and records a bit for it in the typelib.
Created attachment 274530 [details] [review] giscanner: add (nullable) and (optional) annotations Add two new annotations, (nullable) and (optional). (nullable) always means "the type of this value can also contain null". (optional) always means "this out parameter can be ignored by passing NULL to the C function".
Created attachment 274531 [details] [review] giscanner: support nullable return types too Promote the 'nullable' field to the TypeContainer base class (which is shared by Return and Parameter types). Add .gir support for nullability on return values, both in the writer and in the (scanner's) parser.
Created attachment 274532 [details] [review] girepository: ArgBlob: rename allow_none parameter Rename the "allow_none" parameter on internal/private structure ArgBlob to "nullable". This is a straight rename with no other changes.
Created attachment 274533 [details] [review] compiler: girparser: parse 'nullable' attribute Parse the 'nullable' attribute on parameters and function return types. Additionally, tweak the meaning of the 'allow-none' attribute. We now only treat it as equivalent to 'nullable' for non-out parameters. For out parameters, we treat it to mean the same as the already-recognised 'optional' parameter (which we only recently started actually using).
Created attachment 274534 [details] [review] girparser: grok "nullable" Understand the new "nullable" attribute in .gir files. Presently, this is mostly an alias for allow-none='', but it will also allows a new feature: we can explicitly mark out parameters as having a nullable type (as a distinct concept from accepting 'NULL' as a parameter to the C function call in order to ignore the result). .gir may eventually want to remove allow-none='' some day, so make sure we also accept nullable='' in all places that we accepted allow-none='' to mean 'can be NULL'.
Great to see progress here. I think bug 626395 can be marked as a duplicate.
*** Bug 626395 has been marked as a duplicate of this bug. ***
Review of attachment 274526 [details] [review]: Looks good to me.
Review of attachment 274527 [details] [review]: Looks good to me.
Review of attachment 274528 [details] [review]: lgtm, maybe consider adding some asserts to check for consistency between the new attributes and the direction of the argument?
Review of attachment 274529 [details] [review]: Great
Review of attachment 274530 [details] [review]: lgtm ::: giscanner/annotationparser.py @@ +792,3 @@ + def _do_validate_nullable(self, position, ann_name, options): + Extra space?
Review of attachment 274531 [details] [review]: lgtm
Review of attachment 274532 [details] [review]: lgtm
Review of attachment 274533 [details] [review]: lgtm
Have given a look to these patches and haven't been able to find any issues in them. Maybe because my g-i is rusty, maybe because the patches are excellent.
Review of attachment 274531 [details] [review]: I needed the following fix to get it to work with the patch from bug #719966, otherwise I got: ERROR: Failed to re-parse gir file; scanned='/tmp/tmpusVgSE.gir' passthrough='/tmp/tmpHzy42J.gir' ::: giscanner/girparser.py @@ +295,3 @@ raise ValueError('node %r has no return-value' % (name, )) transfer = returnnode.attrib.get('transfer-ownership') + nullable = node.attrib.get('nullable') == '1' Shouldn't this be returnnode.attrib.get?
(In reply to comment #32) > Shouldn't this be returnnode.attrib.get? Yes. Thanks very much for testing this.
Attachment 256839 [details] pushed as 0839e69 - giscanner: don't accept (allow-none) on Returns: Attachment 274526 [details] pushed as f2858cf - maintransformer: split up a complex conditional Attachment 274527 [details] pushed as 289e85c - giscanner: tweak GCancellable null special case Attachment 274528 [details] pushed as 754f196 - giscanner: change some internal field logic Attachment 274529 [details] pushed as 89d51bc - giscanner: write nullable and optional attributes Attachment 274530 [details] pushed as 1459ff3 - giscanner: add (nullable) and (optional) annotations Attachment 274531 [details] pushed as 17b19cf - giscanner: support nullable return types too Attachment 274532 [details] pushed as 3ede3b8 - girepository: ArgBlob: rename allow_none parameter Attachment 274533 [details] pushed as bb9b9bd - compiler: girparser: parse 'nullable' attribute
Review of attachment 274534 [details] [review]: Feel free to push, thanks.
Created attachment 276010 [details] [review] giscanner: fix a comparison In the case that a parameter has not been explicitly annotated, the value of node.direction will be None, not 'in'. Instead of comparing as == 'in' we should therefore check for being != 'out'.
Comment on attachment 276010 [details] [review] giscanner: fix a comparison Attachment 276010 [details] pushed as 4076863 - giscanner: fix a comparison
Attachment 274534 [details] pushed as e935d4b - girparser: grok "nullable"
Comment on attachment 256839 [details] [review] giscanner: don't accept (allow-none) on Returns: Accidentally committed, now reverted.
Is nullable meant to be used for out args as well? Because as far as I can see it, up until now g_arg_info_may_be_null() for out args meant that it was optional and g_arg_info_is_optional() wasn't used at all by bindings. So adding (nullable) to out args which now have (allow-none) will changed the meaning of the may_be_null() return value.
In addition: The current approach defines not annotated arguments as non-nullable. This means that any function which isn't properly annotated pretends that it wont return NULL. So in reality this can't be trusted at all for return values and out args. Imho it would be better if in args default to non-nullable and return/out args to nullable. So added annotation only losens preconditions and tighten postconditions.
re-opening as per last comment(s).
Which binding is this a problem for?
the context is pygobject; pygobject is currently calling g_arg_info_may_be_null(), and not calling g_arg_info_is_optional() when handling arguments. I also just checked the Perl bindings as well as gjs, and is_optional() is unused in both. it seems we'll have to fix all the introspection-based bindings to use is_optional() at this point.
Created attachment 276448 [details] reuse old optional as nullable What about the following (see patch): * The "optional" bit in the typelib wasn't used until now (afaics) so it can be reused to mean nullable. This breaks g_arg_info_is_optional() for older libgirepository, but that was never useful anyway. * Instead of renaming the allow_none bit to nullable, rename it to optional, which is what it was used for until now. * Introduce a new g_arg_info_is_nullable() which exposes the new nullable bit. * g_arg_info_may_be_null() returns if NULL can be passed in, like before and like the docs say. * g_arg_info_is_optional() now is the same as g_arg_info_may_be_null().
(In reply to comment #45) > * Instead of renaming the allow_none bit to nullable, rename it to optional, > which is what it was used for until now. This is only the case for out parameters, which is actually the unusual case. For normal parameters, this is used to find out if passing NULL would cause a segfault and to throw an exception instead. Can someone please show me an actual piece of code that is negatively impacted by these changes? I still don't understand where the problem is...
(In reply to comment #46) > Can someone please show me an actual piece of code that is negatively impacted > by these changes? I still don't understand where the problem is... You're right that it probably wont cause any problems with the bindings, because is_optional() was never used and it seems nothing depended on may_be_null() for out only args. My problem is that I want to expose the nullable property of out only args at runtime as documentation (and I'm thankful that you're working on it). There is no way to know if the return value of g_arg_info_may_be_null() means it can take NULL or if it will write out NULL. The same applies to is_optional(), since it only returns meaningful values as of now. And from what I see this not only depends on the libgirepository version (which isn't exposed) but also on the version writing the typelib.
(In reply to comment #47) > And from what I see this not only depends on the libgirepository version (which > isn't exposed) but also on the version writing the typelib. This is an interesting point, and I hadn't considered it -- I only thought about the bindings as users of libgirepository. Why are you using the typelib for documentation instead of going straight from the .gir? Want to avoid writing your own parser?
PyGObject exposes "allow-none" through docstrings at runtime: $ ipython In [1]: from gi.repository import GLib In [2]: GLib.dcgettext? [..] Docstring: dcgettext(domain:str=None, msgid:str, category:int) It currently only exposes it for in args (see domain=None in this example), but could also expose it for out args if libgirepository would provide that information. And since Python API docs [0] get generated through introspection, this would help there too (I could of course parse the gir). [0] https://lazka.github.io/pgi-docs/#GLib-2.0/functions.html#GLib.dcgettext
(In reply to comment #45) > ... > * Introduce a new g_arg_info_is_nullable() which exposes the new nullable bit. > > * g_arg_info_may_be_null() returns if NULL can be passed in, like before and > like the docs say. > > * g_arg_info_is_optional() now is the same as g_arg_info_may_be_null(). FWIW, I think this is semantically clearer. A return value of NULL and out arguments that can be set to NULL by the callee should use the same annotation (nullable). For me, "optional" has a stronger correlation to input arguments which accept NULL. In PyGI this is very much true because tail end input arguments which accept NULL are truly "optional" and can be left out of the argument list (uses NULL as the default). So I agree with Christoph but possibly for a different reason: attempting semantic clarity is important for future readers of the code and annotations.
I think I was misunderstanding something... (In reply to comment #15) > (nullable) always means "the type of this value can also contain null". > > (optional) always means "this out parameter can be ignored by passing > NULL to the C function". This seems to be mostly inline with what I was thinking. The difference here is optional is strictly used for passing NULL by the callee for optionally receiving the output.
Would it be possible to get the libgirepository version exposed (at compile and runtime), so I can interpret the return values accordingly?
I'm okay with a bit of shuffling to maintain compatibility but I'm quite insistent on one point: We should be left with exactly two non-deprecated APIs, one of which (nullable) always means that the given type is augmented with nullability, and (optional), applying only to out parameters, to mean that passing NULL to the C API is a valid way to ignore the result. I personally like the "may be null" name that the existing API has to express the notion of nullability. Unlike "allow none", it doesn't have an implied direction. I'd also like to avoid muddying the water of the definition of "optional" by conflating it with the (imho unrelated) idea that it's possible to leave these arguments out in the python binding. I've seen discussion elsewhere about the possibility of default values, in which cases it wouldn't just be the nullable parameters that are 'optional' in this sense... (In reply to comment #49) > It currently only exposes it for in args (see domain=None in this example), but > could also expose it for out args if libgirepository would provide that > information. For in arguments there has been absolutely no change in behaviour. For out arguments, I'm not sure what you'd want to have exposed: the idea that it's possible, at the level of the C API, to ignore the out parameter, or the idea that it may return None instead of an object reference?
Created attachment 276485 [details] [review] Rename back nullable to allow_none and use the optional bit for nullable instead. Another take which should be a little clearer (not having two slightly different flags named optional): * Rename nullable back to allow_none, where allow_none means if NULL can be passed to the C functions (independent of it being in or out). This is a superset of the new (optional) annotation. [typelib]->allow_none is TRUE for (allow-none) or (nullable) + (in) or (optional) + (out) * Rename the previously named "optional" bit in the typelib to "nullable" and expose it through g_arg_info_is_nullable(). [typelib]->nullable is TRUE for (nullable) or (allow-none) + (in) This means for the API: g_arg_info_may_be_null() -> if NULL can be passed to C, ignoring the direction (can be considered deprecated) g_arg_info_is_optional() -> if NULL can be passed for (out) to ignore the result g_arg_info_is_nullable() -> if the value passed in our out can be NULL. for the typelib: allow_none has the same value and same meaning as before optional -> nullable
(In reply to comment #53) > I'm okay with a bit of shuffling to maintain compatibility but I'm quite > insistent on one point: > > We should be left with exactly two non-deprecated APIs, one of which (nullable) > always means that the given type is augmented with nullability, and (optional), > applying only to out parameters, to mean that passing NULL to the C API is a > valid way to ignore the result. I've tried to improve my patch to take that into account. > I personally like the "may be null" name that the existing API has to express > the notion of nullability. Unlike "allow none", it doesn't have an implied > direction. > > I'd also like to avoid muddying the water of the definition of "optional" by > conflating it with the (imho unrelated) idea that it's possible to leave these > arguments out in the python binding. I've seen discussion elsewhere about the > possibility of default values, in which cases it wouldn't just be the nullable > parameters that are 'optional' in this sense... > > (In reply to comment #49) > > It currently only exposes it for in args (see domain=None in this example), but > > could also expose it for out args if libgirepository would provide that > > information. > > For in arguments there has been absolutely no change in behaviour. > > For out arguments, I'm not sure what you'd want to have exposed: the idea that > it's possible, at the level of the C API, to ignore the out parameter, or the > idea that it may return None instead of an object reference? The latter.
I still don't understand the problem...
Before the patches: >>> from gi.repository import GLib >>> GLib.KeyFile.load_from_data_dirs.get_arguments()[1].may_be_null() True After the patches: >>> from gi.repository import GLib >>> GLib.KeyFile.load_from_data_dirs.get_arguments()[1].may_be_null() False After the patches with older typelib: >>> from gi.repository import GLib >>> GLib.KeyFile.load_from_data_dirs.get_arguments()[1].may_be_null() True .. but feel free to close this, I don't care that much.
Created attachment 276554 [details] [review] girepository: change giarginfo docs Clarify the meaning of 'may be null' in the docs: it refers to the value of the argument itself, not the reference to the argument.
Attachment 276554 [details] pushed as 0b5802d - girepository: change giarginfo docs Pushed these changes to clarify the new behaviour. After discussing this on IRC we've concluded that there is no exinsting problem caused by this change: only that going forward it's difficult to know how we should know if it's possible to pass NULL to an out parameter. Fortunately, this fails in the 'right' direction: if you ask for 'optional' with an old typelib you'll be told 'no', so you won't crash.
I noted some issues in bug 626395, comment 7 and would like to confirm expectations regarding a couple of those points. (Looks like the girepository interface is now sorted!) 1. For functions that can raise an error, the absence of a nullable annotation for a return value or out/inout parameter means a non-null value is returned or exported **only when no error is raised**. Therefore, when an error is raised, the return value or any exported value could be null, even when no error argument is given. In practice, the return value of a function that can raise an error is likely to be (always?) a flag, so not a nullable type and would indicate the condition under which non-null exported values can be assumed when no error argument is given. 2. For an inout parameter, the presence/absence of a nullable annotation applies to both the imported value and the exported value. That is, either both imported and exported values can be null, or neither can be null. Is that the current expectation?
(In reply to comment #60) > 1. For functions that can raise an error, the absence of a nullable annotation > for a return value or out/inout parameter means a non-null value is returned or > exported **only when no error is raised**. In fact, it's even more likely that the value will not be modified at all. If the variable was previously uninitilised (ie: junk contents) then it may well contain the same junk after the function returns. On the other hand, some functions may clear it. In general, the rule here is simply that you, as the caller, must not touch it. > Therefore, when an error is raised, the return value or any exported value > could be null, even when no error argument is given. It could be anything at all and you must not rely on it being anything in particular. If an error is raised, you must ignore all out parameters. > In practice, the return > value of a function that can raise an error is likely to be (always?) a flag, > so not a nullable type and would indicate the condition under which non-null > exported values can be assumed when no error argument is given. No. There are really a very wide range of different return values on functions that "throw" GError. Widely, there are three "good" categories: - non-nullable pointer return values, where NULL return means "error thrown" - boolean return where FALSE means "error thrown" - non-negative integer return, where -1 means "error thrown" But there are many exceptions to this rule in APIs from before we had such strong conventions. GKeyFile is one such "bad" example: it has APIs that can return NULL, sometimes with a GError set and sometimes without. This is something that we hope to address better in the future. Bug 708301 starts discussing some possibilities (and there is quite a lot to discuss...). > 2. For an inout parameter, the presence/absence of a nullable annotation > applies to both the imported value and the exported value. That is, either > both imported and exported values can be null, or neither can be null. > > Is that the current expectation? Yes.
AFAICT this doesn't work on properties (for example, Gio.DBusObjectManagerClient.name-owner) or fields. Is that intentional?
(In reply to comment #61) > ... > > If an error is raised, you must ignore all out parameters. > > ... > > There are really a very wide range of different return values on functions > that "throw" GError. Widely, there are three "good" categories: > > - non-nullable pointer return values, where NULL return means "error thrown" > > - boolean return where FALSE means "error thrown" > > - non-negative integer return, where -1 means "error thrown" > Thanks for confirming. This is what I was hoping to hear - in particular, that the conventions should be suitable for bindings that raise exceptions for GErrors. Perhaps the defining characteristic of the "good" category is that there is only one return value to indicate an error has occurred. Then, when an error is indicated, no information is unavailable when using a language binding that converts errors to exceptions and control flow jumps to the enclosing handler.
(In reply to comment #62) > AFAICT this doesn't work on properties (for example, > Gio.DBusObjectManagerClient.name-owner) or fields. Is that intentional? For a nullable annotation on properties, I know of [1] bug 669640 [2] bug 672612 Currently, [2] is marked as a duplicate of this bug. I don't believe that this bug addresses properties or fields so perhaps these should be discussed under [1]?
No, bug #669640 is for support in Vala's GIR writer. I've gone ahead and opened a new bug (bug #730479).
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]