Restore return type of BaseModule.toTarget to ModuleTarget (#2117)

Definition/Instance introduced the need for representing the targets of
instances as InstanceTargets. This original implementation changed the
return type of BaseModule.toTarget to express this need. This is a
backwards incompatible change that is actually unnecessary because it is
impossible for users to get references to the internal InstanceClone
objects, instead only accessing such modules via Instance[_] wrappers
and cloned Data. We restored the old API by adding a new internal method
"getTarget" which will give the correct targets for InstanceClones while
maintaining the API of BaseModule.toTarget.

4 files changed, 35 insertions, 22 deletions

diff --git a/core/src/main/scala/chisel3/Module.scala b/core/src/main/scala/chisel3/Module.scala
index d0171693..3ae48821 100644
--- a/core/src/main/scala/chisel3/Module.scala
+++ b/core/src/main/scala/chisel3/Module.scala
@@ -469,17 +469,29 @@ package experimental {
       *
       * @note Should not be called until circuit elaboration is complete
       */
-    final def toTarget: IsModule = {
-      this match {
-        case m: internal.BaseModule.InstanceClone[_] if m._parent.nonEmpty => m._parent.get.toTarget.instOf(instanceName, name)
-        case m: internal.BaseModule.InstanceClone[_] => ModuleTarget(this.circuitName, this.name)
-        case m: internal.BaseModule.ModuleClone[_] if m._madeFromDefinition => m._parent.get.toTarget.instOf(instanceName, name)
-        case m: internal.BaseModule.ModuleClone[_] => ModuleTarget(this.circuitName, this.name)
-        // Without this, we get the wrong CircuitName for the Definition
-        case m: internal.BaseModule.DefinitionClone[_] if m._circuit.nonEmpty => ModuleTarget(this._circuit.get.circuitName, this.name)
-        case m: internal.BaseModule.DefinitionClone[_] => ModuleTarget(this.circuitName, this.name)
-        case m => ModuleTarget(this.circuitName, this.name)
-      }
+    final def toTarget: ModuleTarget = this match {
+      case m: internal.BaseModule.InstanceClone[_] => throwException(s"Internal Error! It's not legal to call .toTarget on an InstanceClone. $m")
+      case m: internal.BaseModule.DefinitionClone[_] => throwException(s"Internal Error! It's not legal to call .toTarget on an DefinitionClone. $m")
+      case _ => ModuleTarget(this.circuitName, this.name)
+    }
+
+    /** Returns the real target of a Module which may be an [[InstanceTarget]]
+      *
+      * BaseModule.toTarget returns a ModuleTarget because the classic Module(new MyModule) API elaborates
+      * Modules in a way that there is a 1:1 relationship between instances and elaborated definitions
+      *
+      * Instance/Definition introduced special internal modules [[InstanceClone]] and [[ModuleClone]] that
+      * do not have this 1:1 relationship so need the ability to return [[InstanceTarget]]s.
+      * Because users can never actually get references to these underlying objects, we can maintain
+      * BaseModule.toTarget's API returning [[ModuleTarget]] while providing an internal API for getting
+      * the correct [[InstanceTarget]]s whenever using the Definition/Instance API.
+      */
+    private[chisel3] def getTarget: IsModule = this match {
+      case m: internal.BaseModule.InstanceClone[_] if m._parent.nonEmpty => m._parent.get.getTarget.instOf(instanceName, name)
+      case m: internal.BaseModule.ModuleClone[_] if m._madeFromDefinition => m._parent.get.getTarget.instOf(instanceName, name)
+      // Without this, we get the wrong CircuitName for the Definition
+      case m: internal.BaseModule.DefinitionClone[_] if m._circuit.nonEmpty => ModuleTarget(this._circuit.get.circuitName, this.name)
+      case _ => this.toTarget
     }
 
     /** Returns a FIRRTL ModuleTarget that references this object
@@ -496,7 +508,7 @@ package experimental {
           // is always unambigous. However, legacy workarounds for Chisel's lack of an instance API
           // have lead some to use .toAbsoluteTarget as a workaround. A proper instance API will make
           // it possible to deprecate and remove .toAbsoluteTarget
-          if (this == ViewParent) ViewParent.absoluteTarget else toTarget
+          if (this == ViewParent) ViewParent.absoluteTarget else getTarget
       }
     }
 
diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
index 2e917dfa..0cc3d131 100644
--- a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
+++ b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
@@ -3,13 +3,14 @@
 package chisel3.experimental.hierarchy
 
 import scala.language.experimental.macros
-
 import chisel3._
+
 import scala.collection.mutable.HashMap
 import chisel3.internal.{Builder, DynamicContext}
 import chisel3.internal.sourceinfo.{DefinitionTransform, DefinitionWrapTransform, SourceInfo}
 import chisel3.experimental.BaseModule
 import chisel3.internal.BaseModule.IsClone
+import firrtl.annotations.{IsModule, ModuleTarget}
 
 /** User-facing Definition type.
   * Represents a definition of an object of type [[A]] which are marked as @instantiable 
@@ -64,12 +65,12 @@ object Definition extends SourceInfoDoc {
     /** If this is an instance of a Module, returns the toTarget of this instance
       * @return target of this instance
       */
-    def toTarget = d.proto.toTarget
+    def toTarget: ModuleTarget = d.proto.toTarget
 
     /** If this is an instance of a Module, returns the toAbsoluteTarget of this instance
       * @return absoluteTarget of this instance
       */
-    def toAbsoluteTarget = d.proto.toAbsoluteTarget
+    def toAbsoluteTarget: IsModule = d.proto.toAbsoluteTarget
   }
   /** A construction method to build a Definition of a Module
     *
diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala
index aec42da3..9b17bfce 100644
--- a/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala
+++ b/core/src/main/scala/chisel3/experimental/hierarchy/Instance.scala
@@ -4,11 +4,11 @@ package chisel3.experimental.hierarchy
 
 import scala.collection.mutable.{ArrayBuffer, HashMap}
 import scala.language.experimental.macros
-
 import chisel3._
-import chisel3.internal.BaseModule.{ModuleClone, IsClone, InstantiableClone}
+import chisel3.internal.BaseModule.{InstantiableClone, IsClone, ModuleClone}
 import chisel3.internal.sourceinfo.{InstanceTransform, SourceInfo}
 import chisel3.experimental.BaseModule
+import firrtl.annotations.IsModule
 
 /** User-facing Instance type.
   * Represents a unique instance of type [[A]] which are marked as @instantiable 
@@ -75,15 +75,15 @@ object Instance extends SourceInfoDoc {
     /** If this is an instance of a Module, returns the toTarget of this instance
       * @return target of this instance
       */
-    def toTarget = i.cloned match {
-      case Left(x: BaseModule) => x.toTarget
-      case Right(x: IsClone[_] with BaseModule) => x.toTarget
+    def toTarget: IsModule = i.cloned match {
+      case Left(x: BaseModule) => x.getTarget
+      case Right(x: IsClone[_] with BaseModule) => x.getTarget
     }
 
     /** If this is an instance of a Module, returns the toAbsoluteTarget of this instance
       * @return absoluteTarget of this instance
       */
-    def toAbsoluteTarget = i.cloned match {
+    def toAbsoluteTarget: IsModule = i.cloned match {
       case Left(x) => x.toAbsoluteTarget
       case Right(x: IsClone[_] with BaseModule) => x.toAbsoluteTarget
     }
diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala
index 1d15247d..441abc92 100644
--- a/core/src/main/scala/chisel3/internal/Builder.scala
+++ b/core/src/main/scala/chisel3/internal/Builder.scala
@@ -309,7 +309,7 @@ private[chisel3] trait NamedComponent extends HasId {
     val root = _parent.map {
       case ViewParent => reifyParent
       case other => other
-    }.get.toTarget // All NamedComponents will have a parent, only the top module can have None here
+    }.get.getTarget // All NamedComponents will have a parent, only the top module can have None here
     Target.toTargetTokens(name).toList match {
       case TargetToken.Ref(r) :: components => root.ref(r).copy(component = components)
       case other =>