Fix renaming of annotations with paths (#967)

* check isLocal before removing target tokens in RenameMap

* add fix for Adam's test case, add more test cases

* fix multiple renaming bug

* call componentGet before checking underlying for ReferenceTargets in recursiveGet

* add ModuleGet that implements new instance rename order

* normalize target before renaming

* fix forall/exists bug

* add guards for isLocal cases

* fix circuit renaming, fix traverseHierarchy, add debug prints

* remove sensitivity stuff

* add more tests

* reapply parent path to renamed subpath, fix reference -> instance renames

* remove debug prints

* add instance as reference test case

* fix Ref->IsMod, IsMod->Ref renamed, fix extra test cases

* fix ofModule renaming for refs/instances

* fix renaming of ofModules, change tests

* fix more InstanceTarget rename bugs

* remove bad ReferenceTarget test case

* cleanup midRename of recursiveGet

* fix comments

* fix multiple ModuleTarget renames for InstanceTargets

* dis-allow renaming of ModuleTargets to References

* add back removed lines in RemoveCHIRRTL

* fix indents

* only add ofModule to refs if renaming an inst as a ref

* change .moduleOpt.get to .module

* disallow renaming ReferenceTarget->ModuleTarget

* disallow ref->mod renames in tests, add inst as ref test cases

* cache results of get functions

* fix bot/mid/top renames, add andThen

* fix andThen, add test case

* add more test cases, fix ++

* fix comments, make things private

* dont quit if earlier returns None, add dedup/inline rename tests

* don't rename OfModules to instances paths

* update dedup test

* don't treat references as instances, don't reapply parents to absolute paths

* fix more test cases

* short-circuit OfModule renames if an absolute path is found

* update andThen, remove orElse, deprecate ++

* removed commented code

* update comments

* respond to comments

1 files changed, 420 insertions, 37 deletions

diff --git a/src/test/scala/firrtlTests/RenameMapSpec.scala b/src/test/scala/firrtlTests/RenameMapSpec.scala
index b063599b..702f3939 100644
--- a/src/test/scala/firrtlTests/RenameMapSpec.scala
+++ b/src/test/scala/firrtlTests/RenameMapSpec.scala
@@ -4,8 +4,10 @@ package firrtlTests
 
 import firrtl.RenameMap
 import firrtl.FIRRTLException
-import firrtl.RenameMap.{CircularRenameException, IllegalRenameException}
+import firrtl.RenameMap.IllegalRenameException
 import firrtl.annotations._
+import firrtl.annotations.Target
+import firrtl.annotations.TargetToken.{Instance, OfModule}
 
 class RenameMapSpec extends FirrtlFlatSpec {
   val cir   = CircuitTarget("Top")
@@ -111,13 +113,13 @@ class RenameMapSpec extends FirrtlFlatSpec {
   it should "rename targets if instance and module in the path are renamed" in {
     val renames = RenameMap()
     renames.record(Middle, Middle2)
-    renames.record(Top.ref("m"), Top.ref("m2"))
+    renames.record(Top.instOf("m", "Middle"), Top.instOf("m2", "Middle"))
     renames.get(Top_m) should be (Some(Seq(Top.instOf("m2", "Middle2"))))
   }
 
   it should "rename targets if instance in the path are renamed" in {
     val renames = RenameMap()
-    renames.record(Top.ref("m"), Top.ref("m2"))
+    renames.record(Top.instOf("m", "Middle"), Top.instOf("m2", "Middle"))
     renames.get(Top_m) should be (Some(Seq(Top.instOf("m2", "Middle"))))
   }
 
@@ -141,7 +143,7 @@ class RenameMapSpec extends FirrtlFlatSpec {
 
   it should "properly rename if middle is inlined" in {
     val renames = RenameMap()
-    renames.record(Top_m.ref("l"), Top.ref("m_l"))
+    renames.record(Top_m_l, Top.instOf("m_l", "Leaf"))
     renames.get(Top_m_l_a) should be (Some(Seq(Top.instOf("m_l", "Leaf").ref("a"))))
   }
 
@@ -186,35 +188,19 @@ class RenameMapSpec extends FirrtlFlatSpec {
   }
 
   it should "rename instances with same ofModule" in {
-    val Middle_o = Middle.ref("o")
-    val Middle_i = Middle.ref("i")
+    val Middle_o = Middle.instOf("o", "O")
+    val Middle_i = Middle.instOf("i", "O")
     val renames = RenameMap()
     renames.record(Middle_o, Middle_i)
     renames.get(Middle.instOf("o", "O")) should be (Some(Seq(Middle.instOf("i", "O"))))
   }
 
-  it should "detect circular renames" in {
-    case class BadRename(from: IsMember, tos: Seq[IsMember])
-    val badRenames =
-      Seq(
-        BadRename(foo, Seq(foo.field("bar"))),
-        BadRename(modA, Seq(foo))
-        //BadRename(cir, Seq(foo)),
-        //BadRename(cir, Seq(modA))
-      )
-    // Run all BadRename tests
-    for (BadRename(from, tos) <- badRenames) {
-      val fromN = from
-      val tosN = tos.mkString(", ")
-      //it should s"error if a $fromN is renamed to $tosN" in {
-      val renames = RenameMap()
-      for (to <- tos) {
-        a [IllegalArgumentException] shouldBe thrownBy {
-          renames.record(from, to)
-        }
-      }
-      //}
-    }
+  it should "not treat references as instances targets" in {
+    val Middle_o = Middle.ref("o")
+    val Middle_i = Middle.ref("i")
+    val renames = RenameMap()
+    renames.record(Middle_o, Middle_i)
+    renames.get(Middle.instOf("o", "O")) should be (None)
   }
 
   it should "be able to rename weird stuff" in {
@@ -222,8 +208,8 @@ class RenameMapSpec extends FirrtlFlatSpec {
     case class BadRename(from: CompleteTarget, tos: Seq[CompleteTarget])
     val badRenames =
       Seq(//BadRename(foo, Seq(cir)),
-        BadRename(foo, Seq(modB)),
-        BadRename(modA, Seq(fooB)),
+        //BadRename(foo, Seq(modB)),
+        //BadRename(modA, Seq(fooB)),
         //BadRename(modA, Seq(cir)),
         //BadRename(cir, Seq(foo)),
         //BadRename(cir, Seq(modA)),
@@ -248,13 +234,13 @@ class RenameMapSpec extends FirrtlFlatSpec {
     }
   }
 
-  it should "error if a circular rename occurs" in {
+  it should "not error if a circular rename occurs" in {
     val renames = RenameMap()
     val top = CircuitTarget("Top")
     renames.record(top.module("A"), top.module("B").instOf("c", "C"))
     renames.record(top.module("B"), top.module("A").instOf("c", "C"))
-    a [CircularRenameException] shouldBe thrownBy {
-      renames.get(top.module("A"))
+    renames.get(top.module("A")) should be {
+      Some(Seq(top.module("B").instOf("c", "C")))
     }
   }
 
@@ -267,17 +253,18 @@ class RenameMapSpec extends FirrtlFlatSpec {
     renames.get(top.module("B")) should be (Some(Seq(top.module("A"))))
   }
 
-  it should "error if a reference is renamed to a module, and then we try to rename the reference's field" in {
+  it should "error if a reference is renamed to a module and vice versa" in {
     val renames = RenameMap()
     val top = CircuitTarget("Top")
     renames.record(top.module("A").ref("ref"), top.module("B"))
-    renames.get(top.module("A").ref("ref")) should be(Some(Seq(top.module("B"))))
+    renames.record(top.module("C"), top.module("D").ref("ref"))
     a [IllegalRenameException] shouldBe thrownBy {
-      renames.get(top.module("A").ref("ref").field("field"))
+      renames.get(top.module("C"))
     }
     a [IllegalRenameException] shouldBe thrownBy {
-      renames.get(top.module("A").instOf("ref", "R"))
+      renames.get(top.module("A").ref("ref").field("field"))
     }
+    renames.get(top.module("A").instOf("ref", "R")) should be(None)
   }
 
   it should "error if we rename an instance's ofModule into a non-module" in {
@@ -300,4 +287,400 @@ class RenameMapSpec extends FirrtlFlatSpec {
       println(renames.get(top.module("E").instOf("f", "F").ref("g")))
     }
   }
+
+  it should "rename reference targets with paths if their corresponding pathless target are renamed" in {
+    val cir = CircuitTarget("Top")
+    val modTop = cir.module("Top")
+    val modA = cir.module("A")
+
+    val aggregate = modA.ref("agg")
+    val subField1 = aggregate.field("field1")
+    val subField2 = aggregate.field("field2")
+
+    val lowered1 = aggregate.copy(ref = "agg_field1")
+    val lowered2 = aggregate.copy(ref = "agg_field2")
+
+    // simulating LowerTypes transform
+    val renames = RenameMap()
+    renames.record(subField1, lowered1)
+    renames.record(subField2, lowered2)
+    renames.record(aggregate, Seq(lowered1, lowered2))
+
+    val path = modTop.instOf("b", "B").instOf("a", "A")
+    val testRef1 = subField1.setPathTarget(path)
+
+    renames.get(testRef1) should be {
+      Some(Seq(testRef1.copy(ref = "agg_field1", component = Nil)))
+    }
+  }
+
+  // ~Top|Module/i1:I1>foo.field, where rename map contains ~Top|Module/i1:I1>foo -> ~Top|Module/i1:I1>bar.
+  it should "properly rename reference targets with the same paths" in {
+    val cir = CircuitTarget("Top")
+    val modTop = cir.module("Top")
+    val mod = cir.module("Module")
+
+    val path = mod.instOf("i1", "I1")
+    val oldAgg = mod.ref("foo").setPathTarget(path)
+    val newAgg = mod.ref("bar").setPathTarget(path)
+
+    val renames = RenameMap()
+    renames.record(oldAgg, newAgg)
+
+    val testRef = oldAgg.field("field")
+    renames.get(testRef) should be {
+      Some(Seq(newAgg.field("field")))
+    }
+  }
+
+  it should "properly rename reference targets with partially matching paths" in {
+    val cir = CircuitTarget("Top")
+    val modTop = cir.module("Top")
+    val modA = cir.module("A")
+    val modB = cir.module("B")
+
+    val path = modB.instOf("a", "A")
+    val oldRef = modA.ref("oldRef").setPathTarget(path)
+    val newRef = modA.ref("newRef").setPathTarget(path)
+
+    val renames = RenameMap()
+    renames.record(oldRef, newRef)
+
+    val testRef = oldRef.addHierarchy("B", "b")
+    renames.get(testRef) should be {
+      Some(Seq(newRef.addHierarchy("B", "b")))
+    }
+  }
+
+  it should "properly rename reference targets with partially matching paths and partially matching target tokens" in {
+    val cir = CircuitTarget("Top")
+    val modTop = cir.module("Top")
+    val modA = cir.module("A")
+    val modB = cir.module("B")
+
+    val path = modB.instOf("a", "A")
+    val oldAgg = modA.ref("oldAgg").setPathTarget(path).field("field1")
+    val newAgg = modA.ref("newAgg").setPathTarget(path)
+
+    val renames = RenameMap()
+    renames.record(oldAgg, newAgg)
+
+    val testRef = oldAgg.addHierarchy("B", "b").field("field2")
+    renames.get(testRef) should be {
+      Some(Seq(newAgg.addHierarchy("B", "b").field("field2")))
+    }
+  }
+
+  it should "rename targets with multiple renames starting from most specific to least specific" in {
+    val cir = CircuitTarget("Top")
+    val modTop = cir.module("Top")
+    val modA = cir.module("A")
+    val modB = cir.module("B")
+    val modC = cir.module("C")
+
+    // from: ~Top|A/b:B/c:C>ref.f1.f2.f3
+    // to: ~Top|A/b:B/c:C>ref.f1.f2.f333
+    // renamed first because it is an exact match
+    val from1 = modA
+      .instOf("b", "B")
+      .instOf("c", "C")
+      .ref("ref")
+      .field("f1")
+      .field("f2")
+      .field("f3")
+    val to1 = modA
+      .instOf("b", "B")
+      .instOf("c", "C")
+      .ref("ref")
+      .field("f1")
+      .field("f2")
+      .field("f33")
+
+    // from: ~Top|A/b:B/c:C>ref.f1.f2
+    // to: ~Top|A/b:B/c:C>ref.f1
+    // renamed second because it is a parent target
+    val from2 = modA
+      .instOf("b", "B")
+      .instOf("c", "C")
+      .ref("ref")
+      .field("f1")
+      .field("f2")
+    val to2   = modA
+      .instOf("b", "B")
+      .instOf("c", "C")
+      .ref("ref")
+      .field("f1")
+      .field("f22")
+
+    // from: ~Top|B/c:C>ref.f1
+    // to: ~Top|B/c:C>ref.f11
+    // renamed third because it has a smaller hierarchy
+    val from3 = modB
+      .instOf("c", "C")
+      .ref("ref")
+      .field("f1")
+    val to3   = modB
+      .instOf("c", "C")
+      .ref("ref")
+      .field("f11")
+
+    // from: ~Top|C>ref
+    // to: ~Top|C>refref
+    // renamed last because it has no path
+    val from4 = modC.ref("ref")
+    val to4   = modC.ref("refref")
+
+    val renames1 = RenameMap()
+    renames1.record(from1, to1)
+    renames1.record(from2, to2)
+    renames1.record(from3, to3)
+    renames1.record(from4, to4)
+
+    renames1.get(from1) should be {
+      Some(Seq(modA
+        .instOf("b", "B")
+        .instOf("c", "C")
+        .ref("ref")
+        .field("f1")
+        .field("f2")
+        .field("f33")
+      ))
+    }
+
+    val renames2 = RenameMap()
+    renames2.record(from2, to2)
+    renames2.record(from3, to3)
+    renames2.record(from4, to4)
+
+    renames2.get(from1) should be {
+      Some(Seq(modA
+        .instOf("b", "B")
+        .instOf("c", "C")
+        .ref("ref")
+        .field("f1")
+        .field("f22")
+        .field("f3")
+      ))
+    }
+
+    val renames3 = RenameMap()
+    renames3.record(from3, to3)
+    renames3.record(from4, to4)
+
+    renames3.get(from1) should be {
+      Some(Seq(modA
+        .instOf("b", "B")
+        .instOf("c", "C")
+        .ref("ref")
+        .field("f11")
+        .field("f2")
+        .field("f3")
+      ))
+    }
+  }
+
+  it should "correctly handle renaming of modules to instances" in {
+    val cir = CircuitTarget("Top")
+    val renames = RenameMap()
+    val from = cir.module("C")
+    val to = cir.module("D").instOf("e", "E").instOf("f", "F")
+    renames.record(from, to)
+    renames.get(cir.module("C")) should be {
+      Some(Seq(cir.module("D").instOf("e", "E").instOf("f", "F")))
+    }
+    renames.get(cir.module("A").instOf("b", "B").instOf("c", "C")) should be {
+      None
+    }
+  }
+
+  it should "correctly handle renaming of paths and components at the same time" in {
+    val cir = CircuitTarget("Top")
+    val renames = RenameMap()
+    val from = cir.module("C").ref("foo").field("bar")
+    val to = cir.module("D").instOf("e", "E").instOf("f", "F").ref("foo").field("foo")
+    renames.record(from, to)
+    renames.get(cir.module("A").instOf("b", "B").instOf("c", "C").ref("foo").field("bar")) should be {
+      Some(Seq(cir.module("A").instOf("b", "B").instOf("c", "D")
+        .instOf("e", "E").instOf("f", "F").ref("foo").field("foo")))
+    }
+  }
+
+  it should "error if an instance is renamed to a ReferenceTarget" in {
+    val top = CircuitTarget("Top").module("Top")
+    val renames = RenameMap()
+    val from = top.instOf("a", "A")
+    val to = top.ref("b")
+    renames.record(from, to)
+    a [IllegalRenameException] shouldBe thrownBy {
+      renames.get(from)
+    }
+  }
+
+  it should "not allow renaming of instances even if there is a matching reference rename" in {
+    val top = CircuitTarget("Top").module("Top")
+    val renames = RenameMap()
+    val from = top.ref("a")
+    val to = top.ref("b")
+    renames.record(from, to)
+    renames.get(top.instOf("a", "Foo")) should be (None)
+  }
+
+  it should "correctly chain renames together" in {
+    val top = CircuitTarget("Top")
+
+    val renames1 = RenameMap()
+    val from1 = top.module("A")
+    val to1 = top.module("Top").instOf("b", "B")
+    renames1.record(from1, to1)
+
+    val renames2 = RenameMap()
+    val from2 = top.module("B")
+    val to2 = top.module("B1")
+    renames2.record(from2, to2)
+
+    val renames = renames1.andThen(renames2)
+
+    renames.get(from1) should be {
+      Some(Seq(top.module("Top").instOf("b", "B1")))
+    }
+
+    renames.get(from2) should be {
+      Some(Seq(to2))
+    }
+  }
+
+  it should "correctly chain deleted targets" in {
+    val top = CircuitTarget("Top")
+    val modA = top.module("A")
+    val modA1 = top.module("A1")
+    val modB = top.module("B")
+    val modB1 = top.module("B1")
+
+    val renames1 = RenameMap()
+    renames1.delete(modA)
+    renames1.record(modB, modB1)
+
+    val renames2 = RenameMap()
+    renames2.record(modA, modA1)
+    renames2.delete(modB1)
+
+    val renames = renames1.andThen(renames2)
+
+    renames.get(modA) should be {
+      Some(Seq.empty)
+    }
+    renames.get(modB) should be {
+      Some(Seq.empty)
+    }
+  }
+
+  it should "correctly ++ renameMaps" in {
+    val top = CircuitTarget("Top")
+    val modA = top.module("A")
+    val modA1 = top.module("A1")
+    val modA2 = top.module("A1")
+    val modB = top.module("B")
+    val modB1 = top.module("B1")
+    val modC = top.module("C")
+    val modC1 = top.module("C1")
+
+    val renames1 = RenameMap()
+    renames1.record(modA, modA1)
+    renames1.record(modC, modC1)
+
+    val renames2 = RenameMap()
+    renames2.record(modA, modA2)
+    renames2.record(modB, modB1)
+
+    val renames = renames1 ++ renames2
+    renames.get(modA) should be {
+      Some(Seq(modA2))
+    }
+    renames.get(modB) should be {
+      Some(Seq(modB1))
+    }
+    renames.get(modC) should be {
+      Some(Seq(modC1))
+    }
+  }
+
+  it should "be able to inline instances" in {
+    val top = CircuitTarget("Top")
+    val inlineRename1 = {
+      val inlineMod = top.module("A")
+      val inlineInst = top.module("A").instOf("b", "B")
+      val oldRef = inlineMod.ref("bar")
+      val prefixRef = inlineMod.ref("foo")
+
+      val renames1 = RenameMap()
+      renames1.record(inlineInst, inlineMod)
+
+      val renames2 = RenameMap()
+      renames2.record(oldRef, prefixRef)
+
+      renames1.andThen(renames2)
+    }
+
+    val inlineRename2 = {
+      val inlineMod = top.module("A1")
+      val inlineInst = top.module("A1").instOf("b", "B1")
+      val oldRef = inlineMod.ref("bar")
+      val prefixRef = inlineMod.ref("foo")
+
+      val renames1 = RenameMap()
+      renames1.record(inlineInst, inlineMod)
+
+      val renames2 = RenameMap()
+      renames2.record(oldRef, prefixRef)
+
+      renames1.andThen(renames2)
+    }
+
+    val renames = inlineRename1 ++ inlineRename2
+    renames.get(top.module("A").instOf("b", "B").ref("bar")) should be {
+      Some(Seq(top.module("A").ref("foo")))
+    }
+
+    renames.get(top.module("A1").instOf("b", "B1").ref("bar")) should be {
+      Some(Seq(top.module("A1").ref("foo")))
+    }
+  }
+
+  it should "be able to dedup modules" in {
+    val top = CircuitTarget("Top")
+    val topMod = top.module("Top")
+    val dedupedMod = top.module("A")
+    val dupMod1 = top.module("A1")
+    val dupMod2 = top.module("A2")
+
+    val relPath1 = dupMod1.addHierarchy("Foo", "a")//top.module("Foo").instOf("a", "A1")
+    val relPath2 = dupMod2.addHierarchy("Foo", "a")//top.module("Foo").instOf("a", "A2")
+
+    val absPath1 = relPath1.addHierarchy("Top", "foo")
+    val absPath2 = relPath2.addHierarchy("Top", "foo")
+
+    val renames1 = RenameMap()
+    renames1.record(dupMod1, absPath1)
+    renames1.record(dupMod2, absPath2)
+    renames1.record(relPath1, absPath1)
+    renames1.record(relPath2, absPath2)
+
+    val renames2 = RenameMap()
+    renames2.record(dupMod1, dedupedMod)
+    renames2.record(dupMod2, dedupedMod)
+
+    val renames = renames1.andThen(renames2)
+
+    renames.get(dupMod1.instOf("foo", "Bar").ref("ref")) should be {
+      Some(Seq(absPath1.copy(ofModule = "A").instOf("foo", "Bar").ref("ref")))
+    }
+
+    renames.get(dupMod2.ref("ref")) should be {
+      Some(Seq(absPath2.copy(ofModule = "A").ref("ref")))
+    }
+
+    renames.get(absPath1.instOf("b", "B").ref("ref")) should be {
+      Some(Seq(absPath1.copy(ofModule = "A").instOf("b", "B").ref("ref")))
+    }
+  }
 }