Skip to content

Commit

Permalink
Force descriptor initialization of dependencies *before* internalUpda…
Browse files Browse the repository at this point in the history
…teFileDescriptor().

This fixes an edge-case where EnumDescriptor for a custom option may be unresolved if used in the same file, since adding the field to ExtensionRegistry doesn't trigger its static init block if the Enum is imported from a dependency.

Also renames feature resolution methods exposed from gencode. Private resolveAllFeaturesInternal() method may be renamed back to resolveAllFeatures() in a followup change.

PiperOrigin-RevId: 603852391
  • Loading branch information
zhangskz authored and copybara-github committed Feb 3, 2024
1 parent 9252b64 commit 2faa9d1
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 34 deletions.
26 changes: 16 additions & 10 deletions java/core/src/main/java/com/google/protobuf/Descriptors.java
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ public FieldDescriptor findExtensionByName(String name) {
* Construct a {@code FileDescriptor}.
*
* @param proto the protocol message form of the FileDescriptort
* @param dependencies {@code FileDescriptor}s corresponding to all of the file's dependencies
* @param dependencies {@code FileDescriptor}s corresponding to all of the file's dependencies.
* @throws DescriptorValidationException {@code proto} is not a valid descriptor. This can occur
* for a number of reasons; for instance, because a field has an undefined type or because
* two messages were defined with the same name.
Expand Down Expand Up @@ -397,7 +397,9 @@ private static FileDescriptor buildFrom(
result.crossLink();
// Skip feature resolution until later for calls from gencode.
if (!allowUnresolvedFeatures) {
result.resolveAllFeatures();
// We do not need to force feature resolution for proto1 dependencies
// since dependencies from non-gencode should already be fully feature resolved.
result.resolveAllFeaturesInternal();
}
return result;
}
Expand Down Expand Up @@ -480,19 +482,19 @@ public static FileDescriptor internalBuildGeneratedFileFrom(
return internalBuildGeneratedFileFrom(descriptorDataParts, dependencies);
}

/**
* This method is to be called by generated code only. It updates the FileDescriptorProto
* associated with the descriptor by parsing it again with the given ExtensionRegistry. This is
* needed to recognize custom options.
*/
public static void internalUpdateFileDescriptor(
public static void internalUpdateFileDescriptorImmutable(
FileDescriptor descriptor, ExtensionRegistry registry) {
internalUpdateFileDescriptor(descriptor, registry, false);
}

private static void internalUpdateFileDescriptor(
FileDescriptor descriptor, ExtensionRegistry registry, boolean mutable) {
ByteString bytes = descriptor.proto.toByteString();
try {
FileDescriptorProto proto = FileDescriptorProto.parseFrom(bytes, registry);
synchronized (descriptor) {
descriptor.setProto(proto);
descriptor.resolveAllFeatures();
descriptor.resolveAllFeaturesImmutable();
}
} catch (InvalidProtocolBufferException e) {
throw new IllegalArgumentException(
Expand Down Expand Up @@ -618,11 +620,15 @@ private FileDescriptor(
pool.addSymbol(message);
}

public void resolveAllFeaturesImmutable() {
resolveAllFeaturesInternal();
}

/**
* This method is to be called by generated code only. It resolves features for the descriptor
* and all of its children.
*/
public void resolveAllFeatures() {
private void resolveAllFeaturesInternal() {
if (this.features != null) {
return;
}
Expand Down
25 changes: 13 additions & 12 deletions src/google/protobuf/compiler/java/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(
// Feature resolution for Java features uses extension registry
// which must happen after internalInit() from
// GenerateNonNestedInitializationCode
printer->Print("descriptor.resolveAllFeatures();\n");
printer->Print("descriptor.resolveAllFeaturesImmutable();\n");

// Proto compiler builds a DescriptorPool, which holds all the descriptors to
// generate, when processing the ".proto" files. We call this DescriptorPool
Expand Down Expand Up @@ -460,6 +460,17 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(
return field->containing_type()->full_name() == "google.protobuf.FeatureSet";
});
}

// Force descriptor initialization of all dependencies.
for (int i = 0; i < file_->dependency_count(); i++) {
if (ShouldIncludeDependency(file_->dependency(i), true)) {
std::string dependency =
name_resolver_->GetImmutableClassName(file_->dependency(i));
printer->Print("$dependency$.getDescriptor();\n", "dependency",
dependency);
}
}

if (!extensions.empty()) {
// Must construct an ExtensionRegistry containing all existing extensions
// and use it to parse the descriptor data again to recognize extensions.
Expand All @@ -479,17 +490,7 @@ void FileGenerator::GenerateDescriptorInitializationCodeForImmutable(
}
printer->Print(
"com.google.protobuf.Descriptors.FileDescriptor\n"
" .internalUpdateFileDescriptor(descriptor, registry);\n");
}

// Force descriptor initialization of all dependencies.
for (int i = 0; i < file_->dependency_count(); i++) {
if (ShouldIncludeDependency(file_->dependency(i), true)) {
std::string dependency =
name_resolver_->GetImmutableClassName(file_->dependency(i));
printer->Print("$dependency$.getDescriptor();\n", "dependency",
dependency);
}
" .internalUpdateFileDescriptorImmutable(descriptor, registry);\n");
}

printer->Outdent();
Expand Down
16 changes: 4 additions & 12 deletions src/google/protobuf/compiler/java/shared_code_generator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,6 @@ void SharedCodeGenerator::Generate(
options_.annotate_code ? info_relative_path : "",
options_);

if (!options_.opensource_runtime) {
printer->Print("@com.google.protobuf.Internal.ProtoNonnullApi\n");
}

printer->Print(

Expand All @@ -87,17 +84,12 @@ void SharedCodeGenerator::Generate(
"returns\n"
" * an incomplete descriptor for internal use only. */\n"
" public static com.google.protobuf.Descriptors.FileDescriptor\n"
" descriptor;\n"
" /* This method is to be called by generated code only. It returns\n"
" * an incomplete descriptor for internal use only. */\n"
" public static com.google.protobuf.Descriptors.FileDescriptor "
"getDescriptor() {\n"
" descriptor.resolveAllFeatures();\n"
" return descriptor;\n"
" }\n"
" static {\n",
" descriptor;\n",
"classname", classname);
printer->Annotate("classname", file_->name());


printer->Print(" static {\n");
printer->Indent();
printer->Indent();
GenerateDescriptors(printer.get());
Expand Down

0 comments on commit 2faa9d1

Please sign in to comment.