some introductions:

(i've trimmed implementations of 'iterable' and 'traversable' because they do not impact the situation here)

public interface Gee.Collection<G>
   public abstract Collection<G> read_only_view { owned get; }

public interface Gee.Set<G> : Collection<G>
   public abstract new Set<G> read_only_view { owned get; }

public abstract class Gee.AbstractCollection<G> : Object, Collection<G>
   public virtual Collection<G> read_only_view { ... }

public abstract class Gee.AbstractSet<G> : Gee.AbstractCollection<G>, Set<G>
   public virtual new Set<G> read_only_view { ... }

so: AbstractX is the abstract class that implements interface X and a set is a collection.

Meanwhile, both "Set" and "Collection" interfaces introduce 'read-only-view' properties with different types.  AbstractSet ends up implementing both of these interfaces.  It sounds impossible that an object should attempt to implement two interfaces that have a similarly-named-property with different types, but it's not totally insane.  Because the property is read-only, we could implement it using the more-restrictive type (in this case 'Set') without running into troubles.

Unfortunately, that's not what happens.

Vala emits this code:

  g_object_class_override_property (G_OBJECT_CLASS (klass),
                                    GEE_ABSTRACT_SET_READ_ONLY_VIEW,
                                    "read-only-view");

Which causes GObject to find the first matching property with that name -- which it most likely finds from the parent class AbstractCollection, with type 'Collection'.  Now we have a property implementation that's incompatible with the newly-added 'Set' interface.

I don't think we can assign *too* much blame to Gee here (other than for being completely architecturally insane).

You could assign blame to GObject for not automatically picking the more restrictive type but that's not so simple.  In the case of write-only properties, for example, you'd actually want GObject to pick the least-restrictive type.  You're also left wondering what blurb strings should end up being used on the overridden property.

You could also assign blame to Vala for not answering these questions explicitly by using g_object_class_install_property().

http://git.gnome.org/browse/glib/commit/?id=5fb7a8e127bde6465a5b9e22b299ca2e439e702c was the commit that introduced the change in GLib (not the related commit linked above).  The original bug was bug 666616.

A quick comment - I can fix the code on 0.7 side (however probably it would be good to have overriding properties) however 0.6 is currently API/ABI stable. It might happen that 0.8 will not be ready for GLib 2.32 - and GNOME 3.4 is likely to use 0.6.

Could the error be change into warning with note that it will be an error in future and/or the diamond property overriding is fixed on vala side?

Created attachment 205381 [details] [review]
Use 'Collection' type for read-only-view property

On all classes and interfaces that may end up being used with a class
implementing 'Collection'.



This is one possible fix for this problem.  It introduces some ugliness
by requiring some extra downcasts but in my opinion these downcasts
should be required in any case, because it's not clear which type of
property 'read_only_view' refers to.

Review of attachment 205381 [details] [review]:

::: tests/testreadonlyset.vala
@@ +43,2 @@
 	protected override Collection<string> get_ro_view (Collection<string> collection) {
+		return ((Gee.Set) collection).read_only_view as Gee.List;

I don't understand why this particular patch was necessary.

Why does 'List<G>' match 'Collection<string>' but 'Collection<G>' does not?

This patch causes breakages in at least libfolks.  All cases of the breaks are trivial to fix by adding downcasts...

Comment on attachment 205381 [details] [review]
Use 'Collection' type for read-only-view property

Sorry for delaying the response - unfortunately I was stopped by 'real life'.

The idea in patch seems to be good one but unfortunately the breakage of API/ABI for 0.6 is not acceptable. The change is accepted for 0.7.

It seems that it cannot pass the tests with vala 0.14.1 in '/ReadOnlySet/[ReadOnlyCollection] unique read-only view instance' - I started to investigate what's wrong. 

Also - since the Set/List/... inherit from Collection is there a reason why the former define new property?

Thank you for your patch and sorry for delay.

I'd like to propose another solution to avoid breaking the API. The problem resides in GObject properties, while the C code is perfectly sane.

Therefore we could completely drop the g_object_class_override_property() line in the generated Vala code. This can be achieved in two ways:
1) By explicitly adding a [CCode (subclasses_override = false)]
2) By looking up the implemented properties. If two properties have different types, and we know there's a scoping issue with GObject, then we just stop overriding the property.

I'm not sure what will be the result from an RTTI view point, when using g_object_set/g_object_set, but I believe this won't be a blocker compared to breaking the API in the case of Gee.

Both solutions need to be implemented in Vala, then Gee and subclassers (like Rygel) need to recompile against latest Vala.

if you will not override the property then you also need to avoid installing it in the interface in the first place, or else you will fail the checks.

Created attachment 205729 [details] [review]
codegen: don't g_object_class_override_property

Instead, always install our own new property with what we believe the
correct type is.  This avoids a problem with libgee providing properties
on some classes that implement two interfaces having the same property
name with different types.

Note that this also affects libgee 0.6.x (which is why it breaks so many of the Folks tests; we don't officially support 0.7.x since it's not API/ABI-stable).

I also consider this a blocker for the next release of Folks, since I don't want to release it unless all of its tests pass.

OK, after a little more checking (and forcing libgee to re-generate its C files after a maintainer-clean), attachment #205729 [details] seems to fix this immediate problem for Folks (even if I apply this directly to 0.15.0, not git master).

To un-break jhbuild, we'll need a new release of libgee that bundles the re-generated C files.

(In reply to comment #13)
> OK, after a little more checking (and forcing libgee to re-generate its C files
> after a maintainer-clean), attachment #205729 [details] seems to fix this immediate
> problem for Folks (even if I apply this directly to 0.15.0, not git master).
> 
> To un-break jhbuild, we'll need a new release of libgee that bundles the
> re-generated C files.

I've planned to do this today. I hope that's ok.

(In reply to comment #14)
> I've planned to do this today. I hope that's ok.

The patch hasn't been committed to vala yet.

(In reply to comment #15)
> (In reply to comment #14)
> > I've planned to do this today. I hope that's ok.
> 
> The patch hasn't been committed to vala yet.

I usually keep my own version of vala master with patches I need to work but haven't been committed yet (and not rejected) - currently this patch, patch for bug #667452 and patch for bug #666478.

Review of attachment 205729 [details] [review]:

commit 1bff29257fa05bf9839583a56f22a1429ea7da73
Author: Ryan Lortie <desrt@desrt.ca>
Date:   Fri Jan 20 15:31:17 2012 -0500

    codegen: Do not use g_object_class_override_property
    
    Instead, always install our own new property with what we believe the
    correct type is.  This avoids a problem with libgee providing properties
    on some classes that implement two interfaces having the same property
    name with different types.
    
    Fixes bug 666728.

The new gee releases (both 0.6.4 and 0.7.2) were built using the patched Vala.  I think we can close this now.

*** Bug 666721 has been marked as a duplicate of this bug. ***

Issue still exists for Maps, e.g.:

void main() {
    var m = new Gee.TreeMap<string, string>().read_only_view;
}


Results in:

(process:26844): GLib-GObject-CRITICAL **: Read-only property 'read-only-view' on class 'GeeReadOnlySortedMap' has type 'GeeMap' which is not equal to or more restrictive than the type 'GeeSortedMap' of the property on the interface 'GeeSortedMap'


(process:26844): GLib-GObject-CRITICAL **: Read-only property 'read-only-view' on class 'GeeReadOnlyBidirSortedMap' has type 'GeeMap' which is not equal to or more restrictive than the type 'GeeBidirSortedMap' of the property on the interface 'GeeBidirSortedMap'

(In reply to comment #20)
> Issue still exists for Maps, e.g.:
> 

Which version of vala, libgee and glib?

I'm using vala 0.21.2 and glib 2.37.7 .

And gee 0.11.91 .

This was libgee bug. Fix should be included in next release (you can try version from git).

Thank you! (Works very well.)