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.

4 files changed, 33 insertions, 21 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
   }
diff --git a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
index c9b37fc5..5c5c690e 100644
--- a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
+++ b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
@@ -257,7 +257,7 @@ private[chisel3] object Builder {
   def errors: ErrorLog = dynamicContext.errors
   def error(m: => String): Unit = errors.error(m)
   def warning(m: => String): Unit = errors.warning(m)
-  def deprecated(m: => String): Unit = errors.deprecated(m)
+  def deprecated(m: => String, location: Option[String] = None): Unit = errors.deprecated(m, location)
 
   /** Record an exception as an error, and throw it.
     *
diff --git a/chiselFrontend/src/main/scala/chisel3/internal/Error.scala b/chiselFrontend/src/main/scala/chisel3/internal/Error.scala
index 6e7e4002..7d209219 100644
--- a/chiselFrontend/src/main/scala/chisel3/internal/Error.scala
+++ b/chiselFrontend/src/main/scala/chisel3/internal/Error.scala
@@ -28,8 +28,16 @@ private[chisel3] class ErrorLog {
     println(new Info("[%2.3f] %s".format(elapsedTime/1e3, m), None))  // scalastyle:ignore regex
 
   /** Log a deprecation warning message */
-  def deprecated(m: => String): Unit = {
-    val thisEntry = (m, getUserLineNumber)
+  def deprecated(m: => String, location: Option[String]): Unit = {
+    val sourceLoc = location match {
+      case Some(loc) => loc
+      case None => getUserLineNumber match {
+        case Some(elt: StackTraceElement) => s"${elt.getFileName}:${elt.getLineNumber}"
+        case None => "(unknown)"
+      }
+    }
+
+    val thisEntry = (m, sourceLoc)
     deprecations += ((thisEntry, deprecations.getOrElse(thisEntry, 0) + 1))
   }
 
@@ -38,12 +46,8 @@ private[chisel3] class ErrorLog {
     val depTag = s"[${Console.BLUE}deprecated${Console.RESET}]"
     val warnTag = s"[${Console.YELLOW}warn${Console.RESET}]"
     val errTag = s"[${Console.RED}error${Console.RESET}]"
-    deprecations foreach { case ((message, stackElement), count) =>
-      val line = stackElement match {
-        case Some(stackElement) => s"$depTag ${stackElement.getFileName}:${stackElement.getLineNumber} ($count calls): $message"
-        case None => s"$depTag (unknown location) ($count calls): $message"
-      }
-      println(line)
+    deprecations.foreach { case ((message, sourceLoc), count) =>
+      println(s"$depTag $sourceLoc ($count calls): $message")
     }
     errors foreach println
 
@@ -99,7 +103,7 @@ private[chisel3] class ErrorLog {
   }
 
   private val errors = ArrayBuffer[LogEntry]()
-  private val deprecations = LinkedHashMap[(String, Option[StackTraceElement]), Int]()
+  private val deprecations = LinkedHashMap[(String, String), Int]()
 
   private val startTime = System.currentTimeMillis
   private def elapsedTime: Long = System.currentTimeMillis - startTime