From 97871178cb511063965f971b768f91c289c4776f Mon Sep 17 00:00:00 2001 From: Richard Lin Date: Wed, 28 Feb 2018 17:31:43 -0800 Subject: [PATCH] Auto Clone Bundles in Companion Objects (#788) --- .../main/scala/chisel3/core/Aggregate.scala | 49 +++++++++++-------- .../scala/chiselTests/AutoClonetypeSpec.scala | 23 +++++++++ 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala index 1f592f38f69..f0c23e52f68 100644 --- a/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala +++ b/chiselFrontend/src/main/scala/chisel3/core/Aggregate.scala @@ -572,12 +572,25 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { val clazz = this.getClass - def reflectError(desc: String): Nothing = { + def autoClonetypeError(desc: String): Nothing = { throw new AutoClonetypeException(s"Unable to automatically infer cloneType on $clazz: $desc") } + val mirror = runtimeMirror(clazz.getClassLoader) + val classSymbolOption = try { + Some(mirror.reflect(this).symbol) + } catch { + case e: scala.reflect.internal.Symbols#CyclicReference => None // Workaround for a scala bug + } + + val enclosingClassOption = (clazz.getEnclosingClass, classSymbolOption) match { + case (null, _) => None + case (_, Some(classSymbol)) if classSymbol.isStatic => None // allows support for members of companion objects + case (outerClass, _) => Some(outerClass) + } + // Check if this is an inner class, and if so, try to get the outer instance - val outerClassInstance = Option(clazz.getEnclosingClass).map { outerClass => + val outerClassInstance = enclosingClassOption.map { outerClass => def canAssignOuterClass(x: Object) = outerClass.isAssignableFrom(x.getClass) val outerInstance = _outerInst match { @@ -599,9 +612,9 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { 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," + + case Nil => autoClonetypeError(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," + + case candidates => autoClonetypeError(s"Unable to determine instance of outer class $outerClass," + s" multiple possible candidates $candidates assignable to outer class type") } } @@ -628,12 +641,13 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { None } case _ => None + } clone match { case Some(clone) => clone._outerInst = this._outerInst if (!clone.typeEquivalent(this)) { - reflectError(s"automatically cloned $clone not type-equivalent to base." + + autoClonetypeError(s"automatically cloned $clone not type-equivalent to base." + " Constructor argument values were not inferred, ensure constructor is deterministic.") } return clone.asInstanceOf[this.type] @@ -642,19 +656,14 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { } // Get constructor parameters and accessible fields - val mirror = runtimeMirror(clazz.getClassLoader) - val classSymbol = try { - mirror.reflect(this).symbol - } catch { - case e: scala.reflect.internal.Symbols#CyclicReference => reflectError(s"got exception $e attempting Scala reflection." + - " This is known to occur with inner classes on anonymous outer classes." + - " In those cases, autoclonetype only works with no-argument constructors, or you can define a custom cloneType.") - } + val classSymbol = classSymbolOption.getOrElse(autoClonetypeError(s"scala reflection failed." + + " This is known to occur with inner classes on anonymous outer classes." + + " In those cases, autoclonetype only works with no-argument constructors, or you can define a custom cloneType.")) val decls = classSymbol.typeSignature.decls val ctors = decls.collect { case meth: MethodSymbol if meth.isConstructor => meth } if (ctors.size != 1) { - reflectError(s"found multiple constructors ($ctors)." + + autoClonetypeError(s"found multiple constructors ($ctors)." + " Either remove all but the default constructor, or define a custom cloneType method.") } val ctor = ctors.head @@ -663,7 +672,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { case Nil => List() case ctorParams :: Nil => ctorParams case ctorParams :: ctorImplicits :: Nil => ctorParams ++ ctorImplicits - case _ => reflectError(s"internal error, unexpected ctorParamss = $ctorParamss") + case _ => autoClonetypeError(s"internal error, unexpected ctorParamss = $ctorParamss") } val ctorParamsNames = ctorParams.map(_.name.toString) @@ -680,7 +689,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { return clone } catch { case e @ (_: java.lang.reflect.InvocationTargetException | _: IllegalArgumentException) => - reflectError(s"unexpected failure at constructor invocation, got $e.") + autoClonetypeError(s"unexpected failure at constructor invocation, got $e.") } } @@ -699,7 +708,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { val accessorsName = accessors.filter(_.isStable).map(_.name.toString) val paramsDiff = ctorParamsNames.toSet -- accessorsName.toSet if (!paramsDiff.isEmpty) { - reflectError(s"constructor has parameters (${paramsDiff.toList.sorted.mkString(", ")}) that are not both immutable and accessible." + + autoClonetypeError(s"constructor has parameters (${paramsDiff.toList.sorted.mkString(", ")}) that are not both immutable and accessible." + " Either make all parameters immutable and accessible (vals) so cloneType can be inferred, or define a custom cloneType method.") } @@ -717,7 +726,7 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { case (paramName, paramVal: Data) if paramVal.hasBinding => paramName } if (boundDataParamNames.nonEmpty) { - reflectError(s"constructor parameters (${boundDataParamNames.sorted.mkString(", ")}) have values that are hardware types, which is likely to cause subtle errors." + + autoClonetypeError(s"constructor parameters (${boundDataParamNames.sorted.mkString(", ")}) have values that are hardware types, which is likely to cause subtle errors." + " Use chisel types instead: use the value before it is turned to a hardware type (with Wire(...), Reg(...), etc) or use chiselTypeOf(...) to extract the chisel type.") } @@ -730,13 +739,13 @@ abstract class Bundle(implicit compileOptions: CompileOptions) extends Record { // Invoke ctor val classMirror = outerClassInstance match { case Some((_, outerInstance)) => mirror.reflect(outerInstance).reflectClass(classSymbol) - case None => mirror.reflectClass(classSymbol) + case _ => mirror.reflectClass(classSymbol) } val clone = classMirror.reflectConstructor(ctor).apply(ctorParamsVals:_*).asInstanceOf[this.type] clone._outerInst = this._outerInst if (!clone.typeEquivalent(this)) { - reflectError(s"Automatically cloned $clone not type-equivalent to base $this." + + autoClonetypeError(s"Automatically cloned $clone not type-equivalent to base $this." + " Constructor argument values were inferred: ensure that variable names are consistent and have the same value throughout the constructor chain," + " and that the constructor is deterministic.") } diff --git a/src/test/scala/chiselTests/AutoClonetypeSpec.scala b/src/test/scala/chiselTests/AutoClonetypeSpec.scala index 0b02c3ab237..59ce98b7fb0 100644 --- a/src/test/scala/chiselTests/AutoClonetypeSpec.scala +++ b/src/test/scala/chiselTests/AutoClonetypeSpec.scala @@ -47,6 +47,15 @@ class ModuleWithInner extends Module { require(myWire.i == 14) } +object CompanionObjectWithBundle { + class ParameterizedInner(val i: Int) extends Bundle { + val data = UInt(i.W) + } + class Inner extends Bundle { + val data = UInt(8.W) + } +} + // A Bundle with an argument that is also a field. // Not necessarily good style (and not necessarily recommended), but allowed to preserve compatibility. class BundleWithArgumentField(val x: Data, val y: Data) extends Bundle @@ -123,4 +132,18 @@ class AutoClonetypeSpec extends ChiselFlatSpec { io.y := 1.U } } } + + "Bundles inside companion objects" should "not need clonetype" in { + elaborate { new Module { + val io = IO(Output(new CompanionObjectWithBundle.Inner)) + io.data := 1.U + } } + } + + "Parameterized bundles inside companion objects" should "not need clonetype" in { + elaborate { new Module { + val io = IO(Output(new CompanionObjectWithBundle.ParameterizedInner(8))) + io.data := 1.U + } } + } }