Skip to content

Reapply "Merge pull request #46 from byroot/use-forward-send" #51

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

Merged
hsbt merged 1 commit into from
Dec 18, 2025
Merged

Reapply "Merge pull request #46 from byroot/use-forward-send" #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions lib/delegate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,17 @@ def DelegateClass(superclass, &block)
protected_instance_methods -= ignores
public_instance_methods = superclass.public_instance_methods
public_instance_methods -= ignores

normal, special = public_instance_methods.partition { |m| m.match?(/\A[a-zA-Z]\w*[!\?]?\z/) }
Copy link
Member

@nobu nobu Dec 18, 2025

Choose a reason for hiding this comment

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

Why "normal" methods need to start with an alphabet, e.g., _ is not the case?

Copy link
Member

@byroot byroot Dec 18, 2025

Choose a reason for hiding this comment

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

It's a mistake _ should be there too. But it's not a big deal, just means they don't benefit from the optimization.


source = normal.map do |method|
"def #{method}(...); __getobj__.#{method}(...); end"
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The direct method call syntax __getobj__.#{method}(...) doesn't work for protected methods because it enforces visibility restrictions. When a protected method is called from outside the class context, Ruby raises a NoMethodError. This is the root cause of the test failure mentioned in the PR description.

The previous implementation used __send__ (via delegating_block), which bypasses visibility checks and works correctly for protected methods. The fix should use __send__ instead of direct method calls to maintain the same behavior.

Suggested change
"def #{method}(...); __getobj__.#{method}(...); end"
"def #{method}(...); __getobj__.__send__(:#{method}, ...); end"

Copilot uses AI. Check for mistakes.
end
Comment on lines +411 to +413
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The generated method definitions are joined with semicolons on a single line, which will make stack traces harder to read and debug. When an error occurs in a delegated method, the backtrace will show all methods as being on the same line.

Consider joining with newlines instead of semicolons to provide clearer stack traces and better debuggability.

Copilot uses AI. Check for mistakes.

protected_instance_methods.each do |method|
source << "def #{method}(...); __getobj__.__send__(#{method.inspect}, ...); end"
end

klass.module_eval do
def __getobj__ # :nodoc:
unless defined?(@delegate_dc_obj)
Expand All @@ -413,18 +424,21 @@ def __getobj__ # :nodoc:
end
@delegate_dc_obj
end

def __setobj__(obj) # :nodoc:
__raise__ ::ArgumentError, "cannot delegate to self" if self.equal?(obj)
@delegate_dc_obj = obj
end
protected_instance_methods.each do |method|
define_method(method, Delegator.delegating_block(method))
protected method
end
public_instance_methods.each do |method|

class_eval(source.join(";"), __FILE__, __LINE__)
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The line number argument __LINE__ in class_eval is incorrect. Since multiple method definitions are joined with semicolons on a single line, all methods will appear to be defined on line 430 in stack traces, making debugging difficult.

Additionally, using __LINE__ here will report the line number of the class_eval call itself, not the line where the method definitions actually start (line 412-414). This will cause confusion when debugging errors in delegated methods.

Copilot uses AI. Check for mistakes.

special.each do |method|
define_method(method, Delegator.delegating_block(method))
end

protected(*protected_instance_methods)
end

klass.define_singleton_method :public_instance_methods do |all=true|
super(all) | superclass.public_instance_methods
end
Expand Down
19 changes: 16 additions & 3 deletions test/test_delegate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,21 @@ def test_override
end

class Parent
def parent_public; end
def parent_public
:public
end

protected

def parent_protected; end
def parent_protected
:protected
end

private

def parent_private; end
def parent_private
:private
end
end

class Child < DelegateClass(Parent)
Expand Down Expand Up @@ -157,6 +163,13 @@ def test_DelegateClass_public_instance_method
assert_instance_of UnboundMethod, Child.public_instance_method(:to_s)
end

def test_call_visibiltiy
obj = Child.new(Parent.new)
assert_equal :public, obj.parent_public
assert_equal :protected, obj.__send__(:parent_protected)
assert_raise(NoMethodError) { obj.__send__(:parent_private) }
end

class IV < DelegateClass(Integer)
attr_accessor :var

Expand Down