Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type coercion of Tuple trait is inconsistent #1619

Closed
mdsmarte opened this issue Feb 21, 2022 · 5 comments
Closed

Type coercion of Tuple trait is inconsistent #1619

mdsmarte opened this issue Feb 21, 2022 · 5 comments
Labels
component: core Issues related to the core library type: bug

Comments

@mdsmarte
Copy link
Contributor

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
Copy link
Member

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 mdickinson added component: core Issues related to the core library type: bug labels Feb 22, 2022
@mdickinson
Copy link
Member

mdickinson commented Feb 22, 2022

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
Copy link
Member

mdickinson commented Feb 22, 2022

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.

trait_validator = self.trait.item_trait.handler.validate

@mdickinson
Copy link
Member

#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
Copy link
Member

Closing here. Current status:

  • The deeper bug of list validation using the handler instead of the CTrait is fixed, with that fix scheduled to be released in Traits 6.4.
  • Acceptance of lists by a plain Tuple() is deprecated.
  • Acceptance of lists by BaseTuple() is not yet deprecated; Deprecate acceptance of lists by BaseTuple and Tuple traits #1626 is open to track this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: core Issues related to the core library type: bug
Projects
None yet
Development

No branches or pull requests

2 participants