Deprecate CompilerAnnotation (#1870)

* CompilerAnnotation$ emits RunFirrtlTransform

Change the CompilerAnnotation object to emit
RunFirrtlTransformAnnotations containing the associated emitter.

This requires a fix in the Driver compatibility layer to know how to
enable one-file-per module emission if either a CompilerAnnotation or
a RunFirrtlTransformAnnotation(_: Emitter) is present.

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

* Add ConvertCompilerAnnotation phase

Add a phase, ConvertCompilerAnnotation, that converts a
CompilerAnnotation to a RunFirrtlTransformAnnotation. This provides a
warning to the user if this path is taken.

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

* Add test of ConvertCompilerAnnotation

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

* Deprecate CompilerAnnotation$, move helper methods

Deprecate the CompilerAnnotation companion object and move it's
private utility inside the RunFirrtlTransformAnnotation companion
object.

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

* Make ConvertCompilerAnnotations private[firrtl]

Make this phase private to avoid adding a deprecation warning. Also,
remove an unused string value.

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

* Fix incorrect string in test

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

* Add test that '-X verilog', no emitter yields file

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

13 files changed, 143 insertions, 43 deletions

diff --git a/src/main/scala/firrtl/ExecutionOptionsManager.scala b/src/main/scala/firrtl/ExecutionOptionsManager.scala
index 50fb30a6..e673a5af 100644
--- a/src/main/scala/firrtl/ExecutionOptionsManager.scala
+++ b/src/main/scala/firrtl/ExecutionOptionsManager.scala
@@ -343,7 +343,7 @@ case class FirrtlExecutionOptions(
     List() ++ (if (inputFileNameOverride.nonEmpty) Seq(FirrtlFileAnnotation(inputFileNameOverride)) else Seq()) ++
       (if (outputFileNameOverride.nonEmpty) { Some(OutputFileAnnotation(outputFileNameOverride)) }
        else { None }) ++
-      Some(CompilerAnnotation(compilerName)) ++
+      Some(RunFirrtlTransformAnnotation.stringToEmitter(compilerName)) ++
       Some(InfoModeAnnotation(infoModeName)) ++
       firrtlSource.map(FirrtlSourceAnnotation(_)) ++
       customTransforms.map(t => RunFirrtlTransformAnnotation(t)) ++
diff --git a/src/main/scala/firrtl/stage/FirrtlAnnotations.scala b/src/main/scala/firrtl/stage/FirrtlAnnotations.scala
index d37d2881..3390e632 100644
--- a/src/main/scala/firrtl/stage/FirrtlAnnotations.scala
+++ b/src/main/scala/firrtl/stage/FirrtlAnnotations.scala
@@ -152,28 +152,16 @@ object FirrtlSourceAnnotation extends HasShellOptions {
   *  - If unset, a [[CompilerAnnotation]] with the default [[VerilogCompiler]]
   * @param compiler compiler name
   */
+@deprecated("Use a RunFirrtlTransformAnnotation targeting a specific Emitter.", "FIRRTL 1.4.0")
 case class CompilerAnnotation(compiler: Compiler = new VerilogCompiler()) extends NoTargetAnnotation with FirrtlOption
 
+@deprecated("Use a RunFirrtlTransformAnnotation targeting a specific Emitter.", "FIRRTL 1.4.0")
 object CompilerAnnotation extends HasShellOptions {
 
-  private[firrtl] def apply(compilerName: String): CompilerAnnotation = {
-    val c = compilerName match {
-      case "none"     => new NoneCompiler()
-      case "high"     => new HighFirrtlCompiler()
-      case "low"      => new LowFirrtlCompiler()
-      case "middle"   => new MiddleFirrtlCompiler()
-      case "verilog"  => new VerilogCompiler()
-      case "mverilog" => new MinimumVerilogCompiler()
-      case "sverilog" => new SystemVerilogCompiler()
-      case _          => throw new OptionsException(s"Unknown compiler name '$compilerName'! (Did you misspell it?)")
-    }
-    CompilerAnnotation(c)
-  }
-
   val options = Seq(
     new ShellOption[String](
       longOption = "compiler",
-      toAnnotationSeq = a => Seq(CompilerAnnotation(a)),
+      toAnnotationSeq = a => Seq(RunFirrtlTransformAnnotation.stringToEmitter(a)),
       helpText = "The FIRRTL compiler to use (default: verilog)",
       shortOption = Some("X"),
       helpValueName = Some("<none|high|middle|low|verilog|mverilog|sverilog>")
@@ -194,6 +182,20 @@ object RunFirrtlTransformAnnotation extends HasShellOptions {
   def apply(transform: TransformDependency): RunFirrtlTransformAnnotation =
     RunFirrtlTransformAnnotation(transform.getObject)
 
+  private[firrtl] def stringToEmitter(a: String): RunFirrtlTransformAnnotation = {
+    val emitter = a match {
+      case "none"     => new ChirrtlEmitter
+      case "high"     => new HighFirrtlEmitter
+      case "low"      => new LowFirrtlEmitter
+      case "middle"   => new MiddleFirrtlEmitter
+      case "verilog"  => new VerilogEmitter
+      case "mverilog" => new MinimumVerilogEmitter
+      case "sverilog" => new SystemVerilogEmitter
+      case _          => throw new OptionsException(s"Unknown compiler name '$a'! (Did you misspell it?)")
+    }
+    RunFirrtlTransformAnnotation(emitter)
+  }
+
   val options = Seq(
     new ShellOption[Seq[String]](
       longOption = "custom-transforms",
@@ -225,6 +227,13 @@ object RunFirrtlTransformAnnotation extends HasShellOptions {
       },
       helpText = "Convert all FIRRTL names to a specific case",
       helpValueName = Some("<lower|upper>")
+    ),
+    new ShellOption[String](
+      longOption = "compiler",
+      toAnnotationSeq = a => Seq(stringToEmitter(a)),
+      helpText = "The FIRRTL compiler to use (default: verilog)",
+      shortOption = Some("X"),
+      helpValueName = Some("<none|high|middle|low|verilog|mverilog|sverilog>")
     )
   )
 
diff --git a/src/main/scala/firrtl/stage/FirrtlCli.scala b/src/main/scala/firrtl/stage/FirrtlCli.scala
index fb5aa09f..c33980dd 100644
--- a/src/main/scala/firrtl/stage/FirrtlCli.scala
+++ b/src/main/scala/firrtl/stage/FirrtlCli.scala
@@ -16,7 +16,6 @@ trait FirrtlCli { this: Shell =>
     OutputFileAnnotation,
     InfoModeAnnotation,
     FirrtlSourceAnnotation,
-    CompilerAnnotation,
     RunFirrtlTransformAnnotation,
     firrtl.EmitCircuitAnnotation,
     firrtl.EmitAllModulesAnnotation,
diff --git a/src/main/scala/firrtl/stage/FirrtlStage.scala b/src/main/scala/firrtl/stage/FirrtlStage.scala
index 58d07e43..b92fc4ef 100644
--- a/src/main/scala/firrtl/stage/FirrtlStage.scala
+++ b/src/main/scala/firrtl/stage/FirrtlStage.scala
@@ -7,7 +7,13 @@ import firrtl.options.{Dependency, Phase, PhaseManager, Shell, Stage, StageMain}
 import firrtl.options.phases.DeletedWrapper
 import firrtl.stage.phases.CatchExceptions
 
-class FirrtlPhase extends PhaseManager(targets = Seq(Dependency[firrtl.stage.phases.Compiler])) {
+class FirrtlPhase
+    extends PhaseManager(
+      targets = Seq(
+        Dependency[firrtl.stage.phases.Compiler],
+        Dependency[firrtl.stage.phases.ConvertCompilerAnnotations]
+      )
+    ) {
 
   override def invalidates(a: Phase) = false
 
diff --git a/src/main/scala/firrtl/stage/phases/AddDefaults.scala b/src/main/scala/firrtl/stage/phases/AddDefaults.scala
index 9f4163cc..b296b59d 100644
--- a/src/main/scala/firrtl/stage/phases/AddDefaults.scala
+++ b/src/main/scala/firrtl/stage/phases/AddDefaults.scala
@@ -6,7 +6,7 @@ import firrtl.{AnnotationSeq, VerilogEmitter}
 import firrtl.options.{Dependency, Phase, TargetDirAnnotation}
 import firrtl.stage.TransformManager.TransformDependency
 import firrtl.transforms.BlackBoxTargetDirAnno
-import firrtl.stage.{CompilerAnnotation, FirrtlOptions, InfoModeAnnotation, RunFirrtlTransformAnnotation}
+import firrtl.stage.{FirrtlOptions, InfoModeAnnotation, RunFirrtlTransformAnnotation}
 
 /** [[firrtl.options.Phase Phase]] that adds default [[FirrtlOption]] [[firrtl.annotations.Annotation Annotation]]s.
   * This is a part of the preprocessing done by [[FirrtlStage]].
@@ -23,10 +23,9 @@ class AddDefaults extends Phase {
 
   /** Append any missing default annotations to an annotation sequence */
   def transform(annotations: AnnotationSeq): AnnotationSeq = {
-    var bb, c, em, im = true
+    var bb, em, im = true
     annotations.foreach {
       case _: BlackBoxTargetDirAnno => bb = false
-      case _: CompilerAnnotation    => c = false
       case _: InfoModeAnnotation    => im = false
       case RunFirrtlTransformAnnotation(_: firrtl.Emitter) => em = false
       case _ =>
@@ -39,7 +38,7 @@ class AddDefaults extends Phase {
 
     (if (bb) Seq(BlackBoxTargetDirAnno(targetDir)) else Seq()) ++
       // if there is no compiler or emitter specified, add the default emitter
-      (if (c && em) Seq(RunFirrtlTransformAnnotation(DefaultEmitterTarget)) else Seq()) ++
+      (if (em) Seq(RunFirrtlTransformAnnotation(DefaultEmitterTarget)) else Seq()) ++
       (if (im) Seq(InfoModeAnnotation()) else Seq()) ++
       annotations
   }
diff --git a/src/main/scala/firrtl/stage/phases/Checks.scala b/src/main/scala/firrtl/stage/phases/Checks.scala
index 24cfa0a3..44828ae6 100644
--- a/src/main/scala/firrtl/stage/phases/Checks.scala
+++ b/src/main/scala/firrtl/stage/phases/Checks.scala
@@ -31,14 +31,13 @@ class Checks extends Phase {
     * @throws firrtl.options.OptionsException if any checks fail
     */
   def transform(annos: AnnotationSeq): AnnotationSeq = {
-    val inF, inS, eam, ec, outF, comp, emitter, im, inC = collection.mutable.ListBuffer[Annotation]()
+    val inF, inS, eam, ec, outF, emitter, im, inC = collection.mutable.ListBuffer[Annotation]()
     annos.foreach(_ match {
       case a: FirrtlFileAnnotation     => a +=: inF
       case a: FirrtlSourceAnnotation   => a +=: inS
       case a: EmitAllModulesAnnotation => a +=: eam
       case a: EmitCircuitAnnotation    => a +=: ec
       case a: OutputFileAnnotation     => a +=: outF
-      case a: CompilerAnnotation       => a +=: comp
       case a: InfoModeAnnotation       => a +=: im
       case a: FirrtlCircuitAnnotation  => a +=: inC
       case a @ RunFirrtlTransformAnnotation(_: firrtl.Emitter) => a +=: emitter
@@ -81,13 +80,13 @@ class Checks extends Phase {
       )
     }
 
-    /* One mandatory compiler (or emitter) must be specified */
-    if (comp.size != 1 && emitter.isEmpty) {
-      val x = comp.map { case CompilerAnnotation(x) => x }
-      val (msg, suggest) = if (comp.size == 0) { ("none found", "forget one of") }
-      else { (s"""found '${x.mkString(", ")}'""", "use multiple of") }
-      throw new OptionsException(s"""|Exactly one compiler must be specified, but $msg. Did you $suggest the following?
-                                     |    - an option or annotation: -X, --compiler, CompilerAnnotation""".stripMargin)
+    /* At least one emitter must be specified */
+    if (emitter.isEmpty) {
+      throw new OptionsException(
+        s"""|At least one compiler must be specified, but none found. Specify a compiler via:
+            |    - a RunFirrtlTransformAnnotation targeting a specific emitter, e.g., VerilogEmitter
+            |    - a command line option: -X, --compiler""".stripMargin
+      )
     }
 
     /* One mandatory info mode must be specified */
diff --git a/src/main/scala/firrtl/stage/phases/ConvertCompilerAnnotations.scala b/src/main/scala/firrtl/stage/phases/ConvertCompilerAnnotations.scala
new file mode 100644
index 00000000..b4743cde
--- /dev/null
+++ b/src/main/scala/firrtl/stage/phases/ConvertCompilerAnnotations.scala
@@ -0,0 +1,35 @@
+// See LICENSE for license details.
+
+package firrtl.stage.phases
+
+import firrtl.AnnotationSeq
+import firrtl.options.{Dependency, OptionsException, Phase}
+import firrtl.stage.{CompilerAnnotation, RunFirrtlTransformAnnotation}
+
+private[firrtl] class ConvertCompilerAnnotations extends Phase {
+
+  override def prerequisites = Seq.empty
+  override def optionalPrerequisites = Seq.empty
+  override def optionalPrerequisiteOf = Seq(Dependency[AddDefaults], Dependency[Checks])
+  override def invalidates(a: Phase) = false
+
+  override def transform(annotations: AnnotationSeq): AnnotationSeq = {
+    annotations.collect {
+      case a: CompilerAnnotation => a
+    } match {
+      case a if a.size > 1 =>
+        throw new OptionsException(
+          s"Zero or one CompilerAnnotation may be specified, but found '${a.mkString(", ")}'.".stripMargin
+        )
+      case _ =>
+    }
+    annotations.map {
+      case CompilerAnnotation(a) =>
+        val suggestion = s"RunFirrtlTransformAnnotation(new ${a.emitter.getClass.getName})"
+        logger.warn(s"CompilerAnnotation is deprecated since FIRRTL 1.4.0. Please use '$suggestion' instead.")
+        RunFirrtlTransformAnnotation(a.emitter)
+      case a => a
+    }
+  }
+
+}
diff --git a/src/main/scala/firrtl/stage/phases/DriverCompatibility.scala b/src/main/scala/firrtl/stage/phases/DriverCompatibility.scala
index 0b558cc0..529f9ca3 100644
--- a/src/main/scala/firrtl/stage/phases/DriverCompatibility.scala
+++ b/src/main/scala/firrtl/stage/phases/DriverCompatibility.scala
@@ -4,7 +4,7 @@ package firrtl.stage.phases
 
 import firrtl.stage._
 
-import firrtl.{AnnotationSeq, EmitAllModulesAnnotation, EmitCircuitAnnotation, FirrtlExecutionResult, Parser}
+import firrtl.{AnnotationSeq, EmitAllModulesAnnotation, EmitCircuitAnnotation, Emitter, FirrtlExecutionResult, Parser}
 import firrtl.annotations.NoTargetAnnotation
 import firrtl.FileUtils
 import firrtl.proto.FromProto
@@ -227,6 +227,9 @@ object DriverCompatibility {
           val b = RunFirrtlTransformAnnotation(a.compiler.emitter)
           if (splitModules) { Seq(a, b, EmitAllModulesAnnotation(c.emitter.getClass)) }
           else { Seq(a, b, EmitCircuitAnnotation(c.emitter.getClass)) }
+        case a @ RunFirrtlTransformAnnotation(e: Emitter) =>
+          if (splitModules) { Seq(a, EmitAllModulesAnnotation(e.getClass)) }
+          else { Seq(a, EmitCircuitAnnotation(e.getClass)) }
         case a => Seq(a)
       }
     }
diff --git a/src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala b/src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala
new file mode 100644
index 00000000..acd37bf5
--- /dev/null
+++ b/src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala
@@ -0,0 +1,40 @@
+// See LICENSE for license details.
+
+package firrtl.stage.phases.tests
+
+import firrtl.{HighFirrtlCompiler, HighFirrtlEmitter, LowFirrtlCompiler}
+import firrtl.options.{Dependency, OptionsException}
+import firrtl.stage.{CompilerAnnotation, RunFirrtlTransformAnnotation}
+import firrtl.stage.phases.ConvertCompilerAnnotations
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+class ConvertCompilerAnnotationsSpec extends AnyFlatSpec with Matchers {
+
+  class Fixture {
+    val phase = new ConvertCompilerAnnotations
+  }
+
+  behavior.of(classOf[ConvertCompilerAnnotations].getName)
+
+  it should "convert a deprecated CompilerAnnotation to a RunFirrtlTransformAnnotation" in new Fixture {
+    val annotations = Seq(CompilerAnnotation(new HighFirrtlCompiler))
+    phase
+      .transform(annotations)
+      .map {
+        case RunFirrtlTransformAnnotation(a) => Dependency.fromTransform(a)
+      }
+      .toSeq should be(Seq(Dependency[HighFirrtlEmitter]))
+  }
+
+  it should "throw an exception if multiple CompilerAnnotations are specified" in new Fixture {
+    val annotations = Seq(
+      CompilerAnnotation(new HighFirrtlCompiler),
+      CompilerAnnotation(new LowFirrtlCompiler)
+    )
+    intercept[OptionsException] {
+      phase.transform(annotations)
+    }.getMessage should include("Zero or one CompilerAnnotation may be specified")
+  }
+}
diff --git a/src/test/scala/firrtl/testutils/FirrtlSpec.scala b/src/test/scala/firrtl/testutils/FirrtlSpec.scala
index a0c41085..9ee5fdaf 100644
--- a/src/test/scala/firrtl/testutils/FirrtlSpec.scala
+++ b/src/test/scala/firrtl/testutils/FirrtlSpec.scala
@@ -101,7 +101,7 @@ trait FirrtlRunners extends BackendCompilationUtilities {
         InfoModeAnnotation("ignore") +:
         RenameTopAnnotation(topName) +:
         stage.FirrtlCircuitAnnotation(circuit) +:
-        stage.CompilerAnnotation("mverilog") +:
+        stage.RunFirrtlTransformAnnotation.stringToEmitter("mverilog") +:
         stage.OutputFileAnnotation(topName) +:
         toAnnos(baseTransforms)
     }
diff --git a/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala b/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala
index 9274bac6..ee942d85 100644
--- a/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala
+++ b/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala
@@ -286,6 +286,12 @@ class FirrtlMainSpec
         args = Array("-X", "sverilog", "-E", "sverilog", "-o", "Foo"),
         stdout = defaultStdOut,
         files = Seq("Foo.sv")
+      ),
+      /* Test that an output is generated if no emitter is specified */
+      FirrtlMainTest(
+        args = Array("-X", "verilog", "-o", "Foo"),
+        stdout = defaultStdOut,
+        files = Seq("Foo.v")
       )
     )
       .foreach(runStageExpectFiles)
diff --git a/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala b/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala
index 686c42ad..eca7b706 100644
--- a/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala
+++ b/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala
@@ -2,11 +2,11 @@
 
 package firrtlTests.stage.phases
 
-import firrtl.NoneCompiler
+import firrtl.ChirrtlEmitter
 import firrtl.annotations.Annotation
 import firrtl.stage.phases.AddDefaults
 import firrtl.transforms.BlackBoxTargetDirAnno
-import firrtl.stage.{CompilerAnnotation, InfoModeAnnotation, RunFirrtlTransformAnnotation}
+import firrtl.stage.{InfoModeAnnotation, RunFirrtlTransformAnnotation}
 import firrtl.options.{Dependency, Phase, TargetDirAnnotation}
 import org.scalatest.flatspec.AnyFlatSpec
 import org.scalatest.matchers.should.Matchers
@@ -32,7 +32,11 @@ class AddDefaultsSpec extends AnyFlatSpec with Matchers {
   }
 
   it should "not overwrite existing annotations" in new Fixture {
-    val input = Seq(BlackBoxTargetDirAnno("foo"), CompilerAnnotation(new NoneCompiler()), InfoModeAnnotation("ignore"))
+    val input = Seq(
+      BlackBoxTargetDirAnno("foo"),
+      RunFirrtlTransformAnnotation(new ChirrtlEmitter),
+      InfoModeAnnotation("ignore")
+    )
 
     phase.transform(input).toSeq should be(input)
   }
diff --git a/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala b/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala
index 65516da5..6c0335b9 100644
--- a/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala
+++ b/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala
@@ -4,7 +4,7 @@ package firrtlTests.stage.phases
 
 import firrtl.stage._
 
-import firrtl.{AnnotationSeq, ChirrtlEmitter, EmitAllModulesAnnotation, NoneCompiler}
+import firrtl.{AnnotationSeq, ChirrtlEmitter, EmitAllModulesAnnotation}
 import firrtl.options.{OptionsException, OutputAnnotationFileAnnotation, Phase}
 import firrtl.stage.phases.Checks
 import org.scalatest.flatspec.AnyFlatSpec
@@ -18,7 +18,7 @@ class ChecksSpec extends AnyFlatSpec with Matchers {
   val outputFile = OutputFileAnnotation("bar")
   val emitAllModules = EmitAllModulesAnnotation(classOf[ChirrtlEmitter])
   val outputAnnotationFile = OutputAnnotationFileAnnotation("baz")
-  val goodCompiler = CompilerAnnotation(new NoneCompiler())
+  val goodCompiler = RunFirrtlTransformAnnotation(new ChirrtlEmitter)
   val infoMode = InfoModeAnnotation("ignore")
 
   val min = Seq(inputFile, goodCompiler, infoMode)
@@ -54,15 +54,15 @@ class ChecksSpec extends AnyFlatSpec with Matchers {
     checkExceptionMessage(phase, in, "No more than one output file can be specified")
   }
 
-  it should "enforce exactly one compiler" in new Fixture {
+  it should "enforce one or more compilers (at this point these are emitters)" in new Fixture {
     info("0 compilers should throw an exception")
     val inZero = Seq(inputFile, infoMode)
-    checkExceptionMessage(phase, inZero, "Exactly one compiler must be specified, but none found")
+    checkExceptionMessage(phase, inZero, "At least one compiler must be specified")
 
-    info("2 compilers should throw an exception")
-    val c = goodCompiler.compiler
+    info("2 compilers should not throw an exception")
+    val c = goodCompiler
     val inTwo = min :+ goodCompiler
-    checkExceptionMessage(phase, inTwo, s"Exactly one compiler must be specified, but found '$c, $c'")
+    phase.transform(inTwo)
   }
 
   it should "validate info mode names" in new Fixture {