mruby-regexp: read the real type of String#split's limit - #7004

Related to #7003, which 8f85a84 already settled by dropping the to_int dispatch. This
pull request does not close that issue. It is the follow-up 8f85a84 deliberately left in
place: the is_a? guard standing in front of the conversion. Rebased onto that commit. The
stray-paren commit is gone, since the branch that raised that message no longer exists.

String#split's limit argument is converted in the override this gem installs over the
core method. The conversion is guarded by limit.is_a?(Integer), which asks the argument
for its own type. is_a? is redefinable, so an argument claiming to be an Integer skips the
conversion entirely and reaches the split loop as itself.

What happens next depends on which operators that argument answers. With none, the failure
surfaces as a NoMethodError naming an internal comparison, which says nothing about the
limit being wrong:

class FakeInt
  def is_a?(klass)
    true
  end
end

"a,b,c".split(/,/, FakeInt.new)
# CRuby: TypeError (no implicit conversion of FakeInt into Integer)
# mruby: NoMethodError (undefined method '>' for FakeInt)

With ==, > and - answered, there is no error at all: the loop runs to completion with
a non-Integer limit and returns a plausible wrong result.

class Cmp
  def is_a?(klass)
    true
  end

  def ==(other)
    false
  end

  def >(other)
    true
  end

  def -(other)
    1
  end
end

"a,b,c".split(/,/, Cmp.new)
# CRuby: TypeError (no implicit conversion of Cmp into Integer)
# mruby: ["a", "b,c"]

This is the same hole #7001 closed for the pattern argument, left open one argument over.

The two halves of the same method disagree

The conversion sits ahead of the delegation to __split, so a String or nil pattern runs
through it too. It survives only because the core implementation converts the limit again in
C, where the argument gets no say:

"a,b,c".split(",",  FakeInt.new)   # TypeError (FakeInt cannot be converted to Integer)
"a,b,c".split(/,/, FakeInt.new)    # NoMethodError (undefined method '>' for FakeInt)

Two calls to one method, one argument, two answers. The regexp path is the one with no C
conversion behind it, and it is the one that gets it wrong.

Fix

Module#=== reads the real type and cannot be redefined, substituted for the one is_a?
the limit block still has:

-    if limit_given && !limit.is_a?(Integer)
+    # `is_a?` is redefinable, so a limit claiming to be an Integer would skip
+    # that conversion and reach the arithmetic below as itself. `Module#===`
+    # reads the real type and cannot be redefined.
+    if limit_given && !(Integer === limit)
       limit = limit.__to_int
     end

This is the substitution #7001 made for the pattern argument a few lines below, so the two
guards in this method now read the same way. 8f85a84 collapsed the body behind this guard
to a single __to_int, and that call is exactly what a redefined is_a? skips.

Dropping the guard altogether and writing limit = limit.__to_int if limit_given would
close the same hole with no redefinable call at all. It is not what this does: 8f85a84
kept that line deliberately, and the substitution above is the shape #7001 already
established for this method. One measured consequence of keeping it is recorded below.

What keeping the guard leaves behind

A BigInt limit is a real Integer, so it passes the guard under either spelling and never
reaches __to_int. The regexp path then runs the loop with it, while the String path
converts it in C and raises. Measured on a plain host build, mruby-bigint being part of
math.gembox and so of default.gembox:

big = 2 ** 70
"a,b,c".split(/,/, big)   # mruby: ["a", "b", "c"]   CRuby: RangeError
"a,b,c".split(",",  big)  # mruby: RangeError        CRuby: RangeError

This is the same two-halves disagreement described above, on a different argument, and it
predates this pull request: limit.is_a?(Integer) answers true for a BigInt exactly as
Integer === limit does, so the change neither introduces nor closes it. Removing the guard
would close it, by sending every limit through mrb_ensure_int_type(), but that is a
behaviour change on a case no test covers, and a wider question than the redefinable guard
this pull request is about. Recorded here rather than folded in, and happy to file it
separately if it is worth fixing.

Test

Added to mrbgems/mruby-regexp/test/regexp.rb:

class StringSplitLimitIsALiar
  def is_a?(klass)
    true
  end
end

class StringSplitLimitComparable
  def is_a?(klass)
    true
  end

  def ==(other)
    false
  end

  def >(other)
    true
  end

  def -(other)
    1
  end
end

assert("String#split limit cannot pose as an Integer") do
  # `is_a?` is redefinable, so a limit claiming to be an Integer used to skip
  # the conversion and reach the split loop as itself.
  assert_raise(TypeError) { "a,b,c".split(/,/, StringSplitLimitIsALiar.new) }
  # The String pattern delegates to __split, which converts the limit again in
  # C, so this one held before the fix too. Asserted so that the two halves of
  # the method stay pinned to the same answer.
  assert_raise(TypeError) { "a,b,c".split(",", StringSplitLimitIsALiar.new) }

  # Answering the operators the loop uses used to produce a wrong result
  # instead of an error.
  assert_raise(TypeError) { "a,b,c".split(/,/, StringSplitLimitComparable.new) }
end

The two classes cover distinct pre-fix paths: StringSplitLimitIsALiar blew up on
limit > 0, StringSplitLimitComparable returned a wrong array with no error at all. They
are named top-level classes rather than anonymous ones because that is how this file already
writes an is_a? liar, in StringMatchIsALiar a few hundred lines above.

The String#split with regexp limit test 8f85a84 rewrote is untouched. The
respond_to? liar it added is a different route from the is_a? one pinned here.

Verified on a default host build at 8f85a8413:

• With this commit: 1941 tests, 1923 OK, 0 failures, 0 crashes, plus 105 bintests.
• With the test kept and the guard reverted to limit.is_a?(Integer), rake test reports
  Fail: String#split limit cannot pose as an Integer, so the test pins the guard rather
  than passing either way.

Comparison against CRuby 4.0.6

Run side by side on the four cases the guard governs:

After the change the exception class agrees in every case and the return value agrees in
every case. What is left is wording, from mrb_ensure_integer_type():

mruby: TypeError: FakeInt cannot be converted to Integer
CRuby: TypeError: no implicit conversion of FakeInt into Integer

That is the wording 8f85a84 chose deliberately for every non-Integer limit, and this
change does not touch it.

Unaffected and green: an ordinary Integer limit, a Float limit, nil, and limits of 0, 1
and -1.