Fix treatment of Vec of Analog and Vec of Bundle of Analog (#1091)

* IO(Analog) fixed for RawModule

* Add a Analog Port for RawModule test & spec

* Fixes around Module instantiation and ports in AnalogPortRawModuleTest

* Shorten Comment

* Add Data.isSynthesizable to distinguish SampleElementBinding

This helps clarify the notion of being bound but not hardware.
Data.topBindingOpt is now used to get the *actual* top binding,
including across SampleElements (eg. in Analog checking that the top is
bound to a Port or a Wire)

* Fix pretty printing for Vec

* Refactor tests for Vec of Analog, add test for Vec of Bundle of Analog

6 files changed, 26 insertions, 18 deletions

diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
index c75974f0..7e3920eb 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
@@ -136,7 +136,8 @@ object Vec extends VecFactory
 sealed class Vec[T <: Data] private[core] (gen: => T, val length: Int)
     extends Aggregate with VecLike[T] {
   override def toString: String = {
-    s"$sample_element[$length]$bindingToString"
+    val elementType = sample_element.cloneType
+    s"$elementType[$length]$bindingToString"
   }
 
   private[core] override def typeEquivalent(that: Data): Boolean = that match {
@@ -155,8 +156,6 @@ sealed class Vec[T <: Data] private[core] (gen: => T, val length: Int)
       child.bind(ChildBinding(this), resolvedDirection)
     }
 
-    // Note: this differs from the Aggregate.bind behavior on an empty Vec, since this looks at the
-    // sample element instead of actual elements.
     direction = sample_element.direction
   }
 
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
index 37ce3f66..e30b91ec 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
@@ -30,7 +30,7 @@ object requireIsHardware {
       case Some(x: BaseModule) => x._compatAutoWrapPorts
       case _ =>
     }
-    if (!node.topBindingOpt.isDefined) {
+    if (!node.isSynthesizable) {
       val prefix = if (msg.nonEmpty) s"$msg " else ""
       throw Binding.ExpectedHardwareException(s"$prefix'$node' must be hardware, " +
         "not a bare Chisel type. Perhaps you forgot to wrap it in Wire(_) or IO(_)?")
@@ -41,7 +41,7 @@ object requireIsHardware {
 /** Requires that a node is a chisel type (not hardware, "unbound")
   */
 object requireIsChiselType {
-  def apply(node: Data, msg: String = ""): Unit = if (node.topBindingOpt.isDefined) {
+  def apply(node: Data, msg: String = ""): Unit = if (node.isSynthesizable) {
     val prefix = if (msg.nonEmpty) s"$msg " else ""
     throw Binding.ExpectedChiselTypeException(s"$prefix'$node' must be a Chisel type, not hardware")
   }
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
index e35d12f0..424db5cb 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
@@ -1776,6 +1776,8 @@ final class Analog private (private[chisel3] val width: Width) extends Element {
     val targetTopBinding = target match {
       case target: TopBinding => target
       case ChildBinding(parent) => parent.topBinding
+      // See https://github.com/freechipsproject/chisel3/pull/946
+      case SampleElementBinding(parent) => parent.topBinding
     }
 
     // Analog counts as different directions based on binding context
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Data.scala b/chiselFrontend/src/main/scala/chisel3/core/Data.scala
index abb5675c..7ff58b54 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Data.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Data.scala
@@ -146,7 +146,7 @@ object DataMirror {
 
   // Internal reflection-style APIs, subject to change and removal whenever.
   object internal { // scalastyle:ignore object.name
-    def isSynthesizable(target: Data): Boolean = target.topBindingOpt.isDefined
+    def isSynthesizable(target: Data): Boolean = target.isSynthesizable
     // For those odd cases where you need to care about object reference and uniqueness
     def chiselTypeClone[T<:Data](target: Data): T = {
       target.cloneTypeFull.asInstanceOf[T]
@@ -314,14 +314,18 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { // sc
     _binding = Some(target)
   }
 
+  // Similar to topBindingOpt except it explicitly excludes SampleElements which are bound but not
+  // hardware
+  private[core] final def isSynthesizable: Boolean = _binding.map {
+    case ChildBinding(parent) => parent.isSynthesizable
+    case _: TopBinding => true
+    case _: SampleElementBinding[_] => false
+  }.getOrElse(false)
+
   private[core] def topBindingOpt: Option[TopBinding] = _binding.flatMap {
     case ChildBinding(parent) => parent.topBindingOpt
     case bindingVal: TopBinding => Some(bindingVal)
-    case _: SampleElementBinding[_] => None
-      // TODO: technically, it's bound, but it's more of a ghost binding and None is probably the most appropriate
-      // Note: sample elements should not be user-accessible, so there's not really a good reason to access its
-      // top binding. However, we can't make this assert out right not because a larger refactoring is needed.
-      // See https://github.com/freechipsproject/chisel3/pull/946
+    case SampleElementBinding(parent) => parent.topBindingOpt
   }
 
   private[core] def topBinding: TopBinding = topBindingOpt.get
@@ -351,6 +355,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { // sc
   // User-friendly representation of the binding as a helper function for toString.
   // Provides a unhelpful fallback for literals, which should have custom rendering per
   // Data-subtype.
+  // TODO Is this okay for sample_element? It *shouldn't* be visible to users
   protected def bindingToString: String = topBindingOpt match {
     case None => ""
     case Some(OpBinding(enclosure)) => s"(OpResult in ${enclosure.desiredName})"
diff --git a/chiselFrontend/src/main/scala/chisel3/core/RawModule.scala b/chiselFrontend/src/main/scala/chisel3/core/RawModule.scala
index b1cae1b7..397debcb 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/RawModule.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/RawModule.scala
@@ -75,12 +75,14 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions)
       id match {
         case id: BaseModule => id.forceName(default=id.desiredName, _namespace)
         case id: MemBase[_] => id.forceName(default="_T", _namespace)
-        case id: Data if id.topBindingOpt.isDefined => id.topBinding match {
-          case OpBinding(_) | MemoryPortBinding(_) | PortBinding(_) | RegBinding(_) | WireBinding(_) =>
-            id.forceName(default="_T", _namespace)
-          case _ =>  // don't name literals
-        }
-        case id: Data if id.topBindingOpt.isEmpty =>  // don't name unbound types
+        case id: Data  =>
+          if (id.isSynthesizable) {
+            id.topBinding match {
+              case OpBinding(_) | MemoryPortBinding(_) | PortBinding(_) | RegBinding(_) | WireBinding(_) =>
+                id.forceName(default="_T", _namespace)
+              case _ =>  // don't name literals
+            }
+          } // else, don't name unbound types
       }
       id._onModuleClose
     }
diff --git a/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala b/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala
index 1689e6ad..63f7a8a0 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala
@@ -161,7 +161,7 @@ abstract class EnumType(private val factory: EnumFactory, selfAnnotating: Boolea
     super.bind(target, parentDirection)
 
     // Make sure we only annotate hardware and not literals
-    if (selfAnnotating && topBindingOpt.isDefined && topBindingOpt.get.isInstanceOf[ConstrainedBinding]) {
+    if (selfAnnotating && isSynthesizable && topBindingOpt.get.isInstanceOf[ConstrainedBinding]) {
       annotateEnum()
     }
   }