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 795225 - Report an error when initializing not auto properties
Report an error when initializing not auto properties
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Parser
unspecified
Other All
: Normal normal
: 0.42
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2018-04-13 08:23 UTC by Jeeyong Um
Modified: 2018-04-22 15:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix this issue (1.08 KB, application/octet-stream)
2018-04-13 08:23 UTC, Jeeyong Um
  Details
codegen: Handle property initializers of non-gobject classes (3.94 KB, patch)
2018-04-14 10:53 UTC, Rico Tzschichholz
none Details | Review
codegen: Handle property initializers of non-gobject classes (4.32 KB, patch)
2018-04-14 14:03 UTC, Rico Tzschichholz
rejected Details | Review
vala: Accessors of abstract properties cannot have bodies (1.76 KB, patch)
2018-04-16 11:12 UTC, Rico Tzschichholz
committed Details | Review
vala: Report a warning when initializing non-auto properties (883 bytes, patch)
2018-04-18 13:06 UTC, Jeeyong Um
none Details | Review
codegen: *Drop* support for non-auto property initializer in gobjects (2.83 KB, patch)
2018-04-22 15:38 UTC, Rico Tzschichholz
committed Details | Review
vala: Report an error when initializing non-auto properties (2.70 KB, patch)
2018-04-22 15:39 UTC, Rico Tzschichholz
committed Details | Review

Description Jeeyong Um 2018-04-13 08:23:43 UTC
Created attachment 370883 [details]
Fix this issue

Setting default values for not auto-implemented properties has no effect. However, there is no reporting about this.

Example:
class Foo {
    private int _val;
    public int val {
        set { _val = value; }
        get { return _val; }
        default = 10; // not work, but not an error reported either
    }
}
Comment 1 Al Thomas 2018-04-13 10:20:29 UTC
Comment on attachment 370883 [details]
Fix this issue

Thanks for the patch.

After giving this a quick look I wondered why the check is not done in the semantic analyzer. The code could be in the check () method of vala/valaproperty.vala and that would also produce an error for Genie as well.

Something like:

if (initializer != null && field == null) {
	error = true;
	Report.error (source_reference, "Property %s has no automatic backing field for default value, either remove default value or remove bodies from `get' and `set'".printf (get_full_name ()));
}

Can you make the example an invalid test like tests/objects/bug773956-1.test ?
Comment 2 Rico Tzschichholz 2018-04-14 10:53:20 UTC
Created attachment 370923 [details] [review]
codegen: Handle property initializers of non-gobject classes

73e553ac3488d641fb08b275bcf2636e3cf0de67 introduced this for GObject classes only

Add a warning for property default values in structs and compact classes.
Comment 3 Rico Tzschichholz 2018-04-14 14:03:21 UTC
Created attachment 370932 [details] [review]
codegen: Handle property initializers of non-gobject classes

73e553ac3488d641fb08b275bcf2636e3cf0de67 introduced this for GObject classes only

Add a warning for property default values in structs and compact classes.
Comment 4 Rico Tzschichholz 2018-04-15 17:44:33 UTC
Attachment 370932 [details] pushed as 9b3eedb - codegen: Handle property initializers of non-gobject classes
Comment 5 Jeeyong Um 2018-04-16 04:57:02 UTC
After applying commit 9b3eedbe81718a7a0bd9e5a97e4796e0eaa65e7f,
still no errors are reported for not working initializers.

According to the patch by Rico Tzschichholz,
he solved this issue not by raising an error, but running initializer for compact and gtype objects.

IMHO, there's a possibility to assign default values repeatedly for backing field and property, so allowing default value for backing field only seems more clear. If we consider the cases of the properties for primitive types, preventing from initializing non-auto properites is consistent.

Example:
class Foo {
    private int _bar = 1; // ambiguous
    public int bar {
        set { _bar = value; }
        get { return _ bar; }
        default = 2; // ambiguous
    }
}

If you allow, I will write new patch like what Al Thomas said. (with test cases)
The only concern is that my patch can cause breaks with some codes. For example,

objects/bug701978.vala:
public class Bar : Object {
    private Foo _foo;

    public Foo foo {
        get { return _foo; }
        set { _foo = value; }
        default = Foo (); // compile error!
    }
}

public class Baz {
    private Foo _foo;

    public Foo foo {
        get { return _foo; }
        set { _foo = value; }
        default = Foo (); // compile error!
    }
}

objectes/properties.vala:
abstract class Maman.EnumDefault {
    public abstract FooEnum bar { get; default = FooEnum.FOO; }
}
// compile error!
// No backing field here, either. C# also raises an error with this code.
// Of course, I don't mean Vala should satisfy all specs of C#.
Comment 6 Rico Tzschichholz 2018-04-16 08:05:42 UTC
> IMHO, there's a possibility to assign default values repeatedly for backing
> field and property, so allowing default value for backing field only seems
> more clear. If we consider the cases of the properties for primitive types,
> preventing from initializing non-auto properites is consistent.
> 
> Example:
> class Foo {
>     private int _bar = 1; // ambiguous
>     public int bar {
>         set { _bar = value; }
>         get { return _ bar; }
>         default = 2; // ambiguous
>     }
> }

This ambiguous setting is obviously a programmer error imho.

The setter can be way more complex and the default value looks like a more convenient way to invoke the custom setter. The problem here is that for non-gobject properties the given default value is actually always the argument of the setter and can not be the expected the return value of the getter. This I what I failed to see while writing my patch.

So you are right that default values are kind of doomed for non-automatic properties and it seems reasonable to forbid them.

I would prefer a warning though, maybe temporarily forcing an error for testing to see the impact of it.

> objectes/properties.vala:
> abstract class Maman.EnumDefault {
>     public abstract FooEnum bar { get; default = FooEnum.FOO; }
> }
> // compile error!
> // No backing field here, either. C# also raises an error with this code.
> // Of course, I don't mean Vala should satisfy all specs of C#.

Abstract properties are another things, but yes, this should trigger at least a warning too.

I will revert my patch.

https://bugzilla.gnome.org/show_bug.cgi?id=701978 needs some reconsideration.
Comment 7 Rico Tzschichholz 2018-04-16 11:12:31 UTC
Created attachment 370985 [details] [review]
vala: Accessors of abstract properties cannot have bodies
Comment 8 Jeeyong Um 2018-04-18 13:06:04 UTC
Created attachment 371090 [details] [review]
vala: Report a warning when initializing non-auto properties

I wrote a patch proposed by Al Thomas. However, I didn't include a test case, because this patch would raise a warning rather than an error. If more modifications are required, please let me know any time.
(I'm not a native English speaker, so don't mind modifying commit message, if necessary)
Comment 9 Rico Tzschichholz 2018-04-22 15:38:59 UTC
Created attachment 371233 [details] [review]
codegen: *Drop* support for non-auto property initializer in gobjects

Partly reverts 73e553ac3488d641fb08b275bcf2636e3cf0de67
https://bugzilla.gnome.org/show_bug.cgi?id=701978
Comment 10 Rico Tzschichholz 2018-04-22 15:39:53 UTC
Created attachment 371234 [details] [review]
vala: Report an error when initializing non-auto properties
Comment 11 Rico Tzschichholz 2018-04-22 15:42:08 UTC
Attachment 370985 [details] pushed as 6780a0f - vala: Accessors of abstract properties cannot have bodies
Attachment 371233 [details] pushed as 9adb863 - codegen: *Drop* support for non-auto property initializer in gobjects
Attachment 371234 [details] pushed as 0d396f7 - vala: Report an error when initializing non-auto properties