ceylon / ceylon-compiler

DEPRECATED
GNU General Public License v2.0
138 stars 36 forks source link

constructor delegation and initializer locals #2171

Closed gavinking closed 9 years ago

gavinking commented 9 years ago

The typechecker accepts this code:

class Super {
    String name;
    shared new ()  {}
    shared new New() {
        name = "Gavin";
    }
    shared new Default() 
            extends New() {}
}

The backend produces this error:

Variable name might not have been initialized
tombentley commented 9 years ago

This breaks the heuristic I was using for cope with capture in the presence of constructor delegation.

Consider these:

class Smashing {
    String name;//captured
    shared new ()  {}
    shared new New() {
        name = "Gavin";
    }
    shared new Default() 
            extends New() {
        print(name);
    }
}
class Great {
    String name;// not captured
    shared new ()  {
        name = "Jim;
    }
    shared new New() {
        name = "Gavin";
    }
    shared new Default() 
            extends New() {}
}

I think a better rule would be when a function or value is referenced by more than one constructor in the chain of extends clauses then we need to consider it captured (since we use the member to pass the value between the two), but if it's just referenced by one constructor in each chain then it's not captured.

FroMage commented 9 years ago

@gavinking can you explain why the typechecker accepts this? Looks to me like the default constructor will leave name uninitialised, no?

FroMage commented 9 years ago

Oh, because it isn't used I suppose?

FroMage commented 9 years ago

Well it's not so clear. This is accepted:

class Super() {
    String name;
    if(true){
        name = "bar";
    }
}

But this not:

class Super() {
// ERROR:forward declaration may not occur in declaration section: 'name'
    String name;
}

So it's not totally clear to me why the default constructor would be allowed to leave things uninitialised.

FroMage commented 9 years ago

This is accepted, though:

class Super {
    String name;
    shared new ()  {}
}

Making it further weird that the previous was not.

FroMage commented 9 years ago

Well, I can fix the capturing. Sounds to me like only constructors which delegate are capturing, not those which are delegated to (the case ATM).

But we have many more issues related to definite initialisation of fields throws by javac, in the case of partial initialisation:

class Super {
    String name;
    String name2;
    shared new constr() {
        name = "Gavin";
// here javac complains that "name2" is not initialised
    }
    shared new default() 
            extends constr() {
// here javac complains that "name2" may already be initialised
        name2 = "";
        print(name);
        print(name2);
    }
}

So it sounds to me like we should add the @NoCheckInit to fields even in the absence of abstract initialisers, because even non-abstract initialisers may be partial initialisers.

FroMage commented 9 years ago

Right, in Java it's the non-delegating (to this) constructor's job to initialise every final field. You can't delegate to another constructor (of this) and initialise any final field.

FroMage commented 9 years ago

Now, is this a compiler (language) limit, or JVM limit, I'm not sure.

gavinking commented 9 years ago

You are allowed to have non-captured declarations in the initializer that are not definitely initialized at the end of the initializer section. Just like you're allowed to have the same thing in a function, or in a method in Java.

However, you're not allowed to have a forward declaration in the declaration section.

So all this is working as specified.

FroMage commented 9 years ago

Oh OK, it's the statements after it that make it part of the init section, I get it. Thanks.

gavinking commented 9 years ago

But we have many more issues related to definite initialisation of fields throws by javac, in the case of partial initialisation:

That code example should not have been accepted by the typechecker, and I'm shocked that it was. WTF? I have tests for that!

UPDATE: ignore this comment.

gavinking commented 9 years ago

OIC, name2 isn't shared. So sure, it should be accepted. My bad.

FroMage commented 9 years ago

And I'm back in MethodOrValueReferenceVisitor which I still think is written backwards, grrrr…

It's not enough to make delegating constructors capturing: the statements after them are also capturing, but not of everything.

FroMage commented 9 years ago

This is in fact extremely hard to get right. If I don't want to capture too many things I have to only mark things as captured if they end up used in two different generated constructors, UNLESS they are actually also captured by something else that captures, and there's absolutely no mechanism in place to differentiate the two sorts of capturers.

FroMage commented 9 years ago

I have made some progress, but my mind just melted. Hope I'll have better luck tomorrow.

FroMage commented 9 years ago

So I got the constructor capturing somewhat under control. Now I broke 88 tests, though.

FroMage commented 9 years ago

Man, one of those days I'll have to rewrite that capturing visitor into something comprehensible and fast. It's so backwards and pointless it's a wonder it works at all. I know I keep saying this and I don't do it, but damn…

FroMage commented 9 years ago

Done. And now I'm starting to understand how constructors work :)

tombentley commented 9 years ago

my mind just melted.

I sympathize, because it made my mind do the same thing when I was trying to get this stuff to work. Well done that you managed to close this one!

imo how we handle capture is one of the weakest, and yet most important parts of the JVM backend, and we should find time to improve it because it's getting very hard to maintain.