Trait inheritance doesn't work as expected for List traits

[mdickinson]

Traits allows specifying a default value for a trait defined in a superclass. But this doesn't work in the expected way for List, Dict and Set traits. For example:

from traits.api import HasTraits, Int, List

class A(HasTraits):
    foo = List(Int)

class B(A):
    foo = [1, 2, 3]

After instantiating B with b = B(), we get the expected default, but b.foo no longer behaves like a validated list:

>>> b = B()
>>> b.foo  # as expected
[1, 2, 3]
>>> b.foo.append(4)  # also works
>>> b.foo.append("a string")  # should fail, but doesn't
>>> b.foo
[1, 2, 3, 4, 'a string']

Worse, the default is shared between all instances of B, which almost certainly isn't what's intended.

>>> c = B()
>>> c.foo
[1, 2, 3, 4, 'a string']

This doesn't appear to be a particularly new problem - it goes back at least to Traits 5.2.

[mdickinson]

It seems as though we always specify a default_value_type of DefaultValue.missing when overriding a default value in this way:

traits/traits/has_traits.py

Lines 593 to 599 in d1949c6

	if value.setattr_original_value:
	# Set the original, non validated value
	value.set_default_value(
	DefaultValue.missing, default_value)
	else:
	value.set_default_value(
	DefaultValue.missing, value.default)

That mostly works, because DefaultValue.missing is treated the same way as DefaultValue.constant for most purposes. But it's a little odd. I'm having a hard time figuring out what the intention was here.

[corranwebster]

A note that enthought/enable#945 is a different twist on the same root problem.

[corranwebster]

Another related failure for Expression traits:

>>> expr = Expression("1")
>>> clone = expr.clone("2")
>>> expr.default_value
'1'
>>> clone.default_value
<code object <module> at 0x7f938de36780, file "<string>", line 1>

This is getting at the setattr_original_value flag is trying to handle, but it should be in the clone method, not in has_traits.

[corranwebster]

My suspicion here is that the intent of DefaultValue.missing was meant to flag “hey, we don’t know what the original default value type was, so let’s look it up on the parent of the trait or something like that.

[corranwebster]

So after some digging and experimentation, in clone:

• traits which have default value types of constant, missing, list_copy, dict_copy, trait_list_object, trait_set_object and trait_dict_object should try to validate the new default and use that with their current default value type (ie. not make everything constant)
• traits which have default value types of object and callable don't make sense to override the default value, so we should just ignore and return the same default values as the trait being cloned (there is some risk of issues here, as callable defaults often use a method on the trait, and we will use something bound to the wrong trait instance; hopefully it will be OK in most cases)
• traits which use callable_and_args we can validate and use that as a constant default value. This keeps the previous behaviour, although it possibly shares the object across instances. Subclasses that don't like this can override clone in the short term.
• in has_traits unless there is setattr_original_value then we don't mess with the default value

Additionally traits which use setattr_original_value on their ctraits won't clone correctly:

• they need the original value passed in, not the validated value
• if we corrected the clone method to store the correct default in these cases then has_traits can simplify its code to not mess with default values at all; and in the longer term I think that the setattr_original_value ctrait attribute could be deprecated (the information would still be needed on the TraitType somewhere).
• some of these classes can be refactored to not need this: all the cases where this is used are mapped traits where the validation is expensive (eg. compiling or adapting a value) but the trait wants to store the original value in the trait and the validated value in the mapped trait. For these traits it may make sense just to refactor to do the computation twice (eg. in an Expression trait, compiling an expression twice is probably negligible), or if the expense is considerable, do the computation twice but wrapped in an appropriate cache.

For callable_and_args in the longer term:

• we may want to standardize a method for taking a validated value and extracting the factory, args and keywords (default for hastraits classes could be class plus non-transient traits as kwargs); or
• we may want to throw an exception if a default value is provided in the default clone method.

[mdickinson]

• traits which have default value types of constant, missing, list_copy, dict_copy, trait_list_object, trait_set_object and trait_dict_object should try to validate the new default and use that with their current default value type (ie. not make everything constant)

I think we shouldn't make any changes for list_copy and dict_copy: those are being phased out anyway for Traits 7. (See #1532 and related issues.)

For trait_list_object, trait_set_object and trait_dict object, I agree that we should either re-validate, or make it an error to override in this way. (And of course that was the original focus of this issue.)

missing should be almost completely irrelevant at this point (except in that it's used in the code pointed to above; those uses should probably be replaced with constant). For Traits 7.0, I'd like to be in a state where missing isn't used at all by the Traits core. It'll likely need to stick around for a bit in case it's being used externally.

constant is fine as-is, of course.

For the general solution, I'd like to avoid doing any kind of guesswork in the TraitType base class: I'd suggest that the only options for TraitType should be either use constant, or raise. If TraitType subclasses want something more dynamic, I think it should be the responsibility of those subclasses to extend or override the TraitType behaviour as appropriate. TraitType should be written in such a way as to make that extension easy, of course.

• we may want to throw an exception if a default value is provided in the default clone method.

Yes, I think this would be the right thing, together with examples / guidance about how to extend clone to provide different behaviour if necessary.

[corranwebster]

I've opened a draft PR starting with a bunch of tests that I think should pass (but which currently don't, for various reasons), and which roughly correspond to the above without breaking backwards compatibility too much. In particular, the tests only expect exceptions when you try to clone a trait with default value type "disallow".

Probably a good first step is to see if these match expectations.