Sanitize Record._elements (backport #3419) (#3426)

* Sanitize Record._elements (#3419)

This moves name sanitation a little earlier and ensures that the
sanitized names are use more consistently throughout Chisel internals.

This fixes a bug in D/I where unsanitary names would lead to a crash.

(cherry picked from commit 5c282db)

* Keep Scala 2.12 support by not using VectorMap

* Fix firrtl in tests for older firrtl spec version

---------

Co-authored-by: Jack Koenig <[email protected]>

core/src/main/scala/chisel3/Aggregate.scala

@@ -981,16 +981,13 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
   // of hardware created when connecting to one of these elements
   private def setElementRefs(): Unit = {
     val opaqueType = this._isOpaqueType
-    // Since elements is a map, it is impossible for two elements to have the same
-    // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog
-    // which can cause collisions
-    val _namespace = Namespace.empty
     require(
       !opaqueType || (_elements.size == 1 && _elements.head._1 == ""),
       s"Opaque types must have exactly one element with an empty name, not ${_elements.size}: ${elements.keys.mkString(", ")}"
     )
+    // Names of _elements have already been namespaced (and therefore sanitized)
     for ((name, elt) <- _elements) {
-      elt.setRef(this, _namespace.name(name, leadingDigitOk = true), opaque = opaqueType)
+      elt.setRef(this, name, opaque = opaqueType)
     }
   }
 
@@ -1215,15 +1212,25 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
   // without having to recurse over all elements after the Record is
   // constructed. Laziness of _elements means that this check will
   // occur (only) at the first instance _elements is referenced.
+  // Also used to sanitize names
   private[chisel3] lazy val _elements: SeqMap[String, Data] = {
-    for ((name, field) <- elements) {
-      if (field.binding.isDefined) {
-        throw RebindingException(
-          s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware."
-        )
-      }
+    // Since elements is a map, it is impossible for two elements to have the same
+    // identifier; however, Namespace sanitizes identifiers to make them legal for Firrtl/Verilog
+    // which can cause collisions
+    // Note that OpaqueTypes cannot have sanitization (the name of the element needs to stay empty)
+    //   Use an empty Namespace to indicate OpaqueType
+    val namespace = if (!this._isOpaqueType) Some(Namespace.empty) else None
+    elements.map {
+      case (name, field) =>
+        if (field.binding.isDefined) {
+          throw RebindingException(
+            s"Cannot create Record ${this.className}; element ${field} of Record must be a Chisel type, not hardware."
+          )
+        }
+        // namespace.name also sanitizes for firrtl, leave name alone for OpaqueTypes
+        val sanitizedName = namespace.map(_.name(name, leadingDigitOk = true)).getOrElse(name)
+        sanitizedName -> field
     }
-    elements
   }
 
   /** Name for Pretty Printing */

src/test/scala/chiselTests/PrintableSpec.scala

@@ -177,6 +177,20 @@ class PrintableSpec extends AnyFlatSpec with Matchers with Utils {
     }
   }
 
+  it should "print use the sanitized names of Bundle elements" in {
+    class MyModule extends Module {
+      class UnsanitaryBundle extends Bundle {
+        val `a-x` = UInt(8.W)
+      }
+      val myBun = Wire(new UnsanitaryBundle)
+      myBun.`a-x` := 0.U
+      printf(p"$myBun")
+    }
+    generateAndCheck(new MyModule) {
+      case Seq(Printf("UnsanitaryBundle(aminusx -> %d)", Seq("myBun.aminusx"))) =>
+    }
+  }
+
   // Unit tests for cf
   it should "print regular scala variables with cf format specifier" in {
 

src/test/scala/chiselTests/experimental/hierarchy/Examples.scala

@@ -294,6 +294,17 @@ object Examples {
     @public val nested = new NestedInstantiable(in = leafIn, out = leafOut)
 
   }
+  @instantiable
+  class HasUnsanitaryBundleField extends Module {
+    class Interface extends Bundle {
+      val `a-x` = UInt(8.W)
+    }
+    val realIn = IO(Input(new Interface))
+    // It's important to have this redirection to trip an old bug
+    @public val in = realIn.`a-x`
+    @public val out = IO(Output(UInt(8.W)))
+    out := in
+  }
 
   class AddTwoNestedInstantiableData(width: Int) extends Module {
     val in = IO(Input(UInt(width.W)))

src/test/scala/chiselTests/experimental/hierarchy/InstanceSpec.scala

@@ -43,7 +43,7 @@ class InstanceSpec extends ChiselFunSpec with Utils {
       val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top)
       chirrtl should include("inst i0 of AddOne")
     }
-    it("0.3: BlackBoxes should be supported") {
+    it("(0.d): BlackBoxes should be supported") {
       class Top extends Module {
         val in = IO(Input(UInt(32.W)))
         val out = IO(Output(UInt(32.W)))
@@ -66,6 +66,15 @@ class InstanceSpec extends ChiselFunSpec with Utils {
       chirrtl should include("i1.in <= io.in")
       chirrtl should include("io.out <= i1.out")
     }
+    it("(0.e): Instances with Bundles with unsanitary names should be supported") {
+      class Top extends Module {
+        val definition = Definition(new HasUnsanitaryBundleField)
+        val i0 = Instance(definition)
+        i0.in := 123.U
+      }
+      val chirrtl = circt.stage.ChiselStage.emitCHIRRTL(new Top)
+      chirrtl should include("""i0.realIn.aminusx <= UInt<7>("h7b")""")
+    }
   }
   describe("(1) Annotations on instances in same chisel compilation") {
     it("(1.a): should work on a single instance, annotating the instance") {