remove_trait does not remove listeners for extended traits

[kitchoi]

Consider this:

from traits.api import HasTraits, Instance, Str

class Foo(HasTraits):
    name = Str()

def handler(object, name, old, new):
    print("Changed!", (object, name, old, new))

foo = Foo()
foo.add_trait("child", Instance(Foo))
foo.on_trait_change(handler, "child:name")

The following behaviour is good:

child = Foo()
foo.child = child
foo.child.name = "Martin"
# Changed! (<__main__.Foo object at 0x10457bb30>, 'name', '', 'Martin')

This is not expected:

foo.remove_trait("child")
child.name = "Paul"
# Changed! (<__main__.Foo object at 0x10457bb30>, 'name', 'Martin', 'Paul')

The listener is still gone, so adding the trait again won't bring it back, which is ~good and expected~ Edited: also unexpected.

foo.add_trait("child", Instance(Foo))
foo.child = Foo()
foo.child.name = "Joanna"
# Nothing is printed.

The second notification (where name is changed from "Martin" to "Paul") is unexpected. While add_trait emits a change event on trait_added, remove_trait does not emit any events. I believe if remove_trait also emits an event for when a trait is removed, it would be possible for the listeners to be unhooked from the removed object.

[kitchoi]

Actually, the last block of code is also unexpected:

The on_trait_change hook has not been removed, so adding the trait back should hook up the listener again. Updated the main comment.

[mdickinson]

This is probably low priority to fix for the existing machinery; I don't know of any actual use-cases for remove_trait. We have various pieces of code where we build up a class dynamically by calling add_trait, but that's usually a one-time thing.

[mdickinson]

I don't know of any actual use-cases for remove_trait

Scratch that. There are uses in blockcanvas and mayavi, as well as in the rkern directory in enthought/internal: https://github.com/enthought/internal/blob/41e1b5d70200e2dc088f175dbc946f75689822ef/rkern/prosper/widgets.py#L274

[mdickinson]

Putting this in 6.2. We may not be able to solve it entirely for 6.2, but we should be working towards a solution.

[kitchoi]

I just realized that remove_trait does cause the listeners to be 'removed' if the trait is a simple trait, not an extended trait. The following test currently passes:

    def test_on_trait_change(self):
        events = []

        def handler(value):
            events.append(value)

        class Foo(HasTraits):
            value1 = Int()

        foo = Foo()
        foo.on_trait_change(handler, "value1")

        foo.value1 += 1
        self.assertEqual(len(events), 1)
        events.clear()

        foo.remove_trait("value1")
        # The handler is gone.
        foo.value1 += 1
        self.assertEqual(len(events), 0)

        foo.add_trait("value1", Int())

        # The handler is still gone.
        foo.value1 += 1
        self.assertEqual(len(events), 0)

It is the extended trait this issue is mainly about.

[mdickinson]

It's not 100% clear from the original description, but this bug does apply to the observe framework as well as the on_trait_change framework, as the following script demonstrates:

from traits.api import HasTraits, Instance, Str

class Child(HasTraits):
    name = Str()

class Parent(HasTraits):
    pass

martin = Child(name="Martin")

foo = Parent()
foo.add_trait("child", Instance(Child))
foo.observe(print, "child:name")

foo.child = martin

foo.remove_trait("child")
# The next line should not generate a notification.
martin.name = "Paul"

There aren't a whole lot of good options for changing this without breaking backwards compatibility: the logical way to solve this is to add a trait_removed special trait name, but there's lots of code out there that wants to list all user traits and does so by getting all trait names and then excluding trait_added and trait_modified; such code would be broken by this.

One possibility would be to add a new private attribute (explicitly not a trait, or at least not exposed through the usual trait dictionaries) to each HasTraits class that acts as a register of all the traits on the class, and issues events when the collection of traits changes; this would be similar to the way that TraitSet works.

[mdickinson]

Not for this milestone.