ir: store FileInfo string in escaped format (#1690)

This should speed up the common case
as the compiler never operates on the
unescaped string.
The new escape function also fixes a bug
where ']' was not escaped even though it
is the delimiting character for FileInfo.
In order to maintain backwards
compatibility for the ProtoBuf format,
this patch adds escape/unescape calls
when going from/to protobuf format.
For better performance we should consider
changing the protobuf format.

12 files changed, 164 insertions, 30 deletions

diff --git a/spec/spec.pdf b/spec/spec.pdf
index 26be49e6..fa3dbe8f 100644
--- a/spec/spec.pdf
+++ b/spec/spec.pdf
diff --git a/spec/spec.tex b/spec/spec.tex
index d65078bc..8410c9d2 100644
--- a/spec/spec.tex
+++ b/spec/spec.tex
@@ -2044,23 +2044,26 @@ circuit Foo :
        b <= not(a)
 \end{lstlisting}
 
-All circuits, modules, ports and statements can optionally be followed with the info token \verb|@[fileinfo]| where fileinfo is a string containing the source file information from where it was generated.
+All circuits, modules, ports and statements can optionally be followed with the info token \verb|@[fileinfo]|
+where fileinfo is a string containing the source file information from where it was generated.
+The following characters need to be escaped with a leading `\verb|\|':
+`\verb|\n|' (new line), `\verb|\t|' (tab), `\verb|]|' and `\verb|\|' itself.
 
 The following example shows the info tokens included:
 
 \begin{lstlisting}
-circuit Top : @["myfile.txt: 14, 8"]
-   module Top : @["myfile.txt: 15, 2"]
-     output out:UInt @["myfile.txt: 16, 3"]
-     input b:UInt<32> @["myfile.txt: 17, 3"]
-     input c:UInt<1> @["myfile.txt: 18, 3"]
-     input d:UInt<16> @["myfile.txt: 19, 3"]
-     wire a:UInt @["myfile.txt: 21, 8"]
-     when c : @["myfile.txt: 24, 8"]
-       a <= b @["myfile.txt: 27, 16"]
+circuit Top : @[myfile.txt 14:8]
+   module Top : @[myfile.txt 15:2]
+     output out:UInt @[myfile.txt 16:3]
+     input b:UInt<32> @[myfile.txt 17:3]
+     input c:UInt<1> @[myfile.txt 18:3]
+     input d:UInt<16> @[myfile.txt 19:3]
+     wire a:UInt @[myfile.txt 21:8]
+     when c : @[myfile.txt 24:8]
+       a <= b @[myfile.txt 27:16]
      else :
-       a <= d @["myfile.txt: 29, 17"]
-     out <= add(a,a) @["myfile.txt: 34, 4"]
+       a <= d @[myfile.txt 29:17]
+     out <= add(a,a) @[myfile.txt 34:4]
 \end{lstlisting}
 
 \section{FIRRTL Language Definition}
diff --git a/src/main/scala/firrtl/Emitter.scala b/src/main/scala/firrtl/Emitter.scala
index 1044f047..b5474769 100644
--- a/src/main/scala/firrtl/Emitter.scala
+++ b/src/main/scala/firrtl/Emitter.scala
@@ -265,7 +265,12 @@ class VerilogEmitter extends SeqTransform with Emitter {
       case (i: BigInt) => w write i.toString
       case (i: Info) => i match {
         case NoInfo => // Do nothing
-        case ix => w.write(s" //$ix")
+        case f: FileInfo =>
+          val escaped = FileInfo.escapedToVerilog(f.escaped)
+          w.write(s" // @[$escaped]")
+        case m: MultiInfo =>
+          val escaped = FileInfo.escapedToVerilog(m.flatten.map(_.escaped).mkString(" "))
+          w.write(s" // @[$escaped]")
       }
       case (s: Seq[Any]) =>
         s foreach (emit(_, top + 1))
diff --git a/src/main/scala/firrtl/Visitor.scala b/src/main/scala/firrtl/Visitor.scala
index 76b5c2cf..8bdab21b 100644
--- a/src/main/scala/firrtl/Visitor.scala
+++ b/src/main/scala/firrtl/Visitor.scala
@@ -69,15 +69,15 @@ class Visitor(infoMode: InfoMode) extends AbstractParseTreeVisitor[FirrtlNode] w
     infoMode match {
       case UseInfo =>
         if (useInfo.length == 0) NoInfo
-        else ir.FileInfo(ir.StringLit.unescape(useInfo))
+        else ir.FileInfo.fromEscaped(useInfo)
       case AppendInfo(filename) if (useInfo.length == 0) =>
-        ir.FileInfo(ir.StringLit.unescape(genInfo(filename)))
+        ir.FileInfo.fromEscaped(genInfo(filename))
       case AppendInfo(filename) =>
-        val useFileInfo = ir.FileInfo(ir.StringLit.unescape(useInfo))
-        val newFileInfo = ir.FileInfo(ir.StringLit.unescape(genInfo(filename)))
+        val useFileInfo = ir.FileInfo.fromEscaped(useInfo)
+        val newFileInfo = ir.FileInfo.fromEscaped(genInfo(filename))
         ir.MultiInfo(useFileInfo, newFileInfo)
       case GenInfo(filename) =>
-        ir.FileInfo(ir.StringLit.unescape(genInfo(filename)))
+        ir.FileInfo.fromEscaped(genInfo(filename))
       case IgnoreInfo => NoInfo
     }
   }
diff --git a/src/main/scala/firrtl/ir/IR.scala b/src/main/scala/firrtl/ir/IR.scala
index 860f8b90..734b475d 100644
--- a/src/main/scala/firrtl/ir/IR.scala
+++ b/src/main/scala/firrtl/ir/IR.scala
@@ -5,7 +5,9 @@ package ir
 
 import Utils.{dec2string, indent, trim}
 import firrtl.constraint.{Constraint, IsKnown, IsVar}
+import org.apache.commons.text.translate.{AggregateTranslator, JavaUnicodeEscaper, LookupTranslator}
 
+import scala.collection.JavaConverters._
 import scala.math.BigDecimal.RoundingMode._
 
 /** Intermediate Representation */
@@ -22,22 +24,71 @@ case object NoInfo extends Info {
   override def toString: String = ""
   def ++(that: Info): Info = that
 }
-case class FileInfo(info: StringLit) extends Info {
-  override def toString: String = " @[" + info.serialize + "]"
+
+/** Stores the string of a file info annotation in its escaped form. */
+case class FileInfo(escaped: String) extends Info {
+  override def toString: String = " @[" + escaped + "]"
   def ++(that: Info): Info = if (that == NoInfo) this else MultiInfo(Seq(this, that))
+  def unescaped: String = FileInfo.unescape(escaped)
+  @deprecated("Use FileInfo.unescaped instead. FileInfo.info will be removed in FIRRTL 1.5.", "FIRRTL 1.4")
+  def info: StringLit = StringLit(this.unescaped)
 }
+
+object FileInfo {
+  @deprecated("Use FileInfo.fromUnEscaped instead. FileInfo.apply will be removed in FIRRTL 1.5.", "FIRRTL 1.4")
+  def apply(info: StringLit): FileInfo = new FileInfo(escape(info.string))
+  def fromEscaped(s: String): FileInfo = new FileInfo(s)
+  def fromUnescaped(s: String): FileInfo = new FileInfo(escape(s))
+  /** prepends a `\` to: `\`, `\n`, `\t` and `]` */
+  def escape(s: String): String = EscapeFirrtl.translate(s)
+  /** removes the `\` in front of `\`, `\n`, `\t` and `]` */
+  def unescape(s: String): String = UnescapeFirrtl.translate(s)
+  /** take an already escaped String and do the additional escaping needed for Verilog comment */
+  def escapedToVerilog(s: String) = EscapedToVerilog.translate(s)
+
+  // custom `CharSequenceTranslator` for FIRRTL Info String escaping
+  type CharMap = (CharSequence, CharSequence)
+  private val EscapeFirrtl = new LookupTranslator(Seq[CharMap](
+    "\\" -> "\\\\",
+    "\n" -> "\\n",
+    "\t" -> "\\t",
+    "]" -> "\\]"
+  ).toMap.asJava)
+  private val UnescapeFirrtl = new LookupTranslator(Seq[CharMap](
+    "\\\\" -> "\\",
+    "\\n" -> "\n",
+    "\\t" -> "\t",
+    "\\]" -> "]"
+  ).toMap.asJava)
+  // EscapeFirrtl + EscapedToVerilog essentially does the same thing as running StringEscapeUtils.unescapeJava
+  private val EscapedToVerilog = new AggregateTranslator(
+    new LookupTranslator(Seq[CharMap](
+      // ] is the one character that firrtl needs to be escaped that does not need to be escaped in
+      "\\]" -> "]",
+      "\"" -> "\\\"",
+      // \n and \t are already escaped
+      "\b" -> "\\b",
+      "\f" -> "\\f",
+      "\r" -> "\\r"
+    ).toMap.asJava),
+    JavaUnicodeEscaper.outsideOf(32, 0x7f)
+  )
+
+}
+
 case class MultiInfo(infos: Seq[Info]) extends Info {
-  private def collectStringLits(info: Info): Seq[StringLit] = info match {
-    case FileInfo(lit) => Seq(lit)
-    case MultiInfo(seq) => seq flatMap collectStringLits
+  private def collectStrings(info: Info): Seq[String] = info match {
+    case f : FileInfo => Seq(f.escaped)
+    case MultiInfo(seq) => seq flatMap collectStrings
     case NoInfo => Seq.empty
   }
   override def toString: String = {
-    val parts = collectStringLits(this)
-    if (parts.nonEmpty) parts.map(_.serialize).mkString(" @[", " ", "]")
+    val parts = collectStrings(this)
+    if (parts.nonEmpty) parts.mkString(" @[", " ", "]")
     else ""
   }
   def ++(that: Info): Info = if (that == NoInfo) this else MultiInfo(infos :+ that)
+  def flatten: Seq[FileInfo] = MultiInfo.flattenInfo(infos)
 }
 object MultiInfo {
   def apply(infos: Info*) = {
@@ -48,6 +99,11 @@ object MultiInfo {
       case _ => new MultiInfo(infosx)
     }
   }
+  private def flattenInfo(infos: Seq[Info]): Seq[FileInfo] = infos.flatMap {
+    case NoInfo => Seq()
+    case f : FileInfo => Seq(f)
+    case MultiInfo(infos) => flattenInfo(infos)
+  }
 }
 
 trait HasName {
diff --git a/src/main/scala/firrtl/ir/Serializer.scala b/src/main/scala/firrtl/ir/Serializer.scala
index 11f15217..8ab4fc6e 100644
--- a/src/main/scala/firrtl/ir/Serializer.scala
+++ b/src/main/scala/firrtl/ir/Serializer.scala
@@ -44,7 +44,7 @@ object Serializer {
   }
 
   private def s(node: Info)(implicit b: StringBuilder, indent: Int): Unit = node match {
-    case FileInfo(info) => b ++= " @[" ; s(info) ; b ++= "]"
+    case f : FileInfo => b ++= " @[" ; b ++= f.escaped ; b ++= "]"
     case NoInfo => // empty string
     case MultiInfo(infos) =>
       val ii = flattenInfo(infos)
diff --git a/src/main/scala/firrtl/proto/FromProto.scala b/src/main/scala/firrtl/proto/FromProto.scala
index 5de92b57..891dc2ba 100644
--- a/src/main/scala/firrtl/proto/FromProto.scala
+++ b/src/main/scala/firrtl/proto/FromProto.scala
@@ -45,9 +45,9 @@ object FromProto {
       case Firrtl.SourceInfo.POSITION_FIELD_NUMBER =>
         val pos = info.getPosition
         val str = s"${pos.getFilename} ${pos.getLine}:${pos.getColumn}"
-        ir.FileInfo(ir.StringLit(str))
+        ir.FileInfo.fromUnescaped(str)
       case Firrtl.SourceInfo.TEXT_FIELD_NUMBER =>
-        ir.FileInfo(ir.StringLit(info.getText))
+        ir.FileInfo.fromUnescaped(info.getText)
       // NONE_FIELD_NUMBER or anything else
       case _ => ir.NoInfo
     }
diff --git a/src/main/scala/firrtl/proto/ToProto.scala b/src/main/scala/firrtl/proto/ToProto.scala
index 6cc6d11f..47fb3cec 100644
--- a/src/main/scala/firrtl/proto/ToProto.scala
+++ b/src/main/scala/firrtl/proto/ToProto.scala
@@ -135,8 +135,8 @@ object ToProto {
     info match {
       case ir.NoInfo =>
         ib.setNone(Firrtl.SourceInfo.None.newBuilder)
-      case ir.FileInfo(ir.StringLit(text)) =>
-        ib.setText(text)
+      case f : ir.FileInfo =>
+        ib.setText(f.unescaped)
       // TODO properly implement MultiInfo
       case ir.MultiInfo(infos) =>
         val x = if (infos.nonEmpty) infos.head else ir.NoInfo
diff --git a/src/test/scala/firrtlTests/InfoSpec.scala b/src/test/scala/firrtlTests/InfoSpec.scala
index 48567d69..01e0a0ac 100644
--- a/src/test/scala/firrtlTests/InfoSpec.scala
+++ b/src/test/scala/firrtlTests/InfoSpec.scala
@@ -172,4 +172,27 @@ class InfoSpec extends FirrtlFlatSpec with FirrtlMatchers {
     val expectedInfos = Seq(FileInfo(StringLit("Top.scala 15:14")), FileInfo(StringLit("myfile.fir 6:4")))
     circuitState should containTree { case MultiInfo(`expectedInfos`) => true }
   }
+
+  "FileInfo" should "be able to contain a escaped characters" in {
+    def input(info: String): String =
+      s"""circuit m: @[$info]
+        |  module m:
+        |    skip
+        |""".stripMargin
+    def parseInfo(info: String): FileInfo = {
+      firrtl.Parser.parse(input(info)).info.asInstanceOf[FileInfo]
+    }
+
+    parseInfo("test\\ntest").escaped should be ("test\\ntest")
+    parseInfo("test\\ntest").unescaped should be ("test\ntest")
+    parseInfo("test\\ttest").escaped should be ("test\\ttest")
+    parseInfo("test\\ttest").unescaped should be ("test\ttest")
+    parseInfo("test\\\\test").escaped should be ("test\\\\test")
+    parseInfo("test\\\\test").unescaped should be ("test\\test")
+    parseInfo("test\\]test").escaped should be ("test\\]test")
+    parseInfo("test\\]test").unescaped should be ("test]test")
+    parseInfo("test[\\][\\]test").escaped should be ("test[\\][\\]test")
+    parseInfo("test[\\][\\]test").unescaped should be ("test[][]test")
+  }
+
 }
diff --git a/src/test/scala/firrtlTests/ParserSpec.scala b/src/test/scala/firrtlTests/ParserSpec.scala
index 2ae5b430..ba5cb889 100644
--- a/src/test/scala/firrtlTests/ParserSpec.scala
+++ b/src/test/scala/firrtlTests/ParserSpec.scala
@@ -244,6 +244,18 @@ class ParserSpec extends FirrtlFlatSpec {
       Driver.execute(manager)
     }
   }
+
+  it should "be able to parse a MultiInfo as a FileInfo" in {
+    // currently MultiInfo gets flattened into a single string which can only be recovered as a FileInfo
+    val info = ir.MultiInfo(Seq(ir.MultiInfo(Seq(ir.FileInfo("a"))), ir.FileInfo("b"), ir.FileInfo("c")))
+    val input =
+      s"""circuit m:${info.serialize}
+        |  module m:
+        |    skip
+        |""".stripMargin
+    val c = firrtl.Parser.parse(input)
+    assert(c.info == ir.FileInfo("a b c"))
+  }
 }
 
 class ParserPropSpec extends FirrtlPropSpec {
diff --git a/src/test/scala/firrtlTests/ProtoBufSpec.scala b/src/test/scala/firrtlTests/ProtoBufSpec.scala
index 14f94cb3..3a94ec3f 100644
--- a/src/test/scala/firrtlTests/ProtoBufSpec.scala
+++ b/src/test/scala/firrtlTests/ProtoBufSpec.scala
@@ -211,4 +211,19 @@ class ProtoBufSpec extends FirrtlFlatSpec {
     val expected = ir.ValidIf(ir.Reference("en"), ir.Reference("x"), UnknownType)
     FromProto.convert(ToProto.convert(vi).build) should equal (expected)
   }
+
+  it should "appropriately escape and unescape FileInfo strings" in {
+    val pairs = Seq(
+      "test\\ntest" -> "test\ntest",
+      "test\\ttest" -> "test\ttest",
+      "test\\\\test" -> "test\\test",
+      "test\\]test" -> "test]test"
+    )
+
+    pairs.foreach { case (escaped, unescaped) =>
+      val info = ir.FileInfo(escaped)
+      ToProto.convert(info).build().getText should equal (unescaped)
+      FromProto.convert(ToProto.convert(info).build) should equal (info)
+    }
+  }
 }
diff --git a/src/test/scala/firrtlTests/VerilogEmitterTests.scala b/src/test/scala/firrtlTests/VerilogEmitterTests.scala
index 171bce78..21d7075e 100644
--- a/src/test/scala/firrtlTests/VerilogEmitterTests.scala
+++ b/src/test/scala/firrtlTests/VerilogEmitterTests.scala
@@ -727,6 +727,26 @@ class VerilogEmitterSpec extends FirrtlFlatSpec {
     result should    containLine("wire [2:0] _GEN_0 = $signed(x) - 3'sh2;")
     result should    containLine("assign z = _GEN_0[1:0];")
   }
+
+  it should "emit FileInfo as Verilog comment" in {
+    def result(info: String): CircuitState = compileBody(
+      s"""input x : UInt<2>
+        |output z : UInt<2>
+        |z <= x @[$info]
+        |""".stripMargin
+    )
+    result("test") should containLine("  assign z = x; // @[test]")
+    // newlines currently are supposed to be escaped for both firrtl and Verilog
+    // (alternatively one could emit a multi-line comment)
+    result("test\\nx") should containLine("  assign z = x; // @[test\\nx]")
+    // not sure why, but we are also escaping tabs
+    result("test\\tx") should containLine("  assign z = x; // @[test\\tx]")
+    // escaping closing square brackets is only a firrtl issue, should not be reflected in the Verilog emission
+    result("test\\]") should containLine("  assign z = x; // @[test]]")
+    // while firrtl allows for Unicode in the info field they should be escaped for Verilog
+    result("test \uD83D\uDE0E") should containLine("  assign z = x; // @[test \\uD83D\\uDE0E]")
+
+  }
 }
 
 class VerilogDescriptionEmitterSpec extends FirrtlFlatSpec {