Better support for autoclonetype of nested Bundles (#769)

* Better support for autoclonetype of nested Bundles

* Move bundleStack to dynamicContext

* prefer $outer if available, make guesses distinct

* Catch IllegalAccessException in autoclonetype
In strange circumstances this type of exception can occur when accessing $outer

2 files changed, 67 insertions, 18 deletions

diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
index 6a2e68e0..fa69bef0 100644
--- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
+++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala
@@ -552,10 +552,15 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
     case _ => None
   }
 
-  // If this Data is an instance of an inner class, record a *guess* at the outer object.
-  // This is only used for cloneType, and is mutable to allow autoCloneType to try guesses other
-  // than the module that was current when the Bundle was constructed.
-  private var _outerInst: Option[Object] = Builder.currentModule
+  // Memoize the outer instance for autoclonetype, especially where this is context-dependent
+  // (like the outer module or enclosing Bundles).
+  private var _outerInst: Option[Object] = None
+
+  // For autoclonetype, record possible candidates for outer instance.
+  // _outerInst should always take precedence, since it should be propagated from the original
+  // object which has the most accurate context.
+  private val _containingModule: Option[BaseModule] = Builder.currentModule
+  private val _containingBundles: Seq[Bundle] = Builder.updateBundleStack(this)
 
   override def cloneType : this.type = {
     // This attempts to infer constructor and arguments to clone this Bundle subtype without
@@ -570,23 +575,36 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
 
     // Check if this is an inner class, and if so, try to get the outer instance
     val clazz = this.getClass
+
     val outerClassInstance = Option(clazz.getEnclosingClass).map { outerClass =>
       def canAssignOuterClass(x: Object) = outerClass.isAssignableFrom(x.getClass)
-      val outerInstance = try {
-        clazz.getDeclaredField("$outer").get(this)  // doesn't work in all cases, namely anonymous inner Bundles
-      } catch {
-        case _: NoSuchFieldException =>
-          // First check if this Bundle is bound and an anonymous inner Bundle of another Bundle
-          this.bindingOpt.collect { case ChildBinding(p) if canAssignOuterClass(p) => p }
-              .orElse(this._outerInst)
-              .getOrElse(reflectError(s"Unable to determine instance of outer class $outerClass"))
-      }
-      if (!canAssignOuterClass(outerInstance)) {
-        reflectError(s"Unable to determine instance of outer class $outerClass," +
-            s" guessed $outerInstance, but is not assignable to outer class $outerClass")
+
+      val outerInstance = _outerInst match {
+        case Some(outerInstance) => outerInstance  // use _outerInst if defined
+        case None =>  // determine outer instance if not already recorded
+          try {
+            // Prefer this if it works, but doesn't work in all cases, namely anonymous inner Bundles
+            val outer = clazz.getDeclaredField("$outer").get(this)
+            _outerInst = Some(outer)
+            outer
+          } catch {
+            case (_: NoSuchFieldException | _: IllegalAccessException) =>
+              // Fallback using guesses based on common patterns
+              val allOuterCandidates = Seq(
+                  _containingModule.toSeq,
+                  _containingBundles
+                ).flatten.distinct
+              allOuterCandidates.filter(canAssignOuterClass(_)) match {
+                case outer :: Nil =>
+                  _outerInst = Some(outer)  // record the guess for future use
+                  outer
+                case Nil => reflectError(s"Unable to determine instance of outer class $outerClass," +
+                    s" no candidates assignable to outer class types; examined $allOuterCandidates")
+                case candidates => reflectError(s"Unable to determine instance of outer class $outerClass," +
+                    s" multiple possible candidates $candidates assignable to outer class type")
+              }
+          }
       }
-      // Record the outer instance for future cloning
-      this._outerInst = Some(outerInstance)
       (outerClass, outerInstance)
     }
 
diff --git a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
index 0b279347..c9b37fc5 100644
--- a/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
+++ b/chiselFrontend/src/main/scala/chisel3/internal/Builder.scala
@@ -156,6 +156,8 @@ private[chisel3] class DynamicContext() {
   var currentClockAndReset: Option[ClockAndReset] = None
   val errors = new ErrorLog
   val namingStack = new internal.naming.NamingStack
+  // Record the Bundle instance, class name, and reverse stack trace position of open Bundles
+  val bundleStack: ArrayBuffer[(Bundle, String, Int)] = ArrayBuffer()
 }
 
 private[chisel3] object Builder {
@@ -223,6 +225,35 @@ private[chisel3] object Builder {
     pushCommand(cmd).id
   }
 
+  // Called when Bundle construction begins, used to record a stack of open Bundle constructors to
+  // record candidates for Bundle autoclonetype. This is a best-effort guess.
+  // Returns the current stack of open Bundles
+  // Note: elt will NOT have finished construction, its elements cannot be accessed
+  def updateBundleStack(elt: Bundle): Seq[Bundle] = {
+    val stackClasses = Thread.currentThread().getStackTrace()
+        .map(_.getClassName)
+        .reverse  // so stack frame numbers are deterministic across calls
+
+    // Prune the existing Bundle stack of closed Bundles
+    val pruneLength = dynamicContext.bundleStack.reverse.prefixLength { case (_, cname, pos) =>
+      pos >= stackClasses.size || stackClasses(pos) != cname
+    }
+    dynamicContext.bundleStack.trimEnd(pruneLength)
+
+    // Return the stack state before adding the most recent bundle
+    val lastStack = dynamicContext.bundleStack.map(_._1).toSeq
+
+    // Append the current Bundle to the stack, if it's on the stack trace
+    val eltClassName = elt.getClass.getName
+    val eltStackPos = stackClasses.lastIndexOf(eltClassName)
+    if (eltStackPos >= 0) {
+      dynamicContext.bundleStack.append((elt, eltClassName, eltStackPos))
+    }
+    // Otherwise discard the stack frame, this shouldn't fail noisily
+
+    lastStack
+  }
+
   def errors: ErrorLog = dynamicContext.errors
   def error(m: => String): Unit = errors.error(m)
   def warning(m: => String): Unit = errors.warning(m)