One thing to note: with GProperty we plan that the default case will be that properties are non-nullable and a G_PROPERTY_FLAG_NULLABLE will have to be explicitly given in order to have nullability.

Sounds good to me.  I don't suppose you have a bug #--I can't seem to find anything.

FWIW I don't think it would make sense to support both an annotation and a flag, so if that's the case then IMHO this bug just became about fields only.

The annotation could still be useful for plain GParamSpec properties...

Alternatively, we could add an API along the lines of g_param_spec_get_nullable() that returns TRUE for (pure) GParamSpec and the appropriate value for GProperty.  Then we could even consider adding the opposite flag to GParamSpec -- G_PARAM_NONNULL or so.

I don't have a bug to link to other than the main GProperty bug and this is already a disaster...

(In reply to comment #3)
> The annotation could still be useful for plain GParamSpec properties...

Okay, I guess I'll go ahead and start adding the annotations as I come across them.  I can tweak the Vala GIR parser to base nullability decisions for properties on the getters and setters pretty easily so I should be able to find a lot of nullable properties pretty quickly once I finish adding (nullable) annotations to return values everywhere (plus mismatches between getter and setter nullability).

> Alternatively, we could add an API along the lines of
> g_param_spec_get_nullable() that returns TRUE for (pure) GParamSpec and the
> appropriate value for GProperty.  Then we could even consider adding the
> opposite flag to GParamSpec -- G_PARAM_NONNULL or so.
>
> I don't have a bug to link to other than the main GProperty bug and this is
> already a disaster...

Ah, okay I'll read that bug next time I have an hour or two to kill ;)

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]

Created attachment 299440 [details] [review]
scanner: Add support for nullable fields

This also allow allow-none for backwards compatibility.

2¢: supporting (allow-none) here is wrong and confusing.  Why add new support for a deprecated feature, while at the same time, further muddying the waters of what its meaning ever was?

I added it because GI already used it in Regress (even though it didn't actually get written out). Some users might already be using the allow-none annotation for fields and expect it to work. Also, for some amount of backwards compatibility in the event that the nullable annotation on fields would cause new warnings on older GI.

(In reply to Garrett Regier from comment #8)
> I added it because GI already used it in Regress (even though it didn't
> actually get written out). Some users might already be using the allow-none
> annotation for fields and expect it to work.

I'd argue that this is another reason _not_ to do it.

> Also, for some amount of
> backwards compatibility in the event that the nullable annotation on fields
> would cause new warnings on older GI.

And again, wouldn't it be better if the user got a warning that what they were trying to do was unsupported in the current version?

Created attachment 299684 [details] [review]
scanner: Add support for nullable fields v2

ping?

Review of attachment 299684 [details] [review]:

Does this actually get written out in the typelib today though?  It looks to me like it doesn't.  Are you only using this with bindings that are statically built from the GIR and not dynamic ones from the typelib?

Also, for backwards compatibility, don't we need to implicitly assume that fields can be NULL, so the annotation would need to be (nonnull)?

I guess that'd be a new annotation piling on to our fun history here of (allow-none) -> (null-ok) -> (nullable)?

(In reply to Colin Walters from comment #12)
> Review of attachment 299684 [details] [review] [review]:
> 
> Does this actually get written out in the typelib today though?  It looks to
> me like it doesn't.  Are you only using this with bindings that are
> statically built from the GIR and not dynamic ones from the typelib?
> 

It doesn't yet, fixing that and adding g_field_info_may_be_null().

> Also, for backwards compatibility, don't we need to implicitly assume that
> fields can be NULL, so the annotation would need to be (nonnull)?
> 
> I guess that'd be a new annotation piling on to our fun history here of
> (allow-none) -> (null-ok) -> (nullable)?

Yeah, seems another annotation is required. What would be the best name? (nonnull), (nullable 0), (not nullable), ...

This would be adding another one but it would also allow for explicit this cannot be NULL which might be useful for the docs, etc.

(In reply to Garrett Regier from comment #13)
> (In reply to Colin Walters from comment #12)
> > Also, for backwards compatibility, don't we need to implicitly assume that
> > fields can be NULL, so the annotation would need to be (nonnull)?
> > 
> > I guess that'd be a new annotation piling on to our fun history here of
> > (allow-none) -> (null-ok) -> (nullable)?
> 
> Yeah, seems another annotation is required. What would be the best name?
> (nonnull), (nullable 0), (not nullable), ...

I have a patch for (not nullable) here, which has been waiting review for a while:

https://bugzilla.gnome.org/show_bug.cgi?id=719966#c34

Garrett, does the work in bug 719966 look good to you as well?

(In reply to Colin Walters from comment #15)
> Garrett, does the work in bug 719966 look good to you as well?

I ask not to use spaces in annotation names so
please prefer (not-nullable) over (not nullable).

Better still would be (nonnull) imho, as it is already used together
with (nullable) in Objective-C, it's shorter to type and equally
clear in it's meaning.

Whatever is chosen, please don't push until you also have a patch
to teach GTK-Doc's gtkdoc-mkdb about the new annotation.

(In reply to Dieter Verfaillie from comment #16)
> (In reply to Colin Walters from comment #15)
> > Garrett, does the work in bug 719966 look good to you as well?
> 
> I ask not to use spaces in annotation names so
> please prefer (not-nullable) over (not nullable).

The whole point of choosing (not nullable) was to introduce a generic syntax for negating annotations. So the annotation here is actually 'not', with 'nullable' as its argument.

See: https://bugzilla.gnome.org/show_bug.cgi?id=719966#c29

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gobject-introspection/issues/112.