Skip to content

Commit abeb130

Browse files
mkruskal-googlecopybara-github
authored andcommitted
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 df7f12a commit abeb130

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
@@ -3104,7 +3104,7 @@ TEST_F(CommandLineInterfaceTest, WriteTransitiveDescriptorSetWithSourceInfo) {
31043104
EXPECT_TRUE(descriptor_set.file(1).has_source_code_info());
31053105
}
31063106

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

31523180
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)