From 019e01afe6c1b3b099ea79d934f82cee5dad057b Mon Sep 17 00:00:00 2001 From: whitequark Date: Sat, 3 Aug 2019 14:00:29 +0000 Subject: [PATCH] hdl.dsl: warn on suspicious statements like `m.If(~True):`. This pattern usually produces an extremely hard to notice bug that will usually break a design when it is triggered, but will also be hidden unless the pathological value of a boolean switch is used. Fixes #159. --- nmigen/hdl/dsl.py | 13 +++++++++++++ nmigen/test/test_hdl_dsl.py | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/nmigen/hdl/dsl.py b/nmigen/hdl/dsl.py index 361fbeb..e090b09 100644 --- a/nmigen/hdl/dsl.py +++ b/nmigen/hdl/dsl.py @@ -165,9 +165,21 @@ class Module(_ModuleBuilderRoot, Elaboratable): self._ctrl_stack.append((name, data)) return data + def _check_signed_cond(self, cond): + cond = Value.wrap(cond) + bits, sign = cond.shape() + if sign: + warnings.warn("Signed values in If/Elif conditions usually result from inverting " + "Python booleans with ~, which leads to unexpected results: ~True is " + "-2, which is truthful. " + "(Silence this warning with `m.If(x)` → `m.If(x.bool())`.)", + SyntaxWarning, stacklevel=4) + return cond + @contextmanager def If(self, cond): self._check_context("If", context=None) + cond = self._check_signed_cond(cond) src_loc = tracer.get_src_loc(src_loc_at=1) if_data = self._set_ctrl("If", { "tests": [], @@ -190,6 +202,7 @@ class Module(_ModuleBuilderRoot, Elaboratable): @contextmanager def Elif(self, cond): self._check_context("Elif", context=None) + cond = self._check_signed_cond(cond) src_loc = tracer.get_src_loc(src_loc_at=1) if_data = self._get_ctrl("If") if if_data is None: diff --git a/nmigen/test/test_hdl_dsl.py b/nmigen/test/test_hdl_dsl.py index 502e9a8..0ff7fef 100644 --- a/nmigen/test/test_hdl_dsl.py +++ b/nmigen/test/test_hdl_dsl.py @@ -268,6 +268,26 @@ class DSLTestCase(FHDLTestCase): ) """) + def test_If_signed_suspicious(self): + m = Module() + with self.assertWarns(SyntaxWarning, + msg="Signed values in If/Elif conditions usually result from inverting Python " + "booleans with ~, which leads to unexpected results: ~True is -2, which is " + "truthful. (Silence this warning with `m.If(x)` → `m.If(x.bool())`.)"): + with m.If(~True): + pass + + def test_Elif_signed_suspicious(self): + m = Module() + with m.If(0): + pass + with self.assertWarns(SyntaxWarning, + msg="Signed values in If/Elif conditions usually result from inverting Python " + "booleans with ~, which leads to unexpected results: ~True is -2, which is " + "truthful. (Silence this warning with `m.If(x)` → `m.If(x.bool())`.)"): + with m.Elif(~True): + pass + def test_Switch(self): m = Module() with m.Switch(self.w1): -- 2.30.2