search

enthought / traits

Observable typed attributes for Python classes
Other
432 stars 85 forks source link

Type coercion of Tuple trait is inconsistent #1619

Closed mdsmarte closed 2 years ago

mdsmarte commented 2 years ago

The Tuple trait coerces list input to a tuple when it is nested within a List trait, but not when it is a top-level trait.

from traits.api import *

class Test(HasTraits):

    foo = Tuple(Int(), Int())
    bar = List(Tuple(Int(), Int()))

Test(bar=[[1, 2], [3, 4]])  # This works
Test(foo=[1, 2])  # This raises an error

It is ambiguous which behavior is "correct." Should a top-level Tuple trait coerce a list to a tuple or should a nested Tuple trait raise an error for list input?

mdickinson commented 2 years ago

Thanks for the report. All things being equal, I'd expect not to be able to assign a List where a tuple is expected, so I'd call this a bug in the bar case. Though there's potential for breakage when we fix this, so I don't think it should be fixed in a bugfix release - it should wait for the next feature release.

mdickinson commented 2 years ago

Part of the difference is coming from the difference in validation between Tuple and BaseTuple: Tuple insists on a tuple, while BaseTuple happily accepts a list. But why we're ending up with the BaseTuple validation behaviour instead of the Tuple validation behaviour inside the list, I'm not sure. It may have to do with the way that the fast validation gets set.

Python 3.10.2 (main, Jan 15 2022, 10:49:49) [Clang 12.0.5 (clang-1205.0.22.11)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from traits.api import *
>>> class Test(HasTraits):
...     foo = Tuple(Int(), Int())
...     bar = BaseTuple(Int(), Int())
... 
>>> t = Test()
>>> t.bar = [1, 2]
>>> t.bar
(1, 2)
>>> t.foo = [1, 2]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/Enthought/ETS/traits/traits/base_trait_handler.py", line 74, in error
    raise TraitError(
traits.trait_errors.TraitError: The 'foo' trait of a Test instance must be a tuple of the form: (an integer, an integer), but a value of [1, 2] <class 'list'> was specified.
>>> t.foo
(0, 0)
mdickinson commented 2 years ago

Okay, I think there are two bugs here.

  1. The shallow, easy-to-fix bug is that the Tuple.validate validation semantics (which are currently inherited from BaseTuple) don't match the fast_validate validation. We can either fix this in BaseTuple, or if we're feeling cautious, we can reimplement the validate method in Tuple. It's probably reasonably safe to fix in BaseTuple if we're not releasing the change in a bugfix release.
  2. The deeper bug is that TraitListObject is using .handler.validate on the inner trait instead of plain old .validate. I'm not sure what might break if we change this.

https://github.com/enthought/traits/blob/c8bd6e5f332d44b512b79f0bee3cb814b9125352/traits/trait_list_object.py#L860

mdickinson commented 2 years ago

1625 should fix the effect that you're seeing.

Separately from this, we might want to tighten the BaseTuple validation to only accept tuples. That would be a breaking change, so it would need a deprecation warning, but my guess would be that there's very little code that's using BaseTuple directly.

mdickinson commented 2 years ago

Closing here. Current status: