Skip to content

Make method id explicit in rb_exec_recursive_outer #6004

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
Merged
merged 2 commits into from
Jun 10, 2022
Merged

Make method id explicit in rb_exec_recursive_outer #6004

merged 2 commits into from
Jun 10, 2022

Conversation

jhawthorn
Copy link
Member

Previously, because opt_aref and opt_aset don't push a frame, when they would call rb_hash to determine the hash value of the key, the initial level of recursion would incorrectly use the method id at the top of the stack instead of "hash".

$ cat test.rb
module Hacks
  # aref from a named "hash" (not overriding hash) causes a different result
  def self.hash(h, k)
    h[k]
  end

  def self.any_other_name(h, k)
    h[k]
  end
end

rec = []; rec << rec

h = {}
h[rec] = 1
p Hacks.hash(h, rec)
p Hacks.any_other_name(h, rec)

$ make run
./miniruby -I./lib -I. -I.ext/common  -r./x86_64-linux-fake  ./test.rb
nil
1

This commit replaces rb_exec_recursive_outer with rb_exec_recursive_outer_mid, which takes an explicit method id, so that we can make the hash calculation behave consistently.

rb_exec_recursive_outer was documented as being internal, so I believe this should be okay to change.

jhawthorn added 2 commits June 10, 2022 13:09
@jhawthorn
Add mid argument to exec_recursive
c85212e
@jhawthorn
Make method id explicit in rb_exec_recursive_outer
29b92f0 
Previously, because opt_aref and opt_aset don't push a frame, when they
would call rb_hash to determine the hash value of the key, the initial
level of recursion would incorrectly use the method id at the top of the
stack instead of "hash".

This commit replaces rb_exec_recursive_outer with
rb_exec_recursive_outer_mid, which takes an explicit method id, so that
we can make the hash calculation behave consistently.

rb_exec_recursive_outer was documented as being internal, so I believe
this should be okay to change.
@jhawthorn jhawthorn merged commit 52da90a into ruby:master Jun 10, 2022
@shyouhei
Copy link
Member

BTW according to https://doxygen.nl/manual/commands.html#cmdinternal the @internal tag means the document is for internal use only, not the documented API.

@jhawthorn
Copy link
Member Author

@shyouhei thank you for the clarification I didn't realize 😳.

I wasn't able to find any uses of this method outside of CRuby, but since it is public it should be easy to restore and I'll do that tomorrow.

@jhawthorn jhawthorn mentioned this pull request Jun 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhawthorn @shyouhei