Skip to content

Commit

Permalink
Fix compatiblity issues.
Browse files Browse the repository at this point in the history
Currently some public API methods are defined in GenreatedMessage.java
and they have a generric return type:
  class GeneratedMessage {
    class Builder<BuilderType extends Builder<BuilderType>> {
      public BuilderType setField(...);
      public BuilderType setExtension(...);
    }
  }
With these definitions, the compiled byte code of a callsite will have
a direct reference to GeneratedMessage. For example:
  fooBuilder.setField(...);
becomes:
  ##: invokevirtual // Method Builder.setField:(...)LGeneratedMessage.Builder
  ##: checkcast     // class Builder

This will prevent us from updating generated classes to subclass a
different versioned GeneratedMessageV3 class in the future (we can't do
it in a binary compatible way).

This change addresses the problem by overriding these methods directly
in the generated class:
  class Foo {
    class Builder extends GeneratedMessage.Builder<Builder> {
      public Builder setField(...) {
        return super.setField(...);
      }
    }
  }
After this, fooBuilder.setField(...) will be compiled to:
  ##: invokevirtual // Method Builder.setField:(...)LFoo.Builder

The callsites will no longer reference GeneratedMessage directly and we
can change Foo to subclass GeneratedMessageV3 without breaking binary
compatiblity.

The downside of this change is:
  1. It increases generated code size (though it saves some instructions
     on the callsites).
  2. We can never stop generating these overrides because doing that
     will break binary compatibility.

Change-Id: I879afbbc1325a66324a51565e017143489b06e97
  • Loading branch information
xfxyjwf committed Jul 14, 2016
1 parent 0b68255 commit 1bce70d
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1396,7 +1396,7 @@ public final <Type> BuilderType setExtension(
return setExtension((ExtensionLite<MessageType, Type>) extension, value);
}
/** Set the value of an extension. */
public final <Type> BuilderType setExtension(
public <Type> BuilderType setExtension(
final GeneratedExtension<MessageType, Type> extension, final Type value) {
return setExtension((ExtensionLite<MessageType, Type>) extension, value);
}
Expand All @@ -1407,7 +1407,7 @@ public final <Type> BuilderType setExtension(
return setExtension((ExtensionLite<MessageType, List<Type>>) extension, index, value);
}
/** Set the value of one element of a repeated extension. */
public final <Type> BuilderType setExtension(
public <Type> BuilderType setExtension(
final GeneratedExtension<MessageType, List<Type>> extension,
final int index, final Type value) {
return setExtension((ExtensionLite<MessageType, List<Type>>) extension, index, value);
Expand All @@ -1418,7 +1418,7 @@ public final <Type> BuilderType addExtension(
return addExtension((ExtensionLite<MessageType, List<Type>>) extension, value);
}
/** Append a value to a repeated extension. */
public final <Type> BuilderType addExtension(
public <Type> BuilderType addExtension(
final GeneratedExtension<MessageType, List<Type>> extension, final Type value) {
return addExtension((ExtensionLite<MessageType, List<Type>>) extension, value);
}
Expand All @@ -1428,7 +1428,7 @@ public final <Type> BuilderType clearExtension(
return clearExtension((ExtensionLite<MessageType, ?>) extension);
}
/** Clear an extension. */
public final <Type> BuilderType clearExtension(
public <Type> BuilderType clearExtension(
final GeneratedExtension<MessageType, ?> extension) {
return clearExtension((ExtensionLite<MessageType, ?>) extension);
}
Expand Down
68 changes: 68 additions & 0 deletions src/google/protobuf/compiler/java/java_message_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,18 @@ Generate(io::Printer* printer) {
" return this;\n"
"}\n"
"\n");
} else {
printer->Print(
"public final Builder setUnknownFields(\n"
" final com.google.protobuf.UnknownFieldSet unknownFields) {\n"
" return super.setUnknownFields(unknownFields);\n"
"}\n"
"\n"
"public final Builder mergeUnknownFields(\n"
" final com.google.protobuf.UnknownFieldSet unknownFields) {\n"
" return super.mergeUnknownFields(unknownFields);\n"
"}\n"
"\n");
}

printer->Print(
Expand Down Expand Up @@ -438,6 +450,62 @@ GenerateCommonBuilderMethods(io::Printer* printer) {
"\n",
"classname", name_resolver_->GetImmutableClassName(descriptor_));

printer->Print(
"public Builder clone() {\n"
" return (Builder) super.clone();\n"
"}\n"
"public Builder setField(\n"
" com.google.protobuf.Descriptors.FieldDescriptor field,\n"
" Object value) {\n"
" return (Builder) super.setField(field, value);\n"
"}\n"
"public Builder clearField(\n"
" com.google.protobuf.Descriptors.FieldDescriptor field) {\n"
" return (Builder) super.clearField(field);\n"
"}\n"
"public Builder clearOneof(\n"
" com.google.protobuf.Descriptors.OneofDescriptor oneof) {\n"
" return (Builder) super.clearOneof(oneof);\n"
"}\n"
"public Builder setRepeatedField(\n"
" com.google.protobuf.Descriptors.FieldDescriptor field,\n"
" int index, Object value) {\n"
" return (Builder) super.setRepeatedField(field, index, value);\n"
"}\n"
"public Builder addRepeatedField(\n"
" com.google.protobuf.Descriptors.FieldDescriptor field,\n"
" Object value) {\n"
" return (Builder) super.addRepeatedField(field, value);\n"
"}\n");

if (descriptor_->extension_range_count() > 0) {
printer->Print(
"public <Type> Builder setExtension(\n"
" com.google.protobuf.GeneratedMessage.GeneratedExtension<\n"
" $classname$, Type> extension,\n"
" Type value) {\n"
" return (Builder) super.setExtension(extension, value);\n"
"}\n"
"public <Type> Builder setExtension(\n"
" com.google.protobuf.GeneratedMessage.GeneratedExtension<\n"
" $classname$, java.util.List<Type>> extension,\n"
" int index, Type value) {\n"
" return (Builder) super.setExtension(extension, index, value);\n"
"}\n"
"public <Type> Builder addExtension(\n"
" com.google.protobuf.GeneratedMessage.GeneratedExtension<\n"
" $classname$, java.util.List<Type>> extension,\n"
" Type value) {\n"
" return (Builder) super.addExtension(extension, value);\n"
"}\n"
"public <Type> Builder clearExtension(\n"
" com.google.protobuf.GeneratedMessage.GeneratedExtension<\n"
" $classname$, ?> extension) {\n"
" return (Builder) super.clearExtension(extension);\n"
"}\n",
"classname", name_resolver_->GetImmutableClassName(descriptor_));
}

// -----------------------------------------------------------------

if (context_->HasGeneratedMethods(descriptor_)) {
Expand Down

0 comments on commit 1bce70d

Please sign in to comment.