scrooge: add all thrift keywords to ThriftParser

Problem: Scrooge [[ #259 | doesn't respect the same keywords ]] as Apache thrift. This mismatch prevents us from upgrading thrift, and it creates friction for customers generating code from existing scrooge-compatible thrift files via apache thrift. Solution: Update our forbidden keywords in ThriftParser to include [[ https://github.com/apache/thrift/blob/f7e6c654bde5d9832bede2b48b460c3e1bbbbb92/doc/specs/idl.md#reserved-keywords | the keywords ]] Apache Thrift disallows, and fix affected files. JIRA Issues: CSL-11183 Differential Revision: https://phabricator.twitter.biz/D707116
twitter · Aug 30, 2021 · 884f360 · 884f360
1 parent 2c5f856
commit 884f360
Show file tree

Hide file tree

Showing 12 changed files with 172 additions and 59 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -7,6 +7,13 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
 Unreleased
 ----------
 
+Breaking API Changes
+~~~~~~~~~~~~~~~~~~~~
+
+* scrooge-generator: Add reserved keywords to ThriftParser. If your field names match
+these keywords, you may need to modify them. This change should not affect backwards 
+and forwards compatiblility if using binary protocol for serde. ``PHAB_ID=D707116`` 
+
 21.8.0 (No 21.7.0 Release)
 --------------------------
 

diff --git a/scrooge-adaptive/src/test/scala/com/twitter/scrooge/adapt/AdaptiveScroogeTest.scala b/scrooge-adaptive/src/test/scala/com/twitter/scrooge/adapt/AdaptiveScroogeTest.scala
@@ -66,7 +66,7 @@ class AdaptiveScroogeTest extends AnyPropSpec with Checkers {
       Accessor("Required field named using reserved word") { _.`type` }
 
     val OptionalReservedWordFieldAccess: Accessor[TestStruct] =
-      Accessor("Optional field named using reserved word") { _.`class` }
+      Accessor("Optional field named using reserved word") { _.`clazz` }
 
     val OptionalWithDefaultFieldAccess: Accessor[TestStruct] =
       Accessor("Optional field with default value") {
@@ -107,7 +107,7 @@ class AdaptiveScroogeTest extends AnyPropSpec with Checkers {
       Accessor("Nested required struct named using reserved word") { _.`type` }
 
     val OptionalReservedWordFieldAccess: Accessor[TestNestedStruct] =
-      Accessor("Nested optional struct named using reserved word") { _.`class` }
+      Accessor("Nested optional struct named using reserved word") { _.`clazz` }
 
     val All = List(
       RequiredFieldAccess,

diff --git a/scrooge-adaptive/src/test/thrift/test.thrift b/scrooge-adaptive/src/test/thrift/test.thrift
@@ -15,7 +15,7 @@ struct TestStruct {
   12: required map<bool,bool> mapField;
   13: required i64 annotatedId (test.space = "User", test.name = "annotated" );
   14: required string type;
-  15: optional string class;
+  15: optional string clazz;
   16: optional string optionalField2;
   17: optional string optionalFieldWithDefaultValue = "default_value";
 
@@ -27,7 +27,7 @@ struct TestStruct {
 struct TestNestedStruct {
   1: required TestStruct field;
   2: required TestStruct type;
-  3: optional TestStruct class;
+  3: optional TestStruct clazz;
   4: optional TestStruct optionalField;
   5: required list<TestStruct> seqField;
   6: required set<TestStruct> setField;

diff --git a/...oge-generator-tests/src/test/scala/com/twitter/scrooge/backend/NamingConventionSpec.scala b/...oge-generator-tests/src/test/scala/com/twitter/scrooge/backend/NamingConventionSpec.scala
@@ -8,7 +8,7 @@ class NamingConventionSpec extends Spec {
     "follow naming conventions" in {
       import thrift.`def`.default._
       Constants.`val` must be(10)
-      Constants.`try` must be(123)
+      Constants.`_try` must be(123)
 
       val naughty = Naughty("car", 100)
       naughty.`type` must be("car")

diff --git a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/backend/ScalaGeneratorSpec.scala b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/backend/ScalaGeneratorSpec.scala
@@ -1310,8 +1310,8 @@ class ScalaGeneratorSpec extends JMockSpec with EvalHelper {
 
     "generate inherited services correctly" in { _ =>
       val dddService = Ddd.ServicePerEndpoint(
-        delete = new Service[Ddd.Delete.Args, Ddd.Delete.SuccessType] {
-          def apply(args: Ddd.Delete.Args) = Future.value(args.input)
+        remove = new Service[Ddd.Remove.Args, Ddd.Remove.SuccessType] {
+          def apply(args: Ddd.Remove.Args) = Future.value(args.input)
         },
         iiii = new Service[CccExtended.Iiii.Args, CccExtended.Iiii.SuccessType] {
           def apply(args: CccExtended.Iiii.Args) = Future.value(345)
@@ -1332,8 +1332,8 @@ class ScalaGeneratorSpec extends JMockSpec with EvalHelper {
 
     "generate inherited services with ServicePerEndpoint correctly" in { _ =>
       val dddService = Ddd.ServicePerEndpoint(
-        delete = new Service[Ddd.Delete.Args, Ddd.Delete.SuccessType] {
-          def apply(args: Ddd.Delete.Args) = Future.value(args.input)
+        remove = new Service[Ddd.Remove.Args, Ddd.Remove.SuccessType] {
+          def apply(args: Ddd.Remove.Args) = Future.value(args.input)
         },
         iiii = new Service[CccExtended.Iiii.Args, CccExtended.Iiii.SuccessType] {
           def apply(args: CccExtended.Iiii.Args) = Future.value(345)
@@ -1354,8 +1354,8 @@ class ScalaGeneratorSpec extends JMockSpec with EvalHelper {
 
     "generate inherited services with ReqRepServicePerEndpoint correctly" in { _ =>
       val dddService = Ddd.ReqRepServicePerEndpoint(
-        delete = new Service[Request[Ddd.Delete.Args], Response[Ddd.Delete.SuccessType]] {
-          def apply(request: Request[Ddd.Delete.Args]) = Future.value(Response(request.args.input))
+        remove = new Service[Request[Ddd.Remove.Args], Response[Ddd.Remove.SuccessType]] {
+          def apply(request: Request[Ddd.Remove.Args]) = Future.value(Response(request.args.input))
         },
         iiii = new Service[Request[CccExtended.Iiii.Args], Response[CccExtended.Iiii.SuccessType]] {
           def apply(request: Request[CccExtended.Iiii.Args]) = Future.value(Response(345))

diff --git a/scrooge-generator-tests/src/test/thrift/scala/colliding_types.thrift b/scrooge-generator-tests/src/test/thrift/scala/colliding_types.thrift
@@ -6,20 +6,20 @@ struct Result {
   1: bool success
 }
 
-struct Args {
+struct Args_ {
   1: bool success
 }
 
 service MyService {
   Result getMethod()
   i32 setMethod(1: Result result)
-  Args anotherMethod(1: Args a)
+  Args_ anotherMethod(1: Args_ a)
 }
 
 
 service ServiceWithCollections {
-  list<Result> query(Args args)
-  map<Args, list<Result>> nested()
-  set<Result> moreNested(set<list<Args>> args)
+  list<Result> query(Args_ a)
+  map<Args_, list<Result>> nested()
+  set<Result> moreNested(set<list<Args_>> a)
 }
 
diff --git a/scrooge-generator-tests/src/test/thrift/scala/colliding_unions.thrift b/scrooge-generator-tests/src/test/thrift/scala/colliding_unions.thrift
@@ -6,13 +6,13 @@ union Result {
   i32 errorCode
 }
 
-union Args {
+union Args_ {
   i32 why
-  i64 not
+  i64 why_not
 }
 
 service ServiceWithCollections {
-  list<Result> query(Args args)
-  map<Args, list<Result>> nested()
-  set<Result> moreNested(set<list<Args>> args)
+  list<Result> query(Args_ a)
+  map<Args_, list<Result>> nested()
+  set<Result> moreNested(set<list<Args_>> a)
 }
diff --git a/scrooge-generator-tests/src/test/thrift/scala/inheritance/ddd.thrift b/scrooge-generator-tests/src/test/thrift/scala/inheritance/ddd.thrift
@@ -4,5 +4,5 @@ namespace java inheritance.thriftjava.ddd
 include "ccc.thrift"
 
 service Ddd extends ccc.CccExtended {
-  i32 delete(i32 input)
+  i32 remove(i32 input)
 }
diff --git a/scrooge-generator-tests/src/test/thrift/standalone/naughty.thrift b/scrooge-generator-tests/src/test/thrift/standalone/naughty.thrift
@@ -1,11 +1,11 @@
 // Scala and Java keywords (but minus thrift keywords of course) are ok
 namespace java thrift.def.default
 const i32 val = 10 // `val` in Scala, val in Java
-const i32 try = 123 // `try` in Scala, _try_ in Java
+const i32 _try = 123 // `_try` in Scala, _try_ in Java
 
-enum super { // rewritten to title case Super in generated code
+enum super_ { // rewritten to title case Super in generated code
   trait = 20  // `trait` in Scala, TRAIT in Java
-  native = 99
+  native_ = 99
 }
 
 struct TestValidate {
@@ -14,14 +14,14 @@ struct TestValidate {
 
 struct naughty { // rewritten to title case Naughty
   1: string type  // `type` in Scala; getType() in Java
-  2: i32 abstract // `abstract` in Scala, getAbstract() in Java
+  2: i32 abstract_ // `abstract_` in Scala, getAbstract_() in Java
   3: optional string runtime
   4: optional string scala
 }
 
 union NaughtyUnion {
   10: i32 value // test primitive type and field name "value"
-  15: super field
+  15: super_ field
   20: bool flag
   30: string text
 }

diff --git a/scrooge-generator-tests/src/test/thrift/standalone/test.thrift b/scrooge-generator-tests/src/test/thrift/standalone/test.thrift
@@ -103,7 +103,7 @@ enum NamespaceCollisions
   Iterable,
   Unit,
   Nothing,
-  protected
+  Protected
 }
 
 /** doc, ignored */

diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftKeywords.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftKeywords.scala
@@ -0,0 +1,136 @@
+package com.twitter.scrooge.frontend
+
+/**
+ *
+ * Set of Thrift keywords so we can raise errors or warnings when used in thrift files
+ *
+ * Many of these keywords are reserved in alignment with ApacheThrift
+ * See https://github.com/apache/thrift/blob/master/doc/specs/idl.md#reserved-keywords
+ */
+private object ThriftKeywords {
+  private[this] val set = Set[String](
+    "abstract",
+    "alias",
+    "and",
+    "args",
+    "as",
+    "assert",
+    "async",
+    "begin",
+    "break",
+    "case",
+    "catch",
+    "class",
+    "clone",
+    "continue",
+    "const",
+    "declare",
+    "def",
+    "default",
+    "del",
+    "delete",
+    "do",
+    "dynamic",
+    "elif",
+    "else",
+    "elseif",
+    "elsif",
+    "end",
+    "enddeclare",
+    "endfor",
+    "endforeach",
+    "endif",
+    "endswitch",
+    "endwhile",
+    "ensure",
+    "enum",
+    "except",
+    "exec",
+    "exception",
+    "extends",
+    "finally",
+    "float",
+    "for",
+    "foreach",
+    "from",
+    "function",
+    "global",
+    "goto",
+    "if",
+    "implements",
+    "import",
+    "in",
+    "include",
+    "inline",
+    "instanceof",
+    "interface",
+    "is",
+    "lambda",
+    "module",
+    "namespace",
+    "native",
+    "new",
+    "next",
+    "nil",
+    "not",
+    "optional",
+    "or",
+    "package",
+    "pass",
+    "public",
+    "print",
+    "private",
+    "protected",
+    "raise",
+    "redo",
+    "rescue",
+    "retry",
+    "register",
+    "return",
+    "required",
+    "self",
+    "service",
+    "sizeof",
+    "static",
+    "struct",
+    "super",
+    "switch",
+    "synchronized",
+    "then",
+    "this",
+    "throw",
+    "throws",
+    "transient",
+    "try",
+    "typedef",
+    "undef",
+    "union",
+    "unless",
+    "unsigned",
+    "until",
+    "use",
+    "var",
+    "virtual",
+    "void",
+    "volatile",
+    "when",
+    "while",
+    "with",
+    "xor",
+    "yield",
+    // Built-in types are also keywords.
+    "binary",
+    "bool",
+    "byte",
+    "double",
+    "i16",
+    "i32",
+    "i64",
+    "list",
+    "map",
+    "set",
+    "string"
+  )
+
+  def contains(str: String): Boolean = set.contains(str)
+}
diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftParser.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftParser.scala
@@ -100,39 +100,9 @@ class ThriftParser(
   val identifierRegex: Regex = "[A-Za-z_][A-Za-z0-9\\._]*".r
   lazy val identifier: Parser[Identifier] = positioned(identifierRegex ^^ { x => Identifier(x) })
 
-  private[this] val thriftKeywords = Set[String](
-    "async",
-    "const",
-    "enum",
-    "exception",
-    "extends",
-    "include",
-    "namespace",
-    "optional",
-    "required",
-    "service",
-    "struct",
-    "throws",
-    "typedef",
-    "union",
-    "void",
-    // Built-in types are also keywords.
-    "binary",
-    "bool",
-    "byte",
-    "double",
-    "i16",
-    "i32",
-    "i64",
-    "list",
-    "map",
-    "set",
-    "string"
-  )
-
   lazy val simpleIDRegex: Regex = "[A-Za-z_][A-Za-z0-9_]*".r
   lazy val simpleID: Parser[SimpleID] = positioned(simpleIDRegex ^^ { x =>
-    if (thriftKeywords.contains(x))
+    if (ThriftKeywords.contains(x))
       failOrWarn(new KeywordException(x))
 
     SimpleID(x)