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.

2 files changed, 18 insertions, 3 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)