#832 Runtime checking of non-nullable fields

ivan Tue 24 Nov 2009

Is it possible to create serializable object with non-nullable fields? For instance, I have very simple class:

@serializable class Foo { Str bar }

Because bar is non-nullable, I have to add constructor:

@serializable class Foo
{
  new make(Str bar) { this.bar = bar }
  Str bar
}

Instances of this class can be serialized, but can't be read back - because we need to either provide makeArgs in sys::IntStream.readObj, or provide default arguments to make arguments. For strings it is simple, but in more complex cases it looks like making fields nullable is the only solution.

Another problem with nullability related to classes with this-blocks, the class below won't compile:

class Bar
{
  new make(|This|? f := null) { f?.call(this) }
  Str foo
}

Probably in some cases compile-time check that all non-nullable fields are set should be replaced with runtime check? This (plus Type.makeFieldSetFunc will also help to solve nullability problem with Fan serialization - ObjDecoder can use Type.makeFieldSetFunc instead of fan.sys.Field.set with checkConst=false ( ObjDecoder.readComplexType seems to be the only place where this method is called)

Yuri Strot Tue 24 Nov 2009

+1 for compile-time check facility. This is particularly need for types without default value. For example:

class ImageView
{
  new make(|This|? f := null) { f?.call(this) }

  Image image
}

It's wrong to have nullable or fake image. On the other hand new make(Image image, |This|? f := null) lose declarative zest.

brian Tue 24 Nov 2009

To set the stage for the current design see #595.

Back in May we decided to do the simplest thing that would work and start off with the most restrictive design which is the current behavior - every non-nullable field must be set in all constructor code paths.

I think the current design works okay. Having the compiler check non-nullable fields has caught a bunch of errors for me (but likewise also sometimes can be annoying).

Perhaps now would be a good time to revisit this issue. Some options:

  • Option A: leave current design in place and deal with boiler plate the compiler requires
  • Option B: skip non-nullable field checks in constructors which take it-blocks
  • Option C: skip non-nullable field checks in constructors which take it-blocks and generate runtime checks at the end of the constructor (kind of tricky)

Or any other proposals?

ivan Tue 24 Nov 2009

Probably the compromise between B and C is to have predefined method like this:

public static Void checkNullableFields(Obj obj) //not sure about name
{
  obj.type.fields.findAll |field| { !field.of.isNullable}.each |field|
  {
    if(field.get(obj) == null) throw NullErr()
  }
}

andrey Tue 24 Nov 2009

Actually A->B->C looks more like roadmap, and Ivan can use proposed method while Fantom is between B and C :)

qualidafial Tue 24 Nov 2009

Option C seems to be both the safest and most convenient for everyday development.

andy Tue 24 Nov 2009

I think Option C should be the end game. Option B seems like a step backwards, so I don't think we should go there.

Yuri Strot Tue 24 Nov 2009

So, if I understand correctly with option C it's possible to have following:

class A
{
  new make(|This|? f := null) { f?.call(this) }
  B b
}

But a := A() will throw an exception, right?

May be it's better to mark field b as "not initialized" and check initialization at compile time? In this case a := A() have compile error "Field b should be initialized". Is it possible?

ivan Tue 24 Nov 2009

Yuri, I doubt it is possible to do a compile check here, except of the simplest case when make is called without parameters, but in fact this can and should be disabled by using |This| f instead of |This|? f.

Yuri Strot Tue 24 Nov 2009

Probably you are right. This is a task for code analyser, not for compiler. So Option C seems the best approach.

qualidafial Tue 24 Nov 2009

I've been thinking about how this affects class hierarchies with non-null fields and it-blocks.

class A
{
  Obj a
  new make(|This|? f := null { f(this) }
}

class B extends A
{
  Obj b
  new make(|This|? f := null) : super.make(f) {}
}

So where should the non-null field assignment take place, inside A.make or B.make? Both?

The problem comes up when you try to do this:

class C extends B
{
  Obj c
  new make(|This|? f := null) : super.make()
  {
    prepForItBlock()
    f(this)
  }
}

Here class C has some special prep to do before the it-block is called, so it does not pass the it-block to the superconstructor. A.make and B.make will return before C.make can invoke the it-block, so we cannot put any null error checking directly inside the superconstructors.

However in the Java implementation there seems to be a clean separation between fan constructors (which are implemented as static methods) and their Java-style constructor counterparts (which the fan constructors delegate to). This implementation detail gives us an opportunity to separate the null check from the main constructor by putting the null check only in the static methods. This way there is only one null check performed after the last constructor returns.

Perhaps we could standardize the "check for null fields" operation into a static method as well so that subclass static constructor methods could delegate the null check for the superclass's fields to a static method in the superclass:

public class C extends B
{
  public static C make(Func f)
  {
    C obj = new C(f);
    nullCheck$(obj);
    return obj;
  }

  public static void nullCheck$(C obj)
  {
    if (obj.c() == null)
      throw new NullErr()
    B.nullcheck$(obj); // chain null check to superclass
  }
}

What do you guys think?

brian Tue 24 Nov 2009

I've been thinking about how this affects class hierarchies with non-null fields and it-blocks.

I've been thinking along similar lines. I mentioned, but didn't get into the details that implementing runtime null checks will be really hairy.

But I think we can make a good case that a non-abstract should be fully configured correctly once its constructor completes, and shouldn't have to rely on additional work in the sub-class constructor to set its fields. That would be a big simplification to how we generate the fcode.

qualidafial Wed 25 Nov 2009

But I think we can make a good case that a non-abstract should be fully configured correctly once its constructor completes, and shouldn't have to rely on additional work in the sub-class constructor to set its fields

Yes, but in client code I don't care whether the object is internally consistent within the constructor call stack, as long as it's consistent before the make method returns. Consider the case from my earlier post:

obj := B() {
  a = "a"
  b = "b"
}

I'm initializing fields belonging to A and B in the same it-block. I don't care if the block is invoked inside A.make or B.make as long as it is called at some point. However if, as in class C I need to do something special before calling the it-block, I need the freedom to initialize those superclass variables inside the subclass constructor.

class C : B
{
  new make(|This|? f := null)
  {
    doStuff()
    f(this)
  }
}

obj2 := C() {
  a = "aa"
  b = "bb"
  c = "c"
}

My point in my earlier post is that it is still possible to ensure runtime safety by putting the null check in the static make method instead of in the actual constructor on the Java side. Assuming that Fan constructor calls (including reflective calls on a constructor slot) are always routed through the static make method and never directly to the Java constructor, we can put off the null check until after construction and still be confident that uninitialized fields will always throw an exception.

class A
{
  Obj a
  new make(|This|? f) { f?(this) }
}

class B : A
{
  Obj b
  new make(|This|? f) : super(f) {}
}

class C : B
{
  Obj c
  new make(|This|? f) : super()
  {
    doStuff()
    f(this)
  }
}

translates roughly to Java as:

public class A
{
  private Object a;

  public static A make(Func f)
  {
    A result = new A(f);
    checkFields(result);
    return result;
  }

  public static void checkFields(A obj)
  {
    if (obj.a==null) throw new InitErr();
  }

  public A(Func f)
  {
    if (f != null)
      f.call(this);
  }
}

public class B extends A
{
  private Object b;

  public static B make(Func f)
  {
    B result = new B(f);
    checkFields(result);
    return result;
  }

  public static void checkFields(B obj)
  {
    A.checkFields(obj);
    if (obj.b==null) throw new InitErr();
  }

  public B(Func f)
  {
    super(f);
  }
}

public class C extends B
{
  private Object c;

  public static C make(Func f)
  {
    C result = new C(f);
    checkFields(result);
    return result;
  }

  public static void checkFields(C obj)
  {
    B.checkFields(obj);
    if (obj.c==null) throw new InitErr();
  }

  public C(Func f)
  {
    super(null);
    doStuff();
    if (f != null)
      f.call(this);
  }
}

The one sure thing is that only one static make method will be called, e.g.

Obj o := C() {
  a = "1"
  b = "2"
  c = "3"
}

Follows the execution path:

  • Enter C.make(itBlock)
    • Enter C.new(itBlock)
      • Enter B.new(null)
        • Enter A.new(null)
        • Exit A.new(null)
      • Exit B.new(null)
      • Enter doStuff()
      • Exit doStuff()
      • Enter itBlock.call(this)
        • Set a
        • Set b
        • Set c
      • Exit itBlock.call(this)
    • Exit C.new()
    • Enter C.checkFields(newObject)
      • Enter B.checkFields(newObject)
        • Enter A.checkFields(newObject)
        • Exit A.checkFields(newObject)
      • Exit B.checkFields(newObject)
    • Exit C.checkFields(newObject)
  • Exit C.make()

No matter which Fan constructor is called, there is always a static make method at the top through which an object's constness / non-nullity of fields can be asserted once, without having to put this logic inside every constructor.

ivan Wed 25 Nov 2009

I agree with qualidafial that runtime null check can be placed only in one place - in the end of static Java make. But there still may be the problem in cases like this:

class A 
{
  new make(|This|? f) { f?.call(this) }
  Obj a
}

class B : A
{
  new make(|This|? f) : super(f) { doSomethingWithA }
  Void doSomethingWithA() { a.toStr }
}

class C : B
{
  new make(|This|? f) : super(null)
  {
    doSomePrelogic
    f?.call(this)
  }
}

In the example above when calling C { a = something}, method B.doSomethingWithA will fail with null error

But this can be avoided by making |This| non-nullable or making B final. So I think the simplicity and pros of qualidafial's proposals overweight this cons.

brian Wed 25 Nov 2009

Running the null checks outside of the construct does make sense from Java bytecode perspective, but we don't actually model things that way in fcode. I don't think it is impossible to do, just very tricky. Probably generate a virtual method called $nullChecks on classes and just require the runtime know it should emit a call to that method after construction.

That would solve both the inheritance and it-block issues regarding flexibility for non-null field checks. Basically moving static checks into the runtime.

I am not entirely happy with all the extra complexity and having to document and explain the rules though.

I also am not sure that we should delay non-nullable field checks until after subclass constructors have run. I am not sure it is consistent with how we deal with const fields where each level of the class hierarchy is responsible for its own fields, then locked down.

So I'd say my leaning is towards running null checks within the actual constructor at each level of the class hierarchy. That is more restrictive, which means we can change it later without breaking backward compatibility.

brian Wed 25 Nov 2009

Renamed from Serialization, nullability and this-blocks to Runtime checking of non-nullable fields

brian Wed 25 Nov 2009

Promoted to ticket #832 and assigned to brian

brian Tue 23 Mar 2010

Ticket resolved in 1.0.52

I finally have this problem fixed. This is another feature which I know many people have been waiting for...

The new rules for definite assignment of non-nullable fields:

As a special case, a constructor which takes an it-block as the last parameter allows definite assignment to be postponed to runtime. The it-block is required to set every non-nullable field or a FieldNotSetErr is raised at runtime. Consider this example:

class Foo
{
  new make(|This| f) { f(this) }
  Str name
}

Foo {}                   // throws FieldNotSetErr
Foo { name = "Brian" }   // ok

tactics Tue 23 Mar 2010

This will make nullable fields so much nicer :)

Good call on keeping the checks in place for non-it constructors. It's a bit more complicated, but I think it's the most important case where this happens.

lbertrand Tue 23 Mar 2010

Was waiting for this... Great.

Login or Signup to reply.