Created attachment 343781 [details] [review]
Fix memory leak when creating instance with object initializer

Object initializer increases reference count by 2 because of access to RHS expression and property setter. The increase of reference count by accessing to RHS expression is required for the field without property setter, but not for the field with that.

Review of attachment 343781 [details] [review]:

Not sure how to add a test for this, maybe:

int global_fake_reference_count;

void main () {
	global_fake_reference_count = 1;
	{
		new Foo() {baz = new Bar()};
	}
	assert_true (global_fake_reference_count == 0);
}

class Foo {
	public Bar baz {get; set;}
}

class Bar {
	~Bar() {
		global_fake_reference_count--;
	}
}

This works, but is not very robust because adding new Bar (); before new Foo() {baz = new Bar()}; will make the test pass.

I'm wondering if the patch should use a floating reference (GInitiallyUnowned) instead, but not sure how. Some useful docs on floating references:

https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#floating-ref
https://blog.conformal.com/gotk3-gtk3-the-go-way/
http://stackoverflow.com/questions/29662609/when-to-unref-a-gvariant-that-has-a-floating-reference

I understand what you mean, but this is a little different.
According to the reference of the link you attached, floating reference is used by newly created instance (which prefers being owned to referring itself).

However, as you can see the test code in the first comment, instantiated Person is strongly owned by local variable "member", and Family's instance just refers "member" one more time. ref_sink () in this case seems not possible. (already sunken by local variable)

Created attachment 343910 [details] [review]
codegen: Fix memory leak when using object initializer for properties

Assigning values to properties this way leads to a ref/copy of the source
and therefore requires a unref/destroy afterwards.

This problem is not restricted to ref-countable values and therefore requires a free of all destroyable values afterwards.

Although not ref'ing/copying in the first place be more appropriate.

@al: Regarding testing, I guess it would be useful to expose the ref/unref/ref_count members of non-gobject classes to be able to access them.

I did more research after my review comment and thought ownership transfer would be the solution.

I tried two things with ownership and the code:

1. Set the property in the class as unowned, so in the first example:
public unowned Person Member {get; set;}

This generates better code, the crucial bit is the person_new () result is assigned directly to a variable that is used in the family_set_Member () call. So now the destructor is called.
Of course _person_unref0 (member) still exists because "Member" exists in the scope. Here is the code produced:

	Person* member;
	gchar* _tmp0_;
	Person* _tmp1_ = NULL;
	Family* f;
	Family* _tmp2_ = NULL;
	_tmp0_ = g_strdup ("mehdi");
	_tmp1_ = person_new ();
	person_set_Name (_tmp1_, _tmp0_);
	member = _tmp1_;
	_tmp2_ = family_new ();
	family_set_Member (_tmp2_, member);
	f = _tmp2_;
	_family_unref0 (f);
	_person_unref0 (member);

Long term we should be aiming to remove as many of these temp variables as possible. So adding code to Vala that unrefs an object created in an object initializer, such as the first patch submitted, seems to embed in to the Vala compiler that temp variable needs to be unrefed. Even though the temp variable in the initializer will never be used because it is only created in the scope of the initializer and transferred to the object.

2. The other thing I tried was tranferring ownership in the initializer:
  var f = new Family() { Member = (owned)member };

This also produces code that means the destructor is called:
	Person* member;
	gchar* _tmp0_;
	Person* _tmp1_ = NULL;
	Family* f;
	Person* _tmp2_;
	Person* _tmp3_;
	Family* _tmp4_ = NULL;
	Family* _tmp5_;
	_tmp0_ = g_strdup ("mehdi");
	_tmp1_ = person_new ();
	person_set_Name (_tmp1_, _tmp0_);
	member = _tmp1_;
	_tmp2_ = member;
	member = NULL;
	_tmp3_ = _tmp2_;
	_tmp4_ = family_new ();
	family_set_Member (_tmp4_, _tmp3_);
	_tmp5_ = _tmp4_;
	_person_unref0 (_tmp3_);
	f = _tmp5_;
	_family_unref0 (f);
	_person_unref0 (member);

As Jee-Yong Um points out, the instantiated Person is strongly owned by local variable "member" and is unrefed at the end of the block. Although, I'm not sure why a transfer of ownership doesn't make _family_unref0 (f) do that.

The problem I had was trying this with my test:
new Foo() {baz = (owned)new Bar()};
gives "error: Reference transfer not supported for this expression"

--

I then looked at the parser to see what MemberInitializer did and also wondered about struct{} which can also use initializers of the form FooStruct bar = { Member = member };. Arrays can also use initializers, e.g. { new Test() }. I didn't get too far with that.

For automated testing it is easier to test for a double free - it will segfault in most cases :) For memory leaks there needs to be some way of tracking the memory. For structs and compact classes I wondered about recording the memory address of the variable, but not sure how it could then be checked after it should have been freed. Both have constructors so they could record &this in a globally scoped variable.

For GObject classes I wondered about a log variable, kind of like mocks. Mocks in testing log method calls. So a global string variable is appended to when the constructor and destructor are called. If they are only called once then the correct test result string is there. 

GType classes that don't inherit from Object also have constructors and destructors. So a logging string could be used. I didn't know ref/unref/ref_count weren't exposed. That would be good to expose. Exposing dup (or is it copy?) as well may allow more use of GType classes with interfaces, e.g. https://bugzilla.gnome.org/show_bug.cgi?id=554329 only works when inheriting from Object.

I wrote the patch like Rico Tzschichholz's one at first, but it cannot solve the following case.

void test2() {
  var member = new Person() {
    Name = "mehdi"
  };
  var f = new Family() {
    Member = member
  };
  var f2 = new Family() {
    Member = member
  };
}

which expands to.

void test2 (void) {
	Person* member;
	gchar* _tmp0_;
	Person* _tmp1_ = NULL;
	Family* f;
	Person* _tmp2_;
	Family* _tmp3_ = NULL;
	Family* f2;
	Person* _tmp4_;
	Family* _tmp5_ = NULL;
	_tmp0_ = g_strdup ("mehdi");
	_tmp1_ = person_new (); // +1
	person_set_Name (_tmp1_, _tmp0_);
	member = _tmp1_;
	_tmp2_ = _person_ref0 (member); // +2
	_tmp3_ = family_new ();
	family_set_Member (_tmp3_, _tmp2_); // +3
	f = _tmp3_;
	_tmp4_ = _person_ref0 (member); // +4
	_tmp5_ = family_new ();
	family_set_Member (_tmp5_, _tmp4_); // +5
	f2 = _tmp5_;
	_family_unref0 (f2); // +4
	_family_unref0 (f); // +3
	_person_unref0 (member); // +2
}

Local variable created temporary should be the target to be unreferenced.

@conr2d: This is not how the generated c-code looks like with my patch.

Your new test2() expands to:

void test2 (void) {
	Person* member;
	gchar* _tmp0_;
	Person* _tmp1_ = NULL;
	Family* f;
	Person* _tmp2_;
	Family* _tmp3_ = NULL;
	Family* f2;
	Person* _tmp4_;
	Family* _tmp5_ = NULL;
	_tmp0_ = g_strdup ("mehdi");
	_tmp1_ = person_new ();
	person_set_Name (_tmp1_, _tmp0_);
	_g_free0 (_tmp0_);
	member = _tmp1_;
	_tmp2_ = _person_ref0 (member);
	_tmp3_ = family_new ();
	family_set_Member (_tmp3_, _tmp2_);
	_person_unref0 (_tmp2_);
	f = _tmp3_;
	_tmp4_ = _person_ref0 (member);
	_tmp5_ = family_new ();
	family_set_Member (_tmp5_, _tmp4_);
	_person_unref0 (_tmp4_);
	f2 = _tmp5_;
	_family_unref0 (f2);
	_family_unref0 (f);
	_person_unref0 (member);
}

@Rico Tzschichholz

OMG, I took the c file generated by my previous patch for the result by yours. Sorry for wrong information. Your patch works well. Thank you. :)

Attachment 343910 [details] pushed as c0e955d - codegen: Fix memory leak when using object initializer for properties