Add support for bundle fields to start with digits (#462)

Also remove parsing support for ids with characters not supported in
Verilog nor in the Firrtl spec

3 files changed, 134 insertions, 68 deletions

diff --git a/src/main/antlr4/FIRRTL.g4 b/src/main/antlr4/FIRRTL.g4
index 9e6a46ee..47802cd6 100644
--- a/src/main/antlr4/FIRRTL.g4
+++ b/src/main/antlr4/FIRRTL.g4
@@ -52,17 +52,17 @@ dir
   ;
 
 type 
-  : 'UInt' ('<' IntLit '>')?
-  | 'SInt' ('<' IntLit '>')?
-  | 'Fixed' ('<' IntLit '>')? ('<' '<' IntLit '>' '>')?
+  : 'UInt' ('<' intLit '>')?
+  | 'SInt' ('<' intLit '>')?
+  | 'Fixed' ('<' intLit '>')? ('<' '<' intLit '>' '>')?
   | 'Clock'
-  | 'Analog' ('<' IntLit '>')?
+  | 'Analog' ('<' intLit '>')?
   | '{' field* '}'        // Bundle
-  | type '[' IntLit ']'   // Vector
+  | type '[' intLit ']'   // Vector
   ;
 
 field
-  : 'flip'? id ':' type
+  : 'flip'? fieldId ':' type
   ;
 
 defname
@@ -70,7 +70,7 @@ defname
   ;
 
 parameter
-  : 'parameter' id '=' IntLit NEWLINE
+  : 'parameter' id '=' intLit NEWLINE
   | 'parameter' id '=' StringLit NEWLINE
   | 'parameter' id '=' DoubleLit NEWLINE
   | 'parameter' id '=' RawString NEWLINE
@@ -105,7 +105,7 @@ stmt
   | exp '<-' exp info?
   | exp 'is' 'invalid' info?
   | when
-  | 'stop(' exp exp IntLit ')' info?
+  | 'stop(' exp exp intLit ')' info?
   | 'printf(' exp exp StringLit ( exp)* ')' info?
   | 'skip' info?
   | 'attach' '(' exp+ ')' info?
@@ -113,9 +113,9 @@ stmt
 
 memField
 	:  'data-type' '=>' type NEWLINE
-	| 'depth' '=>' IntLit NEWLINE
-	| 'read-latency' '=>' IntLit NEWLINE
-	| 'write-latency' '=>' IntLit NEWLINE
+	| 'depth' '=>' intLit NEWLINE
+	| 'read-latency' '=>' intLit NEWLINE
+	| 'write-latency' '=>' intLit NEWLINE
 	| 'read-under-write' '=>' ruw NEWLINE
 	| 'reader' '=>' id+ NEWLINE
 	| 'writer' '=>' id+ NEWLINE
@@ -160,46 +160,55 @@ ruw
   ;
 
 exp
-  : 'UInt' ('<' IntLit '>')? '(' IntLit ')' 
-  | 'SInt' ('<' IntLit '>')? '(' IntLit ')' 
-  | 'UBits' ('<' IntLit '>')? '(' StringLit ')'
-  | 'SBits' ('<' IntLit '>')? '(' StringLit ')'
+  : 'UInt' ('<' intLit '>')? '(' intLit ')'
+  | 'SInt' ('<' intLit '>')? '(' intLit ')'
   | id    // Ref
-  | exp '.' id 
-  | exp '[' IntLit ']'
+  | exp '.' fieldId
+  | exp '[' intLit ']'
   | exp '[' exp ']'
   | 'mux(' exp exp exp ')'
   | 'validif(' exp exp ')'
-  | primop exp* IntLit*  ')' 
+  | primop exp* intLit*  ')'
   ;
 
 id
   : Id
-  | keyword
+  | keywordAsId
   ;
 
-keyword
+fieldId
+  : Id
+  | RelaxedId
+  | UnsignedInt
+  | keywordAsId
+  ;
+
+intLit
+  : UnsignedInt
+  | SignedInt
+  | HexLit
+  ;
+
+// Keywords that are also legal ids
+keywordAsId
   : 'circuit'
   | 'module'
   | 'extmodule'
+  | 'parameter'
   | 'input'
   | 'output'
   | 'UInt'
   | 'SInt'
-  | 'UBits'
-  | 'SBits'
   | 'Clock'
+  | 'Analog'
+  | 'Fixed'
   | 'flip'
   | 'wire'
   | 'reg'
   | 'with'
   | 'reset'
   | 'mem'
-  | 'data-type'
   | 'depth'
-  | 'read-latency'
-  | 'write-latency'
-  | 'read-under-write'
   | 'reader'
   | 'writer'
   | 'readwriter'
@@ -272,19 +281,26 @@ primop
  * LEXER RULES
  *------------------------------------------------------------------*/
 
-IntLit
+UnsignedInt
   : '0'
-  | ( '+' | '-' )? [1-9] ( Digit )*
-  | '"' 'h' ( HexDigit )+ '"'
+  | PosInt
   ;
 
-DoubleLit
-  : ( '+' | '-' )? Digit+ '.' Digit+ ( 'E' Digit+ )?
+SignedInt
+  : ( '+' | '-' ) PosInt
   ;
 
 fragment
-Nondigit
-  : [a-zA-Z_]
+PosInt
+  : [1-9] ( Digit )*
+  ;
+
+HexLit
+  : '"' 'h' ( HexDigit )+ '"'
+  ;
+
+DoubleLit
+  : ( '+' | '-' )? Digit+ '.' Digit+ ( 'E' Digit+ )?
   ;
 
 fragment
@@ -315,16 +331,23 @@ FileInfo
   ;
 
 Id
-  : IdNondigit
-    ( IdNondigit
-    | Digit
-    )*
+  : LegalStartChar (LegalIdChar)*
+  ;
+
+RelaxedId
+  : (LegalIdChar)+
   ;
 
 fragment
-IdNondigit
-  : Nondigit
-  | [~!@#$%^*\-+=?/]
+LegalIdChar
+  : LegalStartChar
+  | Digit
+  | '$'
+  ;
+
+fragment
+LegalStartChar
+  : [a-zA-Z_]
   ;
 
 fragment COMMENT
diff --git a/src/main/scala/firrtl/Visitor.scala b/src/main/scala/firrtl/Visitor.scala
index 592cfa8e..2db46271 100644
--- a/src/main/scala/firrtl/Visitor.scala
+++ b/src/main/scala/firrtl/Visitor.scala
@@ -89,7 +89,7 @@ class Visitor(infoMode: InfoMode) extends FIRRTLBaseVisitor[FirrtlNode] {
 
   private def visitParameter[FirrtlNode](ctx: FIRRTLParser.ParameterContext): Param = {
     val name = ctx.id.getText
-    (ctx.IntLit, ctx.StringLit, ctx.DoubleLit, ctx.RawString) match {
+    (ctx.intLit, ctx.StringLit, ctx.DoubleLit, ctx.RawString) match {
       case (int, null, null, null) => IntParam(name, string2BigInt(int.getText))
       case (null, str, null, null) => StringParam(name, visitStringLit(str))
       case (null, null, dbl, null) => DoubleParam(name, dbl.getText.toDouble)
@@ -114,34 +114,34 @@ class Visitor(infoMode: InfoMode) extends FIRRTLBaseVisitor[FirrtlNode] {
 
   // Match on a type instead of on strings?
   private def visitType[FirrtlNode](ctx: FIRRTLParser.TypeContext): Type = {
-    def getWidth(n: TerminalNode): Width = IntWidth(string2BigInt(n.getText))
+    def getWidth(n: IntLitContext): Width = IntWidth(string2BigInt(n.getText))
     ctx.getChild(0) match {
       case term: TerminalNode =>
         term.getText match {
-          case "UInt" => if (ctx.getChildCount > 1) UIntType(IntWidth(string2BigInt(ctx.IntLit(0).getText)))
+          case "UInt" => if (ctx.getChildCount > 1) UIntType(IntWidth(string2BigInt(ctx.intLit(0).getText)))
           else UIntType(UnknownWidth)
-          case "SInt" => if (ctx.getChildCount > 1) SIntType(IntWidth(string2BigInt(ctx.IntLit(0).getText)))
+          case "SInt" => if (ctx.getChildCount > 1) SIntType(IntWidth(string2BigInt(ctx.intLit(0).getText)))
           else SIntType(UnknownWidth)
-          case "Fixed" => ctx.IntLit.size match {
+          case "Fixed" => ctx.intLit.size match {
             case 0 => FixedType(UnknownWidth, UnknownWidth)
             case 1 => ctx.getChild(2).getText match {
-              case "<" => FixedType(UnknownWidth, getWidth(ctx.IntLit(0)))
-              case _ => FixedType(getWidth(ctx.IntLit(0)), UnknownWidth)
+              case "<" => FixedType(UnknownWidth, getWidth(ctx.intLit(0)))
+              case _ => FixedType(getWidth(ctx.intLit(0)), UnknownWidth)
             }
-            case 2 => FixedType(getWidth(ctx.IntLit(0)), getWidth(ctx.IntLit(1)))
+            case 2 => FixedType(getWidth(ctx.intLit(0)), getWidth(ctx.intLit(1)))
           }
           case "Clock" => ClockType
-          case "Analog" => if (ctx.getChildCount > 1) AnalogType(IntWidth(string2BigInt(ctx.IntLit(0).getText)))
+          case "Analog" => if (ctx.getChildCount > 1) AnalogType(IntWidth(string2BigInt(ctx.intLit(0).getText)))
           else AnalogType(UnknownWidth)
           case "{" => BundleType(ctx.field.map(visitField))
         }
-      case typeContext: TypeContext => new VectorType(visitType(ctx.`type`), string2Int(ctx.IntLit(0).getText))
+      case typeContext: TypeContext => new VectorType(visitType(ctx.`type`), string2Int(ctx.intLit(0).getText))
     }
   }
 
   private def visitField[FirrtlNode](ctx: FIRRTLParser.FieldContext): Field = {
     val flip = if (ctx.getChild(0).getText == "flip") Flip else Default
-    Field(ctx.id.getText, flip, visitType(ctx.`type`))
+    Field(ctx.fieldId.getText, flip, visitType(ctx.`type`))
   }
 
   private def visitBlock[FirrtlNode](ctx: FIRRTLParser.ModuleBlockContext): Statement =
@@ -171,7 +171,7 @@ class Visitor(infoMode: InfoMode) extends FIRRTLBaseVisitor[FirrtlNode] {
             val paramDef = fieldName match {
               case "data-type" => ParamValue(typ = Some(visitType(field.`type`())))
               case "read-under-write" => ParamValue(ruw = Some(field.ruw().getText)) // TODO
-              case _ => ParamValue(lit = Some(field.IntLit().getText.toInt))
+              case _ => ParamValue(lit = Some(field.intLit().getText.toInt))
             }
             if (fieldMap.contains(fieldName))
               throw new ParameterRedefinedException(s"Redefinition of $fieldName in FIRRTL line:${field.start.getLine}")
@@ -267,7 +267,7 @@ class Visitor(infoMode: InfoMode) extends FIRRTLBaseVisitor[FirrtlNode] {
         case "inst" => DefInstance(info, ctx.id(0).getText, ctx.id(1).getText)
         case "node" => DefNode(info, ctx.id(0).getText, visitExp(ctx.exp(0)))
 
-        case "stop(" => Stop(info, string2Int(ctx.IntLit().getText), visitExp(ctx.exp(0)), visitExp(ctx.exp(1)))
+        case "stop(" => Stop(info, string2Int(ctx.intLit().getText), visitExp(ctx.exp(0)), visitExp(ctx.exp(1)))
         case "attach" => Attach(info, ctx.exp map visitExp)
         case "printf(" => Print(info, visitStringLit(ctx.StringLit), ctx.exp.drop(2).map(visitExp),
           visitExp(ctx.exp(0)), visitExp(ctx.exp(1)))
@@ -296,18 +296,18 @@ class Visitor(infoMode: InfoMode) extends FIRRTLBaseVisitor[FirrtlNode] {
           // This could be better
           val (width, value) =
             if (ctx.getChildCount > 4)
-              (IntWidth(string2BigInt(ctx.IntLit(0).getText)), string2BigInt(ctx.IntLit(1).getText))
+              (IntWidth(string2BigInt(ctx.intLit(0).getText)), string2BigInt(ctx.intLit(1).getText))
             else {
-              val bigint = string2BigInt(ctx.IntLit(0).getText)
+              val bigint = string2BigInt(ctx.intLit(0).getText)
               (IntWidth(BigInt(scala.math.max(bigint.bitLength, 1))), bigint)
             }
           UIntLiteral(value, width)
         case "SInt" =>
           val (width, value) =
             if (ctx.getChildCount > 4)
-              (IntWidth(string2BigInt(ctx.IntLit(0).getText)), string2BigInt(ctx.IntLit(1).getText))
+              (IntWidth(string2BigInt(ctx.intLit(0).getText)), string2BigInt(ctx.intLit(1).getText))
             else {
-              val bigint = string2BigInt(ctx.IntLit(0).getText)
+              val bigint = string2BigInt(ctx.intLit(0).getText)
               (IntWidth(BigInt(bigint.bitLength + 1)), bigint)
             }
           SIntLiteral(value, width)
@@ -315,13 +315,13 @@ class Visitor(infoMode: InfoMode) extends FIRRTLBaseVisitor[FirrtlNode] {
         case "mux(" => Mux(visitExp(ctx.exp(0)), visitExp(ctx.exp(1)), visitExp(ctx.exp(2)), UnknownType)
         case _ =>
           ctx.getChild(1).getText match {
-            case "." => new SubField(visitExp(ctx.exp(0)), ctx.id.getText, UnknownType)
+            case "." => new SubField(visitExp(ctx.exp(0)), ctx.fieldId.getText, UnknownType)
             case "[" => if (ctx.exp(1) == null)
-              new SubIndex(visitExp(ctx.exp(0)), string2Int(ctx.IntLit(0).getText), UnknownType)
+              new SubIndex(visitExp(ctx.exp(0)), string2Int(ctx.intLit(0).getText), UnknownType)
             else new SubAccess(visitExp(ctx.exp(0)), visitExp(ctx.exp(1)), UnknownType)
             // Assume primop
             case _ => DoPrim(visitPrimop(ctx.primop), ctx.exp.map(visitExp),
-              ctx.IntLit.map(x => string2BigInt(x.getText)), UnknownType)
+              ctx.intLit.map(x => string2BigInt(x.getText)), UnknownType)
           }
       }
 
diff --git a/src/test/scala/firrtlTests/ParserSpec.scala b/src/test/scala/firrtlTests/ParserSpec.scala
index 0cc893e2..7978ad63 100644
--- a/src/test/scala/firrtlTests/ParserSpec.scala
+++ b/src/test/scala/firrtlTests/ParserSpec.scala
@@ -4,6 +4,8 @@ package firrtlTests
 
 import org.scalatest._
 import firrtl._
+import org.scalacheck.Gen
+import org.scalacheck.Prop.forAll
 
 class ParserSpec extends FirrtlFlatSpec {
 
@@ -29,14 +31,11 @@ class ParserSpec extends FirrtlFlatSpec {
 
   private object KeywordTests {
     val prelude = Seq("circuit top :", "  module top :")
-    val keywords = Seq( "circuit", "module", "extmodule", "input", "output",
-      "UInt", "SInt", "flip", "Clock", "wire", "reg", "reset", "with", "mem",
-      "data-type", "depth", "read-latency", "write-latency",
-      "read-under-write", "reader", "writer", "readwriter", "inst", "of",
-      "node", "is", "invalid", "when", "else", "stop", "printf", "skip", "old",
-      "new", "undefined", "mux", "validif", "UBits", "SBits", "cmem", "smem",
-      "mport", "infer", "read", "write", "rdwr"
-    ) ++ PrimOps.listing
+    val keywords = Seq("circuit", "module", "extmodule", "parameter", "input", "output", "UInt",
+      "SInt", "Analog", "Fixed", "flip", "Clock", "wire", "reg", "reset", "with", "mem", "depth",
+      "reader", "writer", "readwriter", "inst", "of", "node", "is", "invalid", "when", "else",
+      "stop", "printf", "skip", "old", "new", "undefined", "mux", "validif", "cmem", "smem",
+      "mport", "infer", "read", "write", "rdwr") ++ PrimOps.listing
   }
 
   // ********** Memories **********
@@ -94,3 +93,47 @@ class ParserSpec extends FirrtlFlatSpec {
     }
   }
 }
+
+class ParserPropSpec extends FirrtlPropSpec {
+  // Disable shrinking on error.
+  import org.scalacheck.Shrink
+  implicit val noShrinkString = Shrink[String](_ => Stream.empty)
+
+  def legalStartChar = Gen.frequency((1, '_'), (20, Gen.alphaChar))
+  def legalChar = Gen.frequency((1, Gen.numChar), (1, '$'), (10, legalStartChar))
+
+  def identifier = for {
+    x <- legalStartChar
+    xs <- Gen.listOf(legalChar)
+  } yield (x :: xs).mkString
+
+  property("Identifiers should allow [A-Za-z0-9_$] but not allow starting with a digit or $") {
+    forAll (identifier) { id =>
+      whenever(id.nonEmpty) {
+        val input = s"""
+           |circuit Test :
+           |  module Test :
+           |    input $id : UInt<32>
+           |""".stripMargin
+        firrtl.Parser.parse(input split "\n")
+      }
+    }
+  }
+
+  def bundleField = for {
+    xs <- Gen.nonEmptyListOf(legalChar)
+  } yield xs.mkString
+
+  property("Bundle fields should allow [A-Za-z0-9_] including starting with a digit or $") {
+    forAll (identifier, bundleField) { case (id, field) =>
+      whenever(id.nonEmpty && field.nonEmpty) {
+        val input = s"""
+           |circuit Test :
+           |  module Test :
+           |    input $id : { $field : UInt<32> }
+           |""".stripMargin
+        firrtl.Parser.parse(input split "\n")
+      }
+    }
+  }
+}