Skip to content

Commit 34d6c85

Browse files
Ship all option dependencies to plugins along with regular ones.
For most plugins, these will still be ignored because they're disjoint from dependencies in the descriptors themselves. For java (and similar languages), optional dependencies will need to be extracted specially and made to handle the case where they aren't available smoothly. PiperOrigin-RevId: 803702133
1 parent b98f6ee commit 34d6c85

File tree

3 files changed

+59
-3
lines changed

3 files changed

+59
-3
lines changed

src/google/protobuf/compiler/command_line_interface.cc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,14 @@ void CommandLineInterface::GetTransitiveDependencies(
333333
GetTransitiveDependencies(file->dependency(i), already_seen, output,
334334
options);
335335
}
336+
for (int i = 0; i < file->option_dependency_count(); ++i) {
337+
const FileDescriptor* dep =
338+
file->pool()->FindFileByName(file->option_dependency_name(i));
339+
ABSL_CHECK(dep != nullptr)
340+
<< "Option dependency " << file->option_dependency_name(i)
341+
<< " not found in pool. This should never happen.";
342+
GetTransitiveDependencies(dep, already_seen, output, options);
343+
}
336344

337345
// Add this file.
338346
FileDescriptorProto* new_descriptor = output->Add();

src/google/protobuf/compiler/command_line_interface_unittest.cc

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3102,7 +3102,7 @@ TEST_F(CommandLineInterfaceTest, WriteTransitiveDescriptorSetWithSourceInfo) {
31023102
EXPECT_TRUE(descriptor_set.file(1).has_source_code_info());
31033103
}
31043104

3105-
TEST_F(CommandLineInterfaceTest, NoWriteTransitiveOptionImportDescriptorSet) {
3105+
TEST_F(CommandLineInterfaceTest, WriteTransitiveOptionImportDescriptorSet) {
31063106
CreateTempFile("google/protobuf/descriptor.proto",
31073107
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
31083108
CreateTempFile("custom_option.proto",
@@ -3138,13 +3138,41 @@ TEST_F(CommandLineInterfaceTest, NoWriteTransitiveOptionImportDescriptorSet) {
31383138
FileDescriptorSet descriptor_set;
31393139
ReadDescriptorSet("descriptor_set", &descriptor_set);
31403140
if (HasFatalFailure()) return;
3141-
EXPECT_EQ(2, descriptor_set.file_size());
3141+
EXPECT_EQ(4, descriptor_set.file_size());
31423142
if (descriptor_set.file(0).name() == "bar.proto") {
31433143
std::swap(descriptor_set.mutable_file()->mutable_data()[0],
31443144
descriptor_set.mutable_file()->mutable_data()[1]);
31453145
}
31463146
EXPECT_EQ("foo.proto", descriptor_set.file(0).name());
3147-
EXPECT_EQ("bar.proto", descriptor_set.file(1).name());
3147+
EXPECT_EQ("google/protobuf/descriptor.proto", descriptor_set.file(1).name());
3148+
EXPECT_EQ("custom_option.proto", descriptor_set.file(2).name());
3149+
EXPECT_EQ("bar.proto", descriptor_set.file(3).name());
3150+
}
3151+
3152+
TEST_F(CommandLineInterfaceTest, DisallowMissingOptionImportsDescriptorSetIn) {
3153+
FileDescriptorSet file_descriptor_set;
3154+
3155+
FileDescriptorProto* file = file_descriptor_set.add_file();
3156+
file->set_syntax("editions");
3157+
file->set_edition(Edition::EDITION_2024);
3158+
file->set_name("foo.proto");
3159+
file->add_option_dependency("bar.proto");
3160+
file->add_message_type()->set_name("Foo");
3161+
3162+
// Add an unknown field to the file options to make it look like a custom
3163+
// option.
3164+
file->mutable_message_type(0)
3165+
->mutable_options()
3166+
->mutable_unknown_fields()
3167+
->AddVarint(123, 456);
3168+
3169+
WriteDescriptorSet("foo.bin", &file_descriptor_set);
3170+
3171+
Run("protocol_compiler --descriptor_set_out=$tmpdir/descriptor_set "
3172+
"--include_imports --descriptor_set_in=$tmpdir/foo.bin "
3173+
"--proto_path=$tmpdir --experimental_editions foo.proto");
3174+
3175+
ExpectErrorSubstring("foo.proto: Import \"bar.proto\" was not found");
31483176
}
31493177

31503178
TEST_F(CommandLineInterfaceTest, DescriptorSetOptionRetention) {

src/google/protobuf/compiler/java/file.cc

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,15 @@ bool CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
105105
return true;
106106
}
107107

108+
void CollectPublicDependencies(
109+
const FileDescriptor* file,
110+
absl::flat_hash_set<const FileDescriptor*>* dependencies) {
111+
if (file == nullptr || !dependencies->insert(file).second) return;
112+
for (int i = 0; file != nullptr && i < file->public_dependency_count(); i++) {
113+
CollectPublicDependencies(file->public_dependency(i), dependencies);
114+
}
115+
}
116+
108117
// Finds all extensions for custom options in the given file descriptor with the
109118
// builder pool which resolves Java features instead of the generated pool.
110119
void CollectExtensions(const FileDescriptor& file,
@@ -129,6 +138,17 @@ void CollectExtensions(const FileDescriptor& file,
129138
extensions->clear();
130139
// Unknown extensions are ok and expected in the case of option imports.
131140
CollectExtensions(*dynamic_file_proto, extensions);
141+
142+
// TODO Check against dependencies to remove option dependencies
143+
// polluting the pool. These will be handled as optional dependencies.
144+
absl::flat_hash_set<const FileDescriptor*> dependencies;
145+
dependencies.insert(&file);
146+
for (int i = 0; i < file.dependency_count(); i++) {
147+
CollectPublicDependencies(file.dependency(i), &dependencies);
148+
}
149+
absl::erase_if(*extensions, [&](const FieldDescriptor* fieldDescriptor) {
150+
return !dependencies.contains(fieldDescriptor->file());
151+
});
132152
}
133153

134154
// Our static initialization methods can become very, very large.

0 commit comments

Comments
 (0)