Fixes for Annotation serialized/deserialize (#390)

* Fixes for Annotation serialized/deserialize

Made serializer agree with deserializer on text representation
Re-ordered serializations of Named subclasses to be C or C.m or C.m.c where C=circuit, m=module, c=component
Note: component may contain dots
Added serialize deserialize tests to AnnotationSpec
Did some style cleanup on AnnotationSpec
Added explicit return tupe on SimpleTransformSpec#execute

* Make explicit Util.error
remove commented code

* Make Annotation#serialize a nicer format
fix import there and remove new on case class

* In firrtl Driver.execute use annotations passed in through optionsManager#firrtlOptions if nonEmpty
 otherwise read the annotations in from an annotations file
 Add new option to override this behavior, --force-append-anno-file will append annotations in file
 to any that are passed in
A few other style fixes to Driver: remove new with case classes.  don't use match when if(boolean) will do

* Added tests of malformed component and circuit names

8 files changed, 160 insertions, 84 deletions

diff --git a/src/main/scala/firrtl/Compiler.scala b/src/main/scala/firrtl/Compiler.scala
index 106c973f..09e1df60 100644
--- a/src/main/scala/firrtl/Compiler.scala
+++ b/src/main/scala/firrtl/Compiler.scala
@@ -28,8 +28,8 @@ case class AnnotationMap(annotations: Seq[Annotation]) {
   * @constructor Creates a CircuitState object
   * @param circuit The current state of the Firrtl AST
   * @param form The current form of the circuit
-  * @param annotations The current collection of [[Annotations.Annotation]]
-  * @param renames A map of [[Annotations.Named]] things that have been renamed.
+  * @param annotations The current collection of [[Annotation]]
+  * @param renames A map of [[Named]] things that have been renamed.
   *   Generally only a return value from [[Transform]]s
   */
 case class CircuitState(
diff --git a/src/main/scala/firrtl/Driver.scala b/src/main/scala/firrtl/Driver.scala
index fe484965..75a87789 100644
--- a/src/main/scala/firrtl/Driver.scala
+++ b/src/main/scala/firrtl/Driver.scala
@@ -44,7 +44,7 @@ object Driver {
       compiler: Compiler,
       infoMode: InfoMode = IgnoreInfo,
       customTransforms: Seq[Transform] = Seq.empty,
-      annotations: AnnotationMap = new AnnotationMap(Seq.empty)
+      annotations: AnnotationMap = AnnotationMap(Seq.empty)
   ): String = {
     val parsedInput = Parser.parse(Source.fromFile(input).getLines(), infoMode)
     val outputBuffer = new java.io.CharArrayWriter
@@ -78,18 +78,21 @@ object Driver {
     */
   def loadAnnotations(optionsManager: ExecutionOptionsManager with HasFirrtlOptions): Unit = {
     /*
-     Right now annotations will be looked for always based on the
+     If firrtlAnnotations in the firrtlOptions are nonEmpty then these will be the annotations
+     used by firrtl.
+     To use the file annotations make sure that the annotations in the firrtlOptions are empty
+     The annotation file if needed is found via
      s"$targetDirName/$topName.anno" or s"$annotationFileNameOverride.anno"
-     If found they will be added to the annotations already in the
-     optionsManager.firrtlOptions, duplicates may be created, but this should be ok
     */
     val firrtlConfig = optionsManager.firrtlOptions
-    val annotationFileName = firrtlConfig.getAnnotationFileName(optionsManager)
-    val annotationFile = new File(annotationFileName)
-    if(annotationFile.exists) {
-      val annotationsYaml = io.Source.fromFile(annotationFile).getLines().mkString("\n").parseYaml
-      val annotationArray = annotationsYaml.convertTo[Array[Annotation]]
-      optionsManager.firrtlOptions = firrtlConfig.copy(annotations = firrtlConfig.annotations ++ annotationArray)
+    if(firrtlConfig.annotations.isEmpty) {
+      val annotationFileName = firrtlConfig.getAnnotationFileName(optionsManager)
+      val annotationFile = new File(annotationFileName)
+      if (annotationFile.exists) {
+        val annotationsYaml = io.Source.fromFile(annotationFile).getLines().mkString("\n").parseYaml
+        val annotationArray = annotationsYaml.convertTo[Array[Annotation]]
+        optionsManager.firrtlOptions = firrtlConfig.copy(annotations = firrtlConfig.annotations ++ annotationArray)
+      }
     }
   }
 
@@ -126,7 +129,7 @@ object Driver {
           io.Source.fromFile(inputFileName).getLines()
         }
         catch {
-          case e: FileNotFoundException =>
+          case _: FileNotFoundException =>
             val message = s"Input file $inputFileName not found"
             dramaticError(message)
             return FirrtlExecutionFailure(message)
@@ -138,7 +141,7 @@ object Driver {
     val parsedInput = Parser.parse(firrtlSource, firrtlConfig.infoMode)
     val outputBuffer = new java.io.CharArrayWriter
     firrtlConfig.compiler.compile(
-      CircuitState(parsedInput, ChirrtlForm, Some(new AnnotationMap(firrtlConfig.annotations))),
+      CircuitState(parsedInput, ChirrtlForm, Some(AnnotationMap(firrtlConfig.annotations))),
       outputBuffer,
       firrtlConfig.customTransforms
     )
@@ -162,19 +165,19 @@ object Driver {
   def execute(args: Array[String]): FirrtlExecutionResult = {
     val optionsManager = new ExecutionOptionsManager("firrtl") with HasFirrtlOptions
 
-    optionsManager.parse(args) match {
-      case true =>
-        execute(optionsManager) match {
-          case success: FirrtlExecutionSuccess =>
-            success
-          case failure: FirrtlExecutionFailure =>
-            optionsManager.showUsageAsError()
-            failure
-          case result =>
-            throw new Exception(s"Error: Unknown Firrtl Execution result $result")
-        }
-      case _ =>
-        FirrtlExecutionFailure("Could not parser command line options")
+    if(optionsManager.parse(args)) {
+      execute(optionsManager) match {
+        case success: FirrtlExecutionSuccess =>
+          success
+        case failure: FirrtlExecutionFailure =>
+          optionsManager.showUsageAsError()
+          failure
+        case result =>
+          throw new Exception(s"Error: Unknown Firrtl Execution result $result")
+      }
+    }
+    else {
+      FirrtlExecutionFailure("Could not parser command line options")
     }
   }
 
diff --git a/src/main/scala/firrtl/ExecutionOptionsManager.scala b/src/main/scala/firrtl/ExecutionOptionsManager.scala
index 977ef9bb..ab900a36 100644
--- a/src/main/scala/firrtl/ExecutionOptionsManager.scala
+++ b/src/main/scala/firrtl/ExecutionOptionsManager.scala
@@ -143,10 +143,10 @@ case class FirrtlExecutionOptions(
     firrtlSource:           Option[String] = None,
     customTransforms:       Seq[Transform] = List.empty,
     annotations:            List[Annotation] = List.empty,
-    annotationFileNameOverride: String = "")
+    annotationFileNameOverride: String = "",
+    forceAppendAnnoFile:    Boolean = false)
   extends ComposableOptions {
 
-
   def infoMode: InfoMode = {
     infoModeName match {
       case "use" => UseInfo
@@ -237,6 +237,14 @@ trait HasFirrtlOptions {
     "use this to override the default annotation file name, default is empty"
   }
 
+  parser.opt[Unit]("force-append-anno-file")
+    .abbr("ffaaf")
+    .foreach { _ =>
+      firrtlOptions = firrtlOptions.copy(forceAppendAnnoFile = true)
+    }.text {
+      "use this to force appending annotation file to annotations being passed in through optionsManager"
+    }
+
   parser.opt[String]("compiler")
     .abbr("X")
     .valueName ("<high|low|verilog>")
diff --git a/src/main/scala/firrtl/annotations/Annotation.scala b/src/main/scala/firrtl/annotations/Annotation.scala
index 5881001f..2e361833 100644
--- a/src/main/scala/firrtl/annotations/Annotation.scala
+++ b/src/main/scala/firrtl/annotations/Annotation.scala
@@ -3,19 +3,26 @@
 package firrtl
 package annotations
 
-import firrtl.ir._
-
 case class AnnotationException(message: String) extends Exception(message)
 
 final case class Annotation(target: Named, transform: Class[_ <: Transform], value: String) {
   val targetString: String = target.serialize
   val transformClass: String = transform.getName
-  def serialize: String = this.toString
+
+  /**
+    * This serialize is basically a pretty printer, actual serialization is handled by
+    * AnnotationYamlProtocol
+    * @return a nicer string than the raw case class default
+    */
+  def serialize: String = {
+    s"Annotation(${target.serialize},${transform.getCanonicalName},$value)"
+  }
+
   def update(tos: Seq[Named]): Seq[Annotation] = {
     check(target, tos, this)
     propagate(target, tos, duplicate)
   }
   def propagate(from: Named, tos: Seq[Named], dup: Named=>Annotation): Seq[Annotation] = tos.map(dup(_))
   def check(from: Named, tos: Seq[Named], which: Annotation): Unit = {}
-  def duplicate(n: Named) = new Annotation(n, transform, value)
+  def duplicate(n: Named) = Annotation(n, transform, value)
 }
diff --git a/src/main/scala/firrtl/annotations/AnnotationYamlProtocol.scala b/src/main/scala/firrtl/annotations/AnnotationYamlProtocol.scala
index 5da00282..9018d494 100644
--- a/src/main/scala/firrtl/annotations/AnnotationYamlProtocol.scala
+++ b/src/main/scala/firrtl/annotations/AnnotationYamlProtocol.scala
@@ -8,30 +8,40 @@ import net.jcazevedo.moultingyaml._
 object AnnotationYamlProtocol extends DefaultYamlProtocol {
   // bottom depends on top
   implicit object AnnotationYamlFormat extends YamlFormat[Annotation] {
-    def write(a: Annotation) =
-      YamlArray(
-        YamlString(a.targetString),
-        YamlString(a.transformClass),
-        YamlString(a.value))
+    def write(a: Annotation) = YamlObject(
+      YamlString("targetString") -> YamlString(a.targetString),
+      YamlString("transformClass") -> YamlString(a.transformClass),
+      YamlString("value") -> YamlString(a.value)
+    )
 
-    def read(value: YamlValue) = {
-      value.asYamlObject.getFields(
-        YamlString("targetString"),
-        YamlString("transformClass"),
-        YamlString("value")) match {
-        case Seq(
-          YamlString(targetString),
-          YamlString(transformClass),
-          YamlString(value)) =>
-          new Annotation(toTarget(targetString), Class.forName(transformClass).asInstanceOf[Class[_ <: Transform]], value)
-        case _ => deserializationError("Color expected")
+    def read(yamlValue: YamlValue): Annotation = {
+      try {
+        yamlValue.asYamlObject.getFields(
+          YamlString("targetString"),
+          YamlString("transformClass"),
+          YamlString("value")) match {
+          case Seq(YamlString(targetString), YamlString(transformClass), YamlString(value)) =>
+            Annotation(
+              toTarget(targetString), Class.forName(transformClass).asInstanceOf[Class[_ <: Transform]], value)
+          case _ => deserializationError("Annotation expected")
+        }
+      }
+      catch {
+        case annotationException: AnnotationException =>
+          Utils.error(
+            s"Error: ${annotationException.getMessage} while parsing annotation from yaml\n${yamlValue.prettyPrint}")
+        case annotationException: FIRRTLException =>
+          Utils.error(
+            s"Error: ${annotationException.getMessage} while parsing annotation from yaml\n${yamlValue.prettyPrint}")
       }
     }
-    def toTarget(string: String) = string.split('.').toSeq match {
+    def toTarget(string: String): Named = string.split("""\.""", -1).toSeq match {
       case Seq(c) => CircuitName(c)
       case Seq(c, m) => ModuleName(m, CircuitName(c))
-      case Nil => error("BAD")
-      case s => ComponentName(s.drop(2).mkString("."), ModuleName(s(1), CircuitName(s(0))))
+      case Nil => Utils.error("BAD")
+      case s =>
+        val componentString = s.drop(2).mkString(".")
+        ComponentName(componentString, ModuleName(s.tail.head, CircuitName(s.head)))
     }
   }
 }
diff --git a/src/main/scala/firrtl/annotations/Named.scala b/src/main/scala/firrtl/annotations/Named.scala
index e9b89e75..4b39c977 100644
--- a/src/main/scala/firrtl/annotations/Named.scala
+++ b/src/main/scala/firrtl/annotations/Named.scala
@@ -21,11 +21,11 @@ final case class CircuitName(name: String) extends Named {
 
 final case class ModuleName(name: String, circuit: CircuitName) extends Named {
   if(!validModuleName(name)) throw AnnotationException(s"Illegal module name: $name")
-  def serialize: String = name + "." + circuit.serialize
+  def serialize: String = circuit.serialize + "." + name
 }
 
 final case class ComponentName(name: String, module: ModuleName) extends Named {
   if(!validComponentName(name)) throw AnnotationException(s"Illegal component name: $name")
   def expr: Expression = toExp(name)
-  def serialize: String = name + "." + module.serialize
+  def serialize: String = module.serialize + "." + name
 }
diff --git a/src/test/scala/firrtlTests/AnnotationTests.scala b/src/test/scala/firrtlTests/AnnotationTests.scala
index 9ffbbd85..7e1a35dc 100644
--- a/src/test/scala/firrtlTests/AnnotationTests.scala
+++ b/src/test/scala/firrtlTests/AnnotationTests.scala
@@ -2,32 +2,15 @@
 
 package firrtlTests
 
-import java.io.{Writer, StringWriter}
+import java.io.{File, FileWriter, StringWriter, Writer}
 
-import org.scalatest.FlatSpec
+import firrtl.annotations.AnnotationYamlProtocol._
+import firrtl.annotations._
+import firrtl._
+import firrtl.passes.InlineAnnotation
+import firrtl.passes.memlib.PinAnnotation
+import net.jcazevedo.moultingyaml._
 import org.scalatest.Matchers
-import org.scalatest.junit.JUnitRunner
-
-import firrtl.ir.Circuit
-import firrtl.{Parser, AnnotationMap}
-import firrtl.{
-   CircuitState,
-   ResolveAndCheck,
-   RenameMap,
-   Compiler,
-   ChirrtlForm,
-   LowForm,
-   VerilogCompiler,
-   Transform
-}
-import firrtl.annotations.{
-   Named,
-   CircuitName,
-   ModuleName,
-   ComponentName,
-   AnnotationException,
-   Annotation
-}
 
 /**
  * An example methodology for testing Firrtl annotations.
@@ -37,14 +20,14 @@ trait AnnotationSpec extends LowTransformSpec {
   def transform = new CustomResolveAndCheck(LowForm)
 
   // Check if Annotation Exception is thrown
-  override def failingexecute(writer: Writer, annotations: AnnotationMap, input: String) = {
+  override def failingexecute(writer: Writer, annotations: AnnotationMap, input: String): Exception = {
     intercept[AnnotationException] {
       compile(CircuitState(parse(input), ChirrtlForm, Some(annotations)), writer)
     }
   }
-  def execute(writer: Writer, annotations: AnnotationMap, input: String, check: Annotation) = {
+  def execute(writer: Writer, annotations: AnnotationMap, input: String, check: Annotation): Unit = {
     val cr = compile(CircuitState(parse(input), ChirrtlForm, Some(annotations)), writer)
-    (cr.annotations.get.annotations) should be (Seq(check))
+    cr.annotations.get.annotations should be (Seq(check))
   }
 }
 
@@ -57,8 +40,8 @@ trait AnnotationSpec extends LowTransformSpec {
  * Unstable, Fickle, and Insistent can be tested.
  */
 class AnnotationTests extends AnnotationSpec with Matchers {
-  def getAMap (a: Annotation): AnnotationMap = new AnnotationMap(Seq(a))
-  val input =
+  def getAMap (a: Annotation): AnnotationMap = AnnotationMap(Seq(a))
+  val input: String =
     """circuit Top :
        |  module Top :
        |    input a : UInt<1>[2]
@@ -74,4 +57,69 @@ class AnnotationTests extends AnnotationSpec with Matchers {
     val ta = Annotation(cName, classOf[Transform], "")
     execute(w, getAMap(ta), input, ta)
   }
+
+  "Annotations" should "be readable from file" in {
+    val annotationFile = new File("src/test/resources/annotations/SampleAnnotations.anno")
+    val annotationsYaml = io.Source.fromFile(annotationFile).getLines().mkString("\n").parseYaml
+    val annotationArray = annotationsYaml.convertTo[Array[Annotation]]
+    annotationArray.length should be (9)
+    annotationArray(0).targetString should be ("ModC")
+    annotationArray(7).transformClass should be ("firrtl.passes.InlineInstances")
+    val expectedValue = "TopOfDiamond\nWith\nSome new lines"
+    annotationArray(7).value should be (expectedValue)
+  }
+
+  "Badly formatted serializations" should "return reasonable error messages" in {
+    var badYaml =
+      """
+        |- transformClass: firrtl.passes.InlineInstances
+        |  targetString: circuit.module..
+        |  value: ModC.this params 16 32
+      """.stripMargin.parseYaml
+
+    var thrown = intercept[Exception] {
+      badYaml.convertTo[Array[Annotation]]
+    }
+    thrown.getMessage should include ("Illegal component name")
+
+    badYaml =
+      """
+        |- transformClass: firrtl.passes.InlineInstances
+        |  targetString: .circuit.module.component
+        |  value: ModC.this params 16 32
+      """.stripMargin.parseYaml
+
+    thrown = intercept[Exception] {
+      badYaml.convertTo[Array[Annotation]]
+    }
+    thrown.getMessage should include ("Illegal circuit name")
+  }
+
+  "Round tripping annotations through text file" should "preserve annotations" in {
+    val annos: Array[Annotation] = Seq(
+      InlineAnnotation(CircuitName("fox")),
+      InlineAnnotation(ModuleName("dog", CircuitName("bear"))),
+      InlineAnnotation(ComponentName("chocolate", ModuleName("like", CircuitName("i")))),
+      PinAnnotation(CircuitName("Pinniped"), Seq("sea-lion", "monk-seal"))
+    ).toArray
+
+    val annoFile = new File("temp-anno")
+    val writer = new FileWriter(annoFile)
+    writer.write(annos.toYaml.prettyPrint)
+    writer.close()
+
+    val yaml = io.Source.fromFile(annoFile).getLines().mkString("\n").parseYaml
+    annoFile.delete()
+
+    val readAnnos = yaml.convertTo[Array[Annotation]]
+
+    annos.zip(readAnnos).foreach { case (beforeAnno, afterAnno) =>
+      beforeAnno.targetString should be (afterAnno.targetString)
+      beforeAnno.target should be (afterAnno.target)
+      beforeAnno.transformClass should be (afterAnno.transformClass)
+      beforeAnno.transform should be (afterAnno.transform)
+
+      beforeAnno should be (afterAnno)
+    }
+  }
 }
diff --git a/src/test/scala/firrtlTests/PassTests.scala b/src/test/scala/firrtlTests/PassTests.scala
index 44bd0382..c417c4fb 100644
--- a/src/test/scala/firrtlTests/PassTests.scala
+++ b/src/test/scala/firrtlTests/PassTests.scala
@@ -41,7 +41,7 @@ abstract class SimpleTransformSpec extends FlatSpec with Matchers with Compiler
    def squash(c: Circuit): Circuit = RemoveEmpty.run(c)
 
    // Executes the test. Call in tests.
-   def execute(writer: Writer, annotations: AnnotationMap, input: String, check: String) = {
+   def execute(writer: Writer, annotations: AnnotationMap, input: String, check: String): Unit = {
       compile(CircuitState(parse(input), ChirrtlForm, Some(annotations)), writer)
       val actual = RemoveEmpty.run(parse(writer.toString)).serialize
       val expected = parse(check).serialize