Ignore empty aggregates elements when binding aggregate direction (#946)

Previously, including an empty aggregate in a Bundle would cause
a MixedDirectionAggregateException because it has no elements and thus
doesn't have a direction

* Add SampleElementBinding for Vec sample elements
* Add ActualDirection.Empty for bound empty aggregates

5 files changed, 92 insertions, 49 deletions

diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
index b2f01c2a..7a6b9b56 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
@@ -16,7 +16,7 @@ import chisel3.SourceInfoDoc
   * of) other Data objects.
   */
 sealed abstract class Aggregate extends Data {
-  private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) { // scalastyle:ignore cyclomatic.complexity line.size.limit
+  private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) {
     binding = target
 
     val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection)
@@ -25,41 +25,13 @@ sealed abstract class Aggregate extends Data {
     }
 
     // Check that children obey the directionality rules.
-    val childDirections = getElements.map(_.direction).toSet
-    direction = if (childDirections == Set()) {  // Sadly, Scala can't do set matching
-      // If empty, use my assigned direction
-      resolvedDirection match {
-        case SpecifiedDirection.Unspecified | SpecifiedDirection.Flip => ActualDirection.Unspecified
-        case SpecifiedDirection.Output => ActualDirection.Output
-        case SpecifiedDirection.Input => ActualDirection.Input
-      }
-    } else if (childDirections == Set(ActualDirection.Unspecified)) {
-      ActualDirection.Unspecified
-    } else if (childDirections == Set(ActualDirection.Input)) {
-      ActualDirection.Input
-    } else if (childDirections == Set(ActualDirection.Output)) {
-      ActualDirection.Output
-    } else if (childDirections subsetOf
-        Set(ActualDirection.Output, ActualDirection.Input,
-            ActualDirection.Bidirectional(ActualDirection.Default),
-            ActualDirection.Bidirectional(ActualDirection.Flipped))) {
-      resolvedDirection match {
-        case SpecifiedDirection.Unspecified => ActualDirection.Bidirectional(ActualDirection.Default)
-        case SpecifiedDirection.Flip => ActualDirection.Bidirectional(ActualDirection.Flipped)
-        case _ => throw new RuntimeException("Unexpected forced Input / Output")
-      }
-    } else {
-      this match {
-        // Anything flies in compatibility mode
-        case t: Record if !t.compileOptions.dontAssumeDirectionality => resolvedDirection match {
-          case SpecifiedDirection.Unspecified => ActualDirection.Bidirectional(ActualDirection.Default)
-          case SpecifiedDirection.Flip => ActualDirection.Bidirectional(ActualDirection.Flipped)
-          case _ => ActualDirection.Bidirectional(ActualDirection.Default)
-        }
-        case _ =>
-          val childWithDirections = getElements zip getElements.map(_.direction)
-          throw Binding.MixedDirectionAggregateException(s"Aggregate '$this' can't have elements that are both directioned and undirectioned: $childWithDirections") // scalastyle:ignore line.size.limit
-      }
+    val childDirections = getElements.map(_.direction).toSet - ActualDirection.Empty
+    direction = ActualDirection.fromChildren(childDirections, resolvedDirection) match {
+      case Some(dir) => dir
+      case None =>
+        val childWithDirections = getElements zip getElements.map(_.direction)
+        throw Binding.MixedDirectionAggregateException(
+            s"Aggregate '$this' can't have elements that are both directioned and undirectioned: $childWithDirections")
     }
   }
 
@@ -164,10 +136,26 @@ sealed class Vec[T <: Data] private[core] (gen: => T, val length: Int)
     case _ => false
   }
 
+  private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) {
+    binding = target
+
+    val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection)
+    sample_element.bind(SampleElementBinding(this), resolvedDirection)
+    for (child <- getElements) {  // assume that all children are the same
+      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
+  }
+
   // Note: the constructor takes a gen() function instead of a Seq to enforce
   // that all elements must be the same and because it makes FIRRTL generation
   // simpler.
   private val self: Seq[T] = Vector.fill(length)(gen)
+  for ((elt, i) <- self.zipWithIndex)
+    elt.setRef(this, i)
 
   /**
   * sample_element 'tracks' all changes to the elements.
@@ -253,9 +241,6 @@ sealed class Vec[T <: Data] private[core] (gen: => T, val length: Int)
   override def getElements: Seq[Data] =
     (0 until length).map(apply(_))
 
-  for ((elt, i) <- self.zipWithIndex)
-    elt.setRef(this, i)
-
   /** Default "pretty-print" implementation
     * Analogous to printing a Seq
     * Results in "Vec(elt0, elt1, ...)"
@@ -441,6 +426,19 @@ trait VecLike[T <: Data] extends collection.IndexedSeq[T] with HasId with Source
   * RTL writers should use [[Bundle]].  See [[Record#elements]] for an example.
   */
 abstract class Record(private[chisel3] implicit val compileOptions: CompileOptions) extends Aggregate {
+  private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection): Unit = {
+    try {
+      super.bind(target, parentDirection)
+    } catch {  // nasty compatibility mode shim, where anything flies
+      case e: Binding.MixedDirectionAggregateException if !compileOptions.dontAssumeDirectionality =>
+        val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection)
+        direction = resolvedDirection match {
+          case SpecifiedDirection.Unspecified => ActualDirection.Bidirectional(ActualDirection.Default)
+          case SpecifiedDirection.Flip => ActualDirection.Bidirectional(ActualDirection.Flipped)
+          case _ => ActualDirection.Bidirectional(ActualDirection.Default)
+        }
+    }
+  }
 
   /** The collection of [[Data]]
     *
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
index 55e6bcf9..37ce3f66 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Binding.scala
@@ -87,7 +87,7 @@ sealed trait UnconstrainedBinding extends TopBinding {
   def location: Option[BaseModule] = None
 }
 // A constrained binding can only be read/written by specific modules
-// Location will track where this Module is
+// Location will track where this Module is, and the bound object can be referenced in FIRRTL
 sealed trait ConstrainedBinding extends TopBinding {
   def enclosure: BaseModule
   def location: Option[BaseModule] = Some(enclosure)
@@ -107,6 +107,10 @@ case class WireBinding(enclosure: RawModule) extends ConstrainedBinding
 case class ChildBinding(parent: Data) extends Binding {
   def location: Option[BaseModule] = parent.topBinding.location
 }
+/** Special binding for Vec.sample_element */
+case class SampleElementBinding[T <: Data](parent: Vec[T]) extends Binding {
+  def location = parent.topBinding.location
+}
 // A DontCare element has a specific Binding, somewhat like a literal.
 // It is a source (RHS). It may only be connected/applied to sinks.
 case class DontCareBinding() extends UnconstrainedBinding
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
index 7270d92b..43a851ed 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Bits.scala
@@ -27,11 +27,7 @@ abstract class Element extends Data {
   private[chisel3] override def bind(target: Binding, parentDirection: SpecifiedDirection) {
     binding = target
     val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection)
-    direction = resolvedDirection match {
-      case SpecifiedDirection.Unspecified | SpecifiedDirection.Flip => ActualDirection.Unspecified
-      case SpecifiedDirection.Output => ActualDirection.Output
-      case SpecifiedDirection.Input => ActualDirection.Input
-    }
+    direction = ActualDirection.fromSpecified(resolvedDirection)
   }
 
   private[core] override def topBindingOpt: Option[TopBinding] = super.topBindingOpt match {
diff --git a/chiselFrontend/src/main/scala/chisel3/core/Data.scala b/chiselFrontend/src/main/scala/chisel3/core/Data.scala
index daa89045..03c71af5 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Data.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Data.scala
@@ -54,6 +54,10 @@ object SpecifiedDirection {
 sealed abstract class ActualDirection
 
 object ActualDirection {
+  /** The object does not exist / is empty and hence has no direction
+    */
+  case object Empty extends ActualDirection
+
   /** Undirectioned, struct-like
     */
   case object Unspecified extends ActualDirection
@@ -69,6 +73,42 @@ object ActualDirection {
   case object Flipped extends BidirectionalDirection
 
   case class Bidirectional(dir: BidirectionalDirection) extends ActualDirection
+
+  def fromSpecified(direction: SpecifiedDirection): ActualDirection = direction match {
+    case SpecifiedDirection.Unspecified | SpecifiedDirection.Flip => ActualDirection.Unspecified
+    case SpecifiedDirection.Output => ActualDirection.Output
+    case SpecifiedDirection.Input => ActualDirection.Input
+  }
+
+  /** Determine the actual binding of a container given directions of its children.
+    * Returns None in the case of mixed specified / unspecified directionality.
+    */
+  def fromChildren(childDirections: Set[ActualDirection], containerDirection: SpecifiedDirection):
+      Option[ActualDirection] = {
+    if (childDirections == Set()) {  // Sadly, Scala can't do set matching
+      ActualDirection.fromSpecified(containerDirection) match {
+        case ActualDirection.Unspecified => Some(ActualDirection.Empty)  // empty direction if relative / no direction
+        case dir => Some(dir)  // use assigned direction if specified
+      }
+    } else if (childDirections == Set(ActualDirection.Unspecified)) {
+      Some(ActualDirection.Unspecified)
+    } else if (childDirections == Set(ActualDirection.Input)) {
+      Some(ActualDirection.Input)
+    } else if (childDirections == Set(ActualDirection.Output)) {
+      Some(ActualDirection.Output)
+    } else if (childDirections subsetOf
+      Set(ActualDirection.Output, ActualDirection.Input,
+        ActualDirection.Bidirectional(ActualDirection.Default),
+        ActualDirection.Bidirectional(ActualDirection.Flipped))) {
+      containerDirection match {
+        case SpecifiedDirection.Unspecified => Some(ActualDirection.Bidirectional(ActualDirection.Default))
+        case SpecifiedDirection.Flip => Some(ActualDirection.Bidirectional(ActualDirection.Flipped))
+        case _ => throw new RuntimeException("Unexpected forced Input / Output")
+      }
+    } else {
+      None
+    }
+  }
 }
 
 object debug {  // scalastyle:ignore object.name
@@ -274,9 +314,14 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc { // sc
     _binding = Some(target)
   }
 
-  private[core] def topBindingOpt: Option[TopBinding] = _binding.map {
-    case ChildBinding(parent) => parent.topBinding
-    case bindingVal: TopBinding => bindingVal
+  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
   }
 
   private[core] def topBinding: TopBinding = topBindingOpt.get
diff --git a/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala b/chiselFrontend/src/main/scala/chisel3/core/StrongEnum.scala
index 3439cf16..c359a119 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 && litOption.isEmpty) {
+    if (selfAnnotating && topBindingOpt.isDefined && topBindingOpt.get.isInstanceOf[ConstrainedBinding]) {
       annotateEnum()
     }
   }