Merge pull request #898 from seldridge/port-uniqueness-checks

Enforce port uniqueness in Chirrtl/High Checks

4 files changed, 42 insertions, 5 deletions

diff --git a/src/main/scala/firrtl/passes/CheckChirrtl.scala b/src/main/scala/firrtl/passes/CheckChirrtl.scala
index 3722fd0d..8f37c8bf 100644
--- a/src/main/scala/firrtl/passes/CheckChirrtl.scala
+++ b/src/main/scala/firrtl/passes/CheckChirrtl.scala
@@ -31,8 +31,6 @@ object CheckChirrtl extends Pass {
   class NoTopModuleException(info: Info, name: String) extends PassException(
     s"$info: A single module must be named $name.")
 
-  // TODO FIXME
-  // - Do we need to check for uniquness on port names?
   def run (c: Circuit): Circuit = {
     val errors = new Errors()
     val moduleNames = (c.modules map (_.name)).toSet
@@ -105,6 +103,8 @@ object CheckChirrtl extends Pass {
     }
 
     def checkChirrtlP(mname: String, names: NameSet)(p: Port): Port = {
+      if (names(p.name))
+        errors append new NotUniqueException(NoInfo, mname, p.name)
       names += p.name
       (p.tpe map checkChirrtlT(p.info, mname)
              map checkChirrtlW(p.info, mname))
diff --git a/src/main/scala/firrtl/passes/Checks.scala b/src/main/scala/firrtl/passes/Checks.scala
index 215c5425..4c7458bf 100644
--- a/src/main/scala/firrtl/passes/Checks.scala
+++ b/src/main/scala/firrtl/passes/Checks.scala
@@ -54,8 +54,6 @@ object CheckHighForm extends Pass {
   class LsbLargerThanMsbException(info: Info, mname: String, op: String, lsb: Int, msb: Int) extends PassException(
     s"$info: [module $mname] Primop $op lsb $lsb > $msb.")
 
-  // TODO FIXME
-  // - Do we need to check for uniquness on port names?
   def run(c: Circuit): Circuit = {
     val errors = new Errors()
     val moduleGraph = new ModuleGraph
@@ -192,6 +190,8 @@ object CheckHighForm extends Pass {
     }
 
     def checkHighFormP(mname: String, names: NameSet)(p: Port): Port = {
+      if (names(p.name))
+        errors.append(new NotUniqueException(NoInfo, mname, p.name))
       names += p.name
       (p.tpe map checkHighFormT(p.info, mname)
              map checkHighFormW(p.info, mname))
diff --git a/src/test/scala/firrtlTests/CheckSpec.scala b/src/test/scala/firrtlTests/CheckSpec.scala
index 3c9894ba..767f2392 100644
--- a/src/test/scala/firrtlTests/CheckSpec.scala
+++ b/src/test/scala/firrtlTests/CheckSpec.scala
@@ -282,4 +282,32 @@ class CheckSpec extends FlatSpec with Matchers {
       }
     }
   }
+
+  behavior of "Uniqueness"
+  for ((description, input) <- CheckSpec.nonUniqueExamples) {
+    it should s"be asserted for $description" in {
+      assertThrows[CheckHighForm.NotUniqueException] {
+        Seq(ToWorkingIR, CheckHighForm).foldLeft(Parser.parse(input)){ case (c, tx) => tx.run(c) }
+      }
+    }
+  }
 }
+
+object CheckSpec {
+  val nonUniqueExamples = List(
+    ("two ports with the same name",
+     """|circuit Top:
+        |  module Top:
+        |    input a: UInt<1>
+        |    input a: UInt<1>""".stripMargin),
+    ("two nodes with the same name",
+     """|circuit Top:
+        |  module Top:
+        |    node a = UInt<1>("h0")
+        |    node a = UInt<1>("h0")""".stripMargin),
+    ("a port and a node with the same name",
+     """|circuit Top:
+        |  module Top:
+        |    input a: UInt<1>
+        |    node a = UInt<1>("h0") """.stripMargin) )
+  }
diff --git a/src/test/scala/firrtlTests/ChirrtlSpec.scala b/src/test/scala/firrtlTests/ChirrtlSpec.scala
index fd4374f0..774c352b 100644
--- a/src/test/scala/firrtlTests/ChirrtlSpec.scala
+++ b/src/test/scala/firrtlTests/ChirrtlSpec.scala
@@ -16,7 +16,7 @@ class ChirrtlSpec extends FirrtlFlatSpec {
     CInferTypes,
     CInferMDir,
     RemoveCHIRRTL,
-    ToWorkingIR,            
+    ToWorkingIR,
     CheckHighForm,
     ResolveKinds,
     InferTypes,
@@ -71,6 +71,15 @@ class ChirrtlSpec extends FirrtlFlatSpec {
       }
     }
   }
+
+  behavior of "Uniqueness"
+  for ((description, input) <- CheckSpec.nonUniqueExamples) {
+    it should s"be asserted for $description" in {
+      assertThrows[CheckChirrtl.NotUniqueException] {
+        Seq(ToWorkingIR, CheckChirrtl).foldLeft(Parser.parse(input)){ case (c, tx) => tx.run(c) }
+      }
+    }
+  }
 }
 
 class ChirrtlMemsExecutionTest extends ExecutionTest("ChirrtlMems", "/features")