Improve Builder.forceName collision errors (#3012)

Author: jared-barocsi

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

@@ -381,14 +381,14 @@ package experimental {
             if (_namespace.contains(name)) {
               Builder.error(
                 s"""Unable to name port $port to "$name" in $this,""" +
-                  s" name is already taken by another port! ${source}"
+                  s" name is already taken by another port! ${source.makeMessage(x => x)}"
               )(UnlocatableSourceInfo)
             }
             port.setRef(ModuleIO(this, _namespace.name(name)))
           case None =>
             Builder.error(
               s"Unable to name port $port in $this, " +
-                s"try making it a public field of the Module $source"
+                s"try making it a public field of the Module ${source.makeMessage(x => x)}"
             )(UnlocatableSourceInfo)
             port.setRef(ModuleIO(this, "<UNNAMED>"))
         }

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

@@ -48,7 +48,7 @@ abstract class RawModule(implicit moduleCompileOptions: CompileOptions) extends
       if (port._computeName(None).isEmpty) {
         Builder.error(
           s"Unable to name port $port in $this, " +
-            s"try making it a public field of the Module $source"
+            s"try making it a public field of the Module ${source.makeMessage(x => x)}"
         )(UnlocatableSourceInfo)
       }
     }

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

@@ -44,15 +44,6 @@ private[chisel3] class Namespace(keywords: Set[String]) {
     }
   }
 
-  private def sanitize(s: String, leadingDigitOk: Boolean = false): String = {
-    // TODO what character set does FIRRTL truly support? using ANSI C for now
-    def legalStart(c: Char) = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_'
-    def legal(c:      Char) = legalStart(c) || (c >= '0' && c <= '9')
-    val res = if (s.forall(legal)) s else s.filter(legal)
-    val headOk = (!res.isEmpty) && (leadingDigitOk || legalStart(res.head))
-    if (headOk) res else s"_$res"
-  }
-
   /** Checks if `n` ends in `_\d+` and returns the substring before `_` if so, null otherwise */
   // TODO can and should this be folded in to sanitize? Same iteration as the forall?
   private def prefix(n: String): Int = {
@@ -241,11 +232,31 @@ private[chisel3] trait HasId extends chisel3.InstanceId {
     refBuilder: String => Arg = Ref(_)
   ): Unit =
     if (_ref.isEmpty) {
-      val candidate_name = _computeName(Some(default).filterNot(_ => errorIfDup)).get
+      val candidate_name = _computeName(Some(default).filterNot(_ => errorIfDup)).getOrElse {
+        throwException(
+          s"Attempted to name a nameless IO port ($this): this is usually caused by instantiating an IO but not assigning it to a val.\n" +
+            s"Assign $this to a val, or explicitly call suggestName to seed a unique name"
+        )
+      }
+
+      val sanitized = sanitize(candidate_name)
       val available_name = namespace.name(candidate_name)
-      if (errorIfDup && (available_name != candidate_name)) {
-        Builder.error(s"Cannot have duplicate names $available_name and $candidate_name")(UnlocatableSourceInfo)
+
+      // Check for both cases of name duplication
+      if (errorIfDup && (available_name != sanitized)) {
+        // If sanitization occurred, then the sanitized name duplicate an existing name
+        if ((candidate_name != sanitized)) {
+          Builder.error(
+            s"Attempted to name $this with an unsanitary name '$candidate_name': sanitization results in a duplicated name '$sanitized'. Please seed a more unique name"
+          )(UnlocatableSourceInfo)
+        } else {
+          // Otherwise the candidate name duplicates an existing name
+          Builder.error(
+            s"Attempted to name $this with a duplicated name '$candidate_name'. Use suggestName to seed a unique name"
+          )(UnlocatableSourceInfo)
+        }
       }
+
       setRef(refBuilder(available_name))
       // Clear naming prefix to free memory
       naming_prefix = Nil

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

@@ -64,6 +64,16 @@ package object internal {
     }
   }
 
+  // Sanitizes a name, e.g. from a `HasId`, by stripping all non ANSI-C characters
+  private[chisel3] def sanitize(s: String, leadingDigitOk: Boolean = false): String = {
+    // TODO what character set does FIRRTL truly support? using ANSI C for now
+    def legalStart(c: Char) = (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '_'
+    def legal(c:      Char) = legalStart(c) || (c >= '0' && c <= '9')
+    val res = if (s.forall(legal)) s else s.filter(legal)
+    val headOk = (!res.isEmpty) && (leadingDigitOk || legalStart(res.head))
+    if (headOk) res else s"_$res"
+  }
+
   // 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"

src/test/scala/chisel3/internal/NameCollisionSpec.scala

@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package chiselTests
+
+import chisel3._
+import circt.stage.ChiselStage
+
+class NameCollisionSpec extends ChiselFlatSpec with Utils {
+  behavior.of("Builder")
+
+  it should "error on duplicated names with a correct message" in {
+    (the[ChiselException] thrownBy extractCause[ChiselException] {
+      ChiselStage.elaborate(
+        new Module {
+          val foo, bar = IO(Input(UInt(8.W)))
+          val out = IO(Output(UInt(8.W)))
+
+          // Rename both inputs to the same name
+          foo.suggestName("same")
+          bar.suggestName("same")
+
+          out := foo + bar
+        },
+        Array("--throw-on-first-error")
+      )
+    }).getMessage should include(
+      "Attempted to name NameCollisionSpec_Anon.same: IO[UInt<8>] with a duplicated name 'same'."
+    )
+  }
+
+  it should "error on sanitization resulting in duplicated names with a helpful message" in {
+    // Case one: An unsanitary name that results in a collision with an existing name once sanitized
+    (the[ChiselException] thrownBy extractCause[ChiselException] {
+      ChiselStage.elaborate(
+        new Module {
+          val foo, bar = IO(Input(UInt(8.W)))
+          val out = IO(Output(UInt(8.W)))
+
+          // Seed an initial name with no necessary sanitization
+          foo.suggestName("unsanitary")
+          // Seed an additional name which, when sanitized, collides with the previous name
+          bar.suggestName("unsanitary-")
+
+          out := foo + bar
+        },
+        Array("--throw-on-first-error")
+      )
+    }).getMessage should include(
+      "Attempted to name NameCollisionSpec_Anon.unsanitary-: IO[UInt<8>] with an unsanitary name 'unsanitary-'"
+    )
+
+    // Case two: An unsanitary name which does not collide with any names once sanitized. No error is raised
+    ChiselStage.elaborate(
+      new Module {
+        val foo, bar = IO(Input(UInt(8.W)))
+        val out = IO(Output(UInt(8.W)))
+
+        out.suggestName("unsanitary-")
+
+        out := foo + bar
+      },
+      Array("--throw-on-first-error")
+    )
+  }
+
+  it should "error on nameless ports being assigned default names" in {
+
+    (the[ChiselException] thrownBy extractCause[ChiselException] {
+      ChiselStage.elaborate(
+        new Module {
+          // Write to an output port that isn't assigned to a val, and so doesn't get prefixed
+          IO(Output(UInt(8.W))) := 123.U
+        },
+        Array("--throw-on-first-error")
+      )
+    }).getMessage should include("try making it a public field of the Module")
+  }
+}