Revert use of argument-forwarding keyword ... in active_record/relation/delegate.rb

[jamesnakagawa]

Reverts a change from 57ace94

generate_method sometimes creates methods which are reserved Ruby keywords. Using ... in combination with a Ruby keyword seems to generate a syntax error which does not occur with *args, &block. It seems this language feature isn't ready for prime-time, at least where dynamically generated code is involved.

To test this error yourself, try in a Ruby console:

class Works
  def true(*args)
    puts(*args)
  end
end

Works.new.true 1, 2, 3
# => 1, 2, 3

class WontWork
  def true(...)
    puts(...)
  end
end

# => freezes

In the context of ActiveRecord:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.string :enum
  end
end

class Post < ActiveRecord::Base
  enum :shown => [ :true, :false ]
end

class BugTest < Minitest::Test
  def test_delegation_syntax
    post = Post.create!
    refute_nil post
  end
end

[kamipo]

Thank you for the PR, but it is not an issue for Rails but for Ruby.

Can you file a bug ticket on the issue tracker?
https://bugs.ruby-lang.org/projects/ruby-master

[byroot]

I just saw the ticket on MRI.

I agree that this is a Ruby bug, but even if it's fixed soon upstream that will stay a problem for a while. It doesn't really please me to say this, but maybe we should check the attribute name against a list of keywords for the time being.

Or indeed revert, but that would pain me as I'm quite happy to save that allocation.

[matthewd]

It doesn't really please me to say this, but maybe we should check the attribute name against a list of keywords for the time being

We do already have similar technology:

rails/activesupport/lib/active_support/core_ext/module/delegation.rb

Lines 10 to 12 in ac65e56

	RUBY_RESERVED_KEYWORDS = %w(alias and BEGIN begin break case class def defined? do
	else elsif END end ensure false for if in module next nil not or redo rescue retry
	return self super then true undef unless until when while yield)

[kamipo]

Define def __#{method}(...) first and then alias #{method} __#{method} also works.

master...kamipo:bypass_argument_forwording_issue

diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb
index 4c2b413a81..6e70eeff46 100644
--- a/activerecord/lib/active_record/relation/delegation.rb
+++ b/activerecord/lib/active_record/relation/delegation.rb
@@ -62,9 +62,11 @@ def generate_method(method)
           if /\A[a-zA-Z_]\w*[!?]?\z/.match?(method)
             definition = RUBY_VERSION >= "2.7" ? "..." : "*args, &block"
             module_eval <<-RUBY, __FILE__, __LINE__ + 1
-              def #{method}(#{definition})
+              def __#{method}(#{definition})
                 scoping { klass.#{method}(#{definition}) }
               end
+              alias #{method} __#{method}
+              undef __#{method}
             RUBY
           else
             define_method(method) do |*args, &block|
diff --git a/activerecord/test/cases/scoping/named_scoping_test.rb b/activerecord/test/cases/scoping/named_scoping_test.rb
index 232e437be9..744668d94c 100644
--- a/activerecord/test/cases/scoping/named_scoping_test.rb
+++ b/activerecord/test/cases/scoping/named_scoping_test.rb
@@ -64,6 +64,11 @@ def test_method_missing_priority_when_delegating
     assert_equal klazz.to.since.to_a, klazz.since.to.to_a
   end
 
+  def test_define_scope_for_reserved_words
+    assert Topic.true.all?(&:approved?), "all objects should be approved"
+    assert Topic.false.none?(&:approved?), "all objects should not be approved"
+  end
+
   def test_scope_should_respond_to_own_methods_and_methods_of_the_proxy
     assert_respond_to Topic.approved, :limit
     assert_respond_to Topic.approved, :count
diff --git a/activerecord/test/models/topic.rb b/activerecord/test/models/topic.rb
index 19fb704d12..b8f4b30bb9 100644
--- a/activerecord/test/models/topic.rb
+++ b/activerecord/test/models/topic.rb
@@ -10,6 +10,9 @@ class Topic < ActiveRecord::Base
   scope :approved, -> { where(approved: true) }
   scope :rejected, -> { where(approved: false) }
 
+  scope :true, -> { where(approved: true) }
+  scope :false, -> { where(approved: false) }
+
   scope :scope_with_lambda, lambda { all }
 
   scope :by_lifo, -> { where(author_name: "lifo") }

[casperisfine]

@kamipo that aliasing could also handle methods that don't match \A[a-zA-Z_]\w*[!?]?\z/ right?

This could be used to simply this whole methods.

[kamipo]

In that case, we should mangling the name, since we could not do e.g. def a/b but could do define_method("a/b").

[casperisfine]

@kamipo yes I was suggesting to use mangling like for attribute methods generation.

[casperisfine]

@kamipo @matthewd looking into all this, I feel like CodeGenerator in AttributesMethods could be refactored to handle all these concerns, moved in AS and be used in boths.

What do you think?

[kamipo]

I've tried to unify the code, but in this time it is hard, since we could not use scoping { klass.#{method}(#{definition}) } if mangling is required, need to use public_send in that case.

[casperisfine]

It can't be fully unified yes. but it could still be simplified.

[kamipo]

I've just tried, I'm not sure it is simple or not though.

e965529

diff --git a/activerecord/lib/active_record/relation/delegation.rb b/activerecord/lib/active_record/relation/delegation.rb
index 6e70eeff46a2..6938789d9aab 100644
--- a/activerecord/lib/active_record/relation/delegation.rb
+++ b/activerecord/lib/active_record/relation/delegation.rb
@@ -60,20 +60,22 @@ def generate_method(method)
           return if method_defined?(method)
 
           if /\A[a-zA-Z_]\w*[!?]?\z/.match?(method)
-            definition = RUBY_VERSION >= "2.7" ? "..." : "*args, &block"
-            module_eval <<-RUBY, __FILE__, __LINE__ + 1
-              def __#{method}(#{definition})
-                scoping { klass.#{method}(#{definition}) }
-              end
-              alias #{method} __#{method}
-              undef __#{method}
-            RUBY
+            definition  = RUBY_VERSION >= "2.7" ? "..." : "*args, &block"
+            method_call = "#{method}(#{definition})"
           else
-            define_method(method) do |*args, &block|
-              scoping { klass.public_send(method, *args, &block) }
-            end
-            ruby2_keywords(method) if respond_to?(:ruby2_keywords, true)
+            definition  = "*args, &block"
+            method_call = "public_send(#{method.inspect}, #{definition})"
           end
+
+          safe_name = "__temp__#{method.to_s.unpack1("h*")}"
+
+          module_eval <<-RUBY, __FILE__, __LINE__ + 1
+            def #{safe_name}(#{definition})
+              scoping { klass.#{method_call} }
+            end
+            alias #{method.to_sym.inspect} #{safe_name}
+            undef #{safe_name}
+          RUBY
         end
       end
     end

[casperisfine]

@kamipo I was thinking something like this: https://gist.github.com/casperisfine/cad5cfe41f83a716c70cf4f3a5bbfe2b

Basically having a helper deal with all the backward compatibility, and name restriction issues, so that it's abstracted away.

However that first rough prototype is quite leaky abstraction wise, so I'm unsure we'd actually be better off.

[kamipo]

Seems it may worth the abstraction, but it is overkill to backport to 6-0-stable for me.

[casperisfine]

Oh right CodeGenerator was only introduced on master. So not a good solution for 6.0.

[kamipo]

I've post a PR with the branch #39260 (comment).

#39280