An argument that defines __to_int is accepted where to_int is rejected #7014

Summary

Coding conventions > Argument conversion in CONTRIBUTING.md says to ask for
an integer with __to_int in Ruby. __to_int is a public method on Kernel
(src/kernel.c), which Object includes. Writing n.__to_int dispatches on
the argument, and the argument can redefine it.

That is the property Coding conventions > Ruby code > Do not ask an argument what it is warns against:

is_a?, kind_of?, nil?, class and respond_to? are all ordinary
methods and can be redefined, so an argument that answers them dishonestly
decides which branch runs.

The result is that an object defining __to_int is accepted at all twelve
__to_int call sites in the tree, while an object defining to_int is
rejected.

Reproduction

mruby 4.0.0 built from fd6a182 with the
default configuration (build_config/default.rb).

Every method that converts its own argument with __to_int:

class Evil
  def __to_int; 2; end
end

class MyEnum
  include Enumerable
  def each; [10, 20, 30, 40].each { |x| yield x }; self; end
end

"a,b,c,d".split(",", Evil.new)             #=> ["a", "b,c,d"]
[[1, 2], [3, 4], [5, 6]].dig(Evil.new, 0)  #=> 5
[1, 2, 3].combination(Evil.new).to_a       #=> [[1, 2], [1, 3], [2, 3]]
[1, 2, 3].permutation(Evil.new).to_a       #=> [[1, 2], [1, 3], [2, 1], [2, 3], [3, 1], [3, 2]]
[1, 2].repeated_combination(Evil.new).to_a #=> [[1, 1], [1, 2], [2, 2]]
[1, 2].repeated_permutation(Evil.new).to_a #=> [[1, 1], [1, 2], [2, 1], [2, 2]]
(1..9).drop(Evil.new)                      #=> [3, 4, 5, 6, 7, 8, 9]
(1..9).take(Evil.new)                      #=> [1, 2]
(1..9).each_cons(Evil.new).to_a            #=> [[1, 2], [2, 3], [3, 4], [4, 5], [5, 6], [6, 7], [7, 8], [8, 9]]
(1..9).each_slice(Evil.new).to_a           #=> [[1, 2], [3, 4], [5, 6], [7, 8], [9]]
(1..9).first(Evil.new)                     #=> [1, 2]
(1..9).last(Evil.new)                      #=> []
MyEnum.new.first(Evil.new)                 #=> [10, 20]
[1, 2].cycle(Evil.new).to_a                #=> [1, 2, 1, 2]
[9, 8, 7].each.with_index(Evil.new).to_a   #=> [[9, 2], [8, 3], [7, 4]]

Range#last accepts the object like the rest, but returns [] rather than
[8, 9] because of a separate defect in that method, which discards its own
conversion result. That is unrelated to this report and I will send it
separately.

For contrast, an object that defines to_int is rejected, exactly as
Argument conversion describes:

class Polite
  def to_int; 2; end
end

Array.new(Polite.new)  # TypeError: Polite cannot be converted to Integer
[1, 2, 3][Polite.new]  # TypeError: Polite cannot be converted to Integer
"s" * Polite.new       # TypeError: Polite cannot be converted to Integer

C implementations are not reachable this way, since they call
mrb_ensure_int_type() directly instead of dispatching a method:

[1, 2, 3, 4, 5].first(Evil.new)  # TypeError: Evil cannot be converted to Integer

Affected methods

Twelve __to_int call sites, reached by fifteen public methods, because
Array#__combination serves four of them:

• mruby-enum-ext: drop, take, each_cons, each_slice, first, cycle
• mruby-range-ext: first, last
• mruby-array-ext: dig, combination, permutation,
  repeated_combination, repeated_permutation
• mruby-enumerator: with_index
• mruby-regexp: split

Enumerator#each_with_index is not affected, since it passes a literal 0 to
with_index.

Array#dig recurses through n&.dig(*args), and Hash#dig and Struct#dig
do the same, so a nested chain can hand an argument to Array#dig's conversion
without Array#dig being the receiver the caller named:

{a: [10, 20, 30]}.dig(:a, Evil.new)  #=> 30

Why this matters for the stated reasoning

Argument conversion justifies rejecting to_int on the grounds that a method
honouring the protocol would be "more permissive than the tree it sits in,
which is a worse inconsistency than the difference from CRuby". That
inconsistency is already present through __to_int. It is the same
permissiveness under a different method name, and it is reachable from any
object that defines __to_int, including a BasicObject subclass that defines
nothing else.

A comment in the tree states the opposite guarantee

mrbgems/mruby-regexp/mrblib/string_regexp.rb:

# `__to_int` is `mrb_ensure_integer_type()`, which asks the object nothing.

The surrounding code is careful about exactly this hazard: it guards with
Integer === limit because is_a? is redefinable. But limit.__to_int on the
next line does ask the object, and the split case above goes through it.
Only the default implementation of __to_int asks nothing.

Possible directions

I have no strong preference, and the choice looks like yours to make.

1. Dispatch on the receiver instead of on the argument, for example a private
   Kernel method used as __int(n) rather than n.__to_int. The untrusted
   argument then stops deciding, and the change is mechanical across the twelve
   call sites.
2. Accept the exposure and treat redefining __to_int as user error. This is
   the reasoning Do not ask an argument what it is declines to accept for
   is_a?, so it would be worth saying why it is acceptable here.
3. Leave the behavior and add a caveat to Argument conversion.

Happy to prepare a patch once you have picked a direction.

Minor

Argument conversion names mrb_ensure_integer_type() as the C entry point.
__to_int is bound to mrb_ensure_int_type() (src/object.c), which wraps it
and narrows a BigInt result to mrb_int. Both are public API, so it may be
worth naming the one that corresponds to __to_int.