Deprecate implicit conversion of handle values to strings. (#1890)

Note that most visibly, this will make code like the following emit warnings:
```python
log.info("The value of X is %s", dut.X)
```
unless users change it to
```python
log.info("The value of X is %s", dut.X.value)
```

Looking at the examples which needed to be changed, it seems many tests were already expecting the behavior of the latter.

cocotb/handle.py

@@ -479,7 +479,12 @@ def value(self):
         return self._value
 
     def __str__(self):
-        return str(self.value)
+        if isinstance(self.value, bytes):
+            StringObject._emit_str_warning(self)
+            return self.value.decode('ascii')
+        else:
+            ModifiableObject._emit_str_warning(self)
+            return str(self.value)
 
 
 class NonHierarchyIndexableObject(NonHierarchyObject):
@@ -691,7 +696,15 @@ def value(self) -> BinaryValue:
     def __int__(self):
         return int(self.value)
 
+    def _emit_str_warning(self):
+        warnings.warn(
+            "`str({t})` is deprecated, and in future will return `{t}._path`. "
+            "To get a string representation of the value, use `str({t}.value)`."
+            .format(t=type(self).__qualname__),
+            FutureWarning, stacklevel=3)
+
     def __str__(self):
+        self._emit_str_warning()
         return str(self.value)
 
 
@@ -836,6 +849,17 @@ def _set_value(self, value, call_sim):
     def value(self) -> bytes:
         return self._handle.get_signal_val_str()
 
+    def _emit_str_warning(self):
+        warnings.warn(
+            "`str({t})` is deprecated, and in future will return `{t}._path`. "
+            "To access the `bytes` value of this handle, use `{t}.value`."
+            .format(t=type(self).__qualname__),
+            FutureWarning, stacklevel=3)
+
+    def __str__(self):
+        self._emit_str_warning()
+        return self.value.decode('ascii')
+
 
 _handle2obj = {}
 

tests/test_cases/issue_348/issue_348.py

@@ -29,7 +29,7 @@ def signal_mon(signal, idx, edge):
 
 class DualMonitor:
     def __init__(self, edge, signal):
-        self.log = SimLog("cocotb.%s.%s" % (edge, signal))
+        self.log = SimLog("cocotb.%s.%s" % (edge, signal._path))
         self.edge_type = edge
         self.monitor_edges = [0, 0]
         self.signal = signal

tests/test_cases/test_cocotb/test_deprecated.py

@@ -8,7 +8,7 @@
 
 
 @contextmanager
-def assert_deprecated():
+def assert_deprecated(warning_category=DeprecationWarning):
     warns = []
     try:
         with warnings.catch_warnings(record=True) as warns:
@@ -17,7 +17,8 @@ def assert_deprecated():
             yield warns  # note: not a cocotb yield, but a contextlib one!
     finally:
         assert len(warns) == 1
-        assert issubclass(warns[0].category, DeprecationWarning), "Expected DeprecationWarning"
+        msg = "Expected {}".format(warning_category.__qualname__)
+        assert issubclass(warns[0].category, warning_category), msg
 
 
 @cocotb.test()
@@ -43,6 +44,29 @@ async def test_unicode_handle_assignment_deprecated(dut):
     assert "bytes" in str(warns[0].message)
 
 
+@cocotb.test()
+async def test_convert_handle_to_string_deprecated(dut):
+    dut.stream_in_data <= 0
+    await cocotb.triggers.Timer(1, units='ns')
+
+    with assert_deprecated(FutureWarning) as warns:
+        as_str = str(dut.stream_in_data)
+    assert "_path" in str(warns[0].message)
+
+    # in future this will be ` == dut._path`
+    assert as_str == str(dut.stream_in_data.value)
+
+    if cocotb.LANGUAGE == "verilog":
+        # the `NUM_OF_MODULES` parameter is only present in the verilog design
+        with assert_deprecated(FutureWarning) as warns:
+            as_str = str(dut.NUM_OF_MODULES)
+
+        assert "_path" in str(warns[0].message)
+
+        # in future this will be ` == dut._path`
+        assert as_str == str(dut.NUM_OF_MODULES.value)
+
+
 @cocotb.test()
 async def test_create_error_deprecated(dut):
     with assert_deprecated():

tests/test_cases/test_cocotb/test_edge_triggers.py

@@ -29,17 +29,17 @@ def count_edges_cycles(signal, edges):
 def do_single_edge_check(dut, level):
     """Do test for rising edge"""
     old_value = dut.clk.value.integer
-    dut._log.info("Value of %s is %d" % (dut.clk, old_value))
+    dut._log.info("Value of %s is %d" % (dut.clk._path, old_value))
     if old_value is level:
-        raise TestError("%s not to %d start with" % (dut.clk, not level))
+        raise TestError("%s not to %d start with" % (dut.clk._path, not level))
     if level == 1:
         yield RisingEdge(dut.clk)
     else:
         yield FallingEdge(dut.clk)
     new_value = dut.clk.value.integer
-    dut._log.info("Value of %s is %d" % (dut.clk, new_value))
+    dut._log.info("Value of %s is %d" % (dut.clk._path, new_value))
     if new_value is not level:
-        raise TestError("%s not %d at end" % (dut.clk, level))
+        raise TestError("%s not %d at end" % (dut.clk._path, level))
 
 
 @cocotb.test()

tests/test_cases/test_discovery/test_discovery.py

@@ -103,7 +103,7 @@ def access_signal(dut):
     yield Timer(10)
     if dut.stream_in_data.value.integer != 1:
         raise TestError("%s.%s != %d" %
-                        (str(dut.stream_in_data),
+                        (dut.stream_in_data._path,
                          dut.stream_in_data.value.integer, 1))
 
 
@@ -116,12 +116,12 @@ def access_single_bit(dut):
     dut.stream_in_data <= 0
     yield Timer(10)
     dut._log.info("%s = %d bits" %
-                  (str(dut.stream_in_data), len(dut.stream_in_data)))
+                  (dut.stream_in_data._path, len(dut.stream_in_data)))
     dut.stream_in_data[2] <= 1
     yield Timer(10)
     if dut.stream_out_data_comb.value.integer != (1 << 2):
         raise TestError("%s.%s != %d" %
-                        (str(dut.stream_out_data_comb),
+                        (dut.stream_out_data_comb._path,
                          dut.stream_out_data_comb.value.integer, (1 << 2)))
 
 
@@ -134,12 +134,12 @@ def access_single_bit_assignment(dut):
     dut.stream_in_data = 0
     yield Timer(10)
     dut._log.info("%s = %d bits" %
-                  (str(dut.stream_in_data), len(dut.stream_in_data)))
+                  (dut.stream_in_data._path, len(dut.stream_in_data)))
     dut.stream_in_data[2] = 1
     yield Timer(10)
     if dut.stream_out_data_comb.value.integer != (1 << 2):
         raise TestError("%s.%s != %d" %
-                        (str(dut.stream_out_data_comb),
+                        (dut.stream_out_data_comb._path,
                          dut.stream_out_data_comb.value.integer, (1 << 2)))
 
 
@@ -148,7 +148,7 @@ def access_single_bit_erroneous(dut):
     """Access a non-existent single bit"""
     yield Timer(10)
     dut._log.info("%s = %d bits" %
-                  (str(dut.stream_in_data), len(dut.stream_in_data)))
+                  (dut.stream_in_data._path, len(dut.stream_in_data)))
     bit = len(dut.stream_in_data) + 4
     dut.stream_in_data[bit] <= 1
     yield Timer(10)
@@ -208,7 +208,7 @@ def access_string_vhdl(dut):
     tlog = logging.getLogger("cocotb.test")
     yield Timer(10)
     constant_string = dut.isample_module1.EXAMPLE_STRING
-    tlog.info("%r is %s" % (constant_string, str(constant_string)))
+    tlog.info("%r is %s" % (constant_string, constant_string.value))
     if not isinstance(constant_string, ConstantObject):
         raise TestFailure("EXAMPLE_STRING was not constant")
     if constant_string != b"TESTING":
@@ -226,7 +226,7 @@ def access_string_vhdl(dut):
     yield Timer(10)
 
     if variable_string != test_string:
-        raise TestFailure("%r %s != '%s'" % (variable_string, str(variable_string), test_string))
+        raise TestFailure("%r %s != '%s'" % (variable_string, variable_string.value, test_string))
 
     test_string = b"longer_than_the_array"
     tlog.info("Test writing over size with '%s'" % test_string)
@@ -239,7 +239,7 @@ def access_string_vhdl(dut):
     test_string = test_string[:len(variable_string)]
 
     if variable_string != test_string:
-        raise TestFailure("%r %s != '%s'" % (variable_string, str(variable_string), test_string))
+        raise TestFailure("%r %s != '%s'" % (variable_string, variable_string.value, test_string))
 
     tlog.info("Test read access to a string character")
 
@@ -266,7 +266,7 @@ def access_string_vhdl(dut):
 
     test_string = test_string.upper()
 
-    result = str(variable_string)
+    result = variable_string.value
     tlog.info("After setting bytes of string value is %s" % result)
     if variable_string != test_string:
         raise TestFailure("%r %s != '%s'" % (variable_string, result, test_string))
@@ -282,7 +282,7 @@ def access_const_string_verilog(dut):
     string_const = dut.STRING_CONST
 
     yield Timer(10, 'ns')
-    tlog.info("%r is %s" % (string_const, str(string_const)))
+    tlog.info("%r is %s" % (string_const, string_const.value))
     if not isinstance(string_const, StringObject):
         raise TestFailure("STRING_CONST was not StringObject")
     if string_const != b"TESTING_CONST":
@@ -303,7 +303,7 @@ def access_var_string_verilog(dut):
     string_var = dut.STRING_VAR
 
     yield Timer(10, 'ns')
-    tlog.info("%r is %s" % (string_var, str(string_var)))
+    tlog.info("%r is %s" % (string_var, string_var.value))
     if not isinstance(string_var, StringObject):
         raise TestFailure("STRING_VAR was not StringObject")
     if string_var != b"TESTING_VAR":
@@ -326,7 +326,7 @@ def access_constant_boolean(dut):
     if not isinstance(constant_boolean, ConstantObject):
         raise TestFailure("dut.stream_in_int.EXAMPLE_BOOL is not a ConstantObject")
 
-    tlog.info("Value of %s is %d" % (constant_boolean, constant_boolean))
+    tlog.info("Value of %s is %d" % (constant_boolean._path, constant_boolean.value))
 
 
 @cocotb.test(skip=cocotb.LANGUAGE in ["verilog"])
@@ -355,7 +355,7 @@ def access_boolean(dut):
     if length != 1:
         raise TestFailure("Length should be 1 not %d" % length)
 
-    tlog.info("Value of %s is %d" % (boolean, boolean))
+    tlog.info("Value of %s is %d" % (boolean._path, boolean.value))
 
     curr_val = int(boolean)
     output_bool = dut.stream_out_bool
@@ -366,7 +366,7 @@ def access_boolean(dut):
 
     yield Timer(1)
 
-    tlog.info("Value of %s is now %d" % (output_bool, output_bool))
+    tlog.info("Value of %s is now %d" % (output_bool._path, output_bool.value))
     if (int(curr_val) == int(output_bool)):
         raise TestFailure("Value did not propagate")
 
@@ -391,7 +391,7 @@ def skip_a_test(dut):
     """This test shouldn't execute"""
     yield Timer(10)
     dut._log.info("%s = %d bits" %
-                  (str(dut.stream_in_data), len(dut.stream_in_data)))
+                  (dut.stream_in_data._path, len(dut.stream_in_data)))
     bit = len(dut.stream_in_data) + 4
     dut.stream_in_data[bit] <= 1
     yield Timer(10)