Fix Enumerable#sum for Enumerator#lazy

[fatkodima]

Fixes #48712.

The problem was that .first method call, used in the condition, evaluated lazy enumerator, and thus lead to the problematic behavior.

[guilleiguaran]

Don't we need a changelog entry for this fix?

[matthewd]

Lazy is a notable example, but I guess we're just generally wrong to assume it's safe to call first and then restart iteration.

Perhaps we should allow this faster check only for a specific list of known common enumerables, and otherwise always fall back to an implementation that guarantees a single invocation of each.

[fatkodima]

@guilleiguaran Added a changelog entry.

@matthewd If we will check for known safe enumerables, we need to also include Enumerable in this list to not to start generating deprecations for custom enumerable classes (currently these are accepted without deprecations), see

rails/activesupport/test/core_ext/enumerable_test.rb

Line 72 in 4c7f792

def test_sums

But Enumerator::Lazy is also an Enumerable, so that branch will be called for it too and this won't work. We will need a more complex solution 🤔. How about to go ahead with the current implementation, since lazy enumerator is the only one reported so far, and maybe change the implementation to more complex if more cases will be reported?

[matthewd]

My point is that custom Enumerable-implementing classes are not safe to call first on. It's true that this is the first report we've seen, but I'm rather uncomfortable with the idea of noticing a general problem, yet only introducing a specific fix for the one stdlib class we know is affected.

I think I'm specifically suggesting that we need to do something like:

enum = enum_for(:each)
first_value =
  begin
    first_value = enum.next
  rescue StopIteration
    return 0
  end
unless first_value.is_a?(Numeric) || first_value.respond_to?(:coerce)
  # warn
end
Enumerator.produce(first_value) { enum.next }.inject(:+)

or maybe it's easier to just reimplement the inject:

initial = true
a = 0
each do |e|
  if initial
    initial = false
    unless e.is_a?(Numeric) || e.respond_to?(:coerce)
      # warn
    end
    a = e
  else
    a += e
  end
end
a

[fatkodima]

@matthewd Thanks, applied the first suggestion and added you as a coauthor 👍 .

[jonathanhefner]

This is just my opinion, but I feel like @matthewd's second suggestion is more straightforward (and more efficient). If we want a less for-loop-like implementation, we could also use reduce (aka inject) instead of each:

reduce(nil) do |sum, value|
  if sum.nil?
    unless value.is_a?(Numeric) || value.respond_to?(:coerce)
      # warn
    end
    value
  else
    sum + value
  end
end || 0

[fatkodima]

Thanks, looks simpler.

[matthewd]

That assumes the enumerable doesn't start with nil values / sum + value never returns nil.

[matthewd]

(I agree the Enumerator version is probably overcomplicated / needlessly inefficient)

[fatkodima]

That assumes the enumerable doesn't start with nil values / sum + value never returns nil.

When we have nils at the beginning ([nil, nil, 2, 3, 4]), the branch with if sum.nil? will be executed for all of them. When we have nil in the middle somewhere ([1, 2, nil, 3]), we will get an error about not able to coerce, like in native sum. Am I missing something?

[matthewd]

the branch with if sum.nil? will be executed for all of them

That's a behaviour change.

(More obscurely, a redefined NilClass#+ would also deviate.)

[jonathanhefner]

That assumes the enumerable doesn't start with nil values / sum + value never returns nil.

It does, but we want [nil].sum == 0:

rails/activesupport/test/core_ext/enumerable_test.rb

Lines 140 to 142 in f61bcfb

	assert_deprecated do
	assert_equal 0, [nil].sum
	end

I don't think + would purposefully return nil because nil + anything would raise (unless you override NilClass#+, which seems... unwise). But I suppose we could be masking an error here. Alternatively:

first = true

reduce(nil) do |sum, value|
  if first
    unless value.is_a?(Numeric) || value.respond_to?(:coerce)
      # warn
    end

    first = false
    value
  else
    sum + value
  end
end || 0

[matthewd]

we want [nil].sum == 0

Ah, fair.

unless you override NilClass#+, which seems... unwise

Agree, but even while "unwise" might be a reasonable line in main, I think we should aim for unreasonable accomodation in stable branches.

(And I can imagine a risky but loosely-plausible NilClass#+ that exclusively accepts some specific CustomClass as its argument)

[fatkodima]

Updated with the if first variant.

[jonathanhefner]

Thank you, @fatkodima! 1️⃣