GNOME Bugzilla – Bug 729660
giscanner: Mark (closure) parameters as (nullable) by convention
Last modified: 2015-10-10 18:54:23 UTC
In order to prevent an explosion of (nullable) annotations, we should add conventions for user_data parameters and GError** parameters. Patches coming up which do this.
Created attachment 276001 [details] [review] giscanner: Mark (closure) parameters as (nullable) by convention All user_data pointers should be nullable, and they should all be annotated as closures too. I have not found any counter-examples where a closure is non-nullable.
Created attachment 276002 [details] [review] giscanner: Add convention for GError** parameters Add a special convention for GError** parameters which marks them as (nullable) (optional) (transfer full) (inout callee-allocates). If a GError** pointer doesn’t conform to this convention, additional annotations will override the conventional ones if necessary.
Review of attachment 276002 [details] [review]: Aren't GError**'s out not inout? In the normal case you'd have: void callee (GError **error) { /* if error is not NULL, *error must be NULL, hence it is strictly an out argument. */ g_set_error (error, ...); } Or is this patch for a different use case? It would be nice if the patch had tests which show how this changes the output. In terms of GIR, tail end GError** arguments are not included in the list but rather the function is marked as "throws"...
Review of attachment 276001 [details] [review]: This seems right to me. But changing this by itself should break "make check". So the changes to expected GIR and docs files should also be included in the patch. If it doesn't break, then it would be nice to have a new test which demonstrates how it effects GIR and docs...
Created attachment 278830 [details] [review] giscanner: Mark (closure) parameters as (nullable) by convention All user_data pointers should be nullable, and they should all be annotated as closures too. I have not found any counter-examples where a closure is non-nullable.
(In reply to comment #3) > Review of attachment 276002 [details] [review]: > > Aren't GError**'s out not inout? They’re (inout) because they read the value before overwriting it (to check for pre-set errors). So the incoming GError has to be initialised, which wouldn’t be needed if it was an (out) parameter. > It would be nice if the patch had tests which show how this changes the output. > In terms of GIR, tail end GError** arguments are not included in the list but > rather the function is marked as "throws"... So, having tested it a bit more closely, I realised that the GError** parameter is popped off the parameters list when (throws) is set, so the GError patch was redundant. I’ve updated the (closure) patch with test case updates.
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
Created attachment 310462 [details] [review] giscanner: Mark (closure) parameters as (nullable) by convention All user_data pointers should be nullable, and they should all be annotated as closures too. I have not found any counter-examples where a closure is non-nullable.
Created attachment 310463 [details] [review] giscanner: Mark gpointer nodes as nullable by default gpointer parameters and return types should be marked as nullable by default, unless: • also annotated with (type) and not with (nullable); or • explicitly annotated with (not nullable). This introduces the (not nullable) annotation as a direct opposite to (nullable). In future, (not) could be extended to invert other annotations.
Attachment #310463 [details] moved here from bug #719966 so that all the gobject-introspection patches for this are now in one place, since gobject-introspection bugs are now separate from glib bugs. Still needs review. It was originally reviewed as ‘reasonable’ (https://bugzilla.gnome.org/show_bug.cgi?id=719966#c43) but marked as reviewed rather than accepted_commit-now. Should probably have another round of review.
Experience has taught me that the best time to land major introspection changes is at the start of a cycle. Please ping again then =)
(In reply to Colin Walters from comment #11) > Experience has taught me that the best time to land major introspection > changes is at the start of a cycle. Please ping again then =) Isn’t that precisely what accepted_commit-after-freeze is for? We’re both going to forget about this by the start of the next cycle.
Review of attachment 310463 [details] [review]: This looks reasonable. Doesn't seem like we're explicitly rejecting (not transfer) etc.?
Created attachment 312651 [details] [review] giscanner: Mark (closure) parameters as (nullable) by convention All user_data pointers should be nullable, and they should all be annotated as closures too. I have not found any counter-examples where a closure is non-nullable.
Created attachment 312652 [details] [review] giscanner: Mark gpointer nodes as nullable by default gpointer parameters and return types should be marked as nullable by default, unless: • also annotated with (type) and not with (nullable); or • explicitly annotated with (not nullable). This introduces the (not nullable) annotation as a direct opposite to (nullable). In future, (not) could be extended to invert other annotations.
(In reply to Colin Walters from comment #13) > Review of attachment 310463 [details] [review] [review]: > > This looks reasonable. Doesn't seem like we're explicitly rejecting (not > transfer) etc.? That's handled in _do_validate_not() in annotationparser.py. The updated patches are rebased on master, and I've added an annotation parser test (tests/scanner/annotationparser/gi/annotation_not_nullable.xml). If you change the annotation in that to (for example) "(not valid)" the test will fail with the following warning: Warning: Test: invalid "not" annotation option: "valid"
Review of attachment 312651 [details] [review]: LGTM
Comment on attachment 312651 [details] [review] giscanner: Mark (closure) parameters as (nullable) by convention https://git.gnome.org/browse/gobject-introspection/commit/?id=0a134a608f5b471c3a12739785e149ceaf90df27
Review of attachment 312652 [details] [review]: Ok. This is a nice change, and seems to finish the story for nullability. That should make the Vala people happy, right?
Comment on attachment 312652 [details] [review] giscanner: Mark gpointer nodes as nullable by default https://git.gnome.org/browse/gobject-introspection/commit/?id=10cb665fee2cc378dd2f13bad16e6384836a8b16
I just pushed a small follow-up Makefile.am change which I somehow missed out of the patch because I’m a clumsy fool. Commit: 934ce4d1f44cc0f2eea81fe3bbc09037130d8fad. I’ll send an e-mail to gir-devel-list@ to notify people of the GIR change. Thanks for the reviews!
(In reply to Colin Walters from comment #20) > Comment on attachment 312652 [details] [review] [review] > giscanner: Mark gpointer nodes as nullable by default > > https://git.gnome.org/browse/gobject-introspection/commit/ > ?id=10cb665fee2cc378dd2f13bad16e6384836a8b16 That will be massively inconvenient in any strongly typed bindings that have to account for nullable values. In Rust, it basically means every pointer parameter or a return value has to be wrapped into an `Option`. The non-nullable default is actually safe for input parameters. If bindings have a reasonable need to pass null, they will pressure for an annotation. For return values, I understood the agreement (as discussed on the Rust BoF at GUADEC 2015) was that any nullable return or an out value not annotated as such is an annotation bug, and the bindings have the right to treat any unexpected returned null as a programmer error.
(In reply to Mikhail Zabaluev from comment #22) > That will be massively inconvenient in any strongly typed bindings that have > to account for nullable values. In Rust, it basically means every pointer > parameter or a return value has to be wrapped into an `Option`. Sorry, I misread the extent of the change. If it only concerns closures and gpointers where there is no type information to begin with, no problem.
(In reply to Mikhail Zabaluev from comment #23) > (In reply to Mikhail Zabaluev from comment #22) > > That will be massively inconvenient in any strongly typed bindings that have > > to account for nullable values. In Rust, it basically means every pointer > > parameter or a return value has to be wrapped into an `Option`. > > Sorry, I misread the extent of the change. If it only concerns closures and > gpointers where there is no type information to begin with, no problem. Please follow up to https://mail.gnome.org/archives/gir-devel-list/2015-October/msg00001.html
(In reply to Mikhail Zabaluev from comment #22) > For return values, I understood the agreement (as discussed on the Rust BoF > at GUADEC 2015) was that any nullable return or an out value not annotated > as such is an annotation bug, and the bindings have the right to treat any > unexpected returned null as a programmer error. Sorry, I wasn’t able to get to the Rust BoF (or, indeed, GUADEC). Was this written up somewhere? It would be quite useful if it made its way onto the wiki (https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations). :-)
(In reply to Mikhail Zabaluev from comment #23) > (In reply to Mikhail Zabaluev from comment #22) > > That will be massively inconvenient in any strongly typed bindings that have > > to account for nullable values. In Rust, it basically means every pointer > > parameter or a return value has to be wrapped into an `Option`. > > Sorry, I misread the extent of the change. If it only concerns closures and > gpointers where there is no type information to begin with, no problem. It concerns: 1) gpointers which don’t have a (type) annotation; and 2) any (closure) parameter. I don’t know of any examples of closure parameters which have (type) information. Is that OK, or should we restrict (2) to (closure) parameters which don’t have certain other annotations?
(In reply to Philip Withnall from comment #25) > Sorry, I wasn’t able to get to the Rust BoF (or, indeed, GUADEC). Was this > written up somewhere? It would be quite useful if it made its way onto the > wiki (https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations). :-) Yes, I wrote down some notes: https://github.com/gi-rust/grust/wiki/GUADEC-2015-BoF
(In reply to Philip Withnall from comment #26) > It concerns: > 1) gpointers which don’t have a (type) annotation; and > 2) any (closure) parameter. > > I don’t know of any examples of closure parameters which have (type) > information. Is that OK, or should we restrict (2) to (closure) parameters > which don’t have certain other annotations? I don't think there can be meaningful type annotations on closure data. It's OK for a closure in-parameter to be nullable by default, too: the bindings of closures are special enough so that detail can be hidden from the high level.
Regression reported in https://bugzilla.gnome.org/show_bug.cgi?id=756352