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

Traits incorrectly added by depends_on with Property #447

Open
serwy opened this issue Jan 31, 2019 · 4 comments
Open

Traits incorrectly added by depends_on with Property #447

serwy opened this issue Jan 31, 2019 · 4 comments
Labels
topic: traits listener rework Issues related to reworking listener infrastructure; see also EEP2, EEP3 type: bug

Comments

@serwy
Copy link

serwy commented Jan 31, 2019

Here's code that shows the behavior:

import traits.api as tr
class X(tr.HasStrictTraits):
    pass
class Y(tr.HasStrictTraits):
    x = tr.Instance(X)
    z = tr.Property(tr.Int, depends_on="x.jjj")
x = X()
print('jjj' in dir(x))  # False
y = Y(x=x)
print('jjj' in dir(x))  # True

print(x.trait_names())  # ['trait_added', 'trait_modified', 'jjj']

@mdickinson
Copy link
Member

Confirmed on current Traits master.

@mdickinson
Copy link
Member

mdickinson commented Jan 31, 2019

Note: this doesn't happen on Traits 4.6.0, but it's still true that jjj gets added to x's trait_names. It's the dir change in recent Traits that exposed this bug.

@rkern
Copy link
Member

rkern commented Jan 31, 2019

Ultimately, this happens because _on_trait_change() will force the creation of a trait here: https://github.com/enthought/traits/blob/master/traits/has_traits.py#L2525

(see here for the interpretation of that 2: https://github.com/enthought/traits/blob/master/traits/ctraits.c#L1172)

The trait that is added is a Disallow trait. Likely __dir__() should omit Disallow traits from its list. There are some cases where they are explicitly part of the class definition, not just HasStrictTraits.

We probably could explicitly check for the Disallow trait in _on_trait_change(), clean up the created trait, then raise a TraitError so that this is discovered when the change handler is being added.

@FedeMiorelli
Copy link

This issue has the side effect that pickling is broken in case there are references to nonexisting traits in depends_on.

In the example below, if I have a typo in depends_on, I get a pickling error related to Disallow.


import pickle

from traits.api import HasStrictTraits, Str, Property


class MyTrait(HasStrictTraits):
    MyString = Str
    MyProperty = Property(depends_on='MyStringWITHTYPO') # 'MyString' would work
    
    def save(self):
        with open('test.dump', 'wb') as fid:
            pickle.dump(self.__getstate__(), fid, protocol=2)
        
    def load(self):
        with open('test.dump', 'rb') as fid:
            self.__setstate__(pickle.load(fid))
        
    
    def _get_MyProperty(self):
        return self.MyString + ' **** '
    
m1 = MyTrait(MyString='test')
m1.save()


m2 = MyTrait()
m2.load()

print(m2.MyString)
print(m2.MyProperty)

Error:

  File "testpickle.py", line 19, in save
    pickle.dump(self.__getstate__(), fid, protocol=2)
_pickle.PicklingError: Can't pickle <class 'traits.trait_types.Disallow'>: it's not the same object as traits.trait_types.Disallow

@kitchoi kitchoi added the topic: traits listener rework Issues related to reworking listener infrastructure; see also EEP2, EEP3 label May 6, 2020
mdickinson pushed a commit that referenced this issue Jul 21, 2020
* Add tests for Property; they are failing now

* Add a test for pickability, it is passing

* Implement keyword 'observes' for using Property with observation

* Fix docstring format

* Update user manual accordingly

* Update example for user manual

* Add a test to demonstrate new behaviour to prevent issues like in #447

* Update API reference for depends_on

* More API reference update

* Rename observes to observe

* Expand docstring

* Add cross reference link for @cached_property

* Use HasStrictTraits in example

* Drive-by: Remove extra whitespace

* Drive-by: Fix up whitespaces in the moved documentation in appendix

* Two more whitespaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: traits listener rework Issues related to reworking listener infrastructure; see also EEP2, EEP3 type: bug
Projects
None yet
Development

No branches or pull requests

5 participants