From cdfee3694474a0e4daa4d73d85c17f3abe483fc6 Mon Sep 17 00:00:00 2001 From: Eric Wieser Date: Thu, 18 Jun 2020 12:08:35 +0100 Subject: [PATCH] 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 | 26 ++++++++++++++- tests/test_cases/issue_348/issue_348.py | 2 +- .../test_cases/test_cocotb/test_deprecated.py | 28 ++++++++++++++-- .../test_cocotb/test_edge_triggers.py | 8 ++--- .../test_discovery/test_discovery.py | 32 +++++++++---------- 5 files changed, 72 insertions(+), 24 deletions(-) diff --git a/cocotb/handle.py b/cocotb/handle.py index 14f64c08..2c61e8af 100755 --- a/cocotb/handle.py +++ b/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 = {} diff --git a/tests/test_cases/issue_348/issue_348.py b/tests/test_cases/issue_348/issue_348.py index eb213727..e6c4d948 100644 --- a/tests/test_cases/issue_348/issue_348.py +++ b/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 diff --git a/tests/test_cases/test_cocotb/test_deprecated.py b/tests/test_cases/test_cocotb/test_deprecated.py index 3cf327ff..e80fbad9 100644 --- a/tests/test_cases/test_cocotb/test_deprecated.py +++ b/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(): diff --git a/tests/test_cases/test_cocotb/test_edge_triggers.py b/tests/test_cases/test_cocotb/test_edge_triggers.py index 34c6bc8b..e7a01fb8 100644 --- a/tests/test_cases/test_cocotb/test_edge_triggers.py +++ b/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() diff --git a/tests/test_cases/test_discovery/test_discovery.py b/tests/test_cases/test_discovery/test_discovery.py index 0857efff..9bdd2434 100644 --- a/tests/test_cases/test_discovery/test_discovery.py +++ b/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)