Skip to content

Update Message_index to account for oneof's #24181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Update Message_index to account for oneof's #24181

wants to merge 6 commits into from
+69 −36

Conversation

@jez
Copy link

@jez jez commented Oct 28, 2025 •
edited
Loading

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 (72935ab)

    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.

  • field → f (to match Message_method_missing) (640a776)

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

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

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

@jez jez force-pushed the jez-one-of-index branch from ca0c427 to df2d787 Compare October 28, 2025 20:05
@jez jez marked this pull request as ready for review October 28, 2025 20:10
@jez jez requested a review from a team as a code owner October 28, 2025 20:10
@jez jez requested review from JasonLunn and removed request for a team October 28, 2025 20:10
@jez jez mentioned this pull request Oct 28, 2025
Open
@shaod2 shaod2 added the ruby label Oct 28, 2025
@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 29, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 29, 2025
@JasonLunn
Copy link
Contributor

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

jez added 3 commits October 29, 2025 11:11
@jez
Add test showing current behavior
e5ecdfb
@jez
field → f (to match Message_method_missing)
d30f329
@jez
Update Message_index to account for oneof's
c49d529 
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.
@jez jez force-pushed the jez-one-of-index branch from df2d787 to c49d529 Compare October 29, 2025 18:11
@jez
Copy link
Author

jez commented Oct 29, 2025

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

@JasonLunn JasonLunn added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed wait for user action labels Oct 29, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 29, 2025
@JasonLunn
Copy link
Contributor

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

jez added 3 commits October 29, 2025 12:57
@jez
Delete some of the tests
4d39a71 
- method_missing is not a public API (doesn't work in the FFI version)
- the kind of TypeError that they raise is different (`::TypeError` vs
  `::Google::Protobuf::TypeError`)

Neither of these are what was broken, nor do we need to change anything
about it, so I'm just omitting it from the test so that it can pass in
FFI and non-FFI mode.
@jez
Also implement it in pure Ruby
cb80052 
I've factored a helper function `get_which_oneof` to show that I modeled
the implementation off of something that already exists, namely the
generated `msg.my_oneof` methods (because `msg["my_oneof"]` should be
the same), and to alert anyone making future changes that if they're
changing one place, they almost certainly want their changes to apply to
both places.
@jez
Add JRuby implementation
fce7131
@jez
Copy link
Author

jez commented Oct 29, 2025

@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 jez requested a review from JasonLunn October 29, 2025 21:11
@JasonLunn JasonLunn added 🅰️ safe for tests Mark a commit as safe to run presubmits over and removed wait for user action labels Oct 29, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Oct 29, 2025
JasonLunn
JasonLunn reviewed Oct 29, 2025
View reviewed changes
ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java

if (args.length == 1) {
// If we find a Oneof return it's name (use lookupOneof because it has an index)
IRubyObject oneofDescriptor = rubyDescriptor.lookupOneof(context, args[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of this declaration is causing the remaining code below, starting on line 409, to error out during Java compilation because oneofDescriptor is no longer declared.

@JasonLunn
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JasonLunn JasonLunn JasonLunn approved these changes

Assignees

No one assigned

Labels

ruby wait for user action

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ruby: No way to access oneof field if it shares the name of a method on Object

3 participants

@jez @JasonLunn @shaod2