Merge pull request #2562 from chipsalliance/remove-reflective-naming

Remove reflective naming

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

@@ -1110,13 +1110,6 @@ abstract class Record(private[chisel3] implicit val compileOptions: CompileOptio
     case _ => false
   }
 
-  private[chisel3] override def _onModuleClose: Unit = {
-    // This is usually done during binding, but these must still be set for unbound Records
-    if (this.binding.isEmpty) {
-      setElementRefs()
-    }
-  }
-
   private[chisel3] final def allElements: Seq[Element] = elements.toIndexedSeq.flatMap(_._2.allElements)
 
   override def getElements: Seq[Data] = elements.toIndexedSeq.map(_._2)

core/src/main/scala/chisel3/BlackBox.scala

@@ -69,19 +69,8 @@ package experimental {
       require(!_closed, "Can't generate module more than once")
       _closed = true
 
-      val names = nameIds(classOf[ExtModule])
-
       // Ports are named in the same way as regular Modules
-      namePorts(names)
-
-      // All suggestions are in, force names to every node.
-      // While BlackBoxes are not supposed to have an implementation, we still need to call
-      // _onModuleClose on all nodes (for example, Aggregates use it for recursive naming).
-      for (id <- getIds) {
-        id._onModuleClose
-      }
-
-      closeUnboundIds(names)
+      namePorts()
 
       val firrtlPorts = getModulePorts.map { port => Port(port, port.specifiedDirection) }
       val component = DefBlackBox(this, name, firrtlPorts, SpecifiedDirection.Unspecified, params)
@@ -177,14 +166,6 @@ abstract class BlackBox(
       port.setRef(ModuleIO(this, _namespace.name(name)), force = true)
     }
 
-    // We need to call forceName and onModuleClose on all of the sub-elements
-    // of the io bundle, but NOT on the io bundle itself.
-    // Doing so would cause the wrong names to be assigned, since their parent
-    // is now the module itself instead of the io bundle.
-    for (id <- getIds; if id ne io) {
-      id._onModuleClose
-    }
-
     val firrtlPorts = namedPorts.map { namedPort => Port(namedPort._2, namedPort._2.specifiedDirection) }
     val component = DefBlackBox(this, name, firrtlPorts, io.specifiedDirection, params)
     _component = Some(component)

core/src/main/scala/chisel3/Module.scala

@@ -351,9 +351,9 @@ package experimental {
       */
     private[chisel3] def initializeInParent(parentCompileOptions: CompileOptions): Unit
 
-    private[chisel3] def namePorts(names: HashMap[HasId, String]): Unit = {
+    private[chisel3] def namePorts(): Unit = {
       for (port <- getModulePorts) {
-        port._computeName(None, None).orElse(names.get(port)) match {
+        port._computeName(None, None) match {
           case Some(name) =>
             if (_namespace.contains(name)) {
               Builder.error(
@@ -494,46 +494,6 @@ package experimental {
       }
     }
 
-    /** Called at the Module.apply(...) level after this Module has finished elaborating.
-      * Returns a map of nodes -> names, for named nodes.
-      *
-      * Helper method.
-      */
-    protected def nameIds(rootClass: Class[_]): HashMap[HasId, String] = {
-      val names = new HashMap[HasId, String]()
-
-      def name(node: HasId, name: String) {
-        // First name takes priority, like suggestName
-        // TODO: DRYify with suggestName
-        if (!names.contains(node)) {
-          names.put(node, name)
-        }
-      }
-
-      /** Scala generates names like chisel3$util$Queue$$ram for private vals
-        * This extracts the part after $$ for names like this and leaves names
-        * without $$ unchanged
-        */
-      def cleanName(name: String): String = name.split("""\$\$""").lastOption.getOrElse(name)
-
-      for (m <- getPublicFields(rootClass)) {
-        Builder.nameRecursively(cleanName(m.getName), m.invoke(this), name)
-      }
-
-      names
-    }
-
-    /** Invokes _onModuleClose on HasIds found via reflection but not bound to hardware
-      * (thus not part of _ids)
-      * This maintains old naming behavior for non-hardware Data
-      */
-    private[chisel3] def closeUnboundIds(names: HashMap[HasId, String]): Unit = {
-      val idLookup = _ids.toSet
-      for ((id, _) <- names if !idLookup(id)) {
-        id._onModuleClose
-      }
-    }
-
     /** Compatibility function. Allows Chisel2 code which had ports without the IO wrapper to
       * compile under Bindings checks. Does nothing in non-compatibility mode.
       *

core/src/main/scala/chisel3/RawModule.scala

@@ -43,53 +43,14 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions) extends
 
   val compileOptions = moduleCompileOptions
 
-  // This could be factored into a common utility
-  private def canBeNamed(id: HasId): Boolean = id match {
-    case d: Data =>
-      d.binding match {
-        case Some(_: ConstrainedBinding) => true
-        case _ => false
-      }
-    case b: BaseModule => true
-    case m: MemBase[_] => true
-    // These names don't affect hardware
-    case _: VerificationStatement => false
-    // While the above should be comprehensive, since this is used in warning we want to be careful
-    // to never accidentally have a match error
-    case _ => false
-  }
-
   private[chisel3] override def generateComponent(): Option[Component] = {
     require(!_closed, "Can't generate module more than once")
     _closed = true
 
-    val names = nameIds(classOf[RawModule])
-
     // Ports get first naming priority, since they are part of a Module's IO spec
-    namePorts(names)
-
-    // Then everything else gets named
-    val warnReflectiveNaming = Builder.warnReflectiveNaming
-    for ((node, name) <- names) {
-      node match {
-        case d: HasId if warnReflectiveNaming && canBeNamed(d) =>
-          val result = d._suggestNameCheck(name)
-          result match {
-            case None => // All good, no warning
-            case Some((oldName, oldPrefix)) =>
-              val prevName = buildName(oldName, oldPrefix.reverse)
-              val newName = buildName(name, Nil)
-              val msg = s"[module ${this.name}] '$prevName' is renamed by reflection to '$newName'. " +
-                s"Chisel 3.6 removes reflective naming so the name will remain '$prevName'."
-              Builder.warningNoLoc(msg)
-          }
-        // Note that unnamable things end up here (eg. literals), this is supporting backwards
-        // compatibility
-        case _ => node.suggestName(name)
-      }
-    }
+    namePorts()
 
-    // All suggestions are in, force names to every node.
+    // Ports are named, now name everything else
     for (id <- getIds) {
       id match {
         case id: ModuleClone[_]   => id.setRefAndPortsRef(_namespace) // special handling
@@ -118,11 +79,8 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions) extends
             }
           } // else, don't name unbound types
       }
-      id._onModuleClose
     }
 
-    closeUnboundIds(names)
-
     val firrtlPorts = getModulePorts.map { port: Data =>
       // Special case Vec to make FIRRTL emit the direction of its
       // element.

core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala

@@ -102,7 +102,7 @@ object Definition extends SourceInfoDoc {
   ): Definition[T] = {
     val dynamicContext = {
       val context = Builder.captureContext()
-      new DynamicContext(Nil, context.throwOnFirstError, context.warnReflectiveNaming)
+      new DynamicContext(Nil, context.throwOnFirstError)
     }
     Builder.globalNamespace.copyTo(dynamicContext.globalNamespace)
     dynamicContext.inDefinition = true

core/src/main/scala/chisel3/internal/Builder.scala

@@ -87,7 +87,6 @@ trait InstanceId {
 }
 
 private[chisel3] trait HasId extends InstanceId {
-  private[chisel3] def _onModuleClose: Unit = {}
   private[chisel3] var _parent: Option[BaseModule] = Builder.currentModule
 
   // Set if the returned top-level module of a nested call to the Chisel Builder, see Definition.apply
@@ -235,18 +234,8 @@ private[chisel3] trait HasId extends InstanceId {
 
   private def refName(c: Component): String = _ref match {
     case Some(arg) => arg.fullName(c)
-    case None      =>
-      // This is super hacky but this is just for a short term deprecation
-      // These accesses occur after Chisel elaboration so we cannot use the normal
-      // Builder.deprecated mechanism, we have to create our own one off ErrorLog and print the
-      // warning right away.
-      val errors = new ErrorLog
-      val logger = new _root_.logger.Logger(this.getClass.getName)
-      val msg = "Accessing the .instanceName or .toTarget of non-hardware Data is deprecated. " +
-        "This will become an error in Chisel 3.6."
-      errors.deprecated(msg, None)
-      errors.checkpoint(logger)
-      _computeName(None, None).get
+    case None =>
+      throwException("You cannot access the .instanceName or .toTarget of non-hardware Data")
   }
 
   // Helper for reifying views if they map to a single Target
@@ -296,26 +285,6 @@ private[chisel3] trait HasId extends InstanceId {
     case Some(ViewParent) => reifyParent.circuitName
     case Some(p)          => p.circuitName
   }
-
-  private[chisel3] def getPublicFields(rootClass: Class[_]): Seq[java.lang.reflect.Method] = {
-    // Suggest names to nodes using runtime reflection
-    def getValNames(c: Class[_]): Set[String] = {
-      if (c == rootClass) {
-        Set()
-      } else {
-        getValNames(c.getSuperclass) ++ c.getDeclaredFields.map(_.getName)
-      }
-    }
-    val valNames = getValNames(this.getClass)
-    def isPublicVal(m: java.lang.reflect.Method) = {
-      val noParameters = m.getParameterTypes.isEmpty
-      val aVal = valNames.contains(m.getName)
-      val notAssignable = !m.getDeclaringClass.isAssignableFrom(rootClass)
-      val notWeirdVal = !m.getName.contains('$')
-      noParameters && aVal && notAssignable && notWeirdVal
-    }
-    this.getClass.getMethods.filter(isPublicVal).sortWith(_.getName < _.getName)
-  }
 }
 
 /** Holds the implementation of toNamed for Data and MemBase */
@@ -370,9 +339,8 @@ private[chisel3] class ChiselContext() {
 }
 
 private[chisel3] class DynamicContext(
-  val annotationSeq:        AnnotationSeq,
-  val throwOnFirstError:    Boolean,
-  val warnReflectiveNaming: Boolean) {
+  val annotationSeq:     AnnotationSeq,
+  val throwOnFirstError: Boolean) {
   val importDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a }
 
   // Ensure there are no repeated names for imported Definitions
@@ -647,8 +615,6 @@ private[chisel3] object Builder extends LazyLogging {
     throwException("Error: No implicit reset.")
   )
 
-  def warnReflectiveNaming: Boolean = dynamicContext.warnReflectiveNaming
-
   // TODO(twigg): Ideally, binding checks and new bindings would all occur here
   // However, rest of frontend can't support this yet.
   def pushCommand[T <: Command](c: T): T = {

src/main/scala/chisel3/aop/injecting/InjectingAspect.scala

@@ -59,7 +59,7 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
     RunFirrtlTransformAnnotation(new InjectingTransform) +: modules.map { module =>
       val chiselOptions = view[ChiselOptions](annotationsInAspect)
       val dynamicContext =
-        new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError, chiselOptions.warnReflectiveNaming)
+        new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError)
       // 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 =>

src/main/scala/chisel3/stage/ChiselAnnotations.scala

@@ -80,17 +80,24 @@ case object ThrowOnFirstErrorAnnotation
 }
 
 /** Warn when reflective naming changes names of signals */
+@deprecated("Support for reflective naming has been removed, this object no longer does anything", "Chisel 3.6")
 case object WarnReflectiveNamingAnnotation
     extends NoTargetAnnotation
     with ChiselOption
     with HasShellOptions
     with Unserializable {
 
+  private val longOption = "warn:reflective-naming"
+
   val options = Seq(
     new ShellOption[Unit](
-      longOption = "warn:reflective-naming",
-      toAnnotationSeq = _ => Seq(this),
-      helpText = "Warn when reflective naming changes the name of signals (3.6 migration)"
+      longOption = longOption,
+      toAnnotationSeq = _ => {
+        val msg = s"'$longOption' no longer does anything and will be removed in Chisel 3.7"
+        firrtl.options.StageUtils.dramaticWarning(msg)
+        Seq(this)
+      },
+      helpText = "(deprecated, this option does nothing)"
     )
   )
 }

src/main/scala/chisel3/stage/ChiselCli.scala

@@ -3,7 +3,9 @@
 package chisel3.stage
 
 import firrtl.options.Shell
+import scala.annotation.nowarn
 
+@nowarn("cat=deprecation&msg=WarnReflectiveNamingAnnotation")
 trait ChiselCli { this: Shell =>
   parser.note("Chisel Front End Options")
   Seq(

src/main/scala/chisel3/stage/ChiselOptions.scala

@@ -5,27 +5,24 @@ package chisel3.stage
 import chisel3.internal.firrtl.Circuit
 
 class ChiselOptions private[stage] (
-  val runFirrtlCompiler:    Boolean = true,
-  val printFullStackTrace:  Boolean = false,
-  val throwOnFirstError:    Boolean = false,
-  val warnReflectiveNaming: Boolean = false,
-  val outputFile:           Option[String] = None,
-  val chiselCircuit:        Option[Circuit] = None) {
+  val runFirrtlCompiler:   Boolean = true,
+  val printFullStackTrace: Boolean = false,
+  val throwOnFirstError:   Boolean = false,
+  val outputFile:          Option[String] = None,
+  val chiselCircuit:       Option[Circuit] = None) {
 
   private[stage] def copy(
-    runFirrtlCompiler:    Boolean = runFirrtlCompiler,
-    printFullStackTrace:  Boolean = printFullStackTrace,
-    throwOnFirstError:    Boolean = throwOnFirstError,
-    warnReflectiveNaming: Boolean = warnReflectiveNaming,
-    outputFile:           Option[String] = outputFile,
-    chiselCircuit:        Option[Circuit] = chiselCircuit
+    runFirrtlCompiler:   Boolean = runFirrtlCompiler,
+    printFullStackTrace: Boolean = printFullStackTrace,
+    throwOnFirstError:   Boolean = throwOnFirstError,
+    outputFile:          Option[String] = outputFile,
+    chiselCircuit:       Option[Circuit] = chiselCircuit
   ): ChiselOptions = {
 
     new ChiselOptions(
       runFirrtlCompiler = runFirrtlCompiler,
       printFullStackTrace = printFullStackTrace,
       throwOnFirstError = throwOnFirstError,
-      warnReflectiveNaming = warnReflectiveNaming,
       outputFile = outputFile,
       chiselCircuit = chiselCircuit
     )

src/main/scala/chisel3/stage/package.scala

@@ -8,8 +8,11 @@ import firrtl.options.OptionsView
 import chisel3.internal.firrtl.{Circuit => ChiselCircuit}
 import chisel3.stage.CircuitSerializationAnnotation.FirrtlFileFormat
 
+import scala.annotation.nowarn
+
 package object stage {
 
+  @nowarn("cat=deprecation&msg=WarnReflectiveNamingAnnotation")
   implicit object ChiselOptionsView extends OptionsView[ChiselOptions] {
 
     def view(options: AnnotationSeq): ChiselOptions = options.collect { case a: ChiselOption => a }
@@ -18,7 +21,7 @@ package object stage {
           case NoRunFirrtlCompilerAnnotation  => c.copy(runFirrtlCompiler = false)
           case PrintFullStackTraceAnnotation  => c.copy(printFullStackTrace = true)
           case ThrowOnFirstErrorAnnotation    => c.copy(throwOnFirstError = true)
-          case WarnReflectiveNamingAnnotation => c.copy(warnReflectiveNaming = true)
+          case WarnReflectiveNamingAnnotation => c // Do nothing, ignored
           case ChiselOutputFileAnnotation(f)  => c.copy(outputFile = Some(f))
           case ChiselCircuitAnnotation(a)     => c.copy(chiselCircuit = Some(a))
         }

src/main/scala/chisel3/stage/phases/Elaborate.scala

@@ -30,7 +30,7 @@ class Elaborate extends Phase {
       val chiselOptions = view[ChiselOptions](annotations)
       try {
         val context =
-          new DynamicContext(annotations, chiselOptions.throwOnFirstError, chiselOptions.warnReflectiveNaming)
+          new DynamicContext(annotations, chiselOptions.throwOnFirstError)
         val (circuit, dut) =
           Builder.build(Module(gen()), context)
         Seq(ChiselCircuitAnnotation(circuit), DesignAnnotation(dut))

src/test/scala/chiselTests/BetterNamingTests.scala

@@ -48,10 +48,14 @@ class IterableNaming extends NamedModuleTester {
     }
     s
   }
-  def streamFrom(x: Int): Stream[Module] =
-    expectName(Module(new Other(x)), s"list_$x") #:: streamFrom(x + 1)
-  val stream = streamFrom(0) // Check that we don't get into infinite loop
-  val list = stream.take(8).toList
+  // Check that we don't get into infinite loop
+  // When we still had reflective naming, we could have the list take from the Stream and have
+  // everything named list_<n>. Without reflective naming, the first element in the Stream gets a
+  // default name because it is built eagerly but the compiler plugin doesn't know how to handle
+  // infinite-size structures. Scala 2.13 LazyList would give the same old naming behavior but does
+  // not exist in Scala 2.12 so this test has been simplified a bit.
+  val stream = Stream.continually(Module(new Other(8)))
+  val list = List.tabulate(4)(i => expectName(Module(new Other(i)), s"list_$i"))
 }
 
 class DigitFieldNamesInRecord extends NamedModuleTester {