Skip to content
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.

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

Skip refined method when exporting methods with changed visibility #4200

Conversation

jeremyevans
Copy link
Contributor

Previously, attempting to change the visibility of a method in a
singleton class for a class/module that is prepended to and refined
would raise a NoMethodError.

Fixes [Bug #17519]

vm_method.c Outdated
@@ -1378,7 +1386,7 @@ rb_export_method(VALUE klass, ID name, rb_method_visibility_t visi)
VALUE defined_class;
VALUE origin_class = RCLASS_ORIGIN(klass);

me = search_method(origin_class, name, &defined_class);
me = search_method0(origin_class, name, &defined_class, VM_METHOD_TYPE_REFINED);
Copy link
Contributor

@ko1 ko1 Feb 19, 2021

Choose a reason for hiding this comment

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

if the skip type is only REFINED, how about to make skip_refined boolean flag instead of skip_type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The approach of taking a type is a bit more flexible, in case we want to skip other method types in other cases. However, I can make that change if you prefer. This isn't public API, so we could always change it later if needed.

Copy link
Member

Choose a reason for hiding this comment

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

At least, 0 is a possible valid value as rb_method_type_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally tried to use -1 instead of 0, but it complained about that :). From the looks of it, using END_OF_VM_METHOD_TYPE_PLACEHOLDER may not be portable, and VM_METHOD_TYPE_REFINED+1 will just break later if we expand the values in rb_method_type_t. So I'll go with @ko1's suggestion.

@fledman
Copy link

fledman commented Mar 16, 2021

@jeremyevans
what is your plan for this bugfix?

@jeremyevans
Copy link
Contributor Author

Let me rebase on master later today and if CI passes I'll merge it.

Previously, attempting to change the visibility of a method in a
singleton class for a class/module that is prepended to and refined
would raise a NoMethodError.

Fixes [Bug #17519]
@jeremyevans jeremyevans force-pushed the fix-method-visibility-refine-prepend-17519 branch from 86e645c to 250e046 Compare March 16, 2021 18:26
@jeremyevans jeremyevans merged commit 58660e9 into ruby:master Mar 16, 2021
@fledman
Copy link

fledman commented Apr 5, 2021

@jeremyevans
I was running my original set of cases on the just released 2.7.3, and I realized that this fix introduces a new bug.
Now, declaring an unrefined module by itself breaks set_visibility:

RUBY_VERSION

class Thing
  def direction
    'LEFT'
  end
end

Thing.new.singleton_class.send(:private, :direction)

module NeverUsed
  refine Thing do
    def direction
      'UP'
    end
  end
end

Thing.new.singleton_class.send(:private, :direction)
=> "2.7.3"
=> :direction
=> #<Class:#<Thing:0x00007f81500c9580>>
=> #<refinement:Thing@NeverUsed>
NameError (undefined method `direction' for class `#<Class:#<Thing:0x00007f81500b3a00>>')

prepending an empty module avoids this new bug:

module Nothing; end; Thing.prepend(Nothing); Thing.ancestors
=> [Nothing, Thing, Object, PP::ObjectMixin, Kernel, BasicObject]
Thing.new.singleton_class.send(:private, :direction)
=> #<Class:#<Thing:0x00007f9a7a1293e8>>

@fledman
Copy link

fledman commented Apr 6, 2021

#4357

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants