Merge pull request #6174 from jenshnielsen/error_on_existing_attribute

Error on setting parameter if this already exists as an attribute

docs/changes/0.46.0.rst

@@ -1,4 +1,4 @@
-QCoDeS 0.46.0 (2024-06-10)
+QCoDeS 0.46.0 (2024-06-27)
 ==========================
 
 Breaking Changes:
@@ -18,6 +18,12 @@ Breaking Changes:
   All drivers shipping with qcodes for Vendors from A-K have been updated in this pr.
   The remaining drivers were updated in (:pr:`6087`). (:pr:`6012`)
 
+- It is now considered unsupported to modify the `parameters` attribute of an instrument or instrument module after it has been created.
+  To remove a parameter from an instrument use the `remove_parameter` method. (:pr:`6174`)
+
+- InstrumentBase.add_parameter will now error if an attribute of the same name as the parameter added already exists. This
+  is similar to how it would error if a parameter of the same name existed. (:pr:`6174`)
+
 
 Improved:
 ---------

src/qcodes/instrument/instrument_base.py

@@ -186,6 +186,29 @@ def add_parameter(
             self.parameters[name] = param
         return param
 
+    def remove_parameter(self, name: str) -> None:
+        """
+        Remove a Parameter from this instrument.
+
+        Unlike modifying the parameters dict directly, this method will
+        make sure that the parameter is properly unbound from the instrument
+        if the parameter is added as a real attribute to the instrument.
+        If a property of the same name exists it will not be modified.
+        If name is an attribute but not a parameter, it will not be modified.
+
+        Args:
+            name: The name of the parameter to remove.
+
+        Raises:
+            KeyError: If the parameter does not exist on the instrument.
+        """
+        self.parameters.pop(name)
+
+        is_property = isinstance(getattr(self.__class__, name, None), property)
+
+        if not is_property and hasattr(self, name):
+            delattr(self, name)
+
     def add_function(self, name: str, **kwargs: Any) -> None:
         """
         Bind one ``Function`` to this instrument.

src/qcodes/parameters/parameter_base.py

@@ -316,13 +316,28 @@ def __init__(
         self._abstract = abstract
 
         if instrument is not None and bind_to_instrument:
-            existing_parameter = instrument.parameters.get(name, None)
+            found_as_delegate = instrument.parameters.get(name, False)
+            # we allow properties since a pattern that has been seen in the wild
+            # is properties that are used to wrap parameters of the same name
+            # to define an interface for the instrument
+            is_property = isinstance(
+                getattr(instrument.__class__, name, None), property
+            )
+            found_as_attr = not is_property and hasattr(instrument, name)
+
+            if found_as_delegate or found_as_attr:
+                existing_parameter = instrument.parameters.get(name, None)
 
-            if existing_parameter:
-                if not existing_parameter.abstract:
+                if existing_parameter is not None and not existing_parameter.abstract:
                     raise KeyError(
                         f"Duplicate parameter name {name} on instrument {instrument}"
                     )
+                if existing_parameter is None:
+                    existing_attribute = getattr(instrument, name, None)
+                    if existing_attribute is not None:
+                        raise KeyError(
+                            f"Parameter {name} overrides an attribute of the same name on instrument {instrument}"
+                        )
 
             instrument.parameters[name] = self
 

tests/parameter/test_parameter_override.py

@@ -0,0 +1,99 @@
+import pytest
+
+from qcodes.instrument import Instrument
+
+
+class DummyOverrideInstr(Instrument):
+    def __init__(self, name, **kwargs):
+        """
+        This instrument errors because it tries to override an attribute with a parameter.
+        Removing the parameter using `self.parameters.pop("voltage")` is not safe but
+        would work if the parameter was not assigned to an attribute.
+        """
+        super().__init__(name, **kwargs)
+        self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None)
+
+        self.parameters.pop("voltage")
+
+        self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None)
+
+
+class DummyParameterIsAttrInstr(Instrument):
+    def __init__(self, name, **kwargs):
+        """
+        This instrument errors because it tries to override an attribute with a parameter.
+        """
+        super().__init__(name, **kwargs)
+        self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None)
+
+    def voltage(self):
+        return 0
+
+
+class DummyParameterIsPropertyInstr(Instrument):
+    def __init__(self, name, **kwargs):
+        """
+        We allow overriding a property since this pattern has been seen in the wild
+        to define an interface for the instrument.
+        """
+        super().__init__(name, **kwargs)
+        self.add_parameter("voltage", set_cmd=None, get_cmd=None)  # type: ignore
+
+    @property
+    def voltage(self):
+        return self.parameters["voltage"]
+
+
+class DummyInstr(Instrument):
+    def __init__(self, name, **kwargs):
+        """
+        We allow overriding a property since this pattern has been seen in the wild
+        to define an interface for the instrument.
+        """
+        super().__init__(name, **kwargs)
+        self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None)
+        self.add_parameter("current", set_cmd=None, get_cmd=None)
+        self.some_attr = 1
+
+
+def test_overriding_parameter_attribute_with_parameter_raises():
+    with pytest.raises(
+        KeyError,
+        match="Parameter voltage overrides an attribute of the same name on instrument",
+    ):
+        DummyOverrideInstr("my_instr")
+
+
+def test_overriding_attribute_with_parameter_raises():
+    with pytest.raises(
+        KeyError,
+        match="Parameter voltage overrides an attribute of the same name on instrument",
+    ):
+        DummyParameterIsAttrInstr("my_instr")
+
+
+def test_overriding_property_with_parameter_works(request):
+    request.addfinalizer(DummyParameterIsPropertyInstr.close_all)
+    DummyParameterIsPropertyInstr("my_instr")
+
+
+def test_removing_parameter_works(request):
+    request.addfinalizer(DummyInstr.close_all)
+    a = DummyInstr("my_instr")
+
+    a.remove_parameter("voltage")
+
+    a.remove_parameter("current")
+
+    with pytest.raises(KeyError, match="some_attr"):
+        a.remove_parameter("some_attr")
+
+    assert a.some_attr == 1
+
+
+def test_removed_parameter_from_prop_instrument_works(request):
+    request.addfinalizer(DummyParameterIsPropertyInstr.close_all)
+    a = DummyParameterIsPropertyInstr("my_instr")
+    a.remove_parameter("voltage")
+    a.add_parameter("voltage", set_cmd=None, get_cmd=None)
+    a.voltage.set(1)