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

Fix Enumerable#sum redefined warning #28781

Merged
merged 1 commit into from Apr 18, 2017
Merged

Fix Enumerable#sum redefined warning #28781

merged 1 commit into from Apr 18, 2017

Conversation

mtsmfm
Copy link
Contributor

@mtsmfm mtsmfm commented Apr 17, 2017

The purpose of this PR is to remove redefining warning, not speed-up Enumerable#sum.

@amatsuda asked me to fix this warning.

We (I and @amatsuda) know #25202 already addressed this warning but there is no action for a while.

We think Rails 5.1 should be released with no warning.


If we require 'active_support/core_ext/enumerable' on Ruby 2.4,
we'll see following warn because Enumerable#sum and Array#sum
are added in Ruby 2.4.

rails/rails/activesupport/lib/active_support/core_ext/enumerable.rb:20: warning: method redefined; discarding old sum

The minimal way to fix the warning is alias sum sum.

$ ruby -v
ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux]

$ ruby -w -e "def a; end; def a; end"
-e:1: warning: method redefined; discarding old a
-e:1: warning: previous definition of a was here

$ ruby -w -e "def a; end; alias a a; def a; end"

But this behavior is not intended. (@amatsuda was told by @ko1)
So we should use alias as a meaningful way.

Ruby 2.4's sum (orig_sum) assumes an identity is 0 when we omit identity
so we can delegate to orig_sum with explicit identity only.
In a strict sense, we can detect identity by check instance's class
but we don't care at this time about that because calling Enumerable#sum is rare.
In many cases, we will call Array#sum.

@rails-bot
Copy link

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@pixeltrix
Copy link
Contributor

Do we even need to define it for Ruby 2.4? I assume there must be, but I can't spot any difference between our implementation and Ruby 2.4's implementation in terms of outcomes for a given set of arguments.

@mtsmfm
Copy link
Contributor Author

mtsmfm commented Apr 18, 2017

I can't spot any difference between our implementation and Ruby 2.4's implementation in terms of outcomes for a given set of arguments.

Sorry, I noticed a mistake about feature detection 🙇‍♂️
I'll fix that 💨


The difference is following case:

$ ruby -e '(?a..?b).sum'
begin
  (?a..?b).sum
rescue => e
  p e # => #<TypeError: String can't be coerced into Integer>
end

require 'active_support/core_ext/enumerable'

p (?a..?b).sum # => "ab"

if Enumerable.instance_methods(false).include?(:sum) && !((?a..?b).sum rescue false)
# We can't use Refinements here because Refinements with Module which will be prepended
# doesn't work well https://bugs.ruby-lang.org/issues/13446
alias :orig_sum :sum
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we run only the test, it will pass.

$ bundle exec ruby -I test test/core_ext/enumerable_test.rb
Run options: --seed 41825

# Running:

...........

Finished in 0.001698s, 6477.8854 runs/s, 70667.8405 assertions/s.

11 runs, 120 assertions, 0 failures, 0 errors, 0 skips

If we apply following patch, the test fill fail 😨

diff --git a/activesupport/test/core_ext/enumerable_test.rb b/activesupport/test/core_ext/enumerable_test.rb
index 4f1ab993b8..2a786fc874 100644
--- a/activesupport/test/core_ext/enumerable_test.rb
+++ b/activesupport/test/core_ext/enumerable_test.rb
@@ -1,6 +1,7 @@
 require "abstract_unit"
 require "active_support/core_ext/array"
 require "active_support/core_ext/enumerable"
+require "active_support/core_ext/object/json"

 Payment = Struct.new(:price)
 ExpandedPayment = Struct.new(:dollars, :cents)
  1) Error:
EnumerableTests#test_sums:
NoMethodError: undefined method `orig_sum' for #<EnumerableTests::GenericEnumerable:0x00000002898f58>
    /app/activesupport/lib/active_support/core_ext/enumerable.rb:35:in `sum'
    test/core_ext/enumerable_test.rb:51:in `test_sums'


  2) Error:
EnumerableTests#test_empty_sums:
NoMethodError: undefined method `orig_sum' for #<EnumerableTests::GenericEnumerable:0x00000002893238 @values=[]>
    /app/activesupport/lib/active_support/core_ext/enumerable.rb:35:in `sum'
    test/core_ext/enumerable_test.rb:97:in `test_empty_sums'


  3) Error:
EnumerableTests#test_range_sums:
NoMethodError: undefined method `orig_sum' for 1..4:Range
    /app/activesupport/lib/active_support/core_ext/enumerable.rb:35:in `sum'
    /app/activesupport/lib/active_support/core_ext/enumerable.rb:127:in `sum'
    test/core_ext/enumerable_test.rb:112:in `test_range_sums'

11 runs, 83 assertions, 0 failures, 3 errors, 0 skips

The cause is using refinements with prepend.

using Module.new {
  refine Enumerable do
    alias :orig_sum :sum
  end
}

module Enumerable
  def sum(*args)
    orig_sum(*args)
  end
end

class GenericEnumerable
  include Enumerable

  def each
  end
end

# GenericEnumerable.new.sum # if we uncomment this line, `GenericEnumerable#sum` will work
Enumerable.prepend(Module.new) # if we comment out this line, `GenericEnumerable#sum` will work

p GenericEnumerable.new.sum # undefined method `orig_sum' for #<GenericEnumerable:0x0000000127c120 @values=[1, 2, 3]> (NoMethodError)

It may be not intended behavior (I created the tickect https://bugs.ruby-lang.org/issues/13446).

Anyway, we can't use refinements here 😢

Copy link
Member

Choose a reason for hiding this comment

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

Let's consider a longer name, then: if we have to pollute Enumerable, we want something that couldn't possibly conflict with a user's own monkeypatching strategy, for example.

original_sum_with_required_identity? Or maybe that's too long.

Either way, we should also make it private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We call internal method for Hash _deep_transform_keys_in_object.

def _deep_transform_keys_in_object(object, &block)

So let's prepend _.

Copy link
Member

Choose a reason for hiding this comment

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

Array#sum is also using refinements (571f0f3). If anyone use prepend to Array, is it affected by https://bugs.ruby-lang.org/issues/13446?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Strangely but) no it's not 🙃

using Module.new {
  refine Array do
    alias :orig_sum :sum
  end
}

class Array
  def sum(*args)
    orig_sum(*args)
  end
end

Array.prepend(Module.new)

p Array.new.sum #=> 0

Copy link
Member

Choose a reason for hiding this comment

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

weird... thanks anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kamipo I think this is because Array is a class but Enumerable is a module

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's a good point!

If we require 'active_support/core_ext/enumerable' on Ruby 2.4,
we'll see following warning because `Enumerable#sum` and `Array#sum`
are added in Ruby 2.4.

```
rails/rails/activesupport/lib/active_support/core_ext/enumerable.rb:20: warning: method redefined; discarding old sum
```

The minimal way to fix the warning is `alias sum sum`.

```
$ ruby -v
ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux]

$ ruby -w -e "def a; end; def a; end"
-e:1: warning: method redefined; discarding old a
-e:1: warning: previous definition of a was here

$ ruby -w -e "def a; end; alias a a; def a; end"
```

But this behavior is not intended. (@amatsuda was told by @ko1)
So we should use `alias` as a meaningful way.

Ruby 2.4's `sum`  (`orig_sum`) assumes an `identity` is `0` when we omit `identity`
so we can delegate to `orig_sum` with explicit `identity` only.
In a strict sense, we can detect `identity` by check instance's class
but we don't care at this time about that because calling `Enumerable#sum` is rare.
In many cases, we will call `Array#sum`.
@mtsmfm
Copy link
Contributor Author

mtsmfm commented Apr 18, 2017

Test is failed but it seems not related to this PR.

@rafaelfranca rafaelfranca merged commit c30eddb into rails:master Apr 18, 2017
rafaelfranca added a commit that referenced this pull request Apr 18, 2017
Fix Enumerable#sum redefined warning
rafaelfranca added a commit that referenced this pull request Apr 18, 2017
Fix Enumerable#sum redefined warning
@rafaelfranca
Copy link
Member

Backported in a7f6200 and 52ff5de

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

Successfully merging this pull request may close these issues.

None yet

8 participants