scrooge: support fully-qualified type name for fields

Problem Currently scrooge-generator does not support more than one dot in thrift file name. Suppose we have a thrift file with the name com.whatever.product.module1.thrift: ``` struct Foo { 1: i32 foo_field; } ``` and another thrift file that includes com.whatever.product.module1.thrift: ``` include "com.whatever.product.module1.thrift" struct Bar { 1: com.whatever.product.module1.Foo bar_field; } ``` This doesn't work with the current version of scrooge-generator. Solution Changing the `scopePrefix` to `Identifier` & other related places fixes this problem. Signed-off-by: Bryce Anderson <[email protected]> Differential Revision: https://phabricator.twitter.biz/D467516
twitter · Apr 23, 2020 · 4fad471 · 4fad471
1 parent 2189d28
commit 4fad471
Show file tree

Hide file tree

Showing 8 changed files with 38 additions and 16 deletions.
diff --git a/scrooge-generator-tests/src/test/thrift/namespace/namespace_full.qualified.filename.thrift b/scrooge-generator-tests/src/test/thrift/namespace/namespace_full.qualified.filename.thrift
@@ -0,0 +1,5 @@
+namespace java foo.qualified.included
+
+struct Bar {
+    1: i32 x;
+}
diff --git a/scrooge-generator-tests/src/test/thrift/namespace/namespace_qualified.thrift b/scrooge-generator-tests/src/test/thrift/namespace/namespace_qualified.thrift
@@ -0,0 +1,7 @@
+namespace java foo.qualified
+
+include "namespace_full.qualified.filename.thrift"
+
+struct Foo {
+    1: namespace_full.qualified.filename.Bar bar;
+}
diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/AST/Header.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/AST/Header.scala
@@ -21,7 +21,10 @@ case class Include(filePath: String, document: Document) extends Header {
    * Then we can use type Bar like this:
    *    foo.Bar
    */
-  val prefix: SimpleID = SimpleID(filePath.split('/').last.split('.').head)
+  val prefix: Identifier = Identifier(filePath.split('/').last.split('.').toSeq match {
+    case Seq(v) => v
+    case head :+ _ => head.mkString(".")
+  })
 }
 
 case class CppInclude(file: String) extends Header

diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/AST/Type.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/AST/Type.scala
@@ -27,15 +27,16 @@ sealed trait NamedType extends FieldType {
   def sid: SimpleID
 
   /** Filename of the containing file if the type is included from another file */
-  def scopePrefix: Option[SimpleID]
+  def scopePrefix: Option[Identifier]
 }
 
-case class StructType(struct: StructLike, scopePrefix: Option[SimpleID] = None) extends NamedType {
+case class StructType(struct: StructLike, scopePrefix: Option[Identifier] = None)
+    extends NamedType {
   val sid: SimpleID = struct.sid
   override def toString: String = "Struct(?)"
 }
 
-case class EnumType(enum: Enum, scopePrefix: Option[SimpleID] = None) extends NamedType {
+case class EnumType(enum: Enum, scopePrefix: Option[Identifier] = None) extends NamedType {
   val sid: SimpleID = enum.sid
   override def toString: String = "Enum(?)"
 }

diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/backend/CocoaGenerator.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/backend/CocoaGenerator.scala
@@ -262,7 +262,7 @@ class CocoaGenerator(
 
   override def qualifyNamedType(t: NamedType, namespace: Option[Identifier] = None): Identifier =
     t.scopePrefix match {
-      case Some(scope) => t.sid.prepend(getIncludeNamespace(scope.name).fullName)
+      case Some(scope) => t.sid.prepend(getIncludeNamespace(scope.fullName).fullName)
       case None => t.sid.prepend(currentNamespace)
     }
 

diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/backend/Generator.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/backend/Generator.scala
@@ -383,7 +383,7 @@ abstract class TemplateGenerator(val resolvedDoc: ResolvedDocument)
    */
   def qualifyNamedType(t: NamedType, namespace: Option[Identifier] = None): Identifier =
     t.scopePrefix match {
-      case Some(scope) => t.sid.addScope(getIncludeNamespace(scope.name))
+      case Some(scope) => t.sid.addScope(getIncludeNamespace(scope.fullName))
       case None if namespace.isDefined => t.sid.addScope(namespace.get)
       case None => t.sid
     }

diff --git a/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/TypeResolver.scala b/scrooge-generator/src/main/scala/com/twitter/scrooge/frontend/TypeResolver.scala
@@ -59,7 +59,7 @@ case class ResolvedDocument(document: Document, resolver: TypeResolver) {
   def qualifyName(name: NamedType, language: String, defaultNamespace: String): Identifier = {
     name.scopePrefix match {
       case Some(filename) =>
-        resolver.includeMap(filename.name).qualifySimpleID(name.sid, language, defaultNamespace)
+        resolver.includeMap(filename.fullName).qualifySimpleID(name.sid, language, defaultNamespace)
       case None =>
         qualifySimpleID(name.sid, language, defaultNamespace)
     }
@@ -128,7 +128,10 @@ case class TypeResolver(
   def resolveFieldType(id: Identifier): FieldType = id match {
     case sid: SimpleID =>
       typeMap.getOrElse(sid.name, throw new TypeNotFoundException(sid.name, id))
-    case qid: QualifiedID => getResolver(qid.names.head, qid).resolveFieldType(qid.tail)
+    case qid: QualifiedID =>
+      qid.names match {
+        case head :+ tail => getResolver(head.mkString("."), qid).resolveFieldType(SimpleID(tail))
+      }
   }
 
   protected def resolveServiceParent(parent: ServiceParent): Service =
@@ -144,7 +147,10 @@ case class TypeResolver(
     case SimpleID(name, _) =>
       val const = constMap.getOrElse(name, throw new UndefinedConstantException(name, id))
       (const.fieldType, const.value)
-    case qid: QualifiedID => getResolver(qid.names.head).resolveConst(qid.tail)
+    case qid: QualifiedID =>
+      qid.names match {
+        case head :+ tail => getResolver(head.mkString("."), qid).resolveConst(SimpleID(tail))
+      }
   }
 
   /**
@@ -153,7 +159,7 @@ case class TypeResolver(
   private[scrooge] def withInclude(inc: Include): TypeResolver = {
     val resolver = TypeResolver()
     val resolvedDocument = resolver(inc.document, Some(inc.prefix))
-    copy(includeMap = includeMap + (inc.prefix.name -> resolvedDocument))
+    copy(includeMap = includeMap + (inc.prefix.fullName -> resolvedDocument))
   }
 
   /**
@@ -180,7 +186,7 @@ case class TypeResolver(
   /**
    * Returns a new TypeResolver with the top level consts of `doc` added to the type map lazily.
    */
-  protected def withConstsFrom(doc: Document, scopePrefix: Option[SimpleID]): TypeResolver = {
+  protected def withConstsFrom(doc: Document, scopePrefix: Option[Identifier]): TypeResolver = {
     val toAdd = doc.defs.collect {
       case c: ConstDefinition => (c.sid.name -> c)
     }.toMap
@@ -191,7 +197,7 @@ case class TypeResolver(
   /**
    * Returns a new TypeResolver with the top level types of `doc` added to the type map lazily.
    */
-  protected def withTypesFrom(doc: Document, scopePrefix: Option[SimpleID]): TypeResolver = {
+  protected def withTypesFrom(doc: Document, scopePrefix: Option[Identifier]): TypeResolver = {
     val toAdd = doc.defs.collect {
       case d: Typedef => (d.sid.name -> d.fieldType)
       case s: Struct => (s.sid.name -> StructType(s, scopePrefix))
@@ -209,7 +215,7 @@ case class TypeResolver(
    *
    * @param scopePrefix the scope of the document if the document is an include
    */
-  def apply(doc: Document, scopePrefix: Option[SimpleID] = None): ResolvedDocument = {
+  def apply(doc: Document, scopePrefix: Option[Identifier] = None): ResolvedDocument = {
     var resolver = this
     val includes = doc.headers.collect { case i: Include => i }
     val defBuf = new ArrayBuffer[Definition](doc.defs.size)
@@ -245,7 +251,7 @@ case class TypeResolver(
    * typeMap, and then returns an updated TypeResolver with the new
    * definition bound, plus the resolved definition.
    */
-  def apply(definition: Definition, scopePrefix: Option[SimpleID]): ResolvedDefinition = {
+  def apply(definition: Definition, scopePrefix: Option[Identifier]): ResolvedDefinition = {
     definition match {
       case d: Typedef =>
         val resolved = apply(d.fieldType)

diff --git a/...oge-generator/src/main/scala/com/twitter/scrooge/java_generator/ApacheJavaGenerator.scala b/...oge-generator/src/main/scala/com/twitter/scrooge/java_generator/ApacheJavaGenerator.scala
@@ -150,11 +150,11 @@ class ApacheJavaGenerator(
    */
   def qualifyNamedType(
     sid: SimpleID,
-    scopePrefixOption: Option[SimpleID],
+    scopePrefixOption: Option[Identifier],
     fileNamespaceOption: Option[Identifier] = None
   ): Identifier = {
     scopePrefixOption
-      .map { scopePrefix => sid.addScope(getIncludeNamespace(scopePrefix.name)) }.orElse {
+      .map { scopePrefix => sid.addScope(getIncludeNamespace(scopePrefix.fullName)) }.orElse {
         fileNamespaceOption.map { fileNamespace => sid.addScope(fileNamespace) }
       }.getOrElse {
         sid