Fix unchecked type in ManipulateNames (#1726)

Fix a bug where a type check would always yield true. This caused a
bug where allow/block-list annotations would be incorrectly applied to
all subtypes of ManipulateNames.

The tests are updated to check that this now works.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@ibm.com>

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

3 files changed, 59 insertions, 19 deletions

diff --git a/src/main/scala/firrtl/transforms/ManipulateNames.scala b/src/main/scala/firrtl/transforms/ManipulateNames.scala
index 956b39e6..c55dab57 100644
--- a/src/main/scala/firrtl/transforms/ManipulateNames.scala
+++ b/src/main/scala/firrtl/transforms/ManipulateNames.scala
@@ -432,14 +432,24 @@ abstract class ManipulateNames[A <: ManipulateNames[_] : ClassTag] extends Trans
   def execute(state: CircuitState): CircuitState = {
 
     val block = state.annotations.collect {
-      case ManipulateNamesBlocklistAnnotation(targetSeq, _: Dependency[A]) => targetSeq
+      case ManipulateNamesBlocklistAnnotation(targetSeq, t) => t.getObject match {
+        case _: A => targetSeq
+        case _    => Nil
+      }
     }.flatten.flatten.toSet
 
-    val allow = state.annotations.collect {
-      case ManipulateNamesAllowlistAnnotation(targetSeq, _: Dependency[A]) => targetSeq
-    } match {
-      case Nil => (a: Target) => true
-      case a   => a.flatten.flatten.toSet
+    val allow = {
+      val allowx = state.annotations.collect {
+         case ManipulateNamesAllowlistAnnotation(targetSeq, t) => t.getObject match {
+           case _: A => targetSeq
+           case _    => Nil
+         }
+      }.flatten.flatten
+
+      allowx match {
+        case Nil => (a: Target) => true
+        case a   => a.toSet
+      }
     }
 
     val renames = RenameMap()
@@ -447,11 +457,18 @@ abstract class ManipulateNames[A <: ManipulateNames[_] : ClassTag] extends Trans
 
     val annotationsx = state.annotations.flatMap {
       /* Consume blocklist annotations */
-      case ManipulateNamesBlocklistAnnotation(_, _: Dependency[A]) => None
-      /* Convert allowlist annotations to result annotations */
-      case ManipulateNamesAllowlistAnnotation(a, t: Dependency[A]) => (a, a.map(_.map(renames(_)).flatten)) match {
-        case (a, b) => Some(ManipulateNamesAllowlistResultAnnotation(b, t, a))
+      case foo@ ManipulateNamesBlocklistAnnotation(_, t) => t.getObject match {
+        case _: A => None
+        case _    => Some(foo)
       }
+      /* Convert allowlist annotations to result annotations */
+      case foo@ ManipulateNamesAllowlistAnnotation(a, t) =>
+        t.getObject match {
+          case _: A => (a, a.map(_.map(renames(_)).flatten)) match {
+            case (a, b) => Some(ManipulateNamesAllowlistResultAnnotation(b, t, a))
+          }
+          case _  => Some(foo)
+        }
       case a => Some(a)
     }
 
diff --git a/src/test/scala/firrtlTests/features/LetterCaseTransformSpec.scala b/src/test/scala/firrtlTests/features/LetterCaseTransformSpec.scala
index e0053fa8..4299ac7f 100644
--- a/src/test/scala/firrtlTests/features/LetterCaseTransformSpec.scala
+++ b/src/test/scala/firrtlTests/features/LetterCaseTransformSpec.scala
@@ -67,7 +67,8 @@ class LetterCaseTransformSpec extends AnyFlatSpec with Matchers {
     private val Bar = Foo.module("Bar")
 
     val annotations = Seq(TrackingAnnotation(Foo.module("Foo").ref("MeM").field("wRITE")field("en")),
-                          ManipulateNamesBlocklistAnnotation(Seq(Seq(Bar)), Dependency[LowerCaseNames]))
+                          ManipulateNamesBlocklistAnnotation(Seq(Seq(Bar)), Dependency[LowerCaseNames]),
+                          ManipulateNamesBlocklistAnnotation(Seq(Seq(Bar.ref("OuT"))), Dependency[UpperCaseNames]))
     val state = CircuitState(Parser.parse(input), annotations)
   }
 
@@ -125,8 +126,8 @@ class LetterCaseTransformSpec extends AnyFlatSpec with Matchers {
       { case ir.Circuit(_, _, "FOO") => true },
       { case ir.Module(_, "FOO",
                        Seq(ir.Port(_, "CLK", _, _), ir.Port(_, "RST_P", _, _), ir.Port(_, "ADDR", _, _)), _) => true },
-      /* Module "Bar" should be skipped via a ManipulateNamesBlocklistAnnotation */
-      { case ir.Module(_, "Bar", Seq(ir.Port(_, "OUT", _, _)), _) => true },
+      /* "Bar>OuT" should be skipped via a ManipulateNamesBlocklistAnnotation */
+      { case ir.Module(_, "BAR", Seq(ir.Port(_, "OuT", _, _)), _) => true },
       { case ir.Module(_, "BAZ", Seq(ir.Port(_, "OUT", _, _)), _) => true },
       { case ir.Module(_, "BAZ_0", Seq(ir.Port(_, "OUT", _, _)), _) => true },
       /* External module "Ext" is not renamed */
@@ -143,10 +144,7 @@ class LetterCaseTransformSpec extends AnyFlatSpec with Matchers {
       { case ir.IsInvalid(_, WSubField(WSubField(WRef("MEM", _, _, _), "READ", _, _), "addr", _, _)) => true },
       { case ir.IsInvalid(_, WSubField(WSubField(WRef("MEM", _, _, _), "WRITE", _, _), "addr", _, _)) => true },
       { case ir.IsInvalid(_, WSubField(WSubField(WRef("MEM", _, _, _), "RW", _, _), "addr", _, _)) => true },
-      /* Module "Bar" was skipped via a ManipulateNamesBlocklistAnnotation. The instance "SuB1" is renamed to "SUB1"
-       * because this statement occurs before the "sub1" node later. This differs from the lower case test.
-       */
-      { case WDefInstance(_, "SUB1", "Bar", _) => true },
+      { case WDefInstance(_, "SUB1", "BAR", _) => true },
       /* Instance "SuB2" and "SuB3" switch their modules from the lower case test due to namespace behavior. */
       { case WDefInstance(_, "SUB2", "BAZ", _) => true },
       { case WDefInstance(_, "SUB3", "BAZ_0", _) => true },
@@ -154,7 +152,8 @@ class LetterCaseTransformSpec extends AnyFlatSpec with Matchers {
       { case WDefInstance(_, "SUB4", "Ext", _) => true },
       /* Node "sub1" becomes "SUB1_0" because instance "SuB1" already got the "SUB1" name. */
       { case ir.DefNode(_, "SUB1_0", _) => true },
-      { case ir.DefNode(_, "CORGE_CORGE", WSubField(WRef("SUB1", _, _, _), "OUT", _, _)) => true },
+      /* Port "OuT" was skipped via a ManipulateNamesBlocklistAnnotation */
+      { case ir.DefNode(_, "CORGE_CORGE", WSubField(WRef("SUB1", _, _, _), "OuT", _, _)) => true },
       { case ir.DefNode(_, "QUUZQUUZ",
                         ir.DoPrim(_,Seq(WSubField(WRef("SUB2", _, _, _), "OUT", _, _),
                                         WSubField(WRef("SUB3", _, _, _), "OUT", _, _)), _, _)) => true },
diff --git a/src/test/scala/firrtlTests/transforms/ManipulateNamesSpec.scala b/src/test/scala/firrtlTests/transforms/ManipulateNamesSpec.scala
index b1e2eeb9..ec1b505b 100644
--- a/src/test/scala/firrtlTests/transforms/ManipulateNamesSpec.scala
+++ b/src/test/scala/firrtlTests/transforms/ManipulateNamesSpec.scala
@@ -25,10 +25,14 @@ import org.scalatest.matchers.should.Matchers
 
 object ManipulateNamesSpec {
 
-  class AddPrefix extends ManipulateNames {
+  class AddPrefix extends ManipulateNames[AddPrefix] {
     override def manipulate = (a: String, b: Namespace) => Some(b.newName("prefix_" + a))
   }
 
+  class AddSuffix extends ManipulateNames[AddSuffix] {
+    override def manipulate = (a: String, b: Namespace) => Some(b.newName(a + "_suffix"))
+  }
+
 }
 
 class ManipulateNamesSpec extends AnyFlatSpec with Matchers {
@@ -172,6 +176,26 @@ class ManipulateNamesSpec extends AnyFlatSpec with Matchers {
     }.getMessage should include ("LowerTypes")
   }
 
+  it should "only consume annotations whose type parameter matches" in new CircuitFixture {
+    val annotations = Seq(
+      ManipulateNamesBlocklistAnnotation(Seq(Seq(`~Foo|Bar>a`)), Dependency[AddPrefix]),
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo`)), Dependency[AddSuffix]),
+      ManipulateNamesAllowlistAnnotation(Seq(Seq(`~Foo|Bar>a`)), Dependency[AddSuffix])
+    )
+    val state = CircuitState(Parser.parse(input), annotations)
+    override val tm = new firrtl.stage.transforms.Compiler(Seq(Dependency[AddPrefix], Dependency[AddSuffix]))
+    val statex = tm.execute(state)
+    val expected: Seq[PartialFunction[Any, Boolean]] = Seq(
+      { case ir.Circuit(_, _, "prefix_Foo") => true },
+      { case ir.Module(_, "prefix_Foo", _, _) => true},
+      { case ir.DefInstance(_, "prefix_bar", "prefix_Bar", _) => true},
+      { case ir.DefInstance(_, "prefix_bar2", "prefix_Bar", _) => true},
+      { case ir.Module(_, "prefix_Bar", _, _) => true},
+      { case ir.DefNode(_, "a_suffix", _) => true}
+    )
+    expected.foreach(statex should containTree (_))
+  }
+
   behavior of "ManipulateNamesBlocklistAnnotation"
 
   it should "throw an exception if a non-local target is skipped" in new CircuitFixture {