Validate default value upon trait instantiation?

[kitchoi]

This came up a few times in the past (e.g. #886): Currently default value is not validated upon trait initialization, so one could do this

from traits.api import Int, HasTraits

class Foo(HasTraits):
    age = Int("ABC")

>>> f = Foo()
>>> f.age
"ABC"

This issue is for discussing whether or not we should validate default values for all the traits when the trait is instantiated (and inevitably the "how" if we do).

[corranwebster]

Because traits is lazy about populating trait values, this is almost impossible to do at instantiation time without major changes to the way that traits works that will likely break code that is depending on lazy trait population.

That said, it should be possible to validate the default value the first time the trait is used. The default is generated at that point either because the actual value is being used or because it is needed for the old value in a trait change handler.

Edit: Thinking a bit more, this may break things like Enums and Ranges with dynamic values if things aren't created in the right order - and it may be that there is no right order.

[mdickinson]

But for something like Int("ABC") (rather than using a dynamic default), I don't think there's anything stopping us from raising at TraitType instantiation time.

[mdickinson]

For example, Range already does this:

>>> from traits.api import HasTraits, Range
>>> class A(HasTraits):
...     foo = Range(2, 3j)
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in A
  File "/Users/mdickinson/Enthought/ETS/traits/traits/trait_types.py", line 1645, in __init__
    high = int(high)
TypeError: can't convert complex to int

[corranwebster]

So some of that problem is solved by the static type-checking stubs:

It doesn't solve the dynamic validation case (eg. we can't do Range with mypy) but catching them early is even better if we can do it.

[ievacerny]

Validation of the static default value in PrefixMap was removed since no other trait is doing it at the moment. But specifically with PrefixMap there is an issue with this lack of validation.

PrefixMap advertises that a prefix of a key is a valid value that can be mapped. And it does work as expected in a situation like this:

class Foo(HasTraits):
    age = PrefixMap({"yes": 1, "yeah": 1, "no": 0, "nah": 0})

f = Foo()
f.age = "yea"
print(f.age)  # prints "yeah"
print(f.age_)  # print "1"

However, setting the same valid value as default via a trait definition raises a KeyError:

class Foo(HasTraits):
    age = PrefixMap({"yes": 1, "yeah": 1, "no": 0, "nah": 0}, default_value='yea')

f = Foo()
print(f.age)  # prints "yea"
print(f.age_)  # raises "KeyError: 'yea'"

This happens because validate method modifies the value it receives.

[mdickinson]

Adding to the 6.1.0 milestone for the specific case of making sure that things work for PrefixMap.

[kitchoi]

The fact that static default value is not validated and therefore not transformed affects Union too, as I have found from #1092

>>> from traits.api import *
>>> 
>>> class Foo(HasTraits):
...     values = Union(List(Int), Set(Int), default_value=[1, 2])
... 
>>> foo = Foo()
>>> foo.values
[1, 2]
>>> foo.values.append("a")     # does NOT raise
>>> foo.values = [1, 2]
>>> foo.values.append("a")     # TraitError

[kitchoi]

For both Union and PrefixMap, if I move the static default to a dynamic default, then it works fine.:

class Foo(HasTraits):
    age = PrefixMap({"yes": 1, "yeah": 1, "no": 0, "nah": 0})
    def _age_default(self):
        return "yea"

foo = Foo()
print(foo.values)  # print 'yeah'
print(foo.values_)  # print 1

[ievacerny]

As far as I know it only affects default value defined via trait definition. Overridden default value also works fine:

class BaseFoo(HasTraits):
    age = PrefixMap({"yes": 1, "yeah": 1, "no": 0, "nah": 0}, default_value='ye')


class Foo(BaseFoo):
    age = "yea"


f = Foo()
print(f.age)  # prints "yeah"
print(f.age_)  # prints "1"

[mdickinson]

Another example of the general case:

>>> from traits.api import *
>>> class A(HasTraits):
...     foo = Int(True)
... 
>>> a = A()
>>> a.foo
True  # surprise! expect 1
>>> class B(HasTraits):
...     foo = Int()
...     def _foo_default(self): return True
... 
>>> b = B()
>>> b.foo  # works as expected
1

This is similar to the example in the original issue description, except that in this case the default actually should be considered valid. So this is IMO more important to fix if possible - the code in the original is a clear coding error, so to some extent what happens isn't that important, except that it would be nice to get early warning of the error. In this case, we're getting unexpected behaviour from something that is not really an error.

[kitchoi]

Opened #1130 for PrefixMap (see https://github.com/enthought/traits/issues/948#issuecomment-630871034) Opened #1131 for Union (see https://github.com/enthought/traits/issues/948#issuecomment-631480701).

[kitchoi]

The unexpected behaviour shown in https://github.com/enthought/traits/issues/948#issuecomment-632017803 https://github.com/enthought/traits/issues/948#issuecomment-631480701 https://github.com/enthought/traits/issues/948#issuecomment-630871034 all boil down to this issue:

Static valid default value is not transformed (unless it is provided via overriding subclass)

Traits combines validation and transformation. Validation should just mean raising an error when the given value is invalid. Transformation should just mean changing/adapting a given valid value.

Validation error could happen when a trait is defined or it could happen when the attribute is accessed (including changed) . But in general, the earlier a validation error occurs, the better. Different trait types have different constraints on when validation can actually be carried out.

Transformation could also happen at different places, but it is actually desirable for it to happen as late as possible, e.g. when the value is accessed. At that point, the transformation could happen when all the information about the HasTraits object and trait are available.

Ideally validation and transformation could be discussed in separate issues, but the fact that traits combines validation and transformation into one callable (validate) makes discussing them separately less practical: They do need to be discussed together.

The original issue here was initially about validation only: static default is not validated at the time when a trait type is instantiated. As Corran and Mark rightly pointed out, this could be difficult for some trait types (e.g. Enum) where the validation algorithm is dynamic and cannot be carried out prior to instantiating an instance of HasTraits.

At the time when the issue was open, none of the trait types does validation at instantiation. Since then, PrefixList and PrefixMap are introduced and they are the only trait types that raise a TraitError when the static default value is invalid. The consistency argument no longer holds to stop us from rolling out validation at trait types instantiation time, except for those where the validation algorithm depends on the HasTraits instance.

On the transformation matter, I think this is where the dynamic default value is transformed, at the time of attribute access: https://github.com/enthought/traits/blob/7e59d63930c27b7c8a8b40998d173109d9e699ad/traits/ctraits.c#L1822-L1830

For static default value provided via subclassing (BTW I think this is a misfeature, and see this comment too), like the example in https://github.com/enthought/traits/issues/948#issuecomment-631488796, the default value is transformed via a more convoluted route: First here: https://github.com/enthought/traits/blob/7e59d63930c27b7c8a8b40998d173109d9e699ad/traits/has_traits.py#L520 ictrait is an instance of CTrait. If you call it with a default value, you get to here: https://github.com/enthought/traits/blob/7e59d63930c27b7c8a8b40998d173109d9e699ad/traits/ctrait.py#L41-L54 Now the handler is an instance of the trait type, so calling that: https://github.com/enthought/traits/blob/7e59d63930c27b7c8a8b40998d173109d9e699ad/traits/trait_type.py#L336-L339 brings us to: https://github.com/enthought/traits/blob/7e59d63930c27b7c8a8b40998d173109d9e699ad/traits/trait_type.py#L297-L305

Notice that the validation error is silenced: It is doing transformation on valid value only! So if you do this, it is okay:

class Base(HasTraits):
    value = Int()
class Child(Base):
    value = "I am not an int"

The code path for a static default value provided directly on the trait type does not feature validation (hence no transformation), except for PrefixList and PrefixMap.

I am wondering how much easier this could be if we could decouple validation and transformation. This will allow transformation to be applied late at the time of first access, making it agnostic to when/how a default value is defined.

[kitchoi]

Now I am wondering if we should silence the validation error on PrefixList.__init__ and PrefixMap.__init__ before they are released but still performs the transformation if the value is valid. Dilemma: The right thing to do is not necessarily consistent, the consistent thing to do is not necessarily right.

[mdickinson]

Not for this release.