Skip to content

Conversation

@hsbt
Copy link
Member

@hsbt hsbt commented Dec 17, 2025

The original PR is #46

@byroot This change failed with https://github.com/ruby/ruby/blob/master/spec/ruby/library/delegate/delegate_class/instance_method_spec.rb#L16

1)
DelegateClass.instance_method returns a method object for protected instance methods of the delegated class ERROR
NoMethodError: protected method 'prot' called for an instance of DelegateSpecs::Simple
/Users/hsbt/Documents/github.com/ruby/ruby/spec/ruby/library/delegate/fixtures/classes.rb:14:in 'DelegateSpecs::Simple#method_missing'
/Users/hsbt/Documents/github.com/ruby/ruby/lib/delegate.rb:430:in 'prot'
/Users/hsbt/Documents/github.com/ruby/ruby/spec/ruby/library/delegate/delegate_class/instance_method_spec.rb:19:in 'Method#call'
/Users/hsbt/Documents/github.com/ruby/ruby/spec/ruby/library/delegate/delegate_class/instance_method_spec.rb:19:in 'block (2 levels) in <top (required)>'
/Users/hsbt/Documents/github.com/ruby/ruby/spec/ruby/library/delegate/delegate_class/instance_method_spec.rb:4:in '<top (required)>'

Copilot AI review requested due to automatic review settings December 17, 2025 03:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR reapplies changes from PR #46 that attempted to optimize method delegation by using class_eval with string interpolation instead of define_method for most methods. The original PR failed due to a test failure where protected methods could not be properly delegated.

Key Changes:

  • Methods are partitioned into "normal" (simple identifier names) and "special" (operators and other special syntax) categories
  • Normal methods use class_eval for definition, while special methods continue using define_method
  • Protected method marking is moved to after all method definitions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

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.
lib/delegate.rb Outdated
public_instance_methods -= ignores

instance_methods = (public_instance_methods + protected_instance_methods)
normal, special = instance_methods.partition { |m| m.match?(/\A[a-zA-Z]\w*[!\?]?\z/) }
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 regex pattern /\A[a-zA-Z]\w*[!\?]?\z/ is too restrictive. It requires methods to start with a letter, but Ruby allows method names to start with underscores (e.g., _internal_method). These valid Ruby methods would be incorrectly classified as "special" and handled differently.

The pattern should be /\A[a-zA-Z_]\w*[!\?]?\z/ to also match methods starting with underscores.

Suggested change
normal, special = instance_methods.partition { |m| m.match?(/\A[a-zA-Z]\w*[!\?]?\z/) }
normal, special = instance_methods.partition { |m| m.match?(/\A[a-zA-Z_]\w*[!\?]?\z/) }

Copilot uses AI. Check for mistakes.
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.
Comment on lines +412 to +413
source = normal.map do |method|
"def #{method}(...); __getobj__.#{method}(...); end"
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 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.
@byroot
Copy link
Member

byroot commented Dec 17, 2025

Ah indeed. __send__ should be used for protected methods, I can fix that.

This reverts commit fc2bd04.

Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
@byroot
Copy link
Member

byroot commented Dec 17, 2025

I added a regression test and fixed the PR.

@hsbt hsbt merged commit d1c82ca into master Dec 18, 2025
26 checks passed
@hsbt hsbt deleted the reapply-gh-46 branch December 18, 2025 04:07
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

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

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.

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.

4 participants