GNOME Bugzilla – Bug 795225
Report an error when initializing not auto properties
Last modified: 2018-04-22 15:43:28 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 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 ?
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.
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.
Attachment 370932 [details] pushed as 9b3eedb - codegen: Handle property initializers of non-gobject classes
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#.
> 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.
Created attachment 370985 [details] [review] vala: Accessors of abstract properties cannot have bodies
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)
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
Created attachment 371234 [details] [review] vala: Report an error when initializing non-auto properties
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