Add checks for misuse or omission of Module()

Implemented by adding a Boolean to check for alternating invocations of object
Module.apply and the constructor of abstract class Module.
Fixes #192

6 files changed, 61 insertions, 66 deletions

diff --git a/chiselFrontend/src/main/scala/chisel3/core/Module.scala b/chiselFrontend/src/main/scala/chisel3/core/Module.scala
index c3353d85..ca391091 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Module.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Module.scala
@@ -25,9 +25,21 @@ object Module {
     // module de-duplication in FIRRTL emission.
     val childSourceInfo = UnlocatableSourceInfo
 
+    if (Builder.readyForModuleConstr) {
+      throwException("Error: Called Module() twice without instantiating a Module." +
+                     sourceInfo.makeMessage(" See " + _))
+    }
+    Builder.readyForModuleConstr = true
     val parent: Option[Module] = Builder.currentModule
-    val m = bc.setRefs() // This will set currentModule!
+
+    val m = bc.setRefs() // This will set currentModule and unset readyForModuleConstr!!!
     m._commands.prepend(DefInvalid(childSourceInfo, m.io.ref)) // init module outputs
+
+    if (Builder.readyForModuleConstr) {
+      throwException("Error: attempted to instantiate a Module, but nothing happened. " +
+                     "This is probably due to rewrapping a Module instance with Module()." +
+                     sourceInfo.makeMessage(" See " + _))
+    }
     Builder.currentModule = parent // Back to parent!
     val ports = m.computePorts
     val component = Component(m, m.name, ports, m._commands)
@@ -93,6 +105,10 @@ extends HasId {
   private[chisel3] val _commands = ArrayBuffer[Command]()
   private[core] val _ids = ArrayBuffer[HasId]()
   Builder.currentModule = Some(this)
+  if (!Builder.readyForModuleConstr) {
+    throwException("Error: attempted to instantiate a Module without wrapping it in Module().")
+  }
+  readyForModuleConstr = false
 
   /** Desired name of this module. */
   def desiredName = this.getClass.getName.split('.').last
diff --git a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
index b4b0e028..028ce628 100644
--- a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
+++ b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
@@ -146,6 +146,9 @@ private[chisel3] class DynamicContext() {
   val globalNamespace = new Namespace(None, Set())
   val components = ArrayBuffer[Component]()
   var currentModule: Option[Module] = None
+  // Set by object Module.apply before calling class Module constructor
+  // Used to distinguish between no Module() wrapping, multiple wrappings, and rewrapping
+  var readyForModuleConstr: Boolean = false
   val errors = new ErrorLog
 }
 
@@ -170,6 +173,10 @@ private[chisel3] object Builder {
       // A bare api call is, e.g. calling Wire() from the scala console).
     )
   }
+  def readyForModuleConstr: Boolean = dynamicContext.readyForModuleConstr
+  def readyForModuleConstr_=(target: Boolean): Unit = {
+    dynamicContext.readyForModuleConstr = target
+  }
 
   // TODO(twigg): Ideally, binding checks and new bindings would all occur here
   // However, rest of frontend can't support this yet.
diff --git a/src/test/scala/chiselTests/AnnotatingExample.scala b/src/test/scala/chiselTests/AnnotatingExample.scala
index c84edf86..04228d6b 100644
--- a/src/test/scala/chiselTests/AnnotatingExample.scala
+++ b/src/test/scala/chiselTests/AnnotatingExample.scala
@@ -141,5 +141,5 @@ object MyDriver extends BackendCompilationUtilities {
   /**
     * illustrates a chisel3 style driver that, annotations can only processed within this structure
     */
-  def buildAnnotatedCircuit[T <: Module](gen: () => T): Map[String, String] = MyBuilder.build(Module(gen()))
-}
\ No newline at end of file
+  def buildAnnotatedCircuit[T <: Module](gen: () => T): Map[String, String] = MyBuilder.build(gen())
+}
diff --git a/src/test/scala/chiselTests/ChiselSpec.scala b/src/test/scala/chiselTests/ChiselSpec.scala
index d335bdf6..7b915c09 100644
--- a/src/test/scala/chiselTests/ChiselSpec.scala
+++ b/src/test/scala/chiselTests/ChiselSpec.scala
@@ -28,7 +28,7 @@ trait ChiselRunners extends Assertions {
 class ChiselFlatSpec extends FlatSpec with ChiselRunners with Matchers
 
 /** Spec base class for property-based testers. */
-class ChiselPropSpec extends PropSpec with ChiselRunners with PropertyChecks {
+class ChiselPropSpec extends PropSpec with ChiselRunners with PropertyChecks with Matchers {
 
   // Constrain the default number of instances generated for every use of forAll.
   implicit override val generatorDrivenConfig =
diff --git a/src/test/scala/chiselTests/DeqIOSpec.scala b/src/test/scala/chiselTests/DeqIOSpec.scala
deleted file mode 100644
index d41c50e5..00000000
--- a/src/test/scala/chiselTests/DeqIOSpec.scala
+++ /dev/null
@@ -1,62 +0,0 @@
-// See LICENSE for license details.
-
-package chiselTests
-
-import chisel3._
-import chisel3.testers.BasicTester
-import chisel3.util._
-
-/**
-  * Created by chick on 2/8/16.
-  */
-class UsesDeqIOInfo extends Bundle {
-  val test_width = 32
-
-  val info_data = UInt.width(test_width)
-}
-
-class UsesDeqIO extends Module {
-  val io = IO(new Bundle {
-    val in = chisel3.util.DeqIO(new UsesDeqIOInfo)
-    val out = chisel3.util.EnqIO(new UsesDeqIOInfo)
-  })
-}
-
-class DeqIOSpec extends ChiselFlatSpec {
-  runTester {
-    new BasicTester {
-      val dut = new UsesDeqIO
-/*
-      "DeqIO" should "set the direction of it's parameter to INPUT" in {
-        assert(dut.io.in.bits.info_data.dir === INPUT)
-      }
-      "DeqIO" should "create a valid input and ready output" in {
-        assert(dut.io.in.valid.dir === INPUT)
-        assert(dut.io.in.ready.dir === OUTPUT)
-      }
-      "EnqIO" should "set the  direction of it's parameter OUTPUT" in {
-        assert(dut.io.out.bits.info_data.dir === OUTPUT)
-      }
-      "EnqIO" should "create a valid input and ready output" in {
-        assert(dut.io.out.valid.dir === OUTPUT)
-        assert(dut.io.out.ready.dir === INPUT)
-      }
-
-      val in_clone = dut.io.in.cloneType
-      val out_clone = dut.io.out.cloneType
-
-      "A deqIO device" should "clone itself with it's directions intact" in {
-        assert(dut.io.in.bits.info_data.dir == in_clone.bits.info_data.dir)
-        assert(dut.io.in.ready.dir == in_clone.ready.dir)
-        assert(dut.io.in.valid.dir == in_clone.valid.dir)
-      }
-
-      "A enqIO device" should "clone itself with it's directions intact" in {
-        assert(dut.io.out.bits.info_data.dir == out_clone.bits.info_data.dir)
-        assert(dut.io.out.ready.dir == out_clone.ready.dir)
-        assert(dut.io.out.valid.dir == out_clone.valid.dir)
-      }
-      */
-    }
-  }
-}
diff --git a/src/test/scala/chiselTests/Module.scala b/src/test/scala/chiselTests/Module.scala
index 7a4050db..c902d073 100644
--- a/src/test/scala/chiselTests/Module.scala
+++ b/src/test/scala/chiselTests/Module.scala
@@ -69,6 +69,22 @@ class ModuleWhen extends Module {
   } otherwise { io.s.out := io.s.in }
 }
 
+class ModuleForgetWrapper extends Module {
+  val io = IO(new SimpleIO)
+  val inst = new PlusOne
+}
+
+class ModuleDoubleWrap extends Module {
+  val io = IO(new SimpleIO)
+  val inst = Module(Module(new PlusOne))
+}
+
+class ModuleRewrap extends Module {
+  val io = IO(new SimpleIO)
+  val inst = Module(new PlusOne)
+  val inst2 = Module(inst)
+}
+
 class ModuleSpec extends ChiselPropSpec {
 
   property("ModuleVec should elaborate") {
@@ -88,4 +104,22 @@ class ModuleSpec extends ChiselPropSpec {
   }
 
   ignore("ModuleWhenTester should return the correct result") { }
+
+  property("Forgetting a Module() wrapper should result in an error") {
+    (the [ChiselException] thrownBy {
+      elaborate { new ModuleForgetWrapper }
+    }).getMessage should include("attempted to instantiate a Module without wrapping it")
+  }
+
+  property("Double wrapping a Module should result in an error") {
+    (the [ChiselException] thrownBy {
+      elaborate { new ModuleDoubleWrap }
+    }).getMessage should include("Called Module() twice without instantiating a Module")
+  }
+
+  property("Rewrapping an already instantiated Module should result in an error") {
+    (the [ChiselException] thrownBy {
+      elaborate { new ModuleRewrap }
+    }).getMessage should include("This is probably due to rewrapping a Module instance")
+  }
 }