Skip to content

Commit

Permalink
scrooge: Introduce AnnotatedFieldType to support annotating inner con…
Browse files Browse the repository at this point in the history 
…tainer types

# Problem

Currently, thrift annotations inside of collection types pass the parser, but
are subsequently dropped.

# Solution

This change adds a new `FieldType` – `AnnotatedFieldType`. This is currently
used to support thrift annotations on types inside of containers
(maps/lists/sets). But this can be further extended to apply annotations on any
`FieldType`.

# Result

By doing this, annotations inside of collection types are now propagated and are
available to downstream compilers.

JIRA Issues: CSL-11944

Differential Revision: https://phabricator.twitter.biz/D911997
  • Loading branch information
George Leontiev authored and jenkins committed Jun 30, 2022
1 parent c37077b commit 59e91e6
Show file tree
Hide file tree
Showing 19 changed files with 429 additions and 149 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.rst
View file
Expand Up @@ -10,6 +10,10 @@ Unreleased
New Features
~~~~~~~~~~~~

* scrooge-generator: Introduce a `AnnotatedFieldType` to abstract type annotations from
`FieldType` definitions. Currently used to propagate thrift annotations inside of
collection types. ``PHAB_ID=D911997``

* scrooge-core: `c.t.scrooge.ThriftUnion.fieldInfoForUnionClass` API for retrieving
`ThriftStructFieldInfo` for a `ThriftUnion` member class without having to instantiate
it. ``PHAB_ID=D871986``
Expand Down
105 changes: 99 additions & 6 deletions scrooge-generator-tests/src/test/scala/com/twitter/scrooge/frontend/ThriftParserSpec.scala
View file
Expand Up @@ -101,11 +101,29 @@ struct MyStruct {}
parser.parse("list<list<string>>", parser.fieldType) must be(
ListType(ListType(TString, 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))
ListType(
AnnotatedFieldType.wrap(
TI64,
Map("something" -> "else")
),
None
)
)
parser.parse("list<list<string (a = \"b\")>(c = \"d\")>", parser.fieldType) must be(
ListType(ListType(TString, None), None)
ListType(
AnnotatedFieldType.wrap(
ListType(
AnnotatedFieldType.wrap(
TString,
Map("a" -> "b")
),
None
),
Map("c" -> "d")
),
None
)
)
}

Expand All @@ -114,7 +132,13 @@ struct MyStruct {}
SetType(ReferenceType(Identifier("Monster")), None)
)
parser.parse("set<bool (hello = \"goodbye\")>", parser.fieldType) must be(
SetType(TBool, None)
SetType(
AnnotatedFieldType.wrap(
TBool,
Map("hello" -> "goodbye")
),
None
)
)
}

Expand All @@ -125,7 +149,70 @@ struct MyStruct {}
parser.parse(
"map<string (a=\"b\"), list<bool (c=\"d\")> (e=\"f\")>",
parser.fieldType) must be(
MapType(TString, ListType(TBool, None), None)
MapType(
AnnotatedFieldType.wrap(
TString,
Map("a" -> "b")
),
AnnotatedFieldType.wrap(
ListType(
AnnotatedFieldType.wrap(
TBool,
Map("c" -> "d")
),
None
),
Map("e" -> "f")
),
None
)
)
}

"inner collection types annotations" in {
parser.parse(
"""list<map<set<i32 (python.immutable = "0")> (python.immutable = "1"), map<i32,set<list<map<Insanity,string>(python.immutable = "2")> (python.immutable = "3")>>>>""",
parser.fieldType
) must be(
ListType(
MapType(
AnnotatedFieldType.wrap(
SetType(
AnnotatedFieldType.wrap(
TI32,
Map("python.immutable" -> "0")
),
None
),
Map("python.immutable" -> "1")
),
MapType(
TI32,
SetType(
AnnotatedFieldType.wrap(
ListType(
AnnotatedFieldType.wrap(
MapType(
ReferenceType(
Identifier("Insanity")
),
TString,
None
),
Map("python.immutable" -> "2")
),
None
),
Map("python.immutable" -> "3")
),
None
),
None
),
None
),
None
)
)
}

Expand Down Expand Up @@ -862,7 +949,13 @@ enum Foo
) must be(
Typedef(
SimpleID("tiny_float_list", None),
ListType(TDouble, None),
ListType(
AnnotatedFieldType.wrap(
TDouble,
Map("cpp.fixed_point" -> "16")
),
None
),
Map(),
Map()
)
Expand Down
24 changes: 24 additions & 0 deletions scrooge-generator-tests/src/test/scala/com/twitter/scrooge/frontend/TypeResolverSpec.scala
View file
Expand Up @@ -67,6 +67,30 @@ class TypeResolverSpec extends Spec {
}
}

"resolve annotations inside of container types" in {
val input =
""" struct Test {
| 1: list<i32 (my.annotation="list")> testList,
| 2: set<string (my.annotation="set")> testSet,
| 3: map<i32 (my.annotation="mapKey"), i32 (my.annotation="mapValue")> testMap
|}
""".stripMargin
val result = resolve(input)
result.document.defs.head match {
case struct: Struct =>
struct.fields(0).fieldType must be(
ListType(AnnotatedFieldType.wrap(TI32, Map("my.annotation" -> "list")), None))
struct.fields(1).fieldType must be(
SetType(AnnotatedFieldType.wrap(TString, Map("my.annotation" -> "set")), None))
struct.fields(2).fieldType must be(
MapType(
AnnotatedFieldType.wrap(TI32, Map("my.annotation" -> "mapKey")),
AnnotatedFieldType.wrap(TI32, Map("my.annotation" -> "mapValue")),
None))
case _ => fail()
}
}

"resolve self-referencing types" in {
val input =
"""struct S {
Expand Down
10 changes: 10 additions & 0 deletions scrooge-generator/src/main/scala/com/twitter/scrooge/AST/Definition.scala
View file
Expand Up @@ -42,6 +42,16 @@ sealed abstract class StructLike extends Definition {
val fields: Seq[Field]
val docstring: Option[String]
val annotations: Map[String, String]

def withAnnotations(newAnnotations: Map[String, String]): StructLike =
this match {
case s: Struct => s.copy(annotations = annotations ++ newAnnotations)
case u: Union => u.copy(annotations = annotations ++ newAnnotations)
case e: Exception_ => e.copy(annotations = annotations ++ newAnnotations)
// FunctionResult and FunctionArgs don't keep track of annotations
case r: FunctionResult => r
case a: FunctionArgs => a
}
}

case class Struct(
Expand Down
46 changes: 45 additions & 1 deletion scrooge-generator/src/main/scala/com/twitter/scrooge/AST/Type.scala
View file
Expand Up @@ -14,9 +14,53 @@ case object TDouble extends BaseType
case object TString extends BaseType
case object TBinary extends BaseType

/**
* AnnotatedFieldType is used to be able to annotate arbitrary FieldTypes.
* The current use case is to allow annotating types inside of collection hierarchies.
* In the future this can/should be expanded for type annotations in general.
*
* Note: please use AnnotatedFieldType.build to construct instances of this.
*
* @param underlying the type being annotated
* @param annos annotations applied to the underlying type
*/
case class AnnotatedFieldType private (underlying: FieldType, annos: Map[String, String])
extends FieldType {
require(annos.nonEmpty, "Cannot construct an AnnotatedFieldType with empty annotations.")

/**
* Once type annotations from underlying types have fully migrated to
* AnnotatedFieldType, this will not be necessary anymore.
*/
def unwrap: FieldType = underlying match {
case s: StructType => s.copy(struct = s.struct.withAnnotations(annos))
case e: EnumType => e.copy(enum = e.enum.copy(annotations = annos))
// Other types don't keep track of annotations, so there is nothing to propagate
case otherwise => otherwise
}
def annotations: Map[String, String] = annos
}

/**
* Type builder utils where we make the best effort to unwrap
* nested annotated types and avoid producing extra wrappers
* where possible.
*/
object AnnotatedFieldType {
def wrap(t: FieldType, annotations: Map[String, String]): FieldType = {
if (annotations.isEmpty) t
else
t match {
case t: AnnotatedFieldType =>
new AnnotatedFieldType(t.underlying, t.annotations ++ annotations)
case otherwise => new AnnotatedFieldType(otherwise, annotations)
}
}
}

/**
* ReferenceType is generated by ThriftParser in the frontend and
* resolved by TypeResolver. There will only ReferenceTypes after
* resolved by TypeResolver. There will only be ReferenceTypes after
* resolution seen by the backend when self-reference structs,
* mutually recursive structs, or references to further definitions
* (structs/enums) are present in the Document.
Expand Down
24 changes: 15 additions & 9 deletions ...oge-generator/src/main/scala/com/twitter/scrooge/android_generator/AndroidGenerator.scala
View file
@@ -1,18 +1,21 @@
package com.twitter.scrooge.android_generator

import com.github.mustachejava.{Mustache, DefaultMustacheFactory}
import com.twitter.scrooge.ast.ListType
import com.twitter.scrooge.ast.MapType
import com.twitter.scrooge.ast.ReferenceType
import com.twitter.scrooge.ast.SetType
import com.github.mustachejava.Mustache
import com.github.mustachejava.DefaultMustacheFactory
import com.twitter.scrooge.ast._
import com.twitter.scrooge.backend.{GeneratorFactory, Generator, ServiceOption}
import com.twitter.scrooge.backend.GeneratorFactory
import com.twitter.scrooge.backend.Generator
import com.twitter.scrooge.backend.ServiceOption
import com.twitter.scrooge.CompilerDefaults
import com.twitter.scrooge.frontend.{ScroogeInternalException, ResolvedDocument}
import com.twitter.scrooge.frontend.ScroogeInternalException
import com.twitter.scrooge.frontend.ResolvedDocument
import com.twitter.scrooge.frontend.ParseException
import com.twitter.scrooge.java_generator.{ApacheJavaGenerator, TypeController}
import com.twitter.scrooge.java_generator.ApacheJavaGenerator
import com.twitter.scrooge.java_generator.TypeController
import com.twitter.scrooge.mustache.ScalaObjectHandler
import java.io.{FileWriter, File, StringWriter}
import java.io.FileWriter
import java.io.File
import java.io.StringWriter
import scala.collection.concurrent.TrieMap
import scala.collection.mutable

Expand Down Expand Up @@ -70,6 +73,8 @@ class AndroidGenerator(
t match {
case Void => if (inContainer) "Void" else "void"
case OnewayVoid => if (inContainer) "Void" else "void"
case at: AnnotatedFieldType =>
typeName(at.unwrap, inContainer, inInit, skipGeneric, fileNamespace)
case TBool => if (inContainer) "Boolean" else "boolean"
case TByte => if (inContainer) "Byte" else "byte"
case TI16 => if (inContainer) "Short" else "short"
Expand Down Expand Up @@ -120,6 +125,7 @@ class AndroidGenerator(

def isListOrSetType(t: FunctionType): Boolean = {
t match {
case at: AnnotatedFieldType => isListOrSetType(at.unwrap)
case ListType(_, _) => true
case SetType(_, _) => true
case _ => false
Expand Down
19 changes: 13 additions & 6 deletions scrooge-generator/src/main/scala/com/twitter/scrooge/backend/CocoaGenerator.scala
View file
Expand Up @@ -2,10 +2,14 @@ package com.twitter.scrooge.backend

import com.twitter.scrooge.ast._
import com.twitter.scrooge.mustache.Dictionary._
import com.twitter.scrooge.mustache.{Dictionary, HandlebarLoader}
import com.twitter.scrooge.frontend.{ScroogeInternalException, ResolvedDocument}

import java.io.{OutputStreamWriter, FileOutputStream, File}
import com.twitter.scrooge.mustache.Dictionary
import com.twitter.scrooge.mustache.HandlebarLoader
import com.twitter.scrooge.frontend.ScroogeInternalException
import com.twitter.scrooge.frontend.ResolvedDocument

import java.io.OutputStreamWriter
import java.io.FileOutputStream
import java.io.File
import scala.collection.mutable

object CocoaGeneratorFactory extends GeneratorFactory {
Expand Down Expand Up @@ -96,6 +100,7 @@ class CocoaGenerator(
def getDependentTypes(struct: StructLike): Set[FieldType] = {
def getDependentTypes(fieldType: FieldType): Set[FieldType] = {
fieldType match {
case at: AnnotatedFieldType => getDependentTypes(at.unwrap)
case t: ListType => getDependentTypes(t.eltType)
case t: MapType => getDependentTypes(t.keyType) ++ getDependentTypes(t.valueType)
case t: SetType => getDependentTypes(t.eltType)
Expand Down Expand Up @@ -288,7 +293,9 @@ class CocoaGenerator(
def toMutable(f: Field): (String, String) = ("", "")

def genType(t: FunctionType, immutable: Boolean = false): CodeFragment = {
val code = t match {
@scala.annotation.tailrec
def getCode(t: FunctionType): String = t match {
case at: AnnotatedFieldType => getCode(at.unwrap)
case Void => "void"
case OnewayVoid => "void"
case TBool => "BOOL"
Expand All @@ -306,7 +313,7 @@ class CocoaGenerator(
case r: ReferenceType =>
throw new ScroogeInternalException("ReferenceType should not appear in backend")
}
v(code)
v(getCode(t))
}

def genFieldType(f: Field): CodeFragment = {
Expand Down

0 comments on commit 59e91e6

Please sign in to comment.