One of the problems I punted on until we had it-blocks figured out was how the nullable type system applies to fields.
Currently no checks are performed to ensure that a non-nullable field is set in the constructor. We do perform a check if it is assigned to ensure that you can't assign null. But we don't ever check that it actually got assigned. Personally I don't really mind this behavior, but based on previous discussions I think I am in a minority of one.
So we need to define exactly what the behavior should be since this is likely a breaking change.
There are two ways to verify that a field is a set:
at runtime by throwing an exception if the field is null
There are three ways that a field may get set during the construction process:
by the class's constructor(s)
by the sub-class's constructor(s)
by an it-block passed into the constructor/tacked onto the end of the constructor
We have a range of options with various trade-offs for type soundness, simplicity, and expressive flexibility:
Option A: simplest solution is to require that all a class's non-null fields have been assigned in each constructor using definite assignment. This is easy to enforce and guarantees that all non-nullable fields have been assigned a value. It is a little inflexible because it requires fields to be set in the constructor versus an it-block or sub-class. This option is pretty similar to how Java requires a final field to be set (although you could potentially set it multiple times in a given ctor).
Option B: add to option A a special hook if the constructor takes an it-block, to not require definite assignment and add runtime based null error checks at the end of the constructor. This would allow it-blocks to do the field sets. But we lose compile time checking, and we still don't get the flexibility to set the fields in subclasses or appended it-blocks. Given the trade-offs I am not sure the complexity is really worth it.
There are other more complicated options by adding new meta-data to the fcode and doing more sophisticated static analysis. But those two options above and variations on them, seem to the best I can come up with.
I personally lean towards doing nothing or taking the simple approach of option A.
Thoughts?
tacticsWed 13 May 2009
Option A seems simple and solves the problem. It's annoying to have to pick a default value, but option B seems a little magical. At least before a little more detail is put forward on plan B. I think overall, Option A is such a minor wart no one would ever really notice it's there.
tompalmerWed 13 May 2009
I'd recommend doing nothing or else fancy compile-time handling (track which are guaranteed in the constructor and whether an explicit final callback is guaranteed to be called, followed by any constructor calls needing to have it-blocks checked).
I think people would notice option A. In some cases, forcing people to initialize a field will feel ugly. "Why do I have to create a new object? I want people to choose the object in their it-blocks? These objects could be expensive. I'll just make the field nullable to work around it." Maybe won't happen, but it seems possible to see this kind of concern.
Then again, if you enforce option A, you could remove the check in the future. If you are lenient now, you can't ever add it back in without breaking code.
qualidafialWed 13 May 2009
I agree, option A is simple and works.
brianThu 14 May 2009
Promoted to ticket #595 and assigned to brian
Guess I'll go with option A.
andyThu 14 May 2009
Of all the options, I would prob pick A as well. Its going to be annoying - but its the easiest to implement, and is guaranteed to be forward compatible should be choose to do something more comprehensive.
brianThu 14 May 2009
Ticket resolved in 1.0.43
I've implemented this change and updated all the code (painful). I don't really love this design. But this is the most restrictive we could be, so it gives the option to be less strict in the future.
From the updated docs:
Non-nullable fields must be assigned in all code pathes. Each constructor which doesn't chain to this must set each non-nullable field which doesn't meet one of the following criteria:
value type fields don't need to be set (Bool, Int, Float)
Matching rules apply to static fields which must be set by all code paths in the static initializer.
KevinKelleySat 16 May 2009
I don't know, on one hand I've wished for this sometimes. But there's been a lot of times that the looseness, the freedom, of the system, has been a benefit. I'm a really big fan of nullable types, they catch a whole bunch of stuff that used to slip through. I'm a little nervous about changing it; it seems to be working and until somebody can say what's wrong with it, I'd not mess.
qualidafialSun 17 May 2009
I think we could loosen the definite assignment restriction whenever:
The constructor accepts a |This| argument (e.g. an it-block), and
It invokes the |This| function with this as the argument.
In these cases, the compiler should add an implicit runtime assignment check at the end of the constructor for each field not explicitly set by the constructor. My feeling is that it's better to raise an error there in the constructor than to wait for an unexpected null to cause problems later on (where it's impossible to know how/where the problem was introduced).
It may even be possible to do compile-time static analysis on it-blocks passed to these constructors to make sure that the it-block sets all the uninitialized fields, but that could get pretty complicated.
brianSun 17 May 2009
@qualidafial: I agree that we might want to move some of the checks to runtime if an it-block is used (that was option b in the original proposal). There were definitely a few places in the code where the compile time definite assignment was annoying, but fewer then I thought. In my case, most of those classes tended to be internal, utility classes versus public APIs.
So I think we should give a little time with the restrictions as I've gotten them. Because this is as about as strict as you can make it, we can loosen them up in the future without breaking anything.
brian Tue 12 May 2009
One of the problems I punted on until we had it-blocks figured out was how the nullable type system applies to fields.
Currently no checks are performed to ensure that a non-nullable field is set in the constructor. We do perform a check if it is assigned to ensure that you can't assign null. But we don't ever check that it actually got assigned. Personally I don't really mind this behavior, but based on previous discussions I think I am in a minority of one.
So we need to define exactly what the behavior should be since this is likely a breaking change.
There are two ways to verify that a field is a set:
There are three ways that a field may get set during the construction process:
We have a range of options with various trade-offs for type soundness, simplicity, and expressive flexibility:
Option A: simplest solution is to require that all a class's non-null fields have been assigned in each constructor using definite assignment. This is easy to enforce and guarantees that all non-nullable fields have been assigned a value. It is a little inflexible because it requires fields to be set in the constructor versus an it-block or sub-class. This option is pretty similar to how Java requires a final field to be set (although you could potentially set it multiple times in a given ctor).
Option B: add to option A a special hook if the constructor takes an it-block, to not require definite assignment and add runtime based null error checks at the end of the constructor. This would allow it-blocks to do the field sets. But we lose compile time checking, and we still don't get the flexibility to set the fields in subclasses or appended it-blocks. Given the trade-offs I am not sure the complexity is really worth it.
There are other more complicated options by adding new meta-data to the fcode and doing more sophisticated static analysis. But those two options above and variations on them, seem to the best I can come up with.
I personally lean towards doing nothing or taking the simple approach of option A.
Thoughts?
tactics Wed 13 May 2009
Option A seems simple and solves the problem. It's annoying to have to pick a default value, but option B seems a little magical. At least before a little more detail is put forward on plan B. I think overall, Option A is such a minor wart no one would ever really notice it's there.
tompalmer Wed 13 May 2009
I'd recommend doing nothing or else fancy compile-time handling (track which are guaranteed in the constructor and whether an explicit final callback is guaranteed to be called, followed by any constructor calls needing to have it-blocks checked).
I think people would notice option A. In some cases, forcing people to initialize a field will feel ugly. "Why do I have to create a new object? I want people to choose the object in their it-blocks? These objects could be expensive. I'll just make the field nullable to work around it." Maybe won't happen, but it seems possible to see this kind of concern.
Then again, if you enforce option A, you could remove the check in the future. If you are lenient now, you can't ever add it back in without breaking code.
qualidafial Wed 13 May 2009
I agree, option A is simple and works.
brian Thu 14 May 2009
Promoted to ticket #595 and assigned to brian
Guess I'll go with option A.
andy Thu 14 May 2009
Of all the options, I would prob pick A as well. Its going to be annoying - but its the easiest to implement, and is guaranteed to be forward compatible should be choose to do something more comprehensive.
brian Thu 14 May 2009
Ticket resolved in 1.0.43
I've implemented this change and updated all the code (painful). I don't really love this design. But this is the most restrictive we could be, so it gives the option to be less strict in the future.
From the updated docs:
Non-nullable fields must be assigned in all code pathes. Each constructor which doesn't chain to
this
must set each non-nullable field which doesn't meet one of the following criteria:Matching rules apply to static fields which must be set by all code paths in the static initializer.
KevinKelley Sat 16 May 2009
I don't know, on one hand I've wished for this sometimes. But there's been a lot of times that the looseness, the freedom, of the system, has been a benefit. I'm a really big fan of nullable types, they catch a whole bunch of stuff that used to slip through. I'm a little nervous about changing it; it seems to be working and until somebody can say what's wrong with it, I'd not mess.
qualidafial Sun 17 May 2009
I think we could loosen the definite assignment restriction whenever:
|This|
argument (e.g. an it-block), and|This|
function withthis
as the argument.In these cases, the compiler should add an implicit runtime assignment check at the end of the constructor for each field not explicitly set by the constructor. My feeling is that it's better to raise an error there in the constructor than to wait for an unexpected null to cause problems later on (where it's impossible to know how/where the problem was introduced).
It may even be possible to do compile-time static analysis on it-blocks passed to these constructors to make sure that the it-block sets all the uninitialized fields, but that could get pretty complicated.
brian Sun 17 May 2009
@qualidafial: I agree that we might want to move some of the checks to runtime if an it-block is used (that was option b in the original proposal). There were definitely a few places in the code where the compile time definite assignment was annoying, but fewer then I thought. In my case, most of those classes tended to be internal, utility classes versus public APIs.
So I think we should give a little time with the restrictions as I've gotten them. Because this is as about as strict as you can make it, we can loosen them up in the future without breaking anything.