Skip to content

Simpler and faster check for the delegation fastpath #39

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 10, 2025
Merged

Simpler and faster check for the delegation fastpath #39

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
2 changes: 1 addition & 1 deletion forwardable.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Gem::Specification.new do |spec|
spec.licenses = ["Ruby", "BSD-2-Clause"]

spec.required_ruby_version = '>= 2.4.0'
spec.files = ["forwardable.gemspec", "lib/forwardable.rb", "lib/forwardable/impl.rb"]
spec.files = ["forwardable.gemspec", "lib/forwardable.rb"]
spec.bindir = "exe"
spec.executables = []
spec.require_paths = ["lib"]
Expand Down
34 changes: 14 additions & 20 deletions lib/forwardable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@
# +delegate.rb+.
#
module Forwardable
require 'forwardable/impl'

# Version of +forwardable.rb+
VERSION = "1.3.3"
VERSION.freeze
Expand Down Expand Up @@ -206,37 +204,33 @@ def self._delegator_method(obj, accessor, method, ali)
if Module === obj ?
obj.method_defined?(accessor) || obj.private_method_defined?(accessor) :
obj.respond_to?(accessor, true)
accessor = "#{accessor}()"
accessor = "(#{accessor}())"
end

args = RUBY_VERSION >= '2.7' ? '...' : '*args, &block'
method_call = ".__send__(:#{method}, #{args})"
if _valid_method?(method)
if method.match?(/\A[_a-zA-Z]\w*[?!]?\z/)
Copy link

@paracycle paracycle Nov 24, 2025

Choose a reason for hiding this comment

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

But this will fail if the method has any Unicode characters in its name, no?

Copy link

@paracycle paracycle Nov 24, 2025

Choose a reason for hiding this comment

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

By the way, if this is trying to check if the method name is a valid identifier then Prism has methods for doing that check out of the box. Maybe we should use those

Copy link
Member Author

@byroot byroot Nov 24, 2025

Choose a reason for hiding this comment

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

It's checking for two things, first that it's a valid name, but also that it can be called with ..

e.g. foo= is valid, but _.foo = *args, &block isn't.

But this will fail if the method has any Unicode characters in its name, no?

Yes, but the idea is to be conservative, the regexp can be improved (or we could use prism) if we want, but as long as 99.9% of methods go in the fast path, it's fine IMO.

loc, = caller_locations(2,1)
pre = "_ ="
mesg = "#{Module === obj ? obj : obj.class}\##{ali} at #{loc.path}:#{loc.lineno} forwarding to private method "
method_call = "#{<<-"begin;"}\n#{<<-"end;".chomp}"
begin;
unless defined? _.#{method}
::Kernel.warn #{mesg.dump}"\#{_.class}"'##{method}', uplevel: 1
_#{method_call}
else
_.#{method}(#{args})
end
end;
method_call = <<~RUBY.chomp
if defined?(_.#{method})
Copy link
Contributor

@jeremyevans jeremyevans Nov 24, 2025

Choose a reason for hiding this comment

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

Before Ruby 3, using defined? for methods does not work correctly if the method has been refined but the refinement is not activated. See https://bugs.ruby-lang.org/issues/16932 . I found this out the hard way in Sequel: jeremyevans/sequel#1919 .

In this case, defined? would return truthy if the method had an unactivated refinement. If the method being refined was private and the refinement method is public, I believe this would cause the fast path to be used in a case where it would fail.

Example:

module M
  refine Object do
    public :puts
  end
end

o = Object.new
o.puts '' if defined?(o.puts)

On Ruby < 3, we could probably use something like Kernel === _ && _.respond_to?(:#{method}) . Or we could bump the required_ruby_version to 3.0.

Copy link
Member Author

@byroot byroot Nov 24, 2025

Choose a reason for hiding this comment

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

I'm fine with making that change, but note that I only moved the defined? so it's a pre-existing bug, probably better to handle it in a dedicated PR?

Copy link
Contributor

@jeremyevans jeremyevans Nov 24, 2025

Choose a reason for hiding this comment

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

Sure, that makes sense.

_.#{method}(#{args})
else
::Kernel.warn #{mesg.dump}"\#{_.class}"'##{method}', uplevel: 1
_#{method_call}
end
RUBY
end

_compile_method("#{<<-"begin;"}\n#{<<-"end;"}", __FILE__, __LINE__+1)
begin;
eval(<<~RUBY, nil, __FILE__, __LINE__ + 1)
proc do
def #{ali}(#{args})
#{pre}
begin
#{accessor}
end#{method_call}
#{pre}#{accessor}
#{method_call}
end
end
end;
RUBY
end
end

Expand Down
17 changes: 0 additions & 17 deletions lib/forwardable/impl.rb
View file
Open in desktop

This file was deleted.