protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

Update Message_index to account for oneof's

Open jez opened this issue 2 months ago • 5 comments

Fixes #21736

This is mostly patterned off of Message_method_missing, but that function does a bunch of extra stuff (to handle, e.g., has_ and clear_ and setter methods), while Message_index only needs to handle getter methods. So instead of copying the Message_method_missing implementation verbatim, we can "constant fold" away the parts of the method that we know cannot (or should not) happen for msg[...] calls.

Commit summary

  • Add test showing current behavior (72935ab99)

    A note: I struggled to get the test to fail when run via Bazel. e.g. bazel test //ruby/tests/... would not cause this test to fail when I edited this test to intentionally be incorrect. By contrast, it would fail when I ran it via bundle exec rake test.

    I'm not sure whether that's because I'm using Bazel wrong locally, or whether there's something incorrect causing the tests to not actually execute when invoked via Bazel.

  • fieldf (to match Message_method_missing) (640a776ed)

    This is a no-op style change that aligns one name with Message_method_missing.

  • Update Message_index to account for oneof's (df2d78703)

    In this commit, you can see the behavior change in the test.

jez avatar Oct 28 '25 18:10 jez

@jez - could you rebase this? I doubt your changes are the reason C++ tests failed

JasonLunn avatar Oct 29 '25 18:10 JasonLunn

@JasonLunn apologies, I rebased. I should have checked that before developing.

jez avatar Oct 29 '25 18:10 jez

The Ruby test failures look real - ruby/ext/google/protobuf_c/message.c only impacts the CRuby implementation when used without FFI. FFI on CRuby and JRuby both get their Message[] implementation from message.rb, and pure JRuby gets its from RubyMessage.java

JasonLunn avatar Oct 29 '25 18:10 JasonLunn

@JasonLunn I've added alternative implementations in the two files you mentioned.

I tested that the FFI version in message.rb fixed the test failure.

I was unable to get the Java version to run locally. If you can give me a bazel invocation that you expect to work to run the tests under JRuby locally, I would appreciate it. Otherwise, I guess I'll just await the GitHub Action results.

jez avatar Oct 29 '25 21:10 jez

@JasonLunn I've added alternative implementations in the two files you mentioned.

I tested that the FFI version in message.rb fixed the test failure.

I was unable to get the Java version to run locally. If you can give me a bazel invocation that you expect to work to run the tests under JRuby locally, I would appreciate it. Otherwise, I guess I'll just await the GitHub Action results.

bazel test //ruby/... --test_env=BAZEL=true should be sufficient, assuming that you have JRuby installed and selected via rvm or it is the ruby on your path. We have made an effort to make bazel be smart when the Ruby interpreter changes, but on the off chance something in the cache is the issue, try bazel clean --expunge as a last resort. If that still doesn't fix your ability to run the JRuby tests, please open a new issue for that, as we expect tests to be runnable externally.

JasonLunn avatar Oct 29 '25 22:10 JasonLunn