Skip to content

Commit

Permalink
Implement edition 2023 support in all Ruby runtimes.
Browse files Browse the repository at this point in the history
Three of these runtimes are based on upb, and the fourth is based on the Java runtime.  Both of these already have editions support, so this was mostly just a matter of:
- Advertising support to allow editions codegen
- Stripping features from the runtime options
- Hooking up conformance tests
- Adding some lightweight editions tests

There are also a few minor orthogonal fixes included here:
- Ruby's upb hack for treating all enums as open enums needed tweaking
- The `enable_editions` flag is no longer needed in our internal proto rules

PiperOrigin-RevId: 616256211
  • Loading branch information
mkruskal-google authored and copybara-github committed Mar 15, 2024
1 parent 9ca36df commit bca8fb6
Show file tree
Hide file tree
Showing 30 changed files with 388 additions and 57 deletions.
2 changes: 1 addition & 1 deletion bazel/amalgamate.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def amalgamate(self, h_files, c_files):
self.output_c.write('#include "%s"\n' % (self.h_out))
if self.h_out == "ruby-upb.h":
self.output_h.write("// Ruby is still using proto3 enum semantics for proto2\n")
self.output_h.write("#define UPB_DISABLE_PROTO2_ENUM_CHECKING\n")
self.output_h.write("#define UPB_DISABLE_CLOSED_ENUM_CHECKING\n")

self.output_h.write("/* Amalgamated source file */\n")

Expand Down
4 changes: 1 addition & 3 deletions conformance/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ exports_files([
"text_format_failure_list_python.txt",
"text_format_failure_list_python_cpp.txt",
"text_format_failure_list_python_upb.txt",
"text_format_failure_list_ruby.txt",
"text_format_failure_list_jruby.txt",
"text_format_failure_list_jruby_ffi.txt",
])

cc_proto_library(
Expand Down Expand Up @@ -377,6 +374,7 @@ ruby_binary(
deps = [
":conformance_ruby_proto",
"//ruby:conformance_test_ruby_proto",
"//ruby:protobuf",
],
)

Expand Down
16 changes: 9 additions & 7 deletions conformance/conformance_ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,11 @@
# https://developers.google.com/open-source/licenses/bsd

require 'conformance/conformance_pb'
require 'google/protobuf'
require 'google/protobuf/test_messages_proto3_pb'
require 'google/protobuf/test_messages_proto2_pb'
require 'google/protobuf/editions/golden/test_messages_proto2_editions_pb'
require 'google/protobuf/editions/golden/test_messages_proto3_editions_pb'

$test_count = 0
$verbose = false
Expand All @@ -20,7 +23,12 @@ def do_test(request)
descriptor = Google::Protobuf::DescriptorPool.generated_pool.lookup(request.message_type)

unless descriptor
response.skipped = "Unknown message type: " + request.message_type
response.runtime_error = "Unknown message type: " + request.message_type
end

if request.test_category == :TEXT_FORMAT_TEST
response.skipped = "Ruby doesn't support text format"
return response
end

begin
Expand All @@ -45,12 +53,6 @@ def do_test(request)
return response
end

when :text_payload
begin
response.skipped = "Ruby doesn't support text format"
return response
end

when nil
fail "Request didn't have payload"
end
Expand Down
71 changes: 71 additions & 0 deletions conformance/failure_list_jruby.txt
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,74 @@ Required.Proto3.JsonInput.Int32FieldPlusSign
Required.Proto3.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotBool
Required.Proto3.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotInt
Required.Proto3.JsonInput.StringFieldNotAString
Recommended.Editions_Proto3.FieldMaskNumbersDontRoundTrip.JsonOutput
Recommended.Editions_Proto3.FieldMaskPathsDontRoundTrip.JsonOutput
Recommended.Editions_Proto3.FieldMaskTooManyUnderscore.JsonOutput
Recommended.Editions_Proto2.JsonInput.BoolFieldAllCapitalFalse
Recommended.Editions_Proto2.JsonInput.BoolFieldAllCapitalTrue
Recommended.Editions_Proto2.JsonInput.BoolFieldCamelCaseFalse
Recommended.Editions_Proto2.JsonInput.BoolFieldCamelCaseTrue
Recommended.Editions_Proto2.JsonInput.BoolFieldDoubleQuotedFalse
Recommended.Editions_Proto2.JsonInput.BoolFieldDoubleQuotedTrue
Recommended.Editions_Proto2.JsonInput.BoolMapFieldKeyNotQuoted
Recommended.Editions_Proto2.JsonInput.DoubleFieldInfinityNotQuoted
Recommended.Editions_Proto2.JsonInput.DoubleFieldNanNotQuoted
Recommended.Editions_Proto2.JsonInput.DoubleFieldNegativeInfinityNotQuoted
Recommended.Editions_Proto2.JsonInput.FieldNameDuplicate
Recommended.Editions_Proto2.JsonInput.FieldNameExtension.Validator
Recommended.Editions_Proto2.JsonInput.FieldNameNotQuoted
Recommended.Editions_Proto2.JsonInput.FloatFieldInfinityNotQuoted
Recommended.Editions_Proto2.JsonInput.FloatFieldNanNotQuoted
Recommended.Editions_Proto2.JsonInput.FloatFieldNegativeInfinityNotQuoted
Recommended.Editions_Proto2.JsonInput.Int32MapFieldKeyNotQuoted
Recommended.Editions_Proto2.JsonInput.Int64MapFieldKeyNotQuoted
Recommended.Editions_Proto2.JsonInput.JsonWithComments
Recommended.Editions_Proto2.JsonInput.StringFieldSingleQuoteBoth
Recommended.Editions_Proto2.JsonInput.StringFieldSingleQuoteKey
Recommended.Editions_Proto2.JsonInput.StringFieldSingleQuoteValue
Recommended.Editions_Proto2.JsonInput.StringFieldSurrogateInWrongOrder
Recommended.Editions_Proto2.JsonInput.StringFieldUnpairedHighSurrogate
Recommended.Editions_Proto2.JsonInput.StringFieldUnpairedLowSurrogate
Recommended.Editions_Proto2.JsonInput.Uint32MapFieldKeyNotQuoted
Recommended.Editions_Proto2.JsonInput.Uint64MapFieldKeyNotQuoted
Recommended.Editions_Proto3.JsonInput.BoolFieldAllCapitalFalse
Recommended.Editions_Proto3.JsonInput.BoolFieldAllCapitalTrue
Recommended.Editions_Proto3.JsonInput.BoolFieldCamelCaseFalse
Recommended.Editions_Proto3.JsonInput.BoolFieldCamelCaseTrue
Recommended.Editions_Proto3.JsonInput.BoolFieldDoubleQuotedFalse
Recommended.Editions_Proto3.JsonInput.BoolFieldDoubleQuotedTrue
Recommended.Editions_Proto3.JsonInput.BoolMapFieldKeyNotQuoted
Recommended.Editions_Proto3.JsonInput.DoubleFieldInfinityNotQuoted
Recommended.Editions_Proto3.JsonInput.DoubleFieldNanNotQuoted
Recommended.Editions_Proto3.JsonInput.DoubleFieldNegativeInfinityNotQuoted
Recommended.Editions_Proto3.JsonInput.FieldMaskInvalidCharacter
Recommended.Editions_Proto3.JsonInput.FieldNameDuplicate
Recommended.Editions_Proto3.JsonInput.FieldNameNotQuoted
Recommended.Editions_Proto3.JsonInput.FloatFieldInfinityNotQuoted
Recommended.Editions_Proto3.JsonInput.FloatFieldNanNotQuoted
Recommended.Editions_Proto3.JsonInput.FloatFieldNegativeInfinityNotQuoted
Recommended.Editions_Proto3.JsonInput.Int32MapFieldKeyNotQuoted
Recommended.Editions_Proto3.JsonInput.Int64MapFieldKeyNotQuoted
Recommended.Editions_Proto3.JsonInput.JsonWithComments
Recommended.Editions_Proto3.JsonInput.StringFieldSingleQuoteBoth
Recommended.Editions_Proto3.JsonInput.StringFieldSingleQuoteKey
Recommended.Editions_Proto3.JsonInput.StringFieldSingleQuoteValue
Recommended.Editions_Proto3.JsonInput.StringFieldSurrogateInWrongOrder
Recommended.Editions_Proto3.JsonInput.StringFieldUnpairedHighSurrogate
Recommended.Editions_Proto3.JsonInput.StringFieldUnpairedLowSurrogate
Recommended.Editions_Proto3.JsonInput.Uint32MapFieldKeyNotQuoted
Recommended.Editions_Proto3.JsonInput.Uint64MapFieldKeyNotQuoted
Required.Editions_Proto2.JsonInput.EnumFieldNotQuoted
Required.Editions_Proto2.JsonInput.Int32FieldLeadingZero
Required.Editions_Proto2.JsonInput.Int32FieldNegativeWithLeadingZero
Required.Editions_Proto2.JsonInput.Int32FieldPlusSign
Required.Editions_Proto2.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotBool
Required.Editions_Proto2.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotInt
Required.Editions_Proto2.JsonInput.StringFieldNotAString
Required.Editions_Proto3.JsonInput.EnumFieldNotQuoted
Required.Editions_Proto3.JsonInput.Int32FieldLeadingZero
Required.Editions_Proto3.JsonInput.Int32FieldNegativeWithLeadingZero
Required.Editions_Proto3.JsonInput.Int32FieldPlusSign
Required.Editions_Proto3.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotBool
Required.Editions_Proto3.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotInt
Required.Editions_Proto3.JsonInput.StringFieldNotAString
8 changes: 0 additions & 8 deletions conformance/text_format_failure_list_jruby.txt

This file was deleted.

8 changes: 0 additions & 8 deletions conformance/text_format_failure_list_ruby.txt

This file was deleted.

10 changes: 6 additions & 4 deletions ruby/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ internal_copy_files(
srcs = [
"//src/google/protobuf:test_messages_proto2.proto",
"//src/google/protobuf:test_messages_proto3.proto",
"//src/google/protobuf/editions:golden/test_messages_proto2_editions.proto",
"//src/google/protobuf/editions:golden/test_messages_proto3_editions.proto",
],
strip_prefix = "src",
)
Expand Down Expand Up @@ -232,12 +234,12 @@ filegroup(
conformance_test(
name = "conformance_test",
failure_list = "//conformance:failure_list_ruby.txt",
maximum_edition = "2023",
target_compatible_with = select({
":ruby_native": [],
"//conditions:default": ["@platforms//:incompatible"],
}),
testee = "//conformance:conformance_ruby",
text_format_failure_list = "//conformance:text_format_failure_list_ruby.txt",
)

conformance_test(
Expand All @@ -246,23 +248,23 @@ conformance_test(
"PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION": "ffi",
},
failure_list = "//conformance:failure_list_ruby.txt",
maximum_edition = "2023",
target_compatible_with = select({
":ruby_ffi": [],
"//conditions:default": ["@platforms//:incompatible"],
}),
testee = "//conformance:conformance_ruby",
text_format_failure_list = "//conformance:text_format_failure_list_ruby.txt",
)

conformance_test(
name = "conformance_test_jruby",
failure_list = "//conformance:failure_list_jruby.txt",
maximum_edition = "2023",
target_compatible_with = select({
":jruby_native": [],
"//conditions:default": ["@platforms//:incompatible"],
}),
testee = "//conformance:conformance_ruby",
text_format_failure_list = "//conformance:text_format_failure_list_jruby.txt",
)

conformance_test(
Expand All @@ -271,12 +273,12 @@ conformance_test(
"PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION": "ffi",
},
failure_list = "//conformance:failure_list_jruby_ffi.txt",
maximum_edition = "2023",
target_compatible_with = select({
":jruby_ffi": [],
"//conditions:default": ["@platforms//:incompatible"],
}),
testee = "//conformance:conformance_ruby",
text_format_failure_list = "//conformance:text_format_failure_list_jruby.txt",
)

################################################################################
Expand Down
2 changes: 2 additions & 0 deletions ruby/Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ well_known_protos = %w[

test_protos = %w[
tests/basic_test.proto
tests/basic_test_features.proto
tests/basic_test_proto2.proto
tests/generated_code.proto
tests/generated_code_proto2.proto
tests/generated_code_editions.proto
tests/multi_level_nesting_test.proto
tests/repeated_field_test.proto
tests/stress.proto
Expand Down
15 changes: 14 additions & 1 deletion ruby/ext/google/protobuf_c/defs.c
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,20 @@ static VALUE decode_options(VALUE self, const char* option_type, int size,
VALUE desc_rb = get_msgdef_obj(descriptor_pool, msgdef);
const Descriptor* desc = ruby_to_Descriptor(desc_rb);

options_rb = Message_decode_bytes(size, bytes, 0, desc->klass, true);
options_rb = Message_decode_bytes(size, bytes, 0, desc->klass, false);

// Strip features from the options proto to keep it internal.
const upb_MessageDef* decoded_desc = NULL;
upb_Message* options = Message_GetMutable(options_rb, &decoded_desc);
PBRUBY_ASSERT(options != NULL);
PBRUBY_ASSERT(decoded_desc == msgdef);
const upb_FieldDef* field =
upb_MessageDef_FindFieldByName(decoded_desc, "features");
PBRUBY_ASSERT(field != NULL);
upb_Message_ClearFieldByDef(options, field);

Message_freeze(options_rb);

rb_ivar_set(self, options_instancevar_interned, options_rb);
return options_rb;
}
Expand Down
4 changes: 3 additions & 1 deletion ruby/lib/google/protobuf/ffi/descriptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ def options
size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
temporary_arena = Google::Protobuf::FFI.create_arena
buffer = Google::Protobuf::FFI.message_options(self, size_ptr, temporary_arena)
Google::Protobuf::MessageOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
opts = Google::Protobuf::MessageOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze)
opts.clear_features()
opts.freeze
end
end

Expand Down
4 changes: 3 additions & 1 deletion ruby/lib/google/protobuf/ffi/enum_descriptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ def options
size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
temporary_arena = Google::Protobuf::FFI.create_arena
buffer = Google::Protobuf::FFI.enum_options(self, size_ptr, temporary_arena)
Google::Protobuf::EnumOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
opts = Google::Protobuf::EnumOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze)
opts.clear_features()
opts.freeze
end
end

Expand Down
5 changes: 0 additions & 5 deletions ruby/lib/google/protobuf/ffi/ffi.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,6 @@ class FFI
:repeated
)

Syntax = enum(
:Proto2, 2,
:Proto3
)

# All the different kind of well known type messages. For simplicity of check,
# number wrappers and string wrappers are grouped together. Make sure the
# order and merber of these groups are not changed.
Expand Down
4 changes: 3 additions & 1 deletion ruby/lib/google/protobuf/ffi/field_descriptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,9 @@ def options
size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
temporary_arena = Google::Protobuf::FFI.create_arena
buffer = Google::Protobuf::FFI.field_options(self, size_ptr, temporary_arena)
Google::Protobuf::FieldOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
opts = Google::Protobuf::FieldOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze)
opts.clear_features()
opts.freeze
end
end

Expand Down
4 changes: 3 additions & 1 deletion ruby/lib/google/protobuf/ffi/file_descriptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ def options
size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
temporary_arena = Google::Protobuf::FFI.create_arena
buffer = Google::Protobuf::FFI.file_options(@file_def, size_ptr, temporary_arena)
Google::Protobuf::FileOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
opts = Google::Protobuf::FileOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze)
opts.clear_features()
opts.freeze
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion ruby/lib/google/protobuf/ffi/oneof_descriptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ def options
size_ptr = ::FFI::MemoryPointer.new(:size_t, 1)
temporary_arena = Google::Protobuf::FFI.create_arena
buffer = Google::Protobuf::FFI.oneof_options(self, size_ptr, temporary_arena)
Google::Protobuf::OneofOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze).freeze
opts = Google::Protobuf::OneofOptions.decode(buffer.read_string_length(size_ptr.read(:size_t)).force_encoding("ASCII-8BIT").freeze)
opts.clear_features()
opts.freeze
end
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,8 @@ protected FieldDescriptor getDescriptor() {
private void calculateLabel(ThreadContext context) {
if (descriptor.isRepeated()) {
this.label = context.runtime.newSymbol("repeated");
} else if (descriptor.isRequired()) {
this.label = context.runtime.newSymbol("required");
} else if (descriptor.isOptional()) {
this.label = context.runtime.newSymbol("optional");
} else {
Expand Down
Loading

0 comments on commit bca8fb6

Please sign in to comment.