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

* Factor buildName into reusable function

The new function is chisel3.internal.buildName.

(cherry picked from commit 370ca8ac68f6d888dd99e1b9e63f0371add398cf)

* 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 97afd9b9a1155fa7cd5cedf19f9e0c15fbe899ec)

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

4 files changed, 76 insertions, 21 deletions

diff --git a/core/src/main/scala/chisel3/RawModule.scala b/core/src/main/scala/chisel3/RawModule.scala
index c257f0c6..164d3880 100644
--- a/core/src/main/scala/chisel3/RawModule.scala
+++ b/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"
diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
index a8cfac2f..c79c26dc 100644
--- a/core/src/main/scala/chisel3/experimental/hierarchy/Definition.scala
+++ b/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)
diff --git a/core/src/main/scala/chisel3/internal/Builder.scala b/core/src/main/scala/chisel3/internal/Builder.scala
index 97c3bc49..6f02b57c 100644
--- a/core/src/main/scala/chisel3/internal/Builder.scala
+++ b/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.
diff --git a/core/src/main/scala/chisel3/internal/Error.scala b/core/src/main/scala/chisel3/internal/Error.scala
index 62086870..3b0846eb 100644
--- a/core/src/main/scala/chisel3/internal/Error.scala
+++ b/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 =