Fix that clone_traits doesn't respect dynamic traits (#1846)

Closes #1845

This PR fixes `clone_traits` (and `copy_traits`) to respect dynamic
traits.

Previously, the `HasTraits.copy_traits` method used a mixture of trait
information from `self` (the object being copied _to_) and `other` (the
object being copied _from_) to determine how to behave. Since the actual
copying was enclosed in a large `try / except` block, any errors arising
just resulted in the trait value not being copied.

In particular, when cloning a `HasTraits` object with dynamically-added
traits:

- the set of traits to copy was first determined by inspecting the
object, and included those dynamically-added traits
- then, in `copy_traits`, we inspect the cloned object that we're
copying _to_, which doesn't have the dynamically-added traits, and as a
result we fail to copy those trait values.

This PR updates the logic to only use the trait information from `other`
when deciding what / how to copy, instead of using a mixture of sources.
As a result, `clone_traits` should behave as expected on objects with
dynamically-added traits.

We have a large corpus of existing Traits code, and like all changes to
Traits this change risks unintended consequences and surprise breakages
of existing code that's inadvertently relying on the existing behaviour.
While I think it's the right change to make, I'd advocate not
backporting this change to any bugfix branch, and highlighting the
change prominently in release notes.

Author: mdickinson

traits/has_traits.py

@@ -1554,9 +1554,9 @@ def copy_traits(
             The object whose trait attribute values should be copied.
         traits : list of strings
             A list of names of trait attributes to copy. If None or
-            unspecified, the set of names returned by trait_names() is used.
-            If 'all' or an empty list, the set of names returned by
-            all_trait_names() is used.
+            unspecified, the set of names returned by
+            other.copyable_trait_names() is used. If 'all' or an empty list,
+            the set of names returned by other.all_trait_names() is used.
         memo : dict
             A dictionary of objects that have already been copied.
         copy : None | 'deep' | 'shallow'
@@ -1572,9 +1572,9 @@ def copy_traits(
         """
 
         if traits is None:
-            traits = self.copyable_trait_names(**metadata)
+            traits = other.copyable_trait_names(**metadata)
         elif (traits == "all") or (len(traits) == 0):
-            traits = self.all_trait_names()
+            traits = other.all_trait_names()
             if memo is not None:
                 memo["traits_to_copy"] = "all"
 
@@ -1585,7 +1585,7 @@ def copy_traits(
 
         for name in traits:
             try:
-                trait = self.trait(name)
+                trait = other.trait(name)
                 if trait.type in DeferredCopy:
                     deferred.append(name)
                     continue

traits/tests/test_clone.py

@@ -10,7 +10,7 @@
 
 import unittest
 
-from traits.api import HasTraits, Instance, Str, Any, Property
+from traits.api import HasTraits, Instance, Int, Str, Any, Property
 
 
 class Foo(HasTraits):
@@ -280,3 +280,18 @@ def test_Instance_circular_references_deep(self):
         # should reference the new clone.
         self.assertIsNot(bar_copy.shared, baz.shared)
         self.assertIs(bar_copy.shared, baz_copy.shared)
+
+    def test_clone_traits_copies_dynamic_traits(self):
+        # Regression test for https://github.com/enthought/traits/issues/1845
+
+        # Given
+        ref = Foo(s="ref")
+        ref.add_trait("dynamic", Int(42))
+        ref.some_other_value = 35
+
+        # When
+        clone = ref.clone_traits()
+
+        # Then
+        self.assertEqual(clone.some_other_value, 35)
+        self.assertEqual(clone.dynamic, 42)