Add a generation option to control use of forward declarations in hea…

…ders.

Swift importing ObjC drops methods/properties if the type is only a forward
declaration since the type is incomplete. Historically the generator has always
use forward declarations to reduce how much will have rebuild when a proto file
does change; but that puts it at odds with Swift. If ObjC Protos end up spanning
Swift modules, the Swift import behavior could become a problem; so this option
provides a control for the behavior. The current behavior is to continue forward
declarations, but eventually the default will be changed.

Generate the WKTs using imports instead of forward decls.

objectivec/README.md

@@ -182,7 +182,7 @@ supported keys are:
     having to add the runtime directory to the header search path since the
     generate `#import` will be more complete.
 
-  * `package_to_prefix_mappings_path`: The `value` used for this key is a 
+  * `package_to_prefix_mappings_path`: The `value` used for this key is a
     path to a file containing a list of proto packages and prefixes.
     The generator will use this to locate which ObjC class prefix to use when
     generating sources _unless_ the `objc_class_prefix` file option is set.
@@ -218,6 +218,21 @@ supported keys are:
     helps prepare folks before they end up using a lot of protos and getting a
     lot of collisions.
 
+  * `headers_use_forward_declarations`: The `value` for this can be `yes` or
+    `no`, and indicates if the generated headers for use forward declarations
+    for Message and Enum types from other .proto files or if they files should
+    be imported into the generated header instead.
+
+    By using forward declarations, less code is likely to recompile when the
+    files do change, but Swift generally doesn't like forward declarations and
+    will fail to include properties when that concrete definition of the type is
+    know at import time. If your proto usages span modules, this can be a
+    problem.
+
+    `headers_use_forward_declarations` current defaults to `yes` (existing
+    behavior), but in a future release, that default may change to provide
+    better Swift support by default.
+
 Contributing
 ------------
 

src/google/protobuf/compiler/objectivec/objectivec_enum_field.cc

@@ -115,12 +115,16 @@ void EnumFieldGenerator::GenerateCFunctionImplementations(
 }
 
 void EnumFieldGenerator::DetermineForwardDeclarations(
-    std::set<std::string>* fwd_decls) const {
-  SingleFieldGenerator::DetermineForwardDeclarations(fwd_decls);
-  // If it is an enum defined in a different file, then we'll need a forward
-  // declaration for it.  When it is in our file, all the enums are output
-  // before the message, so it will be declared before it is needed.
-  if (descriptor_->file() != descriptor_->enum_type()->file()) {
+    std::set<std::string>* fwd_decls,
+    bool include_external_types) const {
+  SingleFieldGenerator::DetermineForwardDeclarations(
+      fwd_decls, include_external_types);
+  // If it is an enum defined in a different file (and not a WKT), then we'll
+  // need a forward declaration for it.  When it is in our file, all the enums
+  // are output before the message, so it will be declared before it is needed.
+  if (include_external_types &&
+      descriptor_->file() != descriptor_->enum_type()->file() &&
+      !IsProtobufLibraryBundledProtoFile(descriptor_->enum_type()->file())) {
     // Enum name is already in "storage_type".
     const std::string& name = variable("storage_type");
     fwd_decls->insert("GPB_ENUM_FWD_DECLARE(" + name + ")");

src/google/protobuf/compiler/objectivec/objectivec_enum_field.h

@@ -52,7 +52,8 @@ class EnumFieldGenerator : public SingleFieldGenerator {
   virtual void GenerateCFunctionImplementations(
       io::Printer* printer) const override;
   virtual void DetermineForwardDeclarations(
-      std::set<std::string>* fwd_decls) const override;
+      std::set<std::string>* fwd_decls,
+      bool include_external_types) const override;
 
  protected:
   EnumFieldGenerator(const FieldDescriptor* descriptor);

src/google/protobuf/compiler/objectivec/objectivec_field.cc

@@ -183,7 +183,8 @@ void FieldGenerator::GenerateCFunctionImplementations(
 }
 
 void FieldGenerator::DetermineForwardDeclarations(
-    std::set<std::string>* fwd_decls) const {
+    std::set<std::string>* fwd_decls,
+    bool include_external_types) const {
   // Nothing
 }
 

src/google/protobuf/compiler/objectivec/objectivec_field.h

@@ -64,7 +64,8 @@ class FieldGenerator {
 
   // Exposed for subclasses, should always call it on the parent class also.
   virtual void DetermineForwardDeclarations(
-      std::set<std::string>* fwd_decls) const;
+      std::set<std::string>* fwd_decls,
+      bool include_external_types) const;
   virtual void DetermineObjectiveCClassDefinitions(
       std::set<std::string>* fwd_decls) const;
 

src/google/protobuf/compiler/objectivec/objectivec_file.cc

@@ -57,6 +57,10 @@ const int32_t GOOGLE_PROTOBUF_OBJC_VERSION = 30004;
 
 const char* kHeaderExtension = ".pbobjc.h";
 
+std::string BundledFileName(const FileDescriptor* file) {
+  return "GPB" + FilePathBasename(file) + kHeaderExtension;
+}
+
 // Checks if a message contains any enums definitions (on the message or
 // a nested message under it).
 bool MessageContainsEnums(const Descriptor* message) {
@@ -218,6 +222,10 @@ void FileGenerator::GenerateHeader(io::Printer* printer) {
     headers.push_back("GPBDescriptor.h");
     headers.push_back("GPBMessage.h");
     headers.push_back("GPBRootObject.h");
+    for (int i = 0; i < file_->dependency_count(); i++) {
+      const std::string header_name = BundledFileName(file_->dependency(i));
+      headers.push_back(header_name);
+    }
   } else {
     headers.push_back("GPBProtocolBuffers.h");
   }
@@ -239,16 +247,26 @@ void FileGenerator::GenerateHeader(io::Printer* printer) {
       "\n",
       "google_protobuf_objc_version", StrCat(GOOGLE_PROTOBUF_OBJC_VERSION));
 
-  // #import any headers for "public imports" in the proto file.
+  // The bundled protos (WKTs) force off the use of forward declarations.
+  bool headers_use_forward_declarations =
+      generation_options_.headers_use_forward_declarations && !is_bundled_proto_;
+
   {
     ImportWriter import_writer(
         generation_options_.generate_for_named_framework,
         generation_options_.named_framework_to_proto_path_mappings_path,
         generation_options_.runtime_import_prefix,
         /* include_wkt_imports = */ false);
     const std::string header_extension(kHeaderExtension);
-    for (int i = 0; i < file_->public_dependency_count(); i++) {
-      import_writer.AddFile(file_->public_dependency(i), header_extension);
+    if (headers_use_forward_declarations) {
+      // #import any headers for "public imports" in the proto file.
+      for (int i = 0; i < file_->public_dependency_count(); i++) {
+        import_writer.AddFile(file_->public_dependency(i), header_extension);
+      }
+    } else {
+      for (int i = 0; i < file_->dependency_count(); i++) {
+        import_writer.AddFile(file_->dependency(i), header_extension);
+      }
     }
     import_writer.Print(printer);
   }
@@ -268,7 +286,9 @@ void FileGenerator::GenerateHeader(io::Printer* printer) {
 
   std::set<std::string> fwd_decls;
   for (const auto& generator : message_generators_) {
-    generator->DetermineForwardDeclarations(&fwd_decls);
+    generator->DetermineForwardDeclarations(
+        &fwd_decls,
+        /* include_external_types = */ headers_use_forward_declarations);
   }
   for (std::set<std::string>::const_iterator i(fwd_decls.begin());
        i != fwd_decls.end(); ++i) {
@@ -340,16 +360,10 @@ void FileGenerator::GenerateHeader(io::Printer* printer) {
 
 void FileGenerator::GenerateSource(io::Printer* printer) {
   // #import the runtime support.
-  const std::string header_extension(kHeaderExtension);
   std::vector<std::string> headers;
   headers.push_back("GPBProtocolBuffers_RuntimeSupport.h");
   if (is_bundled_proto_) {
-    headers.push_back("GPB" + FilePathBasename(file_) + header_extension);
-    for (int i = 0; i < file_->dependency_count(); i++) {
-      const std::string header_name =
-          "GPB" + FilePathBasename(file_->dependency(i)) + header_extension;
-      headers.push_back(header_name);
-    }
+    headers.push_back(BundledFileName(file_));
   }
   PrintFileRuntimePreamble(printer, headers);
 
@@ -363,27 +377,34 @@ void FileGenerator::GenerateSource(io::Printer* printer) {
   std::vector<const FileDescriptor*> deps_with_extensions;
   CollectMinimalFileDepsContainingExtensions(file_, &deps_with_extensions);
 
+  // The bundled protos (WKTs) force off the use of forward declarations.
+  bool headers_use_forward_declarations =
+      generation_options_.headers_use_forward_declarations && !is_bundled_proto_;
+
   {
     ImportWriter import_writer(
         generation_options_.generate_for_named_framework,
         generation_options_.named_framework_to_proto_path_mappings_path,
         generation_options_.runtime_import_prefix,
         /* include_wkt_imports = */ false);
+    const std::string header_extension(kHeaderExtension);
 
     // #import the header for this proto file.
     import_writer.AddFile(file_, header_extension);
 
-    // #import the headers for anything that a plain dependency of this proto
-    // file (that means they were just an include, not a "public" include).
-    std::set<std::string> public_import_names;
-    for (int i = 0; i < file_->public_dependency_count(); i++) {
-      public_import_names.insert(file_->public_dependency(i)->name());
-    }
-    for (int i = 0; i < file_->dependency_count(); i++) {
-      const FileDescriptor *dep = file_->dependency(i);
-      bool public_import = (public_import_names.count(dep->name()) != 0);
-      if (!public_import) {
-        import_writer.AddFile(dep, header_extension);
+    if (headers_use_forward_declarations) {
+      // #import the headers for anything that a plain dependency of this proto
+      // file (that means they were just an include, not a "public" include).
+      std::set<std::string> public_import_names;
+      for (int i = 0; i < file_->public_dependency_count(); i++) {
+        public_import_names.insert(file_->public_dependency(i)->name());
+      }
+      for (int i = 0; i < file_->dependency_count(); i++) {
+        const FileDescriptor *dep = file_->dependency(i);
+        bool public_import = (public_import_names.count(dep->name()) != 0);
+        if (!public_import) {
+          import_writer.AddFile(dep, header_extension);
+        }
       }
     }
 

src/google/protobuf/compiler/objectivec/objectivec_file.h

@@ -49,10 +49,14 @@ class MessageGenerator;
 class FileGenerator {
  public:
   struct GenerationOptions {
-    GenerationOptions() {}
+    GenerationOptions()
+      // TODO(thomasvl): Eventually flip this default to false for better
+      // interop with Swift if proto usages span modules made from ObjC sources.
+      : headers_use_forward_declarations(true) {}
     std::string generate_for_named_framework;
     std::string named_framework_to_proto_path_mappings_path;
     std::string runtime_import_prefix;
+    bool headers_use_forward_declarations;
   };
 
   FileGenerator(const FileDescriptor* file,

src/google/protobuf/compiler/objectivec/objectivec_generator.cc

@@ -189,8 +189,7 @@ bool ObjectiveCGenerator::GenerateAll(
       // generated files. When integrating ObjC protos into a build system,
       // this can be used to avoid having to add the runtime directory to the
       // header search path since the generate #import will be more complete.
-      generation_options.runtime_import_prefix =
-          StripSuffixString(options[i].second, "/");
+      generation_options.runtime_import_prefix = StripSuffixString(options[i].second, "/");
     } else if (options[i].first == "package_to_prefix_mappings_path") {
       // Path to use for when loading the objc class prefix mappings to use.
       // The `objc_class_prefix` file option is always honored first if one is present.
@@ -231,6 +230,12 @@ bool ObjectiveCGenerator::GenerateAll(
       //   - A comment can go on a line after a expected package/prefix pair.
       //     (i.e. - "some.proto.package # comment")
       SetProtoPackagePrefixExceptionList(options[i].second);
+    } else if (options[i].first == "headers_use_forward_declarations") {
+      if (!StringToBool(options[i].second,
+                        &generation_options.headers_use_forward_declarations)) {
+        *error = "error: Unknown value for headers_use_forward_declarations: " + options[i].second;
+        return false;
+      }
     } else {
       *error = "error: Unknown generator option: " + options[i].first;
       return false;

src/google/protobuf/compiler/objectivec/objectivec_map_field.cc

@@ -160,11 +160,19 @@ void MapFieldGenerator::FinishInitialization(void) {
 }
 
 void MapFieldGenerator::DetermineForwardDeclarations(
-    std::set<std::string>* fwd_decls) const {
-  RepeatedFieldGenerator::DetermineForwardDeclarations(fwd_decls);
+    std::set<std::string>* fwd_decls,
+    bool include_external_types) const {
+  RepeatedFieldGenerator::DetermineForwardDeclarations(
+      fwd_decls, include_external_types);
   const FieldDescriptor* value_descriptor =
       descriptor_->message_type()->map_value();
-  if (GetObjectiveCType(value_descriptor) == OBJECTIVECTYPE_MESSAGE) {
+  // Within a file there is no requirement on the order of the messages, so
+  // local references need a forward declaration. External files (not WKTs),
+  // need one when requested.
+  if (GetObjectiveCType(value_descriptor) == OBJECTIVECTYPE_MESSAGE &&
+      ((include_external_types &&
+        !IsProtobufLibraryBundledProtoFile(value_descriptor->file())) ||
+       descriptor_->file() == value_descriptor->file())) {
     const std::string& value_storage_type =
         value_field_generator_->variable("storage_type");
     fwd_decls->insert("@class " + value_storage_type);

src/google/protobuf/compiler/objectivec/objectivec_map_field.h

@@ -56,7 +56,8 @@ class MapFieldGenerator : public RepeatedFieldGenerator {
   virtual void DetermineObjectiveCClassDefinitions(
       std::set<std::string>* fwd_decls) const override;
   virtual void DetermineForwardDeclarations(
-      std::set<std::string>* fwd_decls) const override;
+      std::set<std::string>* fwd_decls,
+      bool include_external_types) const override;
 
  private:
   std::unique_ptr<FieldGenerator> value_field_generator_;

src/google/protobuf/compiler/objectivec/objectivec_message.cc

@@ -215,17 +215,18 @@ void MessageGenerator::GenerateStaticVariablesInitialization(
 }
 
 void MessageGenerator::DetermineForwardDeclarations(
-    std::set<std::string>* fwd_decls) {
+    std::set<std::string>* fwd_decls,
+    bool include_external_types) {
   if (!IsMapEntryMessage(descriptor_)) {
     for (int i = 0; i < descriptor_->field_count(); i++) {
       const FieldDescriptor* fieldDescriptor = descriptor_->field(i);
       field_generators_.get(fieldDescriptor)
-          .DetermineForwardDeclarations(fwd_decls);
+          .DetermineForwardDeclarations(fwd_decls, include_external_types);
     }
   }
 
   for (const auto& generator : nested_message_generators_) {
-    generator->DetermineForwardDeclarations(fwd_decls);
+    generator->DetermineForwardDeclarations(fwd_decls, include_external_types);
   }
 }
 

src/google/protobuf/compiler/objectivec/objectivec_message.h

@@ -62,7 +62,8 @@ class MessageGenerator {
   void GenerateSource(io::Printer* printer);
   void GenerateExtensionRegistrationSource(io::Printer* printer);
   void DetermineObjectiveCClassDefinitions(std::set<std::string>* fwd_decls);
-  void DetermineForwardDeclarations(std::set<std::string>* fwd_decls);
+  void DetermineForwardDeclarations(std::set<std::string>* fwd_decls,
+                                    bool include_external_types);
 
   // Checks if the message or a nested message includes a oneof definition.
   bool IncludesOneOfDefinition() const;