valagirparser.vala line 921 (Shown with debugging code)

    prop.set_attribute ("NoAccessorMethod", false);
    if (prop.get_accessor != null) {
        var m = getter != null ? getter.symbol as Method : null;
        // ensure getter vfunc if the property is abstract
        if (m != null && (m.is_abstract || m.is_virtual || !prop.is_abstract)) {
            getter.process (parser);
            if (m.return_type is VoidType || m.get_parameters().size != 0) {
                prop.set_attribute ("NoAccessorMethod", true);
            } else {
                if (getter.name == name) {
                    stdout.printf("**** Match ****\n");
                    stdout.printf("  getter.name = %s\n", getter.name);
                    stdout.printf("  name =        %s\n", name);
                    foreach (var node in colliding) {
                        if (node.symbol is Method) {
                            node.merged = true;
                        }
                    }
                }
                else {
                    stdout.printf("No match\n");
                }
                prop.get_accessor.value_type.value_owned = m.return_type.value_owned;
            }
        } else {
            prop.set_attribute ("NoAccessorMethod", true);
        }
    }

The (getter.name == name) compares the getter name (get_enabled) to the property name (enabled). I’m not sure this logic is correct. Adding the debugging code, plenty of “No match” are displayed, but no “**** Match ****” outputs are displayed when parsing gio-2.0.

I’m suspecting that the foreach, inside the body of this if, should iterate through the nodes and eliminate methods that match the method name (get_enabled). But, I don’t know what the intended purpose of this code segment.

You are still reading the same piece code as in the mailing list, look a tad earlier in the file: https://git.gnome.org/browse/vala/tree/vala/valagirparser.vala#n902

As for getter.name == name, that's wrong yes. Maybe it should simply look like:
getter.merged=true

About: for node in colliding: node.merged=true
This behavior must be kept somehow, as there are cases where a method has the same name of the property.

Thanks for looking into it.This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.

Also not only methods, especially there are signals that have the same name as the property. This is the priority: property > signal > method.

Why is this marked as fixed? :S

Why is this marked as fixed? :S

Created attachment 272112 [details]
Minimum GIR file to reproduce the issue

This file is a subset of Gio-2.0.gir for use with testing. The file has been reduced to a single interface with a single property.

To use:

vapigen --library minimum Minimum.gir

(In reply to comment #3)
> As for getter.name == name, that's wrong yes. Maybe it should simply look like:
> getter.merged=true
> 

Some properties have two getter methods associated with them. In the gir file attached, get_enabled has two entries -- one is a "method" and the other is a "virtual-method". Do both these entries need to be merged? Both these nodes appear when searching in valagirparser.vala beginning at line 902.

(In reply to comment #2)
> You are still reading the same piece code as in the mailing list, look a tad
> earlier in the file:
> https://git.gnome.org/browse/vala/tree/vala/valagirparser.vala#n902

I can't see anything wrong there. I've added debugging code:

Node getter = null;
stdout.printf("Looking for getter for property = %s\n", name);
var getters = parent.lookup_all ("get_%s".printf (name));
if (getters != null) {
    foreach (var g in getters) {
        stdout.printf("    Trying getter.name = %s\n", g.name);
        if ((getter == null || !g.merged) && g.get_cname () == parent.get_lower_case_cprefix() + "get_" + name) {
            stdout.printf("    Found getter.name  = %s\n", g.name);
            getter = g;
        }
    }
}

Here is the output:

Looking for getter for property = enabled
    Trying getter.name = get_enabled
    Found getter.name  = get_enabled
    Trying getter.name = get_enabled
No match
Generation succeeded - 0 warning(s)

This code is finding one of the two methods. So, I think the code is correct.

(In reply to comment #8)
> (In reply to comment #3)
> > As for getter.name == name, that's wrong yes. Maybe it should simply look like:
> > getter.merged=true
> > 
> 
> Some properties have two getter methods associated with them. In the gir file
> attached, get_enabled has two entries -- one is a "method" and the other is a
> "virtual-method". Do both these entries need to be merged? Both these nodes
> appear when searching in valagirparser.vala beginning at line 902.

I added more debugging code, and one of the methods is already merged. So, getter.merged=true may be it.

(In reply to comment #8)
> Some properties have two getter methods associated with them. In the gir file
> attached, get_enabled has two entries -- one is a "method" and the other is a
> "virtual-method". Do both these entries need to be merged? Both these nodes
> appear when searching in valagirparser.vala beginning at line 902.

Yes both must be merged, but if one is a virtual/abstract method this information must be propagated to the property. The gir is a bad guy to parse :S

(In reply to comment #8)
> (In reply to comment #3)
> > As for getter.name == name, that's wrong yes. Maybe it should simply look like:
> > getter.merged=true
> > 
> 
> Some properties have two getter methods associated with them. In the gir file
> attached, get_enabled has two entries -- one is a "method" and the other is a
> "virtual-method". Do both these entries need to be merged? Both these nodes
> appear when searching in valagirparser.vala beginning at line 902.

I added more debugging code, and one of the methods is already merged. So, getter.merged=true may be it.

(In reply to comment #11)
> Yes both must be merged, but if one is a virtual/abstract method this
> information must be propagated to the property. The gir is a bad guy to parse
> :S

When parsing the file, the virtual-method does take priority over the plain method. With the following test code, I reversed the method and virtual method in the input file:

    stdout.printf("Looking for node property = %s\n", name);
    Node getter = null;
    int unmerge_count = 0;
    var getters = parent.lookup_all ("get_%s".printf (name));
    if (getters != null) {
        foreach (var g in getters) {
            stdout.printf("    Checking node name=%s, cname=%s, merged=%s, element_name=%s\n", g.name, g.get_cname(), g.merged.to_string(), g.element_type);
            if ((getter == null || !g.merged) && g.get_cname () == parent.get_lower_case_cprefix() + "get_" + name) {
                stdout.printf("    Found node name=%s, cname=%s, merged=%s, element_name=%s\n", g.name, g.get_cname(), g.merged.to_string(), g.element_type);
                getter = g;
            }
            if (!g.merged) {
                unmerge_count++;
            }
        }
    }
    if (getter != null) {
        stdout.printf("Result node type = %s\n", getter.element_type);
    }
    stdout.printf("Count of unmerged items = %d\n", unmerge_count);
    assert (unmerge_count <= 1);

And gives the output with the minimum input file and another file where the two elements are reversed:

Looking for node property = enabled
    Checking node name=get_enabled, cname=g_action_get_enabled, merged=false, element_name=virtual-method
    Found node name=get_enabled, cname=g_action_get_enabled, merged=false, element_name=virtual-method
    Checking node name=get_enabled, cname=g_action_get_enabled, merged=true, element_name=method
Result node type = virtual-method
Count of unmerged items = 1
Generation succeeded - 0 warning(s)
Looking for node property = enabled
    Checking node name=get_enabled, cname=g_action_get_enabled, merged=true, element_name=method
    Found node name=get_enabled, cname=g_action_get_enabled, merged=true, element_name=method
    Checking node name=get_enabled, cname=g_action_get_enabled, merged=false, element_name=virtual-method
    Found node name=get_enabled, cname=g_action_get_enabled, merged=false, element_name=virtual-method
Result node type = virtual-method
Count of unmerged items = 1
Generation succeeded - 0 warning(s)

When both files are parsed, the forward and backward, the virtual-method takes precedence. So, I think the search function matches the behavior you describe, in this testcase.

Additionally, when parsing the full gio-2.0, a maximum of one unmerged item was found. If this case hold true in other gir files, the simple getter.merged=true will merge all the remaining getters.

What would be the next step?

Thanks a lot for digging into it. In the next days I'm going to review your insights and see how we can fix this at best. Feel free to provide a patch if you come with a good solution which keeps compatibility with all other generated vapis.

Created attachment 273638 [details] [review]
fix overridable getter/setter in girparser

Sorry for the delay. What do you think about the attached patch?

(In reply to comment #15)
> Created an attachment (id=273638) [details] [review]
> fix overridable getter/setter in girparser
> 
> Sorry for the delay. What do you think about the attached patch?

This breaks backwards compatibility...  can't we just remove the abstract from the methods instead?  So we would end up with something like

  public interface Action : GLib.Object {
    public bool get_enabled ();
    public abstract bool enabled { get; }
  }

-- 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/vala/issues/433.