From 9964fc6b5768654bfc6747e62ed351116d858a24 Mon Sep 17 00:00:00 2001
From: whitequark <whitequark@whitequark.org>
Date: Fri, 31 Jan 2020 06:37:45 +0000
Subject: [PATCH] hdl.dsl: make `if m.{If,Elif,Else}(...)` a syntax error.

A common typo, and hard to notice when it's silently ignored.

Fixes #284.
---
 nmigen/hdl/dsl.py           | 30 ++++++++++++++++++++++++++----
 nmigen/test/test_hdl_dsl.py | 17 +++++++++++++++++
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/nmigen/hdl/dsl.py b/nmigen/hdl/dsl.py
index 598d812..07c0a1b 100644
--- a/nmigen/hdl/dsl.py
+++ b/nmigen/hdl/dsl.py
@@ -1,6 +1,7 @@
 from collections import OrderedDict, namedtuple
 from collections.abc import Iterable
-from contextlib import contextmanager
+from contextlib import contextmanager, _GeneratorContextManager
+from functools import wraps
 from enum import Enum
 import warnings
 
@@ -113,6 +114,27 @@ class _ModuleBuilderDomainSet:
         self._builder._add_domain(domain)
 
 
+# It's not particularly clean to depend on an internal interface, but, unfortunately, __bool__
+# must be defined on a class to be called during implicit conversion.
+class _GuardedContextManager(_GeneratorContextManager):
+    def __init__(self, keyword, func, args, kwds):
+        self.keyword = keyword
+        return super().__init__(func, args, kwds)
+
+    def __bool__(self):
+        raise SyntaxError("`if m.{kw}(...):` does not work; use `with m.{kw}(...)`"
+                          .format(kw=self.keyword))
+
+
+def _guardedcontextmanager(keyword):
+    def decorator(func):
+        @wraps(func)
+        def helper(*args, **kwds):
+            return _GuardedContextManager(keyword, func, args, kwds)
+        return helper
+    return decorator
+
+
 class FSM:
     def __init__(self, state, encoding, decoding):
         self.state    = state
@@ -183,7 +205,7 @@ class Module(_ModuleBuilderRoot, Elaboratable):
                           SyntaxWarning, stacklevel=4)
         return cond
 
-    @contextmanager
+    @_guardedcontextmanager("If")
     def If(self, cond):
         self._check_context("If", context=None)
         cond = self._check_signed_cond(cond)
@@ -206,7 +228,7 @@ class Module(_ModuleBuilderRoot, Elaboratable):
             self.domain._depth -= 1
             self._statements = _outer_case
 
-    @contextmanager
+    @_guardedcontextmanager("Elif")
     def Elif(self, cond):
         self._check_context("Elif", context=None)
         cond = self._check_signed_cond(cond)
@@ -226,7 +248,7 @@ class Module(_ModuleBuilderRoot, Elaboratable):
             self.domain._depth -= 1
             self._statements = _outer_case
 
-    @contextmanager
+    @_guardedcontextmanager("Else")
     def Else(self):
         self._check_context("Else", context=None)
         src_loc = tracer.get_src_loc(src_loc_at=1)
diff --git a/nmigen/test/test_hdl_dsl.py b/nmigen/test/test_hdl_dsl.py
index 8ebd18d..d590035 100644
--- a/nmigen/test/test_hdl_dsl.py
+++ b/nmigen/test/test_hdl_dsl.py
@@ -300,6 +300,23 @@ class DSLTestCase(FHDLTestCase):
             with m.Elif(~True):
                 pass
 
+    def test_if_If_Elif_Else(self):
+        m = Module()
+        with self.assertRaises(SyntaxError,
+                msg="`if m.If(...):` does not work; use `with m.If(...)`"):
+            if m.If(0):
+                pass
+        with m.If(0):
+            pass
+        with self.assertRaises(SyntaxError,
+                msg="`if m.Elif(...):` does not work; use `with m.Elif(...)`"):
+            if m.Elif(0):
+                pass
+        with self.assertRaises(SyntaxError,
+                msg="`if m.Else(...):` does not work; use `with m.Else(...)`"):
+            if m.Else():
+                pass
+
     def test_Switch(self):
         m = Module()
         with m.Switch(self.w1):
-- 
2.30.2