Make the Parser handle errors more gracefully (#2549)

Ever since introducing the Listener, the firrtl Parser now can hit
errors in the code converting from concrete syntax to abstract syntax
that may be due to syntax errors. These errors are essentially broken
assumptions about the structure of the parsed code because there is an
error. These errors are reported before the standard ANTLR syntax errors
are aggregated and reported, and thus could result in less than elegant
error messages (eg. NullPointerException). Now, the Parser will mask off
such errors in the event of standard syntax errors caught by the
ANTLR-generated parser.

This commit also cleans up some ParserSpec tests slightly to make the
ScalaTest style more canonical.

3 files changed, 55 insertions, 25 deletions

diff --git a/src/main/scala/firrtl/Parser.scala b/src/main/scala/firrtl/Parser.scala
index b2d97e86..a4c366ec 100644
--- a/src/main/scala/firrtl/Parser.scala
+++ b/src/main/scala/firrtl/Parser.scala
@@ -10,6 +10,8 @@ import firrtl.parser.Listener
 import firrtl.Utils.time
 import firrtl.antlr.{FIRRTLParser, _}
 
+import scala.util.control.NonFatal
+
 class ParserException(message: String) extends FirrtlUserException(message)
 
 case class ParameterNotSpecifiedException(message: String) extends ParserException(message)
@@ -42,12 +44,25 @@ object Parser extends LazyLogging {
       parser.getInterpreter.setPredictionMode(PredictionMode.SLL)
       parser.addParseListener(listener)
 
-      // Concrete Syntax Tree
-      parser.circuit
+      // Syntax errors may violate assumptions in the Listener and Visitor.
+      // We need to handle these errors gracefully.
+      val throwable =
+        try {
+          parser.circuit
+          None
+        } catch {
+          case e: ParserException => throw e
+          case NonFatal(e) => Some(e)
+        }
 
       val numSyntaxErrors = parser.getNumberOfSyntaxErrors
       if (numSyntaxErrors > 0) throw new SyntaxErrorsException(s"$numSyntaxErrors syntax error(s) detected")
 
+      // Note that this should never happen because any throwables caught should be due to syntax
+      // errors that are reported above. This is just to ensure that we don't accidentally mask any
+      // bugs in the Parser, Listener, or Visitor.
+      if (throwable.nonEmpty) Utils.throwInternalError(exception = throwable)
+
       listener.getCircuit
     }
 
diff --git a/src/main/scala/firrtl/Utils.scala b/src/main/scala/firrtl/Utils.scala
index ec68d4eb..9e267a39 100644
--- a/src/main/scala/firrtl/Utils.scala
+++ b/src/main/scala/firrtl/Utils.scala
@@ -165,7 +165,7 @@ object Utils extends LazyLogging {
     * @param message - possible string to emit,
     * @param exception - possible exception triggering the error.
     */
-  def throwInternalError(message: String = "", exception: Option[Exception] = None) = {
+  def throwInternalError(message: String = "", exception: Option[Throwable] = None) = {
     // We'll get the first exception in the chain, keeping it intact.
     val first = true
     val throwable = getThrowable(exception, true)
diff --git a/src/test/scala/firrtlTests/ParserSpec.scala b/src/test/scala/firrtlTests/ParserSpec.scala
index bb7c2ab2..5003871d 100644
--- a/src/test/scala/firrtlTests/ParserSpec.scala
+++ b/src/test/scala/firrtlTests/ParserSpec.scala
@@ -129,29 +129,33 @@ class ParserSpec extends FirrtlFlatSpec {
     firrtl.Parser.parse(c.serialize)
   }
 
-  an[UnsupportedVersionException] should be thrownBy {
-    val input = """
-                  |FIRRTL version 1.2.0
-                  |circuit Test :
-                  |  module Test :
-                  |    input in : UInt<1>
-                  |    in <= UInt(0)
-      """.stripMargin
-    firrtl.Parser.parse(input)
+  "Version 1.2.0" should "be rejected" in {
+    an[UnsupportedVersionException] should be thrownBy {
+      val input = """
+                    |FIRRTL version 1.2.0
+                    |circuit Test :
+                    |  module Test :
+                    |    input in : UInt<1>
+                    |    in <= UInt(0)
+        """.stripMargin
+      firrtl.Parser.parse(input)
+    }
   }
 
-  an[UnsupportedVersionException] should be thrownBy {
-    val input = """
-                  |FIRRTL version 2.0.0
-                  |crcuit Test :
-                  |  module Test @@#!# :
-                  |    input in1 : UInt<2>
-                  |    input in2 : UInt<3>
-                  |    output out : UInt<4>
-                  |    out[1:0] <= in1
-                  |    out[3:2] <= in2[1:0]
-      """.stripMargin
-    firrtl.Parser.parse(input)
+  "Version 2.0.0" should "be rejected" in {
+    an[UnsupportedVersionException] should be thrownBy {
+      val input = """
+                    |FIRRTL version 2.0.0
+                    |crcuit Test :
+                    |  module Test @@#!# :
+                    |    input in1 : UInt<2>
+                    |    input in2 : UInt<3>
+                    |    output out : UInt<4>
+                    |    out[1:0] <= in1
+                    |    out[3:2] <= in2[1:0]
+        """.stripMargin
+      firrtl.Parser.parse(input)
+    }
   }
 
   // ********** Memories **********
@@ -379,7 +383,7 @@ class ParserSpec extends FirrtlFlatSpec {
     }
   }
 
-  it should "be able to parse a MultiInfo as a FileInfo" in {
+  "The Parser" 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 =
@@ -390,6 +394,17 @@ class ParserSpec extends FirrtlFlatSpec {
     val c = firrtl.Parser.parse(input)
     assert(c.info == ir.FileInfo("a b c"))
   }
+
+  it should "handle parse errors gracefully" in {
+    val input =
+      s"""circuit test :
+         |  module test :
+         |    output out :
+         |""".stripMargin
+    a[SyntaxErrorsException] should be thrownBy {
+      firrtl.Parser.parse(input)
+    }
+  }
 }
 
 class ParserPropSpec extends FirrtlPropSpec {