From: whitequark <cz@m-labs.hk>
Date: Sat, 3 Aug 2019 14:00:29 +0000 (+0000)
Subject: hdl.dsl: warn on suspicious statements like `m.If(~True):`.
X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=019e01afe6c1b3b099ea79d934f82cee5dad057b;p=nmigen.git

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.
---

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):