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 729660 - giscanner: Mark (closure) parameters as (nullable) by convention
giscanner: Mark (closure) parameters as (nullable) by convention
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: g-ir-scanner
unspecified
Other All
: Normal normal
: ---
Assigned To: gobject-introspection Maintainer(s)
gobject-introspection Maintainer(s)
Depends on:
Blocks: 719966
 
 
Reported: 2014-05-06 16:55 UTC by Philip Withnall
Modified: 2015-10-10 18:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
giscanner: Mark (closure) parameters as (nullable) by convention (1.25 KB, patch)
2014-05-06 16:55 UTC, Philip Withnall
reviewed Details | Review
giscanner: Add convention for GError** parameters (1.51 KB, patch)
2014-05-06 16:55 UTC, Philip Withnall
reviewed Details | Review
giscanner: Mark (closure) parameters as (nullable) by convention (12.79 KB, patch)
2014-06-20 11:48 UTC, Philip Withnall
none Details | Review
giscanner: Mark (closure) parameters as (nullable) by convention (12.78 KB, patch)
2015-09-02 07:47 UTC, Philip Withnall
none Details | Review
giscanner: Mark gpointer nodes as nullable by default (17.89 KB, patch)
2015-09-02 07:47 UTC, Philip Withnall
none Details | Review
giscanner: Mark (closure) parameters as (nullable) by convention (12.78 KB, patch)
2015-10-04 20:57 UTC, Philip Withnall
committed Details | Review
giscanner: Mark gpointer nodes as nullable by default (20.83 KB, patch)
2015-10-04 20:57 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2014-05-06 16:55:16 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.
Comment 1 Philip Withnall 2014-05-06 16:55:18 UTC
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.
Comment 2 Philip Withnall 2014-05-06 16:55:22 UTC
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.
Comment 3 Simon Feltman 2014-05-08 00:44:27 UTC
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"...
Comment 4 Simon Feltman 2014-05-08 00:58:13 UTC
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...
Comment 5 Philip Withnall 2014-06-20 11:48:56 UTC
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.
Comment 6 Philip Withnall 2014-06-20 11:54:50 UTC
(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.
Comment 7 André Klapper 2015-02-07 17:10:49 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]
Comment 8 Philip Withnall 2015-09-02 07:47:14 UTC
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.
Comment 9 Philip Withnall 2015-09-02 07:47:19 UTC
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.
Comment 10 Philip Withnall 2015-09-02 07:49:25 UTC
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.
Comment 11 Colin Walters 2015-09-03 14:34:44 UTC
Experience has taught me that the best time to land major introspection changes is at the start of a cycle.  Please ping again then =)
Comment 12 Philip Withnall 2015-09-03 15:26:34 UTC
(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.
Comment 13 Colin Walters 2015-09-26 16:07:37 UTC
Review of attachment 310463 [details] [review]:

This looks reasonable.  Doesn't seem like we're explicitly rejecting (not transfer) etc.?
Comment 14 Philip Withnall 2015-10-04 20:57:27 UTC
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.
Comment 15 Philip Withnall 2015-10-04 20:57:33 UTC
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.
Comment 16 Philip Withnall 2015-10-04 20:59:04 UTC
(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"
Comment 17 Colin Walters 2015-10-04 21:43:35 UTC
Review of attachment 312651 [details] [review]:

LGTM
Comment 18 Colin Walters 2015-10-04 21:44:05 UTC
Comment on attachment 312651 [details] [review]
giscanner: Mark (closure) parameters as (nullable) by convention

https://git.gnome.org/browse/gobject-introspection/commit/?id=0a134a608f5b471c3a12739785e149ceaf90df27
Comment 19 Colin Walters 2015-10-04 21:50:45 UTC
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 20 Colin Walters 2015-10-04 21:51:16 UTC
Comment on attachment 312652 [details] [review]
giscanner: Mark gpointer nodes as nullable by default

https://git.gnome.org/browse/gobject-introspection/commit/?id=10cb665fee2cc378dd2f13bad16e6384836a8b16
Comment 21 Philip Withnall 2015-10-05 09:16:46 UTC
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!
Comment 22 Mikhail Zabaluev 2015-10-05 18:28:19 UTC
(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.
Comment 23 Mikhail Zabaluev 2015-10-05 18:35:05 UTC
(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.
Comment 24 Colin Walters 2015-10-05 20:04:55 UTC
(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
Comment 25 Philip Withnall 2015-10-06 07:24:15 UTC
(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). :-)
Comment 26 Philip Withnall 2015-10-06 07:27:49 UTC
(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?
Comment 27 Mikhail Zabaluev 2015-10-06 07:34:17 UTC
(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
Comment 28 Mikhail Zabaluev 2015-10-06 07:45:07 UTC
(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.
Comment 29 Colin Walters 2015-10-10 18:54:23 UTC
Regression reported in https://bugzilla.gnome.org/show_bug.cgi?id=756352