Shallow-copy mutable containers in slot values (#826)

param/__init__.py

@@ -36,7 +36,7 @@
 from .parameterized import ( Undefined,
     Parameterized, Parameter, String, ParameterizedFunction, ParamOverrides,
     descendents, get_logger, instance_descriptor, dt_types,
-    _dict_update, _int_types)
+    _int_types)
 
 from .parameterized import (batch_watch, depends, output, script_repr, # noqa: api import
                             discard_events, edit_constant, instance_descriptor)
@@ -48,6 +48,7 @@
     ParamDeprecationWarning as _ParamDeprecationWarning,
     _deprecate_positional_args,
     _deprecated,
+    _dict_update,
     _validate_error_prefix,
 )
 

param/_utils.py

@@ -6,6 +6,7 @@
 
 from textwrap import dedent
 from threading import get_ident
+from collections import abc
 
 DEFAULT_SIGNATURE = inspect.Signature([
     inspect.Parameter('self', inspect.Parameter.POSITIONAL_OR_KEYWORD),
@@ -171,3 +172,17 @@ def _validate_error_prefix(parameter, attribute=None):
         except Exception:
             pass
     return ' '.join(out)
+
+
+def _is_mutable_container(value):
+    """True for mutable containers, which typically need special handling when being copied"""
+    return issubclass(type(value), (abc.MutableSequence, abc.MutableSet, abc.MutableMapping))
+
+
+def _dict_update(dictionary, **kwargs):
+    """
+    Small utility to update a copy of a dict with the provided keyword args.
+    """
+    d = dictionary.copy()
+    d.update(kwargs)
+    return d

param/parameterized.py

@@ -38,10 +38,12 @@
     DEFAULT_SIGNATURE,
     _deprecated,
     _deprecate_positional_args,
+    _dict_update,
     _is_auto_name,
     _recursive_repr,
     _validate_error_prefix,
     ParamDeprecationWarning as _ParamDeprecationWarning,
+    _is_mutable_container,
 )
 
 try:
@@ -398,17 +400,61 @@ def iscoroutinefunction(function):
         return False
 
 
+def _instantiate_param_obj(paramobj, owner=None):
+    """Return a Parameter object suitable for instantiation given the class's Parameter object"""
+
+    # Shallow-copy Parameter object, with special handling for watchers
+    # (from try/except/finally in Parameters.__getitem__ in https://github.com/holoviz/param/pull/306)
+    p = paramobj
+    try:
+        # Do not copy watchers on class parameter
+        watchers = p.watchers
+        p.watchers = {}
+        p = copy.copy(p)
+    except:
+        raise
+    finally:
+        p.watchers = {k: list(v) for k, v in watchers.items()}
+
+    p.owner = owner
+
+    # shallow-copy any mutable slot values other than the actual default
+    for s in p.__class__.__slots__:
+        v = getattr(p, s)
+        if _is_mutable_container(v) and s != "default":
+            setattr(p, s, copy.copy(v))
+    return p
+
+
+def _instantiated_parameter(parameterized, param):
+    """
+    Given a Parameterized object and one of its class Parameter objects,
+    return the appropriate Parameter object for this instance, instantiating
+    it if need be.
+    """
+    if (getattr(parameterized._param__private, 'initialized', False) and param.per_instance and
+        not getattr(type(parameterized)._param__private, 'disable_instance_params', False)):
+        key = param.name
+
+        if key not in parameterized._param__private.params:
+            parameterized._param__private.params[key] = _instantiate_param_obj(param, parameterized)
+
+        param = parameterized._param__private.params[key]
+
+    return param
+
+
 def instance_descriptor(f):
     # If parameter has an instance Parameter, delegate setting
     def _f(self, obj, val):
-        if obj is None:
-            # obj is None when the metaclass is setting
-            return f(self, obj, val)
-        params = obj._param__private.params
-        instance_param = None if params is None else params.get(self.name)
-        if instance_param is not None and self is not instance_param:
-            instance_param.__set__(obj, val)
-            return
+        # obj is None when the metaclass is setting
+        if obj is not None:
+            instance_param = obj._param__private.params.get(self.name)
+            if instance_param is None:
+                instance_param = _instantiated_parameter(obj, self)
+            if instance_param is not None and self is not instance_param:
+                instance_param.__set__(obj, val)
+                return
         return f(self, obj, val)
     return _f
 
@@ -736,15 +782,6 @@ def _m_caller(self, method_name, what='value', changed=None, callback=None):
     return caller
 
 
-def _dict_update(dictionary, **kwargs):
-    """
-    Small utility to update a copy of a dict with the provided keyword args.
-    """
-    d = dictionary.copy()
-    d.update(kwargs)
-    return d
-
-
 def _add_doc(obj, docstring):
     """Add a docstring to a namedtuple"""
     obj.__doc__ = docstring
@@ -1393,7 +1430,7 @@ def __set__(self, obj, val):
                 warnings.warn(
                     'Number.set_hook has been deprecated.',
                     category=_ParamDeprecationWarning,
-                    stacklevel=5,
+                    stacklevel=6,
                 )
 
         self._validate(val)
@@ -1771,26 +1808,9 @@ def __getitem__(self_, key):
         Returns the class or instance parameter
         """
         inst = self_.self
-        parameters = self_.objects(False) if inst is None else inst.param.objects(False)
-        p = parameters[key]
-        if (inst is not None and getattr(inst._param__private, 'initialized', False) and p.per_instance and
-            not getattr(self_.cls._param__private, 'disable_instance_params', False)):
-            if key not in inst._param__private.params:
-                try:
-                    # Do not copy watchers on class parameter
-                    watchers = p.watchers
-                    p.watchers = {}
-                    p = copy.copy(p)
-                except:
-                    raise
-                finally:
-                    p.watchers = {k: list(v) for k, v in watchers.items()}
-                p.owner = inst
-                inst._param__private.params[key] = p
-            else:
-                p = inst._param__private.params[key]
-        return p
-
+        params = self_ if inst is None else inst.param
+        p = params.objects(False)[key]
+        return p if inst is None else _instantiated_parameter(inst, p)
 
     def __dir__(self_):
         """
@@ -1845,10 +1865,10 @@ def _setup_params(self_,**params):
         """
         Initialize default and keyword parameter values.
 
-        First, ensures that all Parameters with 'instantiate=True' (typically
-        used for mutable Parameters) are copied directly into each object, to
-        ensure that there is an independent copy (to avoid surprising aliasing
-        errors). Second, ensures that Parameters with 'constant=True' are
+        First, ensures that values for all Parameters with 'instantiate=True'
+        (typically used for mutable Parameters) are copied directly into each object,
+        to ensure that there is an independent copy of the value (to avoid surprising
+        aliasing errors). Second, ensures that Parameters with 'constant=True' are
         referenced on the instance, to make sure that setting a constant
         Parameter on the class doesn't affect already created instances. Then
         sets each of the keyword arguments, raising when any of them are not
@@ -3358,6 +3378,12 @@ def __param_inheritance(mcs, param_name, param):
                 else:
                     setattr(param, slot, default_val)
 
+            # Avoid crosstalk between mutable slot values in different Parameter objects
+            if slot != "default":
+                v = getattr(param, slot)
+                if _is_mutable_container(v):
+                    setattr(param, slot, copy.copy(v))
+
         # Once all the static slots have been filled in, fill in the dynamic ones
         # (which are only allowed to use static values or results are undefined)
         for slot, fn in callables.items():

tests/testparameterizedobject.py

@@ -16,9 +16,15 @@
 
 import random
 
-from param import parameterized
-from param.parameterized import ParamOverrides, shared_parameters
-from param.parameterized import default_label_formatter, no_instance_params
+from param import parameterized, Parameter
+from param._utils import _dict_update
+from param.parameterized import (
+    ParamOverrides,
+    Undefined,
+    default_label_formatter,
+    no_instance_params,
+    shared_parameters,
+)
 
 class _SomeRandomNumbers:
     def __call__(self):
@@ -1341,6 +1347,114 @@ class B(A):
     # https://github.com/holoviz/param/issues/718
     assert B.p == 1
 
+class TestShallowCopyMutableAttributes:
+
+    @pytest.fixture
+    def foo(self):
+        class Foo:
+            def __init__(self, val):
+                self.val = val
+
+        return Foo
+
+    @pytest.fixture
+    def custom_param(self):
+        class CustomParameter(Parameter):
+
+            __slots__ = ['container']
+
+            _slot_defaults = _dict_update(Parameter._slot_defaults, container=None)
+
+            def __init__(self, default=Undefined, *, container=Undefined, **kwargs):
+                super().__init__(default=default, **kwargs)
+                self.container = container
+
+        return CustomParameter
+
+    def test_shallow_copy_on_class_creation(self, custom_param, foo):
+        clist = [foo(1), foo(2)]
+
+        class P(param.Parameterized):
+            cp = custom_param(container=clist)
+
+
+        # the mutable container has been shallow-copied
+        assert P.param.cp.container is not clist
+        assert all(cval is val for cval, val in zip(P.param.cp.container, clist))
+
+    def test_shallow_copy_inheritance_each_level(self, custom_param):
+
+        clist = [1, 2]
+
+        class A(param.Parameterized):
+            p = custom_param(container=clist)
+
+        class B(A):
+            p = custom_param(default=1)
+
+        clist.append(3)
+
+        assert A.param.p.container == [1, 2]
+        assert B.param.p.container == [1, 2]
+
+        B.param.p.container.append(4)
+
+        assert A.param.p.container == [1, 2]
+        assert B.param.p.container == [1, 2, 4]
+
+    def test_shallow_copy_on_instance_getitem(self, custom_param, foo):
+        clist = [foo(1), foo(2)]
+
+        class P(param.Parameterized):
+            cp = custom_param(container=clist)
+
+        p = P()
+
+        assert 'cp' not in p._param__private.params
+
+        p.param['cp']
+
+        # the mutable container has been shallow-copied
+        assert 'cp' in p._param__private.params
+        assert P.param.cp.container == p._param__private.params['cp'].container
+        assert P.param.cp.container is not p._param__private.params['cp'].container
+        assert all(cval is val for cval, val in zip(p.param.cp.container, clist))
+
+    def test_shallow_copy_on_instance_set(self, custom_param, foo):
+        clist = [foo(1), foo(2)]
+
+        class P(param.Parameterized):
+            cp = custom_param(container=clist)
+
+        p = P()
+
+        assert 'cp' not in p._param__private.params
+
+        p.cp = 'value'
+
+        # the mutable container has been shallow-copied
+        assert 'cp' in p._param__private.params
+        assert P.param.cp.container == p._param__private.params['cp'].container
+        assert P.param.cp.container is not p._param__private.params['cp'].container
+        assert all(cval is val for cval, val in zip(p.param.cp.container, clist))
+
+    def test_modify_class_container_before_shallow_copy(self, custom_param, foo):
+        clist = [foo(1), foo(2)]
+        clist2 = [foo(3), foo(4)]
+
+        class P(param.Parameterized):
+            cp = custom_param(container=clist)
+
+        p1 = P()
+        p2 = P()
+
+        # Setting the class container will affect instances are the shallow copy
+        # is lazy and has not yet been made.
+        P.param.cp.container = clist2
+
+        assert p1.param.cp.container == clist2
+        assert p2.param.cp.container == clist2
+
 
 @pytest.fixture
 def custom_parameter1():

tests/testselector.py

@@ -446,3 +446,51 @@ class A(param.Parameterized):
 
         a = A()
         assert a.p is objs[0]
+
+    def test_objects_not_shared_in_class_hierarchy_1(self):
+        # https://github.com/holoviz/param/issues/793
+
+        class A(param.Parameterized):
+            p = param.Selector(objects=[1, 2], check_on_set=False)
+
+        class B(A):
+            p = param.Selector(default=2)
+
+
+        b = B()
+        b.p = 3
+
+        assert A.param.p.objects == [1, 2]
+        assert B.param.p.objects == [1, 2]
+        assert b.param.p.objects == [1, 2, 3]
+
+    def test_objects_not_shared_in_class_hierarchy_2(self):
+        # https://github.com/holoviz/param/issues/793
+
+        class A(param.Parameterized):
+            p = param.Selector()
+
+        class B(A):
+            p = param.Selector()
+
+
+        b = B()
+        b.p = 2
+
+        assert A.param.p.objects == []
+
+    def test_objects_not_shared_across_instance_class(self):
+        # https://github.com/holoviz/param/issues/746
+
+        class P(param.Parameterized):
+            s = param.Selector(objects=[1, 2], check_on_set=False)
+
+        p = P()
+
+        p.s = 3
+        assert P.param.s.objects == [1, 2]
+        assert p.param.s.objects == [1, 2, 3]
+
+        P.s = 4
+        assert P.param.s.objects == [1, 2, 4]
+        assert p.param.s.objects == [1, 2 ,3]