From 6cf4a175bfae5ff6eb3cd8fcdd1e5f69cda5d92d Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Tue, 13 Sep 2022 11:49:08 -0400 Subject: 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.--- src/main/scala/firrtl/Parser.scala | 19 +++++++++- src/main/scala/firrtl/Utils.scala | 2 +- src/test/scala/firrtlTests/ParserSpec.scala | 59 ++++++++++++++++++----------- 3 files changed, 55 insertions(+), 25 deletions(-) (limited to 'src') 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 { -- cgit v1.2.3