Bugfix - module name collision for injecting aspect (#1635)

* Bugfix - module name collision for injecting aspect

* Fixed mechanism to avoid module name collisions

* Added comments for reviewer feedback

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

4 files changed, 77 insertions, 34 deletions

diff --git a/core/src/main/scala/chisel3/Module.scala b/core/src/main/scala/chisel3/Module.scala
index 82a1708e..236f528e 100644
--- a/core/src/main/scala/chisel3/Module.scala
+++ b/core/src/main/scala/chisel3/Module.scala
@@ -251,7 +251,9 @@ package experimental {
 
     /** Legalized name of this module. */
     final lazy val name = try {
-      Builder.globalNamespace.name(desiredName)
+      // If this is a module aspect, it should share the same name as the original module
+      // Thus, the desired name should be returned without uniquification
+      if(this.isInstanceOf[ModuleAspect]) desiredName else Builder.globalNamespace.name(desiredName)
     } catch {
       case e: NullPointerException => throwException(
         s"Error: desiredName of ${this.getClass.getName} is null. Did you evaluate 'name' before all values needed by desiredName were available?", e)
diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala
index 3988ac68..30fa2db2 100644
--- a/core/src/main/scala/chisel3/internal/Builder.scala
+++ b/core/src/main/scala/chisel3/internal/Builder.scala
@@ -627,7 +627,11 @@ private[chisel3] object Builder {
 
 
   def build[T <: RawModule](f: => T): (Circuit, T) = {
-    dynamicContextVar.withValue(Some(new DynamicContext())) {
+    build(f, new DynamicContext())
+  }
+
+  private [chisel3] def build[T <: RawModule](f: => T, dynamicContext: DynamicContext): (Circuit, T) = {
+    dynamicContextVar.withValue(Some(dynamicContext)) {
       checkScalaVersion()
       errors.info("Elaborating design...")
       val mod = f
diff --git a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
index 747ba18d..8754f4e7 100644
--- a/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
+++ b/src/main/scala/chisel3/aop/injecting/InjectingAspect.scala
@@ -2,9 +2,9 @@
 
 package chisel3.aop.injecting
 
-import chisel3.{Module, ModuleAspect, experimental, withClockAndReset, RawModule, MultiIOModule}
+import chisel3.{Module, ModuleAspect, MultiIOModule, RawModule, experimental, withClockAndReset}
 import chisel3.aop._
-import chisel3.internal.Builder
+import chisel3.internal.{Builder, DynamicContext}
 import chisel3.internal.firrtl.DefModule
 import chisel3.stage.DesignAnnotation
 import firrtl.annotations.ModuleTarget
@@ -42,17 +42,32 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
     injection: M => Unit
 ) extends Aspect[T] {
   final def toAnnotation(top: T): AnnotationSeq = {
-    toAnnotation(selectRoots(top), top.name)
+    val moduleNames = Select.collectDeep(top) { case i => i.name }.toSeq
+    toAnnotation(selectRoots(top), top.name, moduleNames)
   }
 
-  final def toAnnotation(modules: Iterable[M], circuit: String): AnnotationSeq = {
+  /** Returns annotations which contain all injection logic
+    *
+    * @param modules The modules to inject into
+    * @param circuit Top level circuit
+    * @param moduleNames The names of all existing modules in the original circuit, to avoid name collisions
+    * @return
+    */
+  final def toAnnotation(modules: Iterable[M], circuit: String, moduleNames: Seq[String]): AnnotationSeq = {
+    val dynamicContext = new DynamicContext()
+    // Add existing module names into the namespace. If injection logic instantiates new modules
+    //  which would share the same name, they will get uniquified accordingly
+    moduleNames.foreach { n =>
+      dynamicContext.globalNamespace.name(n)
+    }
     RunFirrtlTransformAnnotation(new InjectingTransform) +: modules.map { module =>
       val (chiselIR, _) = Builder.build(Module(new ModuleAspect(module) {
         module match {
           case x: MultiIOModule => withClockAndReset(x.clock, x.reset) { injection(module) }
           case x: RawModule => injection(module)
         }
-      }))
+      }), dynamicContext)
+
       val comps = chiselIR.components.map {
         case x: DefModule if x.name == module.name => x.copy(id = module)
         case other => other
@@ -65,7 +80,7 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
         case m: firrtl.ir.Module if m.name == module.name =>
           stmts += m.body
           Nil
-        case other =>
+        case other: firrtl.ir.Module =>
           Seq(other)
       }
 
diff --git a/src/test/scala/chiselTests/aop/InjectionSpec.scala b/src/test/scala/chiselTests/aop/InjectionSpec.scala
index 3cb0d48b..60233f48 100644
--- a/src/test/scala/chiselTests/aop/InjectionSpec.scala
+++ b/src/test/scala/chiselTests/aop/InjectionSpec.scala
@@ -3,42 +3,46 @@
 package chiselTests.aop
 
 import chisel3.testers.{BasicTester, TesterDriver}
-import chiselTests.ChiselFlatSpec
+import chiselTests.{ChiselFlatSpec, Utils}
 import chisel3._
 import chisel3.aop.injecting.InjectingAspect
+import logger.{LogLevel, LogLevelAnnotation}
 
-class SubmoduleManipulationTester extends BasicTester {
-  val moduleSubmoduleA = Module(new SubmoduleA)
-}
+object InjectionHierarchy {
 
-class SubmoduleA extends Module {
-  val io = IO(new Bundle {
-    val out = Output(Bool())
-  })
-  io.out := false.B
-}
+  class SubmoduleManipulationTester extends BasicTester {
+    val moduleSubmoduleA = Module(new SubmoduleA)
+  }
 
-class SubmoduleB extends Module {
-  val io = IO(new Bundle {
-    val in = Input(Bool())
-  })
-}
+  class SubmoduleA extends Module {
+    val io = IO(new Bundle {
+      val out = Output(Bool())
+    })
+    io.out := false.B
+  }
 
-class AspectTester(results: Seq[Int]) extends BasicTester {
-  val values = VecInit(results.map(_.U))
-  val counter = RegInit(0.U(results.length.W))
-  counter := counter + 1.U
-  when(counter >= values.length.U) {
-    stop()
-  }.otherwise {
-    when(reset.asBool() === false.B) {
-      printf("values(%d) = %d\n", counter, values(counter))
-      assert(counter === values(counter))
+  class SubmoduleB extends Module {
+    val io = IO(new Bundle {
+      val in = Input(Bool())
+    })
+  }
+
+  class AspectTester(results: Seq[Int]) extends BasicTester {
+    val values = VecInit(results.map(_.U))
+    val counter = RegInit(0.U(results.length.W))
+    counter := counter + 1.U
+    when(counter >= values.length.U) {
+      stop()
+    }.otherwise {
+      when(reset.asBool() === false.B) {
+        assert(counter === values(counter))
+      }
     }
   }
 }
 
-class InjectionSpec extends ChiselFlatSpec {
+class InjectionSpec extends ChiselFlatSpec with Utils {
+  import InjectionHierarchy._
   val correctValueAspect = InjectingAspect(
     {dut: AspectTester => Seq(dut)},
     {dut: AspectTester =>
@@ -67,6 +71,16 @@ class InjectionSpec extends ChiselFlatSpec {
     }
   )
 
+  val duplicateSubmoduleAspect = InjectingAspect(
+    {dut: SubmoduleManipulationTester => Seq(dut)},
+    {_: SubmoduleManipulationTester =>
+      // By creating a second SubmoduleA, the module names would conflict unless they were uniquified
+      val moduleSubmoduleA2 = Module(new SubmoduleA)
+      //if we're here then we've elaborated correctly
+      stop()
+    }
+  )
+
   "Test" should "pass if inserted the correct values" in {
     assertTesterPasses{ new AspectTester(Seq(0, 1, 2)) }
   }
@@ -88,4 +102,12 @@ class InjectionSpec extends ChiselFlatSpec {
   "Test" should "pass if the submodules in SubmoduleManipulationTester can be manipulated by manipulateSubmoduleAspect" in {
     assertTesterPasses({ new SubmoduleManipulationTester} , Nil, Seq(manipulateSubmoduleAspect) ++ TesterDriver.verilatorOnly)
   }
+
+  "Module name collisions when adding a new module" should "be resolved" in {
+    assertTesterPasses(
+      { new SubmoduleManipulationTester},
+      Nil,
+      Seq(duplicateSubmoduleAspect) ++ TesterDriver.verilatorOnly
+    )
+  }
 }