From 37edfe5bede6477c3b091e081e5a193c8ff125a7 Mon Sep 17 00:00:00 2001 From: Ryan O'Neill Date: Tue, 24 Nov 2020 18:58:48 +0000 Subject: [PATCH] scrooge-generator: Stop Failing on Annotations inside Container Types Problem Scrooge is incapable of properly parsing Thrift IDL when a container type (e.g. list, set, map) contains (an) annotation(s) inside the container type itself. For example, if the type is 'list', the 'i64' component of the list can contain an annotation. If it has one, scrooge chokes on it. Solution Scrooge now properly parses Thrift IDL that contains these types of annotations. However, it currently silently discards them, and doesn't take them into account when doing code generation. If desired, at a later date we can build full support for them. Result Scrooge no longer fails to properly parse Thrift IDL that contains annotations inside of container types. JIRA Issues: CSL-10359 Differential Revision: https://phabricator.twitter.biz/D583284 --- CHANGELOG.rst | 5 +++ .../scrooge/frontend/ThriftParserSpec.scala | 42 ++++++++++++++++--- .../scrooge/frontend/ThriftParser.scala | 21 ++++++---- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 84e057cef..e50a9b8ae 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,11 @@ Unreleased * scrooge-generator: Java throws an exception when encountering incorrect field types in a struct while deserializing. ``PHAB_ID=D565373`` +* scrooge-generator: Scrooge no longer fails to parse Thrift IDL when annotations are used + on the individual components of a 'container type' (e.g. list, set, map). Those types of + annotations are not currently used by scrooge, instead they are skipped over and discarded, + but the IDL no longer fails to parse when they are encountered. ``PHAB_ID=D583284`` + 20.10.0 ------- diff --git a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/frontend/ThriftParserSpec.scala b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/frontend/ThriftParserSpec.scala index 6c845f82d..31d5d2cef 100644 --- a/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/frontend/ThriftParserSpec.scala +++ b/scrooge-generator-tests/src/test/scala/com/twitter/scrooge/frontend/ThriftParserSpec.scala @@ -96,17 +96,40 @@ struct MyStruct {} parser.parse("binary", parser.fieldType) must be(TBinary) } - "compound types" in { + "list types" in { parser.parse("list", parser.fieldType) must be(ListType(TI64, None)) parser.parse("list>", parser.fieldType) must be( ListType(ListType(TString, None), None) ) - parser.parse("map>", parser.fieldType) must be( - MapType(TString, ListType(TBool, None), None) + // Annotations within container type definitions are parsed properly, but thrown away + parser.parse("list < i64 (something = \"else\") >", parser.fieldType) must be( + ListType(TI64, None)) + parser.parse("list(c = \"d\")>", parser.fieldType) must be( + ListType(ListType(TString, None), None) ) + } + + "set types" in { parser.parse("set", parser.fieldType) must be( SetType(ReferenceType(Identifier("Monster")), None) ) + parser.parse("set", parser.fieldType) must be( + SetType(TBool, None) + ) + } + + "map types" in { + parser.parse("map>", parser.fieldType) must be( + MapType(TString, ListType(TBool, None), None) + ) + parser.parse( + "map (e=\"f\")>", + parser.fieldType) must be( + MapType(TString, ListType(TBool, None), None) + ) + } + + "compound types" in { parser.parse("Monster", parser.fieldType) must be(ReferenceType(Identifier("Monster"))) } @@ -833,8 +856,17 @@ enum Foo ) ) - // TODO(CSL-4127): support annotations on nested types - // parser.parse("""typedef list< double ( cpp.fixed_point = "16" ) > tiny_float_list""", parser.typedef) must be() + parser.parse( + """typedef list< double ( cpp.fixed_point = "16" ) > tiny_float_list""", + parser.typedef + ) must be( + Typedef( + SimpleID("tiny_float_list", None), + ListType(TDouble, None), + Map(), + Map() + ) + ) parser.parse( """ 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 index f6c1b500e..6c2533dc3 100644 --- a/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftParser.scala +++ b/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/ThriftParser.scala @@ -201,18 +201,25 @@ class ThriftParser( lazy val containerType: Parser[ContainerType] = mapType | setType | listType + // Annotations within container types are parsed correctly, but currently thrown away lazy val mapType: Parser[MapType] = - ("map" ~> opt(cppType) <~ "<") ~ (fieldType <~ ",") ~ (fieldType <~ ">") ^^ { + ("map" ~> opt(cppType) <~ "<") ~ + (fieldType <~ defaultedAnnotations <~ ",") ~ + (fieldType <~ defaultedAnnotations <~ ">") ^^ { case cpp ~ key ~ value => MapType(key, value, cpp) } - lazy val setType: Parser[SetType] = ("set" ~> opt(cppType)) ~ ("<" ~> fieldType <~ ">") ^^ { - case cpp ~ t => SetType(t, cpp) - } + // Annotations within container types are parsed correctly, but currently thrown away + lazy val setType: Parser[SetType] = + ("set" ~> opt(cppType)) ~ ("<" ~> fieldType <~ defaultedAnnotations <~ ">") ^^ { + case cpp ~ t => SetType(t, cpp) + } - lazy val listType: Parser[ListType] = ("list" ~ "<") ~> (fieldType <~ ">") ~ opt(cppType) ^^ { - case t ~ cpp => ListType(t, cpp) - } + // Annotations within container types are parsed correctly, but currently thrown away + lazy val listType: Parser[ListType] = + ("list" ~ "<") ~> (fieldType <~ defaultedAnnotations <~ ">") ~ opt(cppType) ^^ { + case t ~ cpp => ListType(t, cpp) + } // FFS. i'm very close to removing this and forcably breaking old thrift files. lazy val cppType: Parser[String] = "cpp_type" ~> stringLiteral ^^ { literal => literal.value }