Skip to content

Commit

Permalink
[ObjC] add "generate_minimal_imports" generation option
Browse files Browse the repository at this point in the history
This new option will cause only the files that provide types to
be imported into the generated code. This helps reduce the inputs
that aren't needed (folks ignoring the protoc warnings) and for
things imported only for custom options as the objc generated code
doesn't capture those and thus the imports aren't needed in the
generated code.

Also do some general import cleanups.

PiperOrigin-RevId: 558867748
  • Loading branch information
thomasvl authored and copybara-github committed Aug 21, 2023
1 parent 230232a commit 0b817d4
Show file tree
Hide file tree
Showing 17 changed files with 213 additions and 13 deletions.
20 changes: 20 additions & 0 deletions src/google/protobuf/compiler/objectivec/enum_field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,14 @@

#include <string>

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_map.h"
#include "absl/container/flat_hash_set.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/compiler/objectivec/field.h"
#include "google/protobuf/compiler/objectivec/names.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/io/printer.h"

namespace google {
Expand Down Expand Up @@ -135,6 +141,13 @@ void EnumFieldGenerator::DetermineForwardDeclarations(
}
}

void EnumFieldGenerator::DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const {
if (descriptor_->file() != descriptor_->enum_type()->file()) {
deps->insert(descriptor_->enum_type()->file());
}
}

RepeatedEnumFieldGenerator::RepeatedEnumFieldGenerator(
const FieldDescriptor* descriptor)
: RepeatedFieldGenerator(descriptor) {
Expand All @@ -153,6 +166,13 @@ void RepeatedEnumFieldGenerator::EmitArrayComment(io::Printer* printer) const {
// because `GPBEnumArray` isn't generic (like `NSArray` would be for messages)
// and thus doesn't reference the type in the header.

void RepeatedEnumFieldGenerator::DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const {
if (descriptor_->file() != descriptor_->enum_type()->file()) {
deps->insert(descriptor_->enum_type()->file());
}
}

} // namespace objectivec
} // namespace compiler
} // namespace protobuf
Expand Down
6 changes: 6 additions & 0 deletions src/google/protobuf/compiler/objectivec/enum_field.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@
#include <string>

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_set.h"
#include "google/protobuf/compiler/objectivec/field.h"
#include "google/protobuf/descriptor.h"

namespace google {
namespace protobuf {
Expand All @@ -52,6 +54,8 @@ class EnumFieldGenerator : public SingleFieldGenerator {
void GenerateCFunctionImplementations(io::Printer* printer) const override;
void DetermineForwardDeclarations(absl::btree_set<std::string>* fwd_decls,
bool include_external_types) const override;
void DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const override;

protected:
explicit EnumFieldGenerator(const FieldDescriptor* descriptor);
Expand All @@ -63,6 +67,8 @@ class RepeatedEnumFieldGenerator : public RepeatedFieldGenerator {

public:
void EmitArrayComment(io::Printer* printer) const override;
void DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const override;

protected:
explicit RepeatedEnumFieldGenerator(const FieldDescriptor* descriptor);
Expand Down
25 changes: 25 additions & 0 deletions src/google/protobuf/compiler/objectivec/extension.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,13 @@
#include <vector>

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_set.h"
#include "absl/log/absl_check.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/compiler/objectivec/helpers.h"
#include "google/protobuf/compiler/objectivec/names.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/descriptor.pb.h"
#include "google/protobuf/io/printer.h"

Expand Down Expand Up @@ -128,6 +132,27 @@ void ExtensionGenerator::DetermineObjectiveCClassDefinitions(
}
}

void ExtensionGenerator::DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const {
const Descriptor* extended_type = descriptor_->containing_type();
if (descriptor_->file() != extended_type->file()) {
deps->insert(extended_type->file());
}

const ObjectiveCType objc_type = GetObjectiveCType(descriptor_);
if (objc_type == OBJECTIVECTYPE_MESSAGE) {
const Descriptor* value_msg_descriptor = descriptor_->message_type();
if (descriptor_->file() != value_msg_descriptor->file()) {
deps->insert(value_msg_descriptor->file());
}
} else if (objc_type == OBJECTIVECTYPE_ENUM) {
const EnumDescriptor* value_enum_descriptor = descriptor_->enum_type();
if (descriptor_->file() != value_enum_descriptor->file()) {
deps->insert(value_enum_descriptor->file());
}
}
}

void ExtensionGenerator::GenerateRegistrationSource(
io::Printer* printer) const {
printer->Emit({{"full_method_name", full_method_name_}},
Expand Down
4 changes: 4 additions & 0 deletions src/google/protobuf/compiler/objectivec/extension.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
#include <string>

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_set.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/io/printer.h"

Expand All @@ -56,6 +58,8 @@ class ExtensionGenerator {
void GenerateRegistrationSource(io::Printer* printer) const;
void DetermineObjectiveCClassDefinitions(
absl::btree_set<std::string>* fwd_decls) const;
void DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const;

private:
std::string method_name_;
Expand Down
9 changes: 9 additions & 0 deletions src/google/protobuf/compiler/objectivec/field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,20 @@
#include <string>
#include <vector>

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_map.h"
#include "absl/container/flat_hash_set.h"
#include "absl/log/absl_check.h"
#include "absl/log/absl_log.h"
#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/compiler/objectivec/enum_field.h"
#include "google/protobuf/compiler/objectivec/helpers.h"
#include "google/protobuf/compiler/objectivec/map_field.h"
#include "google/protobuf/compiler/objectivec/message_field.h"
#include "google/protobuf/compiler/objectivec/names.h"
#include "google/protobuf/compiler/objectivec/primitive_field.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/io/printer.h"

namespace google {
Expand Down Expand Up @@ -237,6 +241,11 @@ void FieldGenerator::DetermineObjectiveCClassDefinitions(
// Nothing
}

void FieldGenerator::DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const {
// Nothing
}

void FieldGenerator::GenerateFieldDescription(io::Printer* printer,
bool include_default) const {
// Printed in the same order as the structure decl.
Expand Down
4 changes: 4 additions & 0 deletions src/google/protobuf/compiler/objectivec/field.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_map.h"
#include "absl/container/flat_hash_set.h"
#include "absl/strings/match.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/io/printer.h"

Expand Down Expand Up @@ -73,6 +75,8 @@ class FieldGenerator {
bool include_external_types) const;
virtual void DetermineObjectiveCClassDefinitions(
absl::btree_set<std::string>* fwd_decls) const;
virtual void DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const;

// Used during generation, not intended to be extended by subclasses.
void GenerateFieldDescription(io::Printer* printer,
Expand Down
70 changes: 57 additions & 13 deletions src/google/protobuf/compiler/objectivec/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@ void FileGenerator::GenerateFile(io::Printer* p, GeneratedFileType file_type,
/* for_bundled_proto = */ is_bundled_proto_);
const std::string header_extension(kHeaderExtension);

absl::flat_hash_set<const FileDescriptor*> file_imports;
switch (file_type) {
case GeneratedFileType::kHeader:
// Generated files bundled with the library get minimal imports,
Expand All @@ -500,35 +501,50 @@ void FileGenerator::GenerateFile(io::Printer* p, GeneratedFileType file_type,
if (HeadersUseForwardDeclarations()) {
// #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);
file_imports.insert(file_->public_dependency(i));
}
} else if (generation_options_.generate_minimal_imports) {
DetermineNeededDeps(&file_imports, PublicDepsHandling::kForceInclude);
} else {
for (int i = 0; i < file_->dependency_count(); i++) {
import_writer.AddFile(file_->dependency(i), header_extension);
file_imports.insert(file_->dependency(i));
}
}
break;
case GeneratedFileType::kSource:
import_writer.AddRuntimeImport("GPBProtocolBuffers_RuntimeSupport.h");
import_writer.AddFile(file_, header_extension);
if (HeadersUseForwardDeclarations()) {
// #import the headers for anything that a plain dependency of this
// proto file (that means they were just an include, not a "public"
// include).
absl::flat_hash_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);
if (!public_import_names.contains(dep->name())) {
import_writer.AddFile(dep, header_extension);
if (generation_options_.generate_minimal_imports) {
DetermineNeededDeps(&file_imports, PublicDepsHandling::kExclude);
} else {
// #import the headers for anything that a plain dependency of this
// proto file (that means they were just an include, not a "public"
// include).
absl::flat_hash_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);
if (!public_import_names.contains(dep->name())) {
file_imports.insert(dep);
}
}
}
}
break;
}

if (!file_imports.empty()) {
for (int i = 0; i < file_->dependency_count(); i++) {
const FileDescriptor* dep = file_->dependency(i);
if (file_imports.contains(dep)) {
import_writer.AddFile(file_->dependency(i), header_extension);
}
}
}

for (const auto& dep : file_options.extra_files_to_import) {
import_writer.AddFile(dep, header_extension);
}
Expand Down Expand Up @@ -752,6 +768,34 @@ void FileGenerator::EmitFileDescription(io::Printer* p) const {
p->Emit("\n");
}

void FileGenerator::DetermineNeededDeps(
absl::flat_hash_set<const FileDescriptor*>* deps,
PublicDepsHandling public_deps_handling) const {
// This logic captures the deps that are needed for types thus removing the
// ones that are only deps because they provide the definitions for custom
// options. If protoc gets something like "import options" then this logic can
// go away as the non "import options" deps would be the ones needed.

if (public_deps_handling == PublicDepsHandling::kForceInclude) {
for (int i = 0; i < file_->public_dependency_count(); i++) {
deps->insert(file_->public_dependency(i));
}
}

for (const auto& generator : message_generators_) {
generator->DetermineNeededFiles(deps);
}
for (const auto& generator : extension_generators_) {
generator->DetermineNeededFiles(deps);
}

if (public_deps_handling == PublicDepsHandling::kExclude) {
for (int i = 0; i < file_->public_dependency_count(); i++) {
deps->erase(file_);
}
}
}

} // namespace objectivec
} // namespace compiler
} // namespace protobuf
Expand Down
4 changes: 4 additions & 0 deletions src/google/protobuf/compiler/objectivec/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ class FileGenerator {
const std::vector<const FileDescriptor*>& deps_with_extensions) const;
void EmitFileDescription(io::Printer* p) const;

enum class PublicDepsHandling : int { kAsUsed, kForceInclude, kExclude };
void DetermineNeededDeps(absl::flat_hash_set<const FileDescriptor*>* deps,
PublicDepsHandling public_deps_handling) const;

bool HeadersUseForwardDeclarations() const {
// The bundled protos (WKTs) don't make use of forward declarations.
return !is_bundled_proto_ &&
Expand Down
12 changes: 12 additions & 0 deletions src/google/protobuf/compiler/objectivec/generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,18 @@ bool ObjectiveCGenerator::GenerateAll(
options[i].second);
return false;
}
} else if (options[i].first == "generate_minimal_imports") {
// Controls if minimal imports should be generated from a files imports.
// Since custom options require imports, they current cause generated
// imports even though there is nothing captured in the generated code,
// this provides smaller imports only for the things referenced.
if (!StringToBool(options[i].second,
&generation_options.generate_minimal_imports)) {
*error =
absl::StrCat("error: Unknown value for generate_minimal_imports: ",
options[i].second);
return false;
}
} else if (options[i].first == "experimental_multi_source_generation") {
// This is an experimental option, and could be removed or change at any
// time; it is not documented in the README.md for that reason.
Expand Down
1 change: 1 addition & 0 deletions src/google/protobuf/compiler/objectivec/helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include <string>
#include <vector>

#include "absl/strings/str_cat.h"
#include "absl/strings/string_view.h"
#include "google/protobuf/descriptor.h"
#include "google/protobuf/descriptor.pb.h"
Expand Down
22 changes: 22 additions & 0 deletions src/google/protobuf/compiler/objectivec/map_field.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,14 @@
#include <vector>

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_set.h"
#include "absl/log/absl_log.h"
#include "absl/strings/match.h"
#include "absl/strings/str_cat.h"
#include "google/protobuf/compiler/objectivec/field.h"
#include "google/protobuf/compiler/objectivec/helpers.h"
#include "google/protobuf/compiler/objectivec/names.h"
#include "google/protobuf/descriptor.h"

namespace google {
namespace protobuf {
Expand Down Expand Up @@ -205,6 +209,24 @@ void MapFieldGenerator::DetermineObjectiveCClassDefinitions(
}
}

void MapFieldGenerator::DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const {
const FieldDescriptor* value_descriptor =
descriptor_->message_type()->map_value();
const ObjectiveCType value_objc_type = GetObjectiveCType(value_descriptor);
if (value_objc_type == OBJECTIVECTYPE_MESSAGE) {
const Descriptor* value_msg_descriptor = value_descriptor->message_type();
if (descriptor_->file() != value_msg_descriptor->file()) {
deps->insert(value_msg_descriptor->file());
}
} else if (value_objc_type == OBJECTIVECTYPE_ENUM) {
const EnumDescriptor* value_enum_descriptor = value_descriptor->enum_type();
if (descriptor_->file() != value_enum_descriptor->file()) {
deps->insert(value_enum_descriptor->file());
}
}
}

} // namespace objectivec
} // namespace compiler
} // namespace protobuf
Expand Down
4 changes: 4 additions & 0 deletions src/google/protobuf/compiler/objectivec/map_field.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
#include <string>

#include "absl/container/btree_set.h"
#include "absl/container/flat_hash_set.h"
#include "google/protobuf/compiler/objectivec/field.h"
#include "google/protobuf/descriptor.h"

namespace google {
namespace protobuf {
Expand All @@ -59,6 +61,8 @@ class MapFieldGenerator : public RepeatedFieldGenerator {
absl::btree_set<std::string>* fwd_decls) const override;
void DetermineForwardDeclarations(absl::btree_set<std::string>* fwd_decls,
bool include_external_types) const override;
void DetermineNeededFiles(
absl::flat_hash_set<const FileDescriptor*>* deps) const override;

private:
std::unique_ptr<FieldGenerator> value_field_generator_;
Expand Down
Loading

0 comments on commit 0b817d4

Please sign in to comment.