Skip to content

Make method id explicit in rb_exec_recursive_outer#6004

Merged
jhawthorn merged 2 commits intoruby:masterfrom
jhawthorn:exec_recursive_mid
Jun 10, 2022
Merged

Make method id explicit in rb_exec_recursive_outer#6004
jhawthorn merged 2 commits intoruby:masterfrom
jhawthorn:exec_recursive_mid

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.

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants