[Ruby] optional field's presence checker method has_...? is broken on JRuby and has cosmetic issue on MRI

[ntkme]

What version of protobuf and what language are you using?
Version: 4.28.2
Language: Ruby

What operating system (Linux, Windows, ...) and version?

All operating systems.

What runtime / compiler are you using (e.g., python version or gcc version)

Ruby 3.3.5 / JRuby 9.4.8.0

What did you do?

Protobuf definition: https://github.com/sass/sass/blob/eba0a58b46788b893983c2886c1be3b9c2d260e1/spec/embedded_sass.proto#L807-L834

Test code:

require_relative 'embedded_sass_pb'

value = Sass::EmbeddedProtocol::Value::Color.new(
  space: 'rgb',
  channel1: 0, # this is an optional field
  channel2: 0, # this is an optional field
  channel3: 0, # this is an optional field
  alpha: nil # this is an optional field
)

# Note, the JRuby bug only occurs on a deserialized object. `.new` created objects have no issue. 
value = Sass::EmbeddedProtocol::Value::Color.decode(value.to_proto)

puts "  value.has_channel1? = #{value.has_channel1?}" # <- should return true
puts "  value.has_alpha?    = #{value.has_alpha?}" # <- should return false
puts "==="
puts value.to_s # <- note that the behavior changes on JRuby before and after this call
puts "==="
puts "  value.has_channel1? = #{value.has_channel1?}"  # <- should return true
puts "  value.has_alpha?    = #{value.has_alpha?}" # <- should return false

What did you expect to see

  value.has_channel1? = true
  value.has_alpha?    = false
===
<Sass::EmbeddedProtocol::Value::Color: space: "rgb", channel1: 0.0, channel2: 0.0, channel3: 0.0>
===
  value.has_channel1? = true
  value.has_alpha?    = false

What did you see instead?

MRI

.has_...? returns 0 instead of true. Note: 0 in ruby is considered as truthy, so this is just a cosmetic issue in MRI.

  value.has_channel1? = 0
  value.has_alpha?    = false
===
<Sass::EmbeddedProtocol::Value::Color: space: "rgb", channel1: 0.0, channel2: 0.0, channel3: 0.0>
===
  value.has_channel1? = 0
  value.has_alpha?    = false

JRuby

.has_...? always return false, until .to_s is called on the object. Somehow calling .to_s changes behavior of .has_...?, that after the call .has_...? returns the expected value.

  value.has_channel1? = false
  value.has_alpha?    = false
===
<Sass::EmbeddedProtocol::Value::Color: space: "rgb", channel1: 0.0, channel2: 0.0, channel3: 0.0>
===
  value.has_channel1? = true
  value.has_alpha?    = false

Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).

Anything else we should know about your project / environment
sass-contrib/sass-embedded-host-ruby#246

[colinbendell]

I believe the issue is that the return is bool instead of Qtrue or Qfalse. The underlying has_? method ultimately calls upb_Message_HasFieldByDef which returns bool. This is coerced into an int 1 for true but integers have loaded meaning in MRI. MRI treats the low bit of a returning integer as a flag (>> 0x01) and the result is then 0. I suspect this is the same underlying issue with jruby.

The solution would be:

return upb_Message_HasFieldByDef(Message_Get(_self, NULL), f) ? Qtrue : Qfalse;

tl;dr 1 == 0 in ruby. Use Qtrue and Qfalse instead.

[ntkme]

Tested 4.29.2 today and somehow the JRuby issue is no longer there. - I briefly looked at the commit history and wasn't able to identify which commit made the difference. Update: never mind, it has to do with NATIVE vs FFI. One is working and the other is not. PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION=FFI works as expected and PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION=NATIVE is broken.

On the other hands, I also tested @colinbendell's suggestion and it indeed fixes the issue with MRI.

@JasonLunn Can you please review PR #19731.

[ntkme]

One more thing about the JRuby bug: new objects that constructed ruby code with .new does not have issue, but decoded object from .decode will always return false on has_...? until inspect or to_s is called on the object.

From what I see in the java code, the inspect or to_s method is actually mutating the fields in some cases, and I suspect the JRuby bug has to do with those fields are not set during .decode, but then updated during inspect.

[ntkme]

Ran it through debugger and I found that during inspect this piece of logic updated the fields, and after that everything works.

protobuf/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java

Lines 1219 to 1225 in 2330983

	if (returnDefaults || builder.hasField(fieldDescriptor)) {
	Object rawValue = builder.getField(oneofCase);
	boolean encodeBytes =
	oneofCase.hasDefaultValue() && rawValue.equals(oneofCase.getDefaultValue());
	IRubyObject value = wrapField(context, oneofCase, rawValue, encodeBytes);
	fields.put(fieldDescriptor, value);
	return value;

Specifically, has...? is broken before fields.put(fieldDescriptor, value); at line 1124, and works correctly after line 1124.

So, the bug here is that at line 457, that it forgot to check for the case where fields does not container fieldDescriptor, but builder does contain fieldDescriptor:

protobuf/ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java

Line 457 in 2330983

return fields.containsKey(fieldDescriptor) ? runtime.getTrue() : runtime.getFalse();