Autoclonetype initial prototype

8 files changed, 190 insertions, 66 deletions

diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
index b89961aa..d4d67018 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
@@ -557,30 +557,107 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
   }
 
   override def cloneType : this.type = {
-    // If the user did not provide a cloneType method, try invoking one of
-    // the following constructors, not all of which necessarily exist:
-    // - A zero-parameter constructor
-    // - A one-paramater constructor, with null as the argument
-    // - A one-parameter constructor for a nested Bundle, with the enclosing
-    //   parent Module as the argument
-    val constructor = this.getClass.getConstructors.head
-    try {
-      val args = Seq.fill(constructor.getParameterTypes.size)(null)
-      constructor.newInstance(args:_*).asInstanceOf[this.type]
-    } catch {
-      case e: java.lang.reflect.InvocationTargetException if e.getCause.isInstanceOf[java.lang.NullPointerException] =>
-        try {
-          constructor.newInstance(_parent.get).asInstanceOf[this.type]
-        } catch {
-          case _: java.lang.reflect.InvocationTargetException | _: java.lang.IllegalArgumentException =>
-            Builder.exception(s"Parameterized Bundle ${this.getClass} needs cloneType method. You are probably using " +
-              "an anonymous Bundle object that captures external state and hence is un-cloneTypeable")
-            this
-        }
-      case _: java.lang.reflect.InvocationTargetException | _: java.lang.IllegalArgumentException =>
-        Builder.exception(s"Parameterized Bundle ${this.getClass} needs cloneType method")
-        this
+    // Automatic Bundle cloning is inferred as follows:
+    // - User-overloaded cloneType is called instead of this (if supplied, so this method is never called)
+    // - Attempt to automatically fill in constructor parameters by using Scala reflection:
+    //   - If multiple constructors, error out
+    //   - If the constructor argument list can be matched to param accessors, call the constructor with
+    //     the result of those accessors
+    //   - Otherwise, error out
+    // - If Scala reflection crashes, fall back to Java reflection and inspect the constructor argument list:
+    //   - If no parameters, invoke constructor directly. That was easy!
+    //   - If the first parameter's type is equal to this Module, assume the Bundle is an inner class
+    //     and invoke it with the current containing Module
+    // - Error out to the user
+
+    def reflectError(desc: String) {
+      Builder.exception(s"Unable to automatically infer cloneType on $this: $desc")
+    }
+
+    // Try Scala reflection
+    import scala.reflect.runtime.universe._
+
+    val mirror = runtimeMirror(this.getClass.getClassLoader)
+    val decls = mirror.classSymbol(this.getClass).typeSignature.decls
+    println(s"$this decls: $decls")
+
+    val ctors = decls.collect { case meth: MethodSymbol if meth.isConstructor => meth }
+    if (ctors.size != 1) {
+      reflectError(s"found multiple constructors ($ctors). " +
+          "Either remove all but the default constructor, or define a custom cloneType method.")
+    }
+
+    val accessors = decls.collect { case meth: MethodSymbol if meth.isParamAccessor => meth }
+    val ctorParamss = ctors.head.paramLists
+    val ctorParams = ctorParamss match {
+      case Nil => List()
+      case ctorParams :: Nil => ctorParams
+      case _ => reflectError(s"internal error, unexpected ctorParamss = $ctorParamss")
+          List()  // make the type system happy
+    }
+
+    // Check that all ctor params are immutable and accessible. Immutability is required to avoid
+    // potential subtle bugs (like values changing after cloning).
+    // This also generates better error messages (all missing elements shown at once) instead of
+    // failing at the use site one at a time.
+    val ctorParamsName = ctorParams.map(_.name.toString)
+    val accessorsName = accessors.filter(_.isStable).map(_.name.toString)
+    val paramsDiff = ctorParamsName.toSet -- accessorsName.toSet
+    if (!paramsDiff.isEmpty) {
+      reflectError(s"constructor has parameters ($paramsDiff) that are not both immutable and accessible." +
+          " Either make all parameters immutable and accessible (vals) so cloneType can be inferred, or define a custom cloneType method.")
     }
+
+    // Get all the argument values
+    val accessorsMap = accessors.map(accessor => accessor.name.toString -> accessor).toMap
+    val instanceReflect = mirror.reflect(this)
+    val ctorParamsVals = ctorParamsName.map {
+      paramName => instanceReflect.reflectMethod(accessorsMap(paramName)).apply()
+    }
+
+    // Invoke ctor
+    val classReflect = mirror.reflectClass(mirror.classSymbol(this.getClass))
+    classReflect.reflectConstructor(ctors.head).apply(ctorParamsVals:_*).asInstanceOf[this.type]
+
+
+
+//    val constructor = this.getClass.getConstructors.head
+//    val args = Seq.fill(constructor.getParameterTypes.size)(null)
+//    constructor.newInstance(args:_*).asInstanceOf[this.type]
+
+//    // If the user did not provide a cloneType method, try invoking one of
+//    // the following constructors, not all of which necessarily exist:
+//    // - A zero-parameter constructor
+//    // - A one-paramater constructor, with null as the argument
+//    // - A one-parameter constructor for a nested Bundle, with the enclosing
+//    //   parent Module as the argument
+//
+//
+//    // val classMirror = runtimeMirror.reflectClass(classSymbol)
+////    val constructors = classSymbol.typeSignature.members.filter(_.isConstructor).toList
+////    println(constructors)
+////
+//    val constructor = this.getClass.getConstructors.head
+//    println(constructor)
+//    println(constructor.getParameterTypes)
+//
+//    try {
+//      val args = Seq.fill(constructor.getParameterTypes.size)(null)
+//      constructor.newInstance(args:_*).asInstanceOf[this.type]
+//    } catch {
+//      case e: java.lang.reflect.InvocationTargetException if e.getCause.isInstanceOf[java.lang.NullPointerException] =>
+//        try {
+//          constructor.newInstance(_parent.get).asInstanceOf[this.type]
+//        } catch {
+//          case _: java.lang.reflect.InvocationTargetException | _: java.lang.IllegalArgumentException =>
+//            Builder.exception(s"Parameterized Bundle ${this.getClass} needs cloneType method. You are probably using " +
+//              "an anonymous Bundle object that captures external state and hence is un-cloneTypeable")
+//            this
+//        }
+//      case _: java.lang.reflect.InvocationTargetException | _: java.lang.IllegalArgumentException =>
+//        Builder.exception(s"Parameterized Bundle ${this.getClass} needs cloneType method")
+//        this
+//    }
   }
 
   /** Default "pretty-print" implementation
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
index 3fdc383c..a6cc8bac 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
@@ -23,7 +23,7 @@ object Binding {
 object requireIsHardware {
   def apply(node: Data, msg: String = "") = {
     node._parent match {  // Compatibility layer hack
-      case Some(x: BaseModule) => x._autoWrapPorts
+      case Some(x: BaseModule) => x._compatAutoWrapPorts
       case _ =>
     }
     if (!node.hasBinding) {
diff --git a/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala b/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala
index aa0f8064..3c99886e 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/BlackBox.scala
@@ -126,12 +126,12 @@ abstract class ExtModule(val params: Map[String, Param] = Map.empty[String, Para
   */
 abstract class BlackBox(val params: Map[String, Param] = Map.empty[String, Param])(implicit compileOptions: CompileOptions) extends BaseBlackBox {
   def io: Record
-
+  
   // Allow access to bindings from the compatibility package
-  protected def _ioPortBound() = portsContains(io)
-
+  protected def _compatIoPortBound() = portsContains(io)
+  
   private[core] override def generateComponent(): Component = {
-    _autoWrapPorts()  // pre-IO(...) compatibility hack
+    _compatAutoWrapPorts()  // pre-IO(...) compatibility hack
 
     // Restrict IO to just io, clock, and reset
     require(io != null, "BlackBox must have io")
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Module.scala b/chiselFrontend/src/main/scala/chisel3/core/Module.scala
index fa9ab082..0c69010b 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Module.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Module.scala
@@ -191,7 +191,7 @@ abstract class BaseModule extends HasId {
     *
     * TODO: remove this, perhaps by removing Bindings checks in compatibility mode.
     */
-  def _autoWrapPorts() {}
+  def _compatAutoWrapPorts() {}
 
   //
   // BaseModule User API functions
@@ -200,29 +200,10 @@ abstract class BaseModule extends HasId {
     Builder.annotations += annotation
   }
 
-  /**
-   * This must wrap the datatype used to set the io field of any Module.
-   * i.e. All concrete modules must have defined io in this form:
-   * [lazy] val io[: io type] = IO(...[: io type])
-   *
-   * Items in [] are optional.
-   *
-   * The granted iodef WILL NOT be cloned (to allow for more seamless use of
-   * anonymous Bundles in the IO) and thus CANNOT have been bound to any logic.
-   * This will error if any node is bound (e.g. due to logic in a Bundle
-   * constructor, which is considered improper).
-   *
-   * Also registers a Data as a port, also performing bindings. Cannot be called once ports are
-   * requested (so that all calls to ports will return the same information).
-   * Internal API.
-   *
-   * TODO(twigg): Specifically walk the Data definition to call out which nodes
-   * are problematic.
-   */
-  protected def IO[T<:Data](iodef: T): iodef.type = {
-    require(!_closed, "Can't add more ports after module close")
-    requireIsChiselType(iodef, "io type")
-
+  /** Chisel2 code didn't require the IO(...) wrapper and would assign a Chisel type directly to
+    * io, then do operations on it. This binds a Chisel type in-place (mutably) as an IO. 
+    */
+  protected def _bindIoInPlace(iodef: Data) {
     // Compatibility code: Chisel2 did not require explicit direction on nodes
     // (unspecified treated as output, and flip on nothing was input).
     // This sets assigns the explicit directions required by newer semantics on
@@ -248,11 +229,38 @@ abstract class BaseModule extends HasId {
       }
     }
     assignCompatDir(iodef, false)
-
-    // Bind each element of the iodef to being a Port
+    
     iodef.bind(PortBinding(this))
     _ports += iodef
-    iodef
+  }
+      
+  /**
+   * This must wrap the datatype used to set the io field of any Module.
+   * i.e. All concrete modules must have defined io in this form:
+   * [lazy] val io[: io type] = IO(...[: io type])
+   *
+   * Items in [] are optional.
+   *
+   * The granted iodef WILL NOT be cloned (to allow for more seamless use of
+   * anonymous Bundles in the IO) and thus CANNOT have been bound to any logic.
+   * This will error if any node is bound (e.g. due to logic in a Bundle
+   * constructor, which is considered improper).
+   *
+   * Also registers a Data as a port, also performing bindings. Cannot be called once ports are
+   * requested (so that all calls to ports will return the same information).
+   * Internal API.
+   *
+   * TODO(twigg): Specifically walk the Data definition to call out which nodes
+   * are problematic.
+   */
+  protected def IO[T<:Data](iodef: T): iodef.type = {
+    require(!_closed, "Can't add more ports after module close")
+    requireIsChiselType(iodef, "io type")
+    
+    // Clone the IO so we preserve immutability of data types
+    val iodefClone = iodef.cloneTypeFull
+    _bindIoInPlace(iodefClone)
+    iodefClone.asInstanceOf[iodef.type]
   }
 
   //
diff --git a/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala b/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala
index 9c923037..ac87eda5 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/UserModule.scala
@@ -151,8 +151,8 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions)
   def io: Record
 
   // Allow access to bindings from the compatibility package
-  protected def _ioPortBound() = portsContains(io)
-
+  protected def _compatIoPortBound() = portsContains(io)
+  
   protected override def nameIds(rootClass: Class[_]): HashMap[HasId, String] = {
     val names = super.nameIds(rootClass)
 
@@ -165,7 +165,7 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions)
   }
 
   private[core] override def generateComponent(): Component = {
-    _autoWrapPorts()  // pre-IO(...) compatibility hack
+    _compatAutoWrapPorts()  // pre-IO(...) compatibility hack
 
     // Restrict IO to just io, clock, and reset
     require(io != null, "Module must have io")
diff --git a/src/main/scala/chisel3/compatibility.scala b/src/main/scala/chisel3/compatibility.scala
index f299a287..3c12b483 100644
--- a/src/main/scala/chisel3/compatibility.scala
+++ b/src/main/scala/chisel3/compatibility.scala
@@ -250,9 +250,9 @@ package object Chisel {     // scalastyle:ignore package.object.name
   abstract class BlackBox(params: Map[String, Param] = Map.empty[String, Param]) extends chisel3.core.BlackBox(params) {
     // This class auto-wraps the BlackBox with IO(...), allowing legacy code (where IO(...) wasn't
     // required) to build.
-    override def _autoWrapPorts(): Unit = { // scalastyle:ignore method.name
-      if (!_ioPortBound()) {
-        IO(io)
+    override def _compatAutoWrapPorts(): Unit = { // scalastyle:ignore method.name
+      if (!_compatIoPortBound()) {
+        _bindIoInPlace(io)
       }
     }
   }
@@ -278,9 +278,9 @@ package object Chisel {     // scalastyle:ignore package.object.name
     def this(_clock: Clock, _reset: Bool)(implicit moduleCompileOptions: CompileOptions) =
       this(Option(_clock), Option(_reset))(moduleCompileOptions)
 
-    override def _autoWrapPorts(): Unit = { // scalastyle:ignore method.name
-      if (!_ioPortBound() && io != null) {
-        IO(io)
+    override def _compatAutoWrapPorts(): Unit = { // scalastyle:ignore method.name
+      if (!_compatIoPortBound() && io != null) {
+        _bindIoInPlace(io)
       }
     }
   }
diff --git a/src/main/scala/chisel3/testers/BasicTester.scala b/src/main/scala/chisel3/testers/BasicTester.scala
index 6d1a4913..4cd03863 100644
--- a/src/main/scala/chisel3/testers/BasicTester.scala
+++ b/src/main/scala/chisel3/testers/BasicTester.scala
@@ -11,9 +11,14 @@ import internal.firrtl._
 import internal.sourceinfo.SourceInfo
 //import chisel3.core.ExplicitCompileOptions.NotStrict
 
-class BasicTester extends Module() {
+class TesterIO extends Bundle {
   // The testbench has no IOs, rather it should communicate using printf, assert, and stop.
-  val io = IO(new Bundle())
+  // This is here (instead of just `new Bundle()`, since that has an implicit compileOptions
+  // constructor argument which is misapplied by clonetype
+}
+
+class BasicTester extends Module() {
+  val io = IO(new TesterIO)
 
   def popCount(n: Long): Int = n.toBinaryString.count(_=='1')
 
diff --git a/src/test/scala/chiselTests/AutoClonetypeSpec.scala b/src/test/scala/chiselTests/AutoClonetypeSpec.scala
new file mode 100644
index 00000000..93031c1c
--- /dev/null
+++ b/src/test/scala/chiselTests/AutoClonetypeSpec.scala
@@ -0,0 +1,34 @@
+// See LICENSE for license details.
+
+package chiselTests
+
+import chisel3._
+
+import chisel3.testers.BasicTester
+
+class BundleWithIntArg(val i: Int) extends Bundle {
+  val out = Output(UInt(i.W))
+}
+
+class ModuleWithInner extends Module {
+  class InnerBundle(val i: Int) extends Bundle {
+    val out = Output(UInt(i.W))
+  }
+
+  val io = IO(new InnerBundle(14))
+  io.out := 1.U
+}
+
+
+class AutoClonetypeSpec extends ChiselFlatSpec {
+  "Bundles with Scala args" should "not need clonetype" in {
+    elaborate { new Module {
+      val io = IO(new BundleWithIntArg(8))
+      io.out := 1.U
+    } }
+  }
+
+  "Inner bundles with Scala args" should "not need clonetype" in {
+    elaborate { new ModuleWithInner }
+  }
+}