Make uncloneable IO deprecated instead of error, improve error handling (#771)

It appears #754 breaks more code than I thought, so this makes it a soft error (with a runtime deprecation warning - to get people to fix their stuff before we break it for real) for now.

This additionally changes the autoclonetype errors to be more deterministic (reporting class names instead of object names) in the most common cases, to allow the deprecations manager to deduplicate warnings.

2 files changed, 19 insertions, 11 deletions

diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
index fa69bef0..4b35c163 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
@@ -473,6 +473,8 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
   def toPrintable: Printable = toPrintableHelper(elements.toList)
 }
 
+class AutoClonetypeException(message: String) extends ChiselException(message, null)
+
 /** Base class for data types defined as a bundle of other data types.
   *
   * Usage: extend this class (either as an anonymous or named class) and define
@@ -566,16 +568,15 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
     // This attempts to infer constructor and arguments to clone this Bundle subtype without
     // requiring the user explicitly overriding cloneType.
     import scala.language.existentials
+    import scala.reflect.runtime.universe._
+
+    val clazz = this.getClass
 
     def reflectError(desc: String): Nothing = {
-      Builder.exception(s"Unable to automatically infer cloneType on $this: $desc").asInstanceOf[Nothing]
+      throw new AutoClonetypeException(s"Unable to automatically infer cloneType on $clazz: $desc")
     }
 
-    import scala.reflect.runtime.universe._
-
     // Check if this is an inner class, and if so, try to get the outer instance
-    val clazz = this.getClass
-
     val outerClassInstance = Option(clazz.getEnclosingClass).map { outerClass =>
       def canAssignOuterClass(x: Object) = outerClass.isAssignableFrom(x.getClass)
 
@@ -632,7 +633,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
         case Some(clone) =>
           clone._outerInst = this._outerInst
           if (!clone.typeEquivalent(this)) {
-            reflectError(s"Automatically cloned $clone not type-equivalent to base $this." +
+            reflectError(s"automatically cloned $clone not type-equivalent to base." +
             " Constructor argument values were not inferred, ensure constructor is deterministic.")
           }
           return clone.asInstanceOf[this.type]
@@ -645,7 +646,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
     val classSymbol = try {
       mirror.reflect(this).symbol
     } catch {
-      case e: scala.reflect.internal.Symbols#CyclicReference => reflectError(s"Scala cannot reflect on $this, got exception $e." +
+      case e: scala.reflect.internal.Symbols#CyclicReference => reflectError(s"got exception $e attempting Scala reflection." +
           " This is known to occur with inner classes on anonymous outer classes." +
           " In those cases, autoclonetype only works with no-argument constructors, or you can define a custom cloneType.")
     }
@@ -679,7 +680,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
         return clone
       } catch {
         case e @ (_: java.lang.reflect.InvocationTargetException | _: IllegalArgumentException) =>
-          reflectError(s"Unexpected failure at constructor invocation, got $e.")
+          reflectError(s"unexpected failure at constructor invocation, got $e.")
       }
     }
 
@@ -698,7 +699,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
     val accessorsName = accessors.filter(_.isStable).map(_.name.toString)
     val paramsDiff = ctorParamsNames.toSet -- accessorsName.toSet
     if (!paramsDiff.isEmpty) {
-      reflectError(s"constructor has parameters $paramsDiff that are not both immutable and accessible." +
+      reflectError(s"constructor has parameters (${paramsDiff.toList.sorted.mkString(", ")}) 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.")
     }
 
@@ -716,7 +717,7 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
       case (paramName, paramVal: Data) if paramVal.hasBinding => paramName
     }
     if (boundDataParamNames.nonEmpty) {
-      reflectError(s"constructor parameters ($boundDataParamNames) have values that are hardware types, which is likely to cause subtle errors." +
+      reflectError(s"constructor parameters (${boundDataParamNames.sorted.mkString(", ")}) have values that are hardware types, which is likely to cause subtle errors." +
           " Use chisel types instead: use the value before it is turned to a hardware type (with Wire(...), Reg(...), etc) or use chiselTypeOf(...) to extract the chisel type.")
     }
 
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Module.scala b/chiselFrontend/src/main/scala/chisel3/core/Module.scala
index 74479c6b..5ba6dbc8 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Module.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Module.scala
@@ -255,7 +255,14 @@ abstract class BaseModule extends HasId {
     requireIsChiselType(iodef, "io type")
 
     // Clone the IO so we preserve immutability of data types
-    val iodefClone = iodef.cloneTypeFull
+    val iodefClone = try {
+      iodef.cloneTypeFull
+    } catch {
+      // For now this is going to be just a deprecation so we don't suddenly break everyone's code
+      case e: AutoClonetypeException =>
+        Builder.deprecated(e.getMessage, Some(s"${iodef.getClass}"))
+        iodef
+    }
     _bindIoInPlace(iodefClone)
     iodefClone
   }