The same problem occurs when calling instance methods of the class before base(), it compiles without warning and fails at run time.

classC: Object {
  public classC() {
    someMethod();
    base();
  }

  someMethod() {
     ...
  }
}

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

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

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

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

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

This shouldn't be hard to implement in the control flow check by checking for the "this" parameter being used before being initialized, right? Much like what we did for out parameters.

Created attachment 240371 [details] [review]
Fix is_chainup method

The is_chainup method didn't accept all forms of chain-up calls. Now this(), base() and Object() are supported as well

Required for bug 567269

Created attachment 240372 [details] [review]
Report error when using this before before chain-up

Fixes bug 567269

Review of attachment 240372 [details] [review]:

This looks quite safe thanks.

::: vala/valaflowanalyzer.vala
@@ +418,3 @@
 							// parameter
+							if (used_var.name == "this") {
+								Report.error (used_var.source_reference, "use of possibly unassigned local variable `%s'".printf (used_var.name));

Here we need an error specifically for "this". Something like: can't reference "this" before chaining up, or something like that.

::: vala/valamemberaccess.vala
@@ +924,3 @@
 		if (local != null) {
 			collection.add (local);
+		} else if (param != null && (param.direction == ParameterDirection.OUT || param.name == "this" && param.parent_node is CreationMethod && ((CreationMethod)param.parent_node).chain_up)) {

What if you use parent_symbol instead of parent_node?

::: vala/valamethodcall.vala
@@ +801,3 @@
+			var this_parameter = call.symbol_reference as Parameter;
+			if (this_parameter == null) {
+				var stmt = parent_statement;

You should be able to do (Block) parent_statement.parent_node; here. That I know, Block is the only container of statements.

Created attachment 240393 [details] [review]
Report error when using this before before chain-up

Addressed the mentioned issues:
- Change error message to something more meaningful
- use parent_symbol, no need to set parent_node for this_parameter
- cast directly to Block

Review of attachment 240393 [details] [review]:

It seems you attached the same patch as before.

Also, the patch seems to introduce a regression for structs:

public struct Foo {
        int i;
        public Foo () {
                this.bar();
                i=2;
        }
        public Foo.bar () {
        }
}

void main () {
}

With the patch it errors out, but it shouldn't.

Created attachment 240475 [details] [review]
Report error when using this before before chain-up

My bad, here is the correct version. With this patch I could not reproduce the error you found. However, I found another bug related to fixing this one:

struct Foo {
    public Foo () {}
    public Foo.foo () {}
    public Foo.bar () {
        this ();
        this ();
        this.foo ();
    }
}

Compiles without errors on master. The fix for bug 608548 might be related. There 'is_chainup' was added in order to only consider CreationMethods as chain-ups.

Also currently my patch breaks Object chain-ups because is_chainup returns false for Object() calls.

Created attachment 240826 [details] [review]
Report error when using this before before chain-up

I was wrong in that the previous patch did indeed introduce a regression. Since chain-ups to named constructors of struct are replaced by ObjectCreationExpressions, those were not found in the MethodCall.get_defined_variables.
What's now left are:
- this() chain-up in structs: the semantic analyzer needs to find those as well.
- Object() chain-up: not sure how I could find that from is_chainup. I'm not in the semantic analyzer pass anymore so I can't just compare with analyzer.object_type.

this() chain up in structs work, what's the problem? this.foo() doesn't work, so don't bother about that now.

Object(): CodeContext.get().analyzer

Ah sorry, I meant the flow check for this() chain-up in structs. this() for structs is not recorded with chain_up = true and so no error is reported for the following:

public struct Foo {
	int i;
	public Foo () {}
	public Foo.foo () {
		i = 1;
		this ();
	}
}

void main () {
}

Created attachment 240827 [details] [review]
Report error when using this before before chain-up

Make is_chainup recognize Object()

Your new patch doesn't change is_chainup.

Created attachment 240832 [details] [review]
Report error when using this before before chain-up

Right, something went wrong with git format-patch. This should be the correct patch.

Created attachment 245655 [details] [review]
Report error when using this before before chain-up

Changes in this patch:

* Changed the way check() looks for chain-ups. Should also fix bug 697342.
* is_chainup is now a property set by check(); also added a property 'struct_chainup' to ObjectCreationExpression as struct invocations are transformed.
* Made sure lambda expressions in CreationMethods define 'this'

Review of attachment 245655 [details] [review]:

Thanks for the patch. However, lambda expression do not define "this", but use "this".

The following must be an error:

class Foo : Object {
        int i;
        Foo () {
                SourceFunc foo = () => { return i++ == 0;};
                base();
        }
}

The following is fine:

class Foo : Object {
        int i;
        Foo () {
                base();
                SourceFunc foo = () => { return i++ == 0;};
        }
}

Other than that, the patch looks fine overall.

Lambdas at a whole must be threaten like any other "this" usage.

Notice that lambdas may or may not capture "this". They use "this" only if "this" is captured, i.e. when the lambda is a closure.

Created attachment 246348 [details] [review]
Report error when using this before before chain-up

* don't define 'this' parameter, add to used variables of lambdaexpression.
* Create new 'this' parameter for the generated method so we don't reference the creation method

Fixes bug 567269

Review of attachment 246348 [details] [review]:

Thanks for the patch. Anyway, did you ever try using lambdas in creation methods? This must work:

public class Bar {
        public Bar () {
                SourceFunc f = () => { return true; };
        }
}

void main () {
}

::: vala/valaflowanalyzer.vala
@@ +418,3 @@
 							// parameter
+							if (used_var.name == "this") {
+								Report.error (used_var.source_reference, "can't reference `this' before chaining up");

Please issue a warning here, "possible reference to `this' before chaining up".

@@ +454,3 @@
 						// parameter
+						if (var_symbol.name == "this") {
+							Report.error (node.source_reference, "can't reference `this' before chaining up");

Warning here.

::: vala/valalambdaexpression.vala
@@ +255,3 @@
+		if (captures_creation_method) {
+			Symbol sym = (Block)parent_statement.parent_node;
+			do {

Is this necessary for this patch to work? And why?

Created attachment 291542 [details] [review]
Improve constructor chain-up checks

I decided to work on this once more and I think it is complete now. I split the changes into smaller patches to make them easier to understand.

Created attachment 291543 [details] [review]
Warn when using this before chain up

Created attachment 291544 [details] [review]
Make lambdas work in chain ups again

Created attachment 291545 [details] [review]
Allow 'static' lambdas before chain up

Created attachment 291546 [details]
Test cases for valid/invalid chainups

Thought I'd also share the test cases I used. With all patches applied, all tests should pass. Also contains negative tests (those must fail) so I can't add them to the official tests.

Missing negative test cases in our compiler is very bad. Hope I can work on it soon. It's not trivial due to performance bottlenecks when using make check.

Thanks for the patches will look at them.

The following code says there are multiple constructor calls:

class Foo : Object {
	public Foo () {
		var a = 2+3 == 5;
		if (a) {
			Object();
		} else {
			Object();
		}
	}
}

void main () {
}

It's the last standing in valamethodcall.vala

---

The following code must warn, because if the method is marked to chain up, it must chain up always:

class Foo : Object {
	public Foo () {
		var a = 2+3 == 5;
		if (a) {
			Object();
		}
	}
}

void main () {
}

This is fixable in the flowanalyzer, similar to out parameters by checking at the end of the method that "this" is being defined.

To point it out, you are doing a really *great* work. Sorry for rejecting the patches as needs-work, however I'd like to fix this once for all and not think about it once again.

Created attachment 291655 [details] [review]
Allow multiple chain-up calls in the constructor

Ok, how about this? I had the concern that this would allow reassignment to 'this', for example:

class Foo : Object {
    public Foo ()
        Object ();
        if (2 == 2) {
            Object();  // maybe already init'd
        }
    }
}


So I added a warning in cases like this. Another possibility is to unref 'this' before chainup (if not NULL).

A warning is fine. We cannot first create then unref, as constructors may have side effects.

Just saw that my 4th patch introduces a bug in combination with generics, will have to look at it later.

Created attachment 291729 [details] [review]
Also track instance access when using generic types

This should fix the last bug. Variables with generic types implicitly need instance access in order to call the destroy/dup functions of the type parameter.

Right. Good catch, will review.

Review of attachment 291729 [details] [review]:

I'm not able to udnerstand the need of this patch, can you provide a quick test case of when is this needed?

Ok, this is what I came up with. I guess it is a rather uncommon case but still:

public class Test<G> : Object {
	public Test () {
		SourceFunc f = () => {
			G a = null;
			G b = a;
			return true;
		};
		f ();

		Object ();
	}

	static void main () {
	}
}

In the generated function you can see that generic dup/free functions are accessed which require self to be initialized.

Actually the example compiles just fine as 'a' is null. A better example would involve capturing a generic variable/parameter, which is prevented by bug 746200.

Comment on attachment 291542 [details] [review]
Improve constructor chain-up checks

commit 73b9e4b4a2c059ceb6bce7ab173050fb70739892
Author: Simon Werbeck <simon.werbeck@gmail.com>
Date:   Wed Nov 26 11:53:31 2014 +0100

    Improve constructor chain-up checks

If this should be fixed by calling first base constructors, as should be, this should not be a bug but a programming error, following GObject construction rules. At the end Vala code is GObject, then you should know very well how it is working in order to avoid object construction errors.

Set is as non-bug. Feel free to create a new one as enhancement on GObject construction checks, but tagged for Vala 2.0.

Please this open.

Please leave this open.

(In reply to Daniel Espinosa from comment #44)
> If this should be fixed by calling first base constructors, as should be,
> this should not be a bug but a programming error, following GObject
> construction rules. At the end Vala code is GObject, then you should know
> very well how it is working in order to avoid object construction errors.
> 
> Set is as non-bug. Feel free to create a new one as enhancement on GObject
> construction checks, but tagged for Vala 2.0.

This bug has significant work done on it. Maybe it is not a "bug" because you argue it is programmer error, but it is certainly an "enhancement" and with several patches being submitted. To close this report and ask for a new submission is counter-productive.

I have looked over this recently and the comments suggest it is close to completion. It may also serve as a useful example of using the flow analyzer in Vala.

Comment 31 has a zip file of test cases, both pass and fail. What would be really helpful would be to change those zipped tests into a patch that can be applied to /tests in Vala.

The patches are staged here for some time.
https://git.gnome.org/browse/vala/log/?h=wip/bug567269

I am surprised that the following code no longer compiles after commit 73b9e4b4a2c059ceb6bce7ab173050fb70739892.  Is it intended?

$ cat > chainup.vala <<EOF
class Foo : Object {
    public Foo.with_foo (int a) {
        print (a.to_string ());
    }
    public Foo.with_bar (int b) {
        with_foo (b); // "this." is omitted
    }
}
EOF

$ valac -C chainup.vala
chainup.vala:6.9-6.20: error: use `new' operator to create new objects
        with_foo (b);
        ^^^^^^^^^^^^
Compilation failed: 1 error(s), 0 warning(s)

The compiler suggest to use `new' here, but that makes the generated code return unref()'ed object.

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

Created attachment 368568 [details] [review]
tests: Add some invalid "chain-up" tests

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

-- 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/22.