Improve exception message for aliased bundle fields (#2304)

- Shows groups of field names that share a common id (i.e. aliased)
- Show, as much as possible, them in the order that fields appear in bundle
- Updated BundleSpec's relevant tests

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

3 files changed, 54 insertions, 7 deletions

diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala
index 600e2d11..db354e1f 100644
--- a/core/src/main/scala/chisel3/Aggregate.scala
+++ b/core/src/main/scala/chisel3/Aggregate.scala
@@ -29,7 +29,26 @@ sealed abstract class Aggregate extends Data {
     val resolvedDirection = SpecifiedDirection.fromParent(parentDirection, specifiedDirection)
     val duplicates = getElements.groupBy(identity).collect { case (x, elts) if elts.size > 1 => x }
     if (!duplicates.isEmpty) {
-      throw new AliasedAggregateFieldException(s"Aggregate $this contains aliased fields $duplicates")
+      this match {
+        case b: Record =>
+          // show groups of names of fields with duplicate id's
+          // The sorts make the displayed order of fields deterministic and matching the order of occurrence in the Bundle.
+          // It's a bit convoluted but happens rarely and makes the error message easier to understand
+          val dupNames = duplicates.toSeq.sortBy(_._id).map { duplicate =>
+            b.elements
+              .collect { case x if x._2._id == duplicate._id => x }
+              .toSeq.sortBy(_._2._id)
+              .map(_._1).reverse
+              .mkString("(", ",", ")")
+          }.mkString(",")
+          throw new AliasedAggregateFieldException(
+            s"${b.className} contains aliased fields named ${dupNames}"
+          )
+        case _ =>
+          throw new AliasedAggregateFieldException(
+            s"Aggregate ${this.getClass} contains aliased fields $duplicates ${duplicates.mkString(",")}"
+          )
+      }
     }
     for (child <- getElements) {
       child.bind(ChildBinding(this), resolvedDirection)
diff --git a/src/test/scala/chiselTests/BundleSpec.scala b/src/test/scala/chiselTests/BundleSpec.scala
index 51dedfb1..d9f82e6d 100644
--- a/src/test/scala/chiselTests/BundleSpec.scala
+++ b/src/test/scala/chiselTests/BundleSpec.scala
@@ -111,17 +111,21 @@ class BundleSpec extends ChiselFlatSpec with BundleSpecUtils with Utils {
     }
   }
 
-  "Bundles" should "not have aliased fields" in {
+  "Bundles" should "with aliased fields, should show a helpful error message" in {
+    class AliasedBundle extends Bundle {
+      val a = UInt(8.W)
+      val b = a
+      val c = SInt(8.W)
+      val d = c
+    }
+
     (the[ChiselException] thrownBy extractCause[ChiselException] {
       ChiselStage.elaborate { new Module {
-        val io = IO(Output(new Bundle {
-          val a = UInt(8.W)
-          val b = a
-        }))
+        val io = IO(Output(new AliasedBundle))
         io.a := 0.U
         io.b := 1.U
       } }
-    }).getMessage should include("aliased fields")
+    }).getMessage should include("contains aliased fields named (a,b),(c,d)")
   }
 
   "Bundles" should "not have bound hardware" in {
diff --git a/src/test/scala/chiselTests/RecordSpec.scala b/src/test/scala/chiselTests/RecordSpec.scala
index f0edca8b..e6986efb 100644
--- a/src/test/scala/chiselTests/RecordSpec.scala
+++ b/src/test/scala/chiselTests/RecordSpec.scala
@@ -8,6 +8,8 @@ import chisel3.testers.BasicTester
 import chisel3.util.{Counter, Queue}
 import chisel3.experimental.DataMirror
 
+import scala.collection.immutable.SeqMap
+
 trait RecordSpecUtils {
   class MyBundle extends Bundle {
     val foo = UInt(32.W)
@@ -64,6 +66,11 @@ trait RecordSpecUtils {
     }
   }
 
+  class AliasedRecord extends Module {
+    val field = UInt(32.W)
+    val io = IO(new CustomBundle("in" -> Input(field), "out" -> Output(field)))
+  }
+
   class RecordIOModule extends Module {
     val io = IO(new CustomBundle("in" -> Input(UInt(32.W)), "out" -> Output(UInt(32.W))))
     io("out") := io("in")
@@ -103,6 +110,23 @@ class RecordSpec extends ChiselFlatSpec with RecordSpecUtils with Utils {
     ChiselStage.elaborate { new MyModule(new MyBundle, fooBarType) }
   }
 
+  they should "not allow aliased fields" in {
+    class AliasedFieldRecord extends Record {
+      val foo = UInt(8.W)
+      val elements = SeqMap("foo" -> foo, "bar" -> foo)
+      override def cloneType: AliasedFieldRecord.this.type = this
+    }
+
+    val e = intercept[AliasedAggregateFieldException] {
+      ChiselStage.elaborate {
+        new Module {
+          val io = IO(new AliasedFieldRecord)
+        }
+      }
+    }
+    e.getMessage should include ("contains aliased fields named (bar,foo)")
+  }
+
   they should "follow UInt serialization/deserialization API" in {
     assertTesterPasses { new RecordSerializationTest }
   }