#443 Constructor Validation

brian Mon 26 Jan 2009

Of the all the critical issues remaining, perhaps the most critical one to tackle is how constructors and with-blocks interact. Today a with-block has full access to change any public field of the instance after the constructor has run. Obviously this might prevent the class designer from being able to validate the instance completely in the constructor.

Although the issue isn't really with-blocks per se, because if you expose a public non-const field then anyone can set it and you must handle validation in your setter or some other way. The issue here is that I allow with-blocks to modify const fields after the constructor is run (the with-block must be attached to the ctor).

It is desirable to set const fields in a with-block, in fact I use this technique extensively in the fwt where some styling properties must be defined at construction time, but I don't want to create a zillion constructors. But in these cases I don't particular care about validation (anymore than I care about any other public field). So my point is that being able to set const fields in a with-block is extremely important.

The simplest work around is to do nothing, and let the developer handle it "Java style". A Java style work around would be to make the field private, expose it via a getter, and force initialization through the constructor. This isn't the best solution, but I don't think its really all the bad because this issue rarely comes up for me.

Another proposed language change might be to combine the readonly and const keyword to mean that a given field cannot be set via a with-block, and must be validated by the constructor.

Another option is to add a new hook which as been given various names although most of proposals called it onNew - a callback run after constructor and with-blocks. The advantage of this mechanism is that we can also use it after serialization.

So are you guys happy with the status quo and Java style work arounds? If not which of the other two solutions do you prefer (or do you have another suggestion)?

jodastephen Wed 28 Jan 2009

I haven't ignored this BTW, I'm just a little busy to come up with a full proposal. I don't like the "Java style" workaround idea but haven't bottomed out an alternative yet.

jodastephen Sat 31 Jan 2009

I've spent the whole afternoon re-reading the old threads and trying to come up with a good solution. There are a lot of competing forces, and I've not found anything that great. Requirements/points I seem to have found mentioned are:

  • should be able to to const state set in constructor the can't be changed by with-blocks (aids readability of client)
  • should be able to set const state set in construction-with-blocks (simplicity of client setting up objects)
  • need to be able to validate the class at the end of construction process (to ensure state is valid)
  • should be able to set const state set in validator (override other settings)
  • need to support combination of constructor and construction-with-block (eg. definite state setup in constructor, additional params in construction-with-block)
  • constructors are familiar to Java/C#
  • with-block is very useful declarative style
  • both constructor and with-block are useful ways to initialise an object
  • code reader does not know whether field is const or not
  • relying on closures passed to constructor results in excessive numbers of closures
  • excessive verbosity is poor
  • the serialization syntax is very neat and important to get right
  • deserialization needs to be able to replace an input object with a singleton (normal code might want to as well)

Experiments and investigation

Point() {x=6; y=10} {x=7; y=9}

The second with-block does not compile. Only one with-block is permitted.

Point() {x=6; y=methodOnPoint()}
p := Point() {x=6; y=p.methodOnPoint()}
p := Point() {x=6; y=p.x}

Cannot call method/field on Point from within construction-with-block. This means that the caller can never see the invalid state of the Point part way through construction (a good thing).

Point() {x=6; y=10}

The meaning of this expression varies depending on whether Point() is calling a static factory or a constructor. In the former, the block is a standard-with-block and cannot set const fields. In the latter it is a construction-with-block and can set const fields. There's no way to tell without looking at the Point class, which is rather confusing.

Point {x=6; y=10}

Worse is that the above only compiles if this refers to a constructor, which means that the "brackets are optional" rule doesn't hold.

Serialization is not a subset of the general language syntax. Taking a serialized file and pasting it into source code will not work, as any private fields will fail to compile. In addition, setting const fields will fail to compile depending on whether the make method is a constructor or not.

There is no way to replace the created instance at the end of the construction or with-block phase. If the factories of a class are trying to maintain a singleton, it just won't work. For example, we may have a singleton Point instance that we have to use if x and y are zero. This requires the ability to return a replacement instance instead of the originally create instance.

I also have concerns about immutability, as the Java Memory Model requires all fields of a class to be final in order for the object to be safely published. The approach we are taking will involve creating "immutable" classes that don't follow that rule. Not sure how that maps to JVM bytecode.

brian Sun 1 Feb 2009

Thanks for taking the time to do such a long writeup. I think your requirements and points are all pretty much on the money.

In the latter it is a construction-with-block and can set const fields. There's no way to tell without looking at the Point class, which is rather confusing.

I agree this isn't perfect - although in some respects with-blocks are orthogonal to construction. A general with-block can be appended to any expression. It just so happens that we make a special exception to allow setting const fields as long as we can guarantee that the object is still being constructed. The only way we can guarantee that, is that the target expression is actually a constructor. So it is indeed a case where the uniform access principle b/w ctor and factory breaks down.

I've been writing a ton of Fan code this month, and I'm finding that I use const fields in three different ways:

  1. as a config property which must be set at construction time which I expect to be set via with-block (FWT is perfect example)
  2. as the state fields of an immutable const class (like id, name, etc) which is pretty much always set in ctor and never in with-block
  3. to override a method in super class or mixin with a const fixed value. This isn't allowed yet, but I need to fix the compiler to allow it. Basically it will work like field overrides of methods, except you only generate a getter method (no setter).

So a simple solution to issue 2 would go a long way to helping this issue. So I come back around to my original simple proposal:

You can annotate a const field with the readonly keyword which prevents the field be set in a with-block. This has the exact semantics as readonly works already on fields - which basically is sugar for making the setter private. Note that it already works this way for non-const readonly fields - you can't set them via with-block on the ctor.

Example:

class Foo
{
  const Int x
  readonly const Int y
} 

Foo { x = 5 }  // ok
Foo { y = 6 }  // illegal - "setter" is private

Obviously this doesn't take into all the possible use cases. But I do believe based on my experience that is solves a huge chunk of the use cases and doesn't introduce any new complexity. Then until we get some experience under our belt with Fan 1.0, we can tackle remaining use cases with the tried and true techniques we have learned to use in our Java code (they all work fine Fan).

So I really like this change, it is simple and the more I think about it, the more it is actually self consistent with how readonly already works today. And I believe this small change will let us move to solving most issues with normal constructors.

jodastephen Sun 1 Feb 2009

A general with-block can be appended to any expression

This isn't entirely true either. You can do:

Window { Label { text = "Hello world" } }.open

(from Fan's examples) which uses the construction-with-block as an expression on which "open" is called. Yet you can't do:

Window { Label { text = "Hello world" } } { Label { text = "Hello world" } }

as far as I can tell. A constructor with a construction-with-block is not an expression (yet it is assigned to a variable as though it is - what about passing it in a method signature?)

brian Sun 1 Feb 2009

A constructor with a construction-with-block is not an expression (yet it is assigned to a variable as though it is

Yeah you are right. According the BNF grammar I have defined you should be able to chain with-blocks. So the parser needs to be tweaked to allow that. Not that I think that is necessarily good practice, but I guess the grammar should be consistent with regard to expressions.

So Stephen what do you think of readonly const fields to prevent with-block sets and then moving other checks into normal constructors?

brian Fri 6 Feb 2009

I think no matter what more sophisticated solutions we develop, that it still makes sense to do this: allow readonly to be used on a const field to prevent it from being set outside via with-block.

I am also going to allow const fields to override a virtual method:

abstract class A
{
  abstract Str id()
}

class B : A
{
  override const Str id
}

jodastephen Fri 6 Feb 2009

I believe the semantics on readonly const aren't right. Its not that you don't want to allow a setter, its that you don't want to allow setting during a construction-with-block.

This matters is a feature is added to Fan to allow const field to be set. By this I mean setters which return a new instance of the parent object:

// Joda-Time
LocalDate date = new LocalDate(2008, 6, 30);
date = date.withYear(2009);

Here the "with" method is a setter for const. (Fan has similar in gwt::Font). Currently these methods have to be hand coded in Fan, but a future feature could generate them. If that feature were added, readonly becomes meaningful to prevent that feature operating.

As such, I'm not convinced that using readonly in this way is the best option.

andy Fri 6 Feb 2009

allow readonly to be used on a const field to prevent it from being set outside via with-block.

That really doesn't make sense to me - const implies readonly already - so that just seems like a hack to get around the root issue.

brian Sat 7 Feb 2009

I believe the semantics on readonly const aren't right

Just to be the clear: the semantics of readonly today are that the setter is private and that only declaring class can set the field.

That is exactly what we are talking about here - only the declaring class can set the const field, not an external with-block. The fact that the field only has a limited window of time where it can be set doesn't make it any less consistent with protecting who can set it.

Note that this issue is really orthogonal (or most likely orthoganal to other discussions). It is really just about the protection scope of who can set the const field.

So I don't think I could be convinced otherwise that readonly is the most consistent way to do it. However, I do think there is the problem that readonly isn't necessarily the best term to mean "private setter". That is why readonly const seems weird.

Can anyone suggest a better term to use than readonly?

alexlamsl Sat 7 Feb 2009

Got some pretty unsatisfactory ones:

concealed masked instinctive shielded

I think we are trying to capture a word that means "cannot be affected externally" yet does not mean "constant"...

EDIT: I am not a native english speaker, but IMHO const implies readonly to me. So in fact const does not mean readonly in Fan today already sounds unnatural to me...

brian Sat 7 Feb 2009

EDIT: I am not a native english speaker, but IMHO const implies readonly to me. So in fact const does not mean readonly in Fan today already sounds unnatural to me...

I originally picked readonly to imply that from the client's perspective you can only read it, not set it. But I agree it is really a terrible keyword, especially when used in conjunction with const fields.

So I think I've convinced myself that readonly needs to get replaced with a more intuitive keyword.

Or another option is to force you to write it out:

Str someField { private set }

andy Sat 7 Feb 2009

Yeah I've never really liked readonly wrt const either. Probably the easiest thing to do is just remove it and force you to specify the protection level manually. Then if we find we want to add a keyword as a shortcut it would be non-breaking (besides of course using up an identifier).

alexlamsl Sun 8 Feb 2009

If so, we shall have:

class A {
  const Str a {private set}
}

which does look intuitive enough to me, and there isn't really that much more writings involved, is there?

brian Fri 13 Feb 2009

I have pushed the change set to allow a const field to override a virtual method with no parameters. This comes in really handy:

mixin Rec
{
  abstract Str id()
}

class MyRec : Rec
{
  override const Str id
}

I haven't gotten any further comments about how to deal with protection scope of field setters including const fields. The current proposal is to remove the readonly keyword and force you to write:

const Str id { private set }

Any more feedback on this issue before I make that change?

alexlamsl Fri 13 Feb 2009

So we can have immutable subclass of mutable superclass (and vice versa)?

brian Fri 13 Feb 2009

So we can have immutable subclass of mutable superclass (and vice versa)?

No you cannot. A const class can only be subclassed from Obj or another const class. You cannot create a non-const subclass from a const class either.

Note mixins work a little different in that by definition they define no state only interfaces, so mixins can be const or non-const (new to next build). Const mixins can only be mixed into const classes. Non-const mixins can be mixed into const classes too, but must be implemented immutability (so you couldn't actually include a mixin with abstract fields into a const class).

Note that being able to override a method with const is a bit orthogonal to that concept. Non-const classes can have const fields just fine too as my example shows.

Login or Signup to reply.