Repository: phanrahan/magma

Issue:
Magma should raise an exception when a design contains wiring errors
Here's an example of a circuit that has a wiring error, but will compile
```python
class T(m.Product):
    a = m.In(m.Clock)
    b = m.In(m.Clock)

class Foo(m.Circuit):
    io = m.IO(I=In(T), O=m.Out(m.Clock))
    m.wire(io.I, io.O)

m.compile("build/Foo", Foo)
```
This will produce the following error output:
```
ERROR:magma:Cannot wire Foo.I (Tuple(a=Out(Clock),b=Out(Clock))) to Foo.O (In(Clock))
```

However, it also generates a verilog file since the backend automatically wires up the undriven clock (Foo.O) to a default driver and compiles fine through coreir.

We know our users are not always the best at reading error messages, especially when their code contains a bunch of warnings that might hide the errors.

I propose that for each circuit definitions, we track the number of logged errors.  Then, when compiling a design we raise an exception at the Python level if the number of errors is non zero for all the circuits in a design.  We could then report all the errors that have been collected.

Another useful mode might be to have errors scoped per circuit.  So, at the end of each circuit definition, check if any errors have been logged and raise an error if so.  The idea here is that while we may not want to stop on every error (so we can collect multiple errors and report them at once, similar to how many software compilers will report more than one error if possible), but it may be useful to limit multiple errors on a per-circuit basis (since if there are multiple errors in a circuit, we probably want to fix those first before looking at other errors in the design). 

Discussion:

Patch:
diff --git a/magma/backend/coreir/coreir_compiler.py b/magma/backend/coreir/coreir_compiler.py
index 81e519e2..338af60e 100644
--- a/magma/backend/coreir/coreir_compiler.py
+++ b/magma/backend/coreir/coreir_compiler.py
@@ -8,6 +8,7 @@ from magma.frontend import coreir_ as coreir_frontend
 from magma.is_definition import isdefinition
 from magma.logging import root_logger
 from magma.passes import InstanceGraphPass
+from magma.passes.find_errors import find_errors_pass
 
 
 _logger = root_logger()
@@ -66,6 +67,7 @@ class CoreIRCompiler(Compiler):
         result = {}
         insert_coreir_wires(self.main)
         insert_wrap_casts(self.main)
+        find_errors_pass(self.main)
         backend = self.backend
         opts = _make_opts(backend, self.opts)
         backend.compile(self.main, opts)
diff --git a/magma/circuit.py b/magma/circuit.py
index 9b0c4153..86f9f3d4 100644
--- a/magma/circuit.py
+++ b/magma/circuit.py
@@ -7,6 +7,7 @@ import functools
 import operator
 from collections import namedtuple
 import os
+import logging as py_logging  # to avoid confusion
 
 import six
 from . import cache_definition
@@ -122,7 +123,8 @@ class DefinitionContext:
         self._finalize_file_opens()  # so displays can refer to open files
         self._finalize_displays()
         self._finalize_file_close()  # close after displays
-        unstage_logger()
+        logs = unstage_logger()
+        defn._has_errors_ = any(log[1] is py_logging.ERROR for log in logs)
 
 
 _definition_context_stack = Stack()
diff --git a/magma/logging.py b/magma/logging.py
index 5c553365..814e7341 100644
--- a/magma/logging.py
+++ b/magma/logging.py
@@ -138,9 +138,11 @@ _root_logger.setLevel(config.log_level)
 
 def flush():
     global _staged_logs
-    for logger, level, obj, args, kwargs in _staged_logs:
+    curr_logs = _staged_logs.copy()
+    for logger, level, obj, args, kwargs in curr_logs:
         logger.log(level, obj, *args, **kwargs)
     _staged_logs = []
+    return curr_logs
 
 
 def root_logger():
@@ -155,4 +157,4 @@ def stage_logger():
 def unstage_logger():
     global _staged_logging
     _staged_logging = False
-    flush()
+    return flush()
diff --git a/magma/passes/find_errors.py b/magma/passes/find_errors.py
new file mode 100644
index 00000000..92d5c474
--- /dev/null
+++ b/magma/passes/find_errors.py
@@ -0,0 +1,61 @@
+import enum
+
+from magma.passes.passes import CircuitPass, pass_lambda
+
+
+_HAS_ERROR_KEY = "_has_errors_"
+
+
+def _has_errors(ckt):
+    return getattr(ckt, _HAS_ERROR_KEY, False)
+
+
+class FindErrorsPass(CircuitPass):
+
+    class ErrorReportingMode(enum.Enum):
+        FIRST = enum.auto()
+        ALL = enum.auto()
+        COLLECT = enum.auto()
+
+    def __init__(self, main,
+                 mode: ErrorReportingMode = ErrorReportingMode.FIRST):
+        super().__init__(main)
+        self._mode = mode
+        self._errors = []
+
+    def __call__(self, ckt):
+        if not _has_errors(ckt):
+            return
+        self._handle_error(ckt)
+
+    @property
+    def errors(self):
+        if self._mode == FindErrorsPass.ErrorReportingMode.FIRST:
+            return None
+        collect = (self._mode == FindErrorsPass.ErrorReportingMode.ALL or
+                   self._mode == FindErrorsPass.ErrorReportingMode.COLLECT)
+        if collect:
+            return self._errors.copy()
+        raise NotImplementedError(self._mode)
+
+    def _handle_error(self, ckt):
+        if self._mode == FindErrorsPass.ErrorReportingMode.FIRST:
+            assert not self._errors
+            raise Exception(f"Found circuit with errors: {ckt.name}")
+        collect = (self._mode == FindErrorsPass.ErrorReportingMode.ALL or
+                   self._mode == FindErrorsPass.ErrorReportingMode.COLLECT)
+        if collect:
+            self._errors.append(ckt)
+            return
+        raise NotImplementedError(self._mdoe)
+
+    def done(self):
+        # If mode == ALL, then raise an error with all the circuit names. In the
+        # case of COLLECT or FIRST, do nothing.
+        if self._mode == FindErrorsPass.ErrorReportingMode.ALL:
+            msg = (f"Found circuits with errors: "
+                   f"{ckt.name for ckt in self._errors}")
+            raise Exception(msg)
+
+
+find_errors_pass = pass_lambda(FindErrorsPass)
diff --git a/magma/passes/passes.py b/magma/passes/passes.py
index 368ae79c..29b7a4e1 100644
--- a/magma/passes/passes.py
+++ b/magma/passes/passes.py
@@ -125,7 +125,9 @@ class EditDefinitionPass(DefinitionPass):
 #
 def pass_lambda(cls):
 
-    def _fn(ckt):
-        cls(ckt).run()
+    def _fn(main, *args, **kwargs):
+        p = cls(main, *args, **kwargs)
+        p.run()
+        return p
 
     return _fn


Test patch:
diff --git a/tests/test_errors/test_tuple_errors.py b/tests/test_errors/test_tuple_errors.py
index 68a63240..5b486361 100644
--- a/tests/test_errors/test_tuple_errors.py
+++ b/tests/test_errors/test_tuple_errors.py
@@ -110,10 +110,7 @@ def test_product_width_mismatch(caplog):
 
     with pytest.raises(Exception) as e:
         m.compile("build/Foo", Foo)
-    assert str(e.value) == """\
-Found unconnected port: Foo.A
-Foo.A: Unconnected\
-"""
+    assert str(e.value) == "Found circuit with errors: Foo"
     assert caplog.messages[0] == """\
 Cannot wire bits(1, 1) (Out(Bits[1])) to Foo.A.x (In(Bits[2]))\
 """
@@ -134,10 +131,7 @@ def test_product_width_mismatch2(caplog):
 
     with pytest.raises(Exception) as e:
         m.compile("build/Foo", Foo)
-    assert str(e.value) == """\
-Found unconnected port: Foo.A.y
-Foo.A.y: Unconnected\
-"""
+    assert str(e.value) == "Found circuit with errors: Foo"
     assert caplog.messages[0] == """\
 Cannot wire Foo.A.x (Out(Bits[4])) to Foo.A.y (In(Bits[2]))\
 """
diff --git a/tests/test_wire/test_errors.py b/tests/test_wire/test_errors.py
index 9f6d2279..bb82a456 100644
--- a/tests/test_wire/test_errors.py
+++ b/tests/test_wire/test_errors.py
@@ -201,4 +201,8 @@ def test_wire_tuple_to_clock():
         io = m.IO(I=In(T), O=m.Out(m.Clock))
         m.wire(io.I, io.O)
 
-    m.compile("build/Foo", Foo)
+    # Imported here to avoid changing line numbers for above tests
+    import pytest
+    with pytest.raises(Exception) as e:
+        m.compile("build/Foo", Foo)
+    assert str(e.value) == "Found circuit with errors: Foo"