Refactor Data.toString (#2197)

Provides a more intuitive implementation of toString for Data.
Utilizes the fact that the compiler plugin provides names earlier than
Chisel had in the past so we can accurately guess the name of
signals even in the currently elaborating module.

Co-authored-by: Megan Wachs <megan@sifive.com>
Co-authored-by: Jack Koenig <jack.koenig3@gmail.com>

8 files changed, 125 insertions, 72 deletions

diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala
index 7c7db1c6..6c6d89c3 100644
--- a/core/src/main/scala/chisel3/Aggregate.scala
+++ b/core/src/main/scala/chisel3/Aggregate.scala
@@ -164,16 +164,14 @@ sealed class Vec[T <: Data] private[chisel3] (gen: => T, val length: Int)
     extends Aggregate with VecLike[T] {
 
   override def toString: String = {
-    val bindingString = topBindingOpt match {
+    topBindingOpt match {
       case Some(VecLitBinding(vecLitBinding)) =>
         val contents = vecLitBinding.zipWithIndex.map { case ((data, lit), index) =>
           s"$index=$lit"
         }.mkString(", ")
-        s"($contents)"
-      case _ => bindingToString
+        s"${sample_element.cloneType}[$length]($contents)"
+      case _ => stringAccessor(s"${sample_element.cloneType}[$length]")
     }
-    val elementType = sample_element.cloneType
-    s"$elementType[$length]$bindingString"
   }
 
   private[chisel3] override def typeEquivalent(that: Data): Boolean = that match {
@@ -922,15 +920,14 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
     * }}}
     */
   override def toString: String = {
-    val bindingString = topBindingOpt match {
+    topBindingOpt match {
       case Some(BundleLitBinding(_)) =>
         val contents = elements.toList.reverse.map { case (name, data) =>
           s"$name=$data"
         }.mkString(", ")
-        s"($contents)"
-      case _ => bindingToString
+        s"$className($contents)"
+      case _ => stringAccessor(s"$className")
     }
-    s"$className$bindingString"
   }
 
   def elements: SeqMap[String, Data]
diff --git a/core/src/main/scala/chisel3/Bits.scala b/core/src/main/scala/chisel3/Bits.scala
index 58ec2585..5ab04d13 100644
--- a/core/src/main/scala/chisel3/Bits.scala
+++ b/core/src/main/scala/chisel3/Bits.scala
@@ -399,11 +399,10 @@ sealed abstract class Bits(private[chisel3] val width: Width) extends Element wi
   */
 sealed class UInt private[chisel3] (width: Width) extends Bits(width) with Num[UInt] {
   override def toString: String = {
-    val bindingString = litOption match {
-      case Some(value) => s"($value)"
-      case _ => bindingToString
+    litOption match {
+      case Some(value) => s"UInt$width($value)"
+      case _ => stringAccessor(s"UInt$width")
     }
-    s"UInt$width$bindingString"
   }
 
   private[chisel3] override def typeEquivalent(that: Data): Boolean =
@@ -773,11 +772,10 @@ sealed class UInt private[chisel3] (width: Width) extends Bits(width) with Num[U
   */
 sealed class SInt private[chisel3] (width: Width) extends Bits(width) with Num[SInt] {
   override def toString: String = {
-    val bindingString = litOption match {
-      case Some(value) => s"($value)"
-      case _ => bindingToString
+    litOption match {
+      case Some(value) => s"SInt$width($value)"
+      case _ => stringAccessor(s"SInt$width")
     }
-    s"SInt$width$bindingString"
   }
 
   private[chisel3] override def typeEquivalent(that: Data): Boolean =
@@ -1039,7 +1037,7 @@ object Reset {
   * super type due to Bool inheriting from abstract class UInt
   */
 final class ResetType(private[chisel3] val width: Width = Width(1)) extends Element with Reset {
-  override def toString: String = s"Reset$bindingToString"
+  override def toString: String = stringAccessor("Reset")
 
   def cloneType: this.type = Reset().asInstanceOf[this.type]
 
@@ -1081,7 +1079,7 @@ object AsyncReset {
   * asychronously reset registers.
   */
 sealed class AsyncReset(private[chisel3] val width: Width = Width(1)) extends Element with Reset {
-  override def toString: String = s"AsyncReset$bindingToString"
+  override def toString: String = stringAccessor("AsyncReset")
 
   def cloneType: this.type = AsyncReset().asInstanceOf[this.type]
 
@@ -1121,11 +1119,10 @@ sealed class AsyncReset(private[chisel3] val width: Width = Width(1)) extends El
   */
 sealed class Bool() extends UInt(1.W) with Reset {
   override def toString: String = {
-    val bindingString = litToBooleanOption match {
-      case Some(value) => s"($value)"
-      case _ => bindingToString
+    litToBooleanOption match {
+      case Some(value) => s"Bool($value)"
+      case _ => stringAccessor("Bool")
     }
-    s"Bool$bindingString"
   }
 
   private[chisel3] override def cloneTypeWidth(w: Width): this.type = {
@@ -1282,11 +1279,10 @@ package experimental {
     extends Bits(width) with Num[FixedPoint] with HasBinaryPoint {
 
     override def toString: String = {
-      val bindingString = litToDoubleOption match {
-        case Some(value) => s"($value)"
-        case _ => bindingToString
+      litToDoubleOption match {
+        case Some(value) => s"FixedPoint$width$binaryPoint($value)"
+        case _ => stringAccessor(s"FixedPoint$width$binaryPoint")
       }
-      s"FixedPoint$width$binaryPoint$bindingString"
     }
 
     private[chisel3] override def typeEquivalent(that: Data): Boolean = that match {
@@ -1702,11 +1698,10 @@ package experimental {
     extends Bits(range.getWidth) with Num[Interval] with HasBinaryPoint {
 
     override def toString: String = {
-      val bindingString = litOption match {
-        case Some(value) => s"($value)"
-        case _ => bindingToString
+      litOption match {
+        case Some(value) => s"Interval$width($value)"
+        case _ => stringAccessor(s"Interval$width")
       }
-      s"Interval$width$bindingString"
     }
 
     private[chisel3] override def cloneTypeWidth(w: Width): this.type =
diff --git a/core/src/main/scala/chisel3/Clock.scala b/core/src/main/scala/chisel3/Clock.scala
index 0400697d..e4be6558 100644
--- a/core/src/main/scala/chisel3/Clock.scala
+++ b/core/src/main/scala/chisel3/Clock.scala
@@ -14,7 +14,7 @@ object Clock {
 
 // TODO: Document this.
 sealed class Clock(private[chisel3] val width: Width = Width(1)) extends Element {
-  override def toString: String = s"Clock$bindingToString"
+  override def toString: String = stringAccessor("Clock")
 
   def cloneType: this.type = Clock().asInstanceOf[this.type]
 
diff --git a/core/src/main/scala/chisel3/Data.scala b/core/src/main/scala/chisel3/Data.scala
index 4ae29ce8..89908401 100644
--- a/core/src/main/scala/chisel3/Data.scala
+++ b/core/src/main/scala/chisel3/Data.scala
@@ -435,27 +435,44 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc {
     _direction = Some(actualDirection)
   }
 
+  private[chisel3] def stringAccessor(chiselType: String): String = {
+    topBindingOpt match {
+      case None => chiselType
+      // Handle DontCares specially as they are "literal-like" but not actually literals
+      case Some(DontCareBinding()) => s"$chiselType(DontCare)"
+      case Some(topBinding) =>
+        val binding: String = _bindingToString(topBinding)
+        val name = earlyName
+        val mod = parentNameOpt.map(_ + ".").getOrElse("")
+
+        s"$mod$name: $binding[$chiselType]"
+    }
+  }
+
   // 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 = Try(topBindingOpt match {
-    case None => ""
-    case Some(OpBinding(enclosure, _)) => s"(OpResult in ${enclosure.desiredName})"
-    case Some(MemoryPortBinding(enclosure, _)) => s"(MemPort in ${enclosure.desiredName})"
-    case Some(PortBinding(enclosure)) if !enclosure.isClosed => s"(IO in unelaborated ${enclosure.desiredName})"
-    case Some(PortBinding(enclosure)) if enclosure.isClosed =>
-      DataMirror.fullModulePorts(enclosure).find(_._2 eq this) match {
-        case Some((name, _)) => s"(IO $name in ${enclosure.desiredName})"
-        case None => s"(IO (unknown) in ${enclosure.desiredName})"
-      }
-    case Some(RegBinding(enclosure, _)) => s"(Reg in ${enclosure.desiredName})"
-    case Some(WireBinding(enclosure, _)) => s"(Wire in ${enclosure.desiredName})"
-    case Some(DontCareBinding()) => s"(DontCare)"
-    case Some(ElementLitBinding(litArg)) => s"(unhandled literal)"
-    case Some(BundleLitBinding(litMap)) => s"(unhandled bundle literal)"
-    case Some(VecLitBinding(litMap)) => s"(unhandled vec literal)"
-  }).getOrElse("")
+  @deprecated("This was never intended to be visible to user-defined types", "Chisel 3.5.0")
+  protected def bindingToString: String = _bindingToString(topBinding)
+
+  private[chisel3] def _bindingToString(topBindingOpt: TopBinding): String =
+    topBindingOpt match {
+      case OpBinding(_, _) => "OpResult"
+      case MemoryPortBinding(_, _) => "MemPort"
+      case PortBinding(_) => "IO"
+      case RegBinding(_, _) => "Reg"
+      case WireBinding(_, _) => "Wire"
+      case DontCareBinding() => "(DontCare)"
+      case ElementLitBinding(litArg) => "(unhandled literal)"
+      case BundleLitBinding(litMap) => "(unhandled bundle literal)"
+      case VecLitBinding(litMap) => "(unhandled vec literal)"
+      case _ => ""
+    }
+
+  private[chisel3] def earlyName: String = Arg.earlyLocalName(this)
+
+  private[chisel3] def parentNameOpt: Option[String] = this._parent.map(_.name)
 
   // Return ALL elements at root of this type.
   // Contasts with flatten, which returns just Bits
diff --git a/core/src/main/scala/chisel3/StrongEnum.scala b/core/src/main/scala/chisel3/StrongEnum.scala
index 9ae4c889..fa420e80 100644
--- a/core/src/main/scala/chisel3/StrongEnum.scala
+++ b/core/src/main/scala/chisel3/StrongEnum.scala
@@ -72,17 +72,18 @@ import EnumAnnotations._
 
 
 abstract class EnumType(private val factory: EnumFactory, selfAnnotating: Boolean = true) extends Element {
+
+  // Use getSimpleName instead of enumTypeName because for debugging purposes
+  //   the fully qualified name isn't necessary (compared to for the
+  //  Enum annotation), and it's more consistent with Bundle printing.
   override def toString: String = {
-    val bindingString = litOption match {
+    litOption match {
       case Some(value) => factory.nameOfValue(value) match {
-        case Some(name) => s"($value=$name)"
-        case None => s"($value=(invalid))"
+        case Some(name) => s"${factory.getClass.getSimpleName.init}($value=$name)"
+        case None => stringAccessor(s"${factory.getClass.getSimpleName.init}($value=(invalid))")
       }
-      case _ => bindingToString
+      case _ => stringAccessor(s"${factory.getClass.getSimpleName.init}")
     }
-    // Use getSimpleName instead of enumTypeName because for debugging purposes the fully qualified name isn't
-    // necessary (compared to for the Enum annotation), and it's more consistent with Bundle printing.
-    s"${factory.getClass.getSimpleName.init}$bindingString"
   }
 
   override def cloneType: this.type = factory().asInstanceOf[this.type]
diff --git a/core/src/main/scala/chisel3/experimental/Analog.scala b/core/src/main/scala/chisel3/experimental/Analog.scala
index 6cca81f5..e94bae2d 100644
--- a/core/src/main/scala/chisel3/experimental/Analog.scala
+++ b/core/src/main/scala/chisel3/experimental/Analog.scala
@@ -27,9 +27,7 @@ import scala.collection.mutable
 final class Analog private (private[chisel3] val width: Width) extends Element {
   require(width.known, "Since Analog is only for use in BlackBoxes, width must be known")
 
-  override def toString: String = {
-    s"Analog$width$bindingToString"
-  }
+  override def toString: String = stringAccessor(s"Analog$width")
 
   private[chisel3] override def typeEquivalent(that: Data): Boolean =
     that.isInstanceOf[Analog] && this.width == that.width
diff --git a/core/src/main/scala/chisel3/internal/firrtl/IR.scala b/core/src/main/scala/chisel3/internal/firrtl/IR.scala
index 1a06cd36..a352c96a 100644
--- a/core/src/main/scala/chisel3/internal/firrtl/IR.scala
+++ b/core/src/main/scala/chisel3/internal/firrtl/IR.scala
@@ -86,6 +86,19 @@ case class Node(id: HasId) extends Arg {
   }
 }
 
+private[chisel3] object Arg {
+  def earlyLocalName(id: HasId): String = id.getOptionRef match {
+    case Some(Index(Node(imm), Node(value))) => s"${earlyLocalName(imm)}[${earlyLocalName(imm)}]"
+    case Some(Index(Node(imm), arg)) => s"${earlyLocalName(imm)}[${arg.localName}]"
+    case Some(Slot(Node(imm), name)) => s"${earlyLocalName(imm)}.$name"
+    case Some(arg) => arg.name
+    case None => id match {
+      case data: Data => data._computeName(None, Some("?")).get
+      case _ => "?"
+    }
+  }
+}
+
 abstract class LitArg(val num: BigInt, widthArg: Width) extends Arg {
   private[chisel3] def forcedWidth = widthArg.known
   private[chisel3] def width: Width = if (forcedWidth) widthArg else Width(minWidth)
@@ -196,7 +209,7 @@ case class Slot(imm: Node, name: String) extends Arg {
     if (immName.isEmpty) name else s"$immName.$name"
   }
 }
-case class Index(imm: Arg, value: Arg) extends Arg {
+case class Index(imm: Node, value: Arg) extends Arg {
   def name: String = s"[$value]"
   override def contextualName(ctx: Component): String = s"${imm.contextualName(ctx)}[${value.contextualName(ctx)}]"
   override def localName: String = s"${imm.localName}[${value.localName}]"
diff --git a/src/test/scala/chiselTests/DataPrint.scala b/src/test/scala/chiselTests/DataPrint.scala
index b5f96c4d..7fb790a8 100644
--- a/src/test/scala/chiselTests/DataPrint.scala
+++ b/src/test/scala/chiselTests/DataPrint.scala
@@ -20,6 +20,14 @@ class DataPrintSpec extends ChiselFlatSpec with Matchers {
     val b = Bool()
   }
 
+  class PartialBundleTest extends Bundle {
+    val a = UInt(8.W)
+    val b = Bool()
+    val c = SInt(8.W)
+    val e = FixedPoint(5.W, 3.BP)
+    val f = EnumTest.Type()
+  }
+
   "Data types" should "have a meaningful string representation" in {
     ChiselStage.elaborate { new RawModule {
       UInt().toString should be ("UInt")
@@ -31,18 +39,20 @@ class DataPrintSpec extends ChiselFlatSpec with Matchers {
       Vec(3, UInt(2.W)).toString should be ("UInt<2>[3]")
       EnumTest.Type().toString should be ("EnumTest")
       (new BundleTest).toString should be ("BundleTest")
-    } }
+      new Bundle { val a = UInt(8.W) }.toString should be ("AnonymousBundle")
+      new Bundle { val a = UInt(8.W) }.a.toString should be ("UInt<8>")
+    }}
   }
 
   class BoundDataModule extends Module {  // not in the test to avoid anon naming suffixes
-    Wire(UInt()).toString should be("UInt(Wire in BoundDataModule)")
-    Reg(SInt()).toString should be("SInt(Reg in BoundDataModule)")
+    Wire(UInt()).toString should be("BoundDataModule.?: Wire[UInt]")
+    Reg(SInt()).toString should be("BoundDataModule.?: Reg[SInt]")
     val io = IO(Output(Bool()))  // needs a name so elaboration doesn't fail
-    io.toString should be("Bool(IO in unelaborated BoundDataModule)")
+    io.toString should be("BoundDataModule.io: IO[Bool]")
     val m = Mem(4, UInt(2.W))
-    m(2).toString should be("UInt<2>(MemPort in BoundDataModule)")
-    (2.U + 2.U).toString should be("UInt<2>(OpResult in BoundDataModule)")
-    Wire(Vec(3, UInt(2.W))).toString should be ("UInt<2>[3](Wire in BoundDataModule)")
+    m(2).toString should be("BoundDataModule.?: MemPort[UInt<2>]")
+    (2.U + 2.U).toString should be("BoundDataModule.?: OpResult[UInt<2>]")
+    Wire(Vec(3, UInt(2.W))).toString should be ("BoundDataModule.?: Wire[UInt<2>[3]]")
 
     class InnerModule extends Module {
       val io = IO(Output(new Bundle {
@@ -50,8 +60,31 @@ class DataPrintSpec extends ChiselFlatSpec with Matchers {
       }))
     }
     val inner = Module(new InnerModule)
-    inner.clock.toString should be ("Clock(IO clock in InnerModule)")
-    inner.io.a.toString should be ("UInt<4>(IO io_a in InnerModule)")
+    inner.clock.toString should be ("InnerModule.clock: IO[Clock]")
+    inner.io.a.toString should be ("InnerModule.io.a: IO[UInt<4>]")
+
+    class FooTypeTest extends Bundle {
+      val foo = Vec(2, UInt(8.W))
+      val fizz = UInt(8.W)
+    }
+    val tpe = new FooTypeTest
+    val fooio: FooTypeTest = IO(Input(tpe))
+    fooio.foo(0).toString should be ("BoundDataModule.fooio.foo[0]: IO[UInt<8>]")
+
+    class NestedBundle extends Bundle {
+      val nestedFoo = UInt(8.W)
+      val nestedFooVec = Vec(2, UInt(8.W))
+    }
+    class NestedType extends Bundle {
+      val foo = new NestedBundle
+    }
+
+    val nestedTpe = new NestedType
+    val nestedio = IO(Input(nestedTpe))
+    (nestedio.foo.nestedFoo.toString should be
+      ("BoundDataModule.nestedio.foo.nestedFoo: IO[UInt<8>]"))
+    (nestedio.foo.nestedFooVec(0).toString should be
+      ("BoundDataModule.nestedio.foo.nestedFooVec[0]: IO[UInt<8>]"))
   }
 
   "Bound data types" should "have a meaningful string representation" in {
@@ -67,13 +100,12 @@ class DataPrintSpec extends ChiselFlatSpec with Matchers {
       true.B.toString should be ("Bool(true)")
       2.25.F(6.W, 2.BP).toString should be ("FixedPoint<6><<2>>(2.25)")
       -2.25.F(6.W, 2.BP).toString should be ("FixedPoint<6><<2>>(-2.25)")
+      Vec(3, UInt(4.W)).toString should be ("UInt<4>[3]")
       EnumTest.sNone.toString should be ("EnumTest(0=sNone)")
       EnumTest.sTwo.toString should be ("EnumTest(2=sTwo)")
       EnumTest(1.U).toString should be ("EnumTest(1=sOne)")
       (new BundleTest).Lit(_.a -> 2.U, _.b -> false.B).toString should be ("BundleTest(a=UInt<8>(2), b=Bool(false))")
-      new Bundle {
-        val a = UInt(8.W)
-      }.toString should be ("AnonymousBundle")
+      (new PartialBundleTest).Lit().toString should be ("PartialBundleTest(a=UInt<8>(DontCare), b=Bool(DontCare), c=SInt<8>(DontCare), e=FixedPoint<5><<3>>(DontCare), f=EnumTest(DontCare))")
       DontCare.toString should be ("DontCare()")
     } }
   }