Bug #2495: Matrix: Vector#each2 should check its argument

• Status: Closed
• Assignee: keiju (Keiju Ishitsuka)
• Target version: 1.9.2
• ruby -v: ruby 1.9.2dev (2009-12-19 trunk 26121) [x86_64-darwin10.2.0]

[marcandre (Marc-Andre Lafortune)]

=begin
$ rubydev -r matrix -e 'p Vector[*1..4].each2(nil){|x, y| p "#{x}, #{y}"}'
/usr/local/rubydev/lib/ruby/1.9.1/matrix.rb:1149:in each2': undefined method size' for nil:NilClass (NoMethodError)

$ rubydev -r matrix -e 'p Vector[*1..4].each2(42){|x, y| p "#{x}, #{y}"}'
/usr/local/rubydev/lib/ruby/1.9.1/matrix.rb:1149:in each2': Vector dimension mismatch (ExceptionForMatrix::ErrDimensionMismatch) from -e:1:in '

$ rubydev -r matrix -e 'p Vector[*1..8].each2(42){|x, y| p "#{x}, #{y}"}'
"1, 0"
"2, 1"
"3, 0"
"4, 1"
"5, 0"
"6, 1"
"7, 0"
"8, 0"

(or vice versa, if on a 32 bit platform)
=end

[mame (Yusuke Endoh)]

=begin
Hi,

2009/12/19 Marc-Andre Lafortune [e-mail]:

Bug #2495: Matrix: Vector#each2 should check its argument
http://redmine.ruby-lang.org/issues/show/2495

Author: Marc-Andre Lafortune
Status: Open, Priority: Normal
Assigned to: Keiju Ishitsuka, Category: lib, Target version: 1.9.2
ruby -v: ruby 1.9.2dev (2009-12-19 trunk 26121) [x86_64-darwin10.2.0]

$ rubydev -r matrix -e 'p Vector[*1..4].each2(nil){|x, y| p "#{x}, #{y}"}'
/usr/local/rubydev/lib/ruby/1.9.1/matrix.rb:1149:in each2': undefined method size' for nil:NilClass (NoMethodError)

$ rubydev -r matrix -e 'p Vector[*1..4].each2(42){|x, y| p "#{x}, #{y}"}'
/usr/local/rubydev/lib/ruby/1.9.1/matrix.rb:1149:in each2': Vector dimension mismatch (ExceptionForMatrix::ErrDimensionMismatch)        from -e:1:in '

$ rubydev -r matrix -e 'p Vector[*1..8].each2(42){|x, y| p "#{x}, #{y}"}'
"1, 0"
"2, 1"
"3, 0"
"4, 1"
"5, 0"
"6, 1"
"7, 0"
"8, 0"

Agreed.

diff --git a/lib/matrix.rb b/lib/matrix.rb
index e6f5fe1..3a8c76a 100644
--- a/lib/matrix.rb
+++ b/lib/matrix.rb
@@ -1144,6 +1144,9 @@ class Vector
# Iterate over the elements of this vector and +v+ in conjunction.
#
def each2(v) # :yield: e1, e2

• unless v.is_a?(Array) || v.is_a?(Vector)

•  Vector.Raise TypeError, v.class, "Array or Vector"
  

• end
  Vector.Raise ErrDimensionMismatch if size != v.size
  return to_enum(:each2, v) unless block_given?
  size.times do |i|

--
Yusuke ENDOH [e-mail]

=end

[mame (Yusuke Endoh)]

• Status changed from Open to Rejected

=begin
According to [ruby-dev:40237] and [ruby-dev:40267], it is the fate of
duck typing and compatibility.

[ruby-dev:40237] by Keiju (the author of lib/matrix.rb):

• Integer#[] and #size have wrong names from the aspect of duck typing.
• Type checking to restrict only Vector and Array will prevent NVector
  of NArray.

[ruby-dev:40267] by matz:

• Integer#[] and #size are older than the concept of duck typing, and
  it is too late to change their names.
• From the aspect of duck typing, type checking should not be done and
  we should accept such an error of [ruby-core:27901].

--
Yusuke ENDOH [e-mail]
=end

[marcandre (Marc-Andre Lafortune)]

• Status changed from Rejected to Open

=begin
Hi

On Sun, Jan 31, 2010 at 10:12 AM, Yusuke Endoh [e-mail] wrote:

According to [ruby-dev:40237] and [ruby-dev:40267], it is the fate of
duck typing and compatibility.

Thank you for relaying this discussion.

I am all in favor of duck typing. I believe there is a solution here, which would be to insure that the argument responds to :each (or to :to_a.) Indeed, the name of the method is :each2, so I feel it is quite reasonable to request that the argument responds to :each. This would solve the issue for both Integer and String.

Finally, I feel it would be superior to raise a TypeError instead of a NoMethodError in a case where duck typing fails, in a similar fashion to "$stdout = nil" which raises TypeError: $stdout must have write method, NilClass given

So I'd recommend something like:

diff --git a/lib/matrix.rb b/lib/matrix.rb
index e6f5fe1..3a8c76a 100644
--- a/lib/matrix.rb
+++ b/lib/matrix.rb
@@ -1144,6 +1144,9 @@ class Vector
# Iterate over the elements of this vector and +v+ in conjunction.
#
def each2(v) # :yield: e1, e2

• unless v.respond_to?(:each) && v.respond_to?(:size) && v.respond_to?(:[])

•  raise TypeError, "Argument should be array-like"
  

• end
  Vector.Raise ErrDimensionMismatch if size != v.size
  return to_enum(:each2, v) unless block_given?
  size.times do |i|

=end

[mame (Yusuke Endoh)]

=begin
Hi,

2010/2/2 Marc-Andre Lafortune [e-mail]:

Finally, I feel it would be superior to raise a TypeError instead of a NoMethodError in a case where duck typing fails, in a similar fashion to "$stdout = nil" which raises TypeError: $stdout must have write method, NilClass given

I really understand your intuition and even wrote the patch once,
but to achieve it, I guess that very many type checks will be needed,
not only in Vector#each2 but also in other libraries. It may not be
realistic.
I think that `the fate of duck typing' includes such a error.

Now that I think about it, a type means "what methods its instance
responds to." Thus, it may be strange to distinguish TypeError and
NoMethodError.

That's just my personal opinion, though.
Matz or Keiju, will you give us your opinion?

--
Yusuke ENDOH [e-mail]

=end

[marcandre (Marc-Andre Lafortune)]

=begin

Hi,

On Mon, Feb 1, 2010 at 10:44 PM, Yusuke ENDOH [e-mail] wrote:

Matz or Keiju, will you give us your opinion?

Ist Keiju is even registered on the ruby-core mailing list?

I'd like to point out an alternate implementation can use #each instead of #[], and at least avoid the non-sensical results on integers. Example:

diff --git a/lib/matrix.rb b/lib/matrix.rb
index d452bf4..f9b2f58 100644
--- a/lib/matrix.rb
+++ b/lib/matrix.rb
@@ -1146,8 +1146,10 @@ class Vector
def each2(v) # :yield: e1, e2
Vector.Raise ErrDimensionMismatch if size != v.size
return to_enum(:each2, v) unless block_given?

• size.times do |i|

•  yield @elements[i], v[i]
  

• i = 0
• v.each do |elem|

•  yield @elements[i], elem
  

•  i+=1
  

  end
  self
  end

I'm all for duck typing, as long as we give an idea of what type of "quacking" we're expecting. I would still suggest adding at the beginning of the method:

unless v.respond_to?(:size) && v.respond_to?(:each)
raise TypeError, "Argument must respond to :size and :each"
end

=end

[matz (Yukihiro Matsumoto)]

=begin
Hi,

In message "Re: [ruby-core:28399] [Bug #2495] Matrix: Vector#each2 should check its argument"
on Tue, 2 Mar 2010 16:52:31 +0900, Marc-Andre Lafortune [e-mail] writes:

|On Mon, Feb 1, 2010 at 10:44 PM, Yusuke ENDOH [e-mail] wrote:
|> Matz or Keiju, will you give us your opinion?
|
|Ist Keiju is even registered on the ruby-core mailing list?

|I'd like to point out an alternate implementation can use #each instead of #[], and at least avoid the non-sensical results on integers. Example:

Accepted, but I prefer each_with_index though.

|I'm all for duck typing, as long as we give an idea of what type of "quacking" we're expecting. I would still suggest adding at the beginning of the method:
|
|unless v.respond_to?(:size) && v.respond_to?(:each)
| raise TypeError, "Argument must respond to :size and :each"
|end

If we change the policy and decide to add respond_to? check to
methods, we add infinite number of checks everywhere (exaggerated of
course). I am not in a mood to make such big change during 1.9.2
period.

						matz.

=end

[matz (Yukihiro Matsumoto)]

=begin
Hi,

In message "Re: [ruby-core:28401] Re: [Bug #2495] Matrix: Vector#each2 should check its argument"
on Tue, 2 Mar 2010 17:29:46 +0900, Yukihiro Matsumoto [e-mail] writes:

||I'd like to point out an alternate implementation can use #each instead of #[], and at least avoid the non-sensical results on integers. Example:
|
|Accepted, but I prefer each_with_index though.

I tried, but test failed, because Vector does not know how to each.
I am not sure if it's sufficient to make Vector enumerable.

						matz.

=end

[keiju (Keiju Ishitsuka)]

• Status changed from Open to Closed

=begin
I think that there is not the necessity to change.
However, I acknowledge that it is wrong a result for Integer argument.
And it will happen often mistake Integer pass.

I add type check for Integer only.

=end