Add --warn:reflective-naming (backport #2561) (#2565)

* Factor buildName into reusable function

The new function is chisel3.internal.buildName.

(cherry picked from commit 370ca8a)

* Add --warn:reflective-naming

This new argument (and associated annotation) will turn on a warning
whenever reflective naming changes the name of a signal. This is
provided to help migrate from Chisel 3.5 to 3.6 since reflective naming
is removed in Chisel 3.6.

(cherry picked from commit 97afd9b)

Co-authored-by: Jack Koenig <koenig@sifive.com>

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

@@ -43,6 +43,22 @@ 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
@@ -53,8 +69,24 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions) extends
     namePorts(names)
 
     // Then everything else gets named
+    val warnReflectiveNaming = Builder.warnReflectiveNaming
     for ((node, name) <- names) {
-      node.suggestName(name)
+      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)
+      }
     }
 
     // All suggestions are in, force names to every node.
@@ -154,6 +186,20 @@ package object internal {
   /** Marker trait for modules that are not true modules */
   private[chisel3] trait PseudoModule extends BaseModule
 
+  /** Creates a name String from a prefix and a seed
+    * @param prefix The prefix associated with the seed (must be in correct order, *not* reversed)
+    * @param seed The seed for computing the name (if available)
+    */
+  def buildName(seed: String, prefix: Prefix): String = {
+    val builder = new StringBuilder()
+    prefix.foreach { p =>
+      builder ++= p
+      builder += '_'
+    }
+    builder ++= seed
+    builder.toString
+  }
+
   // Private reflective version of "val io" to maintain Chisel.Module semantics without having
   // io as a virtual method. See https://github.com/freechipsproject/chisel3/pull/1550 for more
   // information about the removal of "val io"

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

@@ -101,7 +101,10 @@ object Definition extends SourceInfoDoc {
     implicit sourceInfo: SourceInfo,
     compileOptions:      CompileOptions
   ): Definition[T] = {
-    val dynamicContext = new DynamicContext(Nil, Builder.captureContext().throwOnFirstError)
+    val dynamicContext = {
+      val context = Builder.captureContext()
+      new DynamicContext(Nil, context.throwOnFirstError, context.warnReflectiveNaming)
+    }
     Builder.globalNamespace.copyTo(dynamicContext.globalNamespace)
     dynamicContext.inDefinition = true
     val (ir, module) = Builder.build(Module(proto), dynamicContext, false)

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

@@ -137,6 +137,17 @@ private[chisel3] trait HasId extends InstanceId {
     this
   }
 
+  // Private internal version of suggestName that tells you if the name changed
+  // Returns Some(old name, old prefix) if name changed, None otherwise
+  private[chisel3] def _suggestNameCheck(seed: => String): Option[(String, Prefix)] = {
+    val oldSeed = this.seedOpt
+    val oldPrefix = this.naming_prefix
+    suggestName(seed)
+    if (oldSeed.nonEmpty && (oldSeed != this.seedOpt || oldPrefix != this.naming_prefix)) {
+      Some(oldSeed.get -> oldPrefix)
+    } else None
+  }
+
   /** Takes the first seed suggested. Multiple calls to this function will be ignored.
     * If the final computed name conflicts with another name, it may get uniquified by appending
     * a digit at the end.
@@ -171,22 +182,6 @@ private[chisel3] trait HasId extends InstanceId {
     * @return the name, if it can be computed
     */
   private[chisel3] def _computeName(defaultPrefix: Option[String], defaultSeed: Option[String]): Option[String] = {
-
-    /** Computes a name of this signal, given the seed and prefix
-      * @param seed
-      * @param prefix
-      * @return
-      */
-    def buildName(seed: String, prefix: Prefix): String = {
-      val builder = new StringBuilder()
-      prefix.foreach { p =>
-        builder ++= p
-        builder += '_'
-      }
-      builder ++= seed
-      builder.toString
-    }
-
     if (hasSeed) {
       Some(buildName(seedOpt.get, naming_prefix.reverse))
     } else {
@@ -374,7 +369,10 @@ private[chisel3] class ChiselContext() {
   val viewNamespace = Namespace.empty
 }
 
-private[chisel3] class DynamicContext(val annotationSeq: AnnotationSeq, val throwOnFirstError: Boolean) {
+private[chisel3] class DynamicContext(
+  val annotationSeq:        AnnotationSeq,
+  val throwOnFirstError:    Boolean,
+  val warnReflectiveNaming: Boolean) {
   val importDefinitionAnnos = annotationSeq.collect { case a: ImportDefinitionAnnotation[_] => a }
 
   // Ensure there are no repeated names for imported Definitions
@@ -649,6 +647,8 @@ 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 = {
@@ -690,8 +690,9 @@ private[chisel3] object Builder extends LazyLogging {
       throwException(m)
     }
   }
-  def warning(m:    => String): Unit = if (dynamicContextVar.value.isDefined) errors.warning(m)
-  def deprecated(m: => String, location: Option[String] = None): Unit =
+  def warning(m:      => String): Unit = if (dynamicContextVar.value.isDefined) errors.warning(m)
+  def warningNoLoc(m: => String): Unit = if (dynamicContextVar.value.isDefined) errors.warningNoLoc(m)
+  def deprecated(m:   => String, location: Option[String] = None): Unit =
     if (dynamicContextVar.value.isDefined) errors.deprecated(m, location)
 
   /** Record an exception as an error, and throw it.

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

@@ -186,6 +186,11 @@ private[chisel3] class ErrorLog {
   def warning(m: => String): Unit =
     errors += new Warning(m, getUserLineNumber)
 
+  /** Log a warning message without a source locator */
+  def warningNoLoc(m: => String): Unit = {
+    errors += new Warning(m, None)
+  }
+
   /** Emit an informational message */
   @deprecated("This method will be removed in 3.5", "3.4")
   def info(m: String): Unit =

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

@@ -58,7 +58,8 @@ abstract class InjectorAspect[T <: RawModule, M <: RawModule](
   final def toAnnotation(modules: Iterable[M], circuit: String, moduleNames: Seq[String]): AnnotationSeq = {
     RunFirrtlTransformAnnotation(new InjectingTransform) +: modules.map { module =>
       val chiselOptions = view[ChiselOptions](annotationsInAspect)
-      val dynamicContext = new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError)
+      val dynamicContext =
+        new DynamicContext(annotationsInAspect, chiselOptions.throwOnFirstError, chiselOptions.warnReflectiveNaming)
       // 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

@@ -79,6 +79,22 @@ case object ThrowOnFirstErrorAnnotation
 
 }
 
+/** Warn when reflective naming changes names of signals */
+case object WarnReflectiveNamingAnnotation
+    extends NoTargetAnnotation
+    with ChiselOption
+    with HasShellOptions
+    with Unserializable {
+
+  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)"
+    )
+  )
+}
+
 /** An [[firrtl.annotations.Annotation]] storing a function that returns a Chisel module
   * @param gen a generator function
   */

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

@@ -10,6 +10,7 @@ trait ChiselCli { this: Shell =>
     NoRunFirrtlCompilerAnnotation,
     PrintFullStackTraceAnnotation,
     ThrowOnFirstErrorAnnotation,
+    WarnReflectiveNamingAnnotation,
     ChiselOutputFileAnnotation,
     ChiselGeneratorAnnotation
   )

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

@@ -5,24 +5,27 @@ 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 outputFile:          Option[String] = None,
-  val chiselCircuit:       Option[Circuit] = None) {
+  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) {
 
   private[stage] def copy(
-    runFirrtlCompiler:   Boolean = runFirrtlCompiler,
-    printFullStackTrace: Boolean = printFullStackTrace,
-    throwOnFirstError:   Boolean = throwOnFirstError,
-    outputFile:          Option[String] = outputFile,
-    chiselCircuit:       Option[Circuit] = chiselCircuit
+    runFirrtlCompiler:    Boolean = runFirrtlCompiler,
+    printFullStackTrace:  Boolean = printFullStackTrace,
+    throwOnFirstError:    Boolean = throwOnFirstError,
+    warnReflectiveNaming: Boolean = warnReflectiveNaming,
+    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

@@ -15,11 +15,12 @@ package object stage {
     def view(options: AnnotationSeq): ChiselOptions = options.collect { case a: ChiselOption => a }
       .foldLeft(new ChiselOptions()) { (c, x) =>
         x match {
-          case NoRunFirrtlCompilerAnnotation => c.copy(runFirrtlCompiler = false)
-          case PrintFullStackTraceAnnotation => c.copy(printFullStackTrace = true)
-          case ThrowOnFirstErrorAnnotation   => c.copy(throwOnFirstError = true)
-          case ChiselOutputFileAnnotation(f) => c.copy(outputFile = Some(f))
-          case ChiselCircuitAnnotation(a)    => c.copy(chiselCircuit = Some(a))
+          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 ChiselOutputFileAnnotation(f)  => c.copy(outputFile = Some(f))
+          case ChiselCircuitAnnotation(a)     => c.copy(chiselCircuit = Some(a))
         }
       }
 

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

@@ -29,8 +29,10 @@ class Elaborate extends Phase {
     case ChiselGeneratorAnnotation(gen) =>
       val chiselOptions = view[ChiselOptions](annotations)
       try {
+        val context =
+          new DynamicContext(annotations, chiselOptions.throwOnFirstError, chiselOptions.warnReflectiveNaming)
         val (circuit, dut) =
-          Builder.build(Module(gen()), new DynamicContext(annotations, chiselOptions.throwOnFirstError))
+          Builder.build(Module(gen()), context)
         Seq(ChiselCircuitAnnotation(circuit), DesignAnnotation(dut))
       } catch {
         /* if any throwable comes back and we're in "stack trace trimming" mode, then print an error and trim the stack trace