Project

General

Profile

Actions

Bug #2495

closed

Matrix: Vector#each2 should check its argument

Added by marcandre (Marc-Andre Lafortune) about 14 years ago. Updated almost 13 years ago.

Status:
Closed
Target version:
ruby -v:
ruby 1.9.2dev (2009-12-19 trunk 26121) [x86_64-darwin10.2.0]
Backport:
[ruby-core:27223]

Description

=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

Actions #1

Updated by mame (Yusuke Endoh) about 14 years ago

=begin
Hi,

2009/12/19 Marc-Andre Lafortune :

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

=end

Actions #2

Updated by mame (Yusuke Endoh) about 14 years ago

  • 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
=end

Actions #3

Updated by marcandre (Marc-Andre Lafortune) about 14 years ago

  • Status changed from Rejected to Open

=begin
Hi

On Sun, Jan 31, 2010 at 10:12 AM, Yusuke Endoh 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

Actions #4

Updated by mame (Yusuke Endoh) about 14 years ago

=begin
Hi,

2010/2/2 Marc-Andre Lafortune :

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

=end

Actions #5

Updated by marcandre (Marc-Andre Lafortune) about 14 years ago

=begin

Hi,

On Mon, Feb 1, 2010 at 10:44 PM, Yusuke ENDOH 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

Actions #6

Updated by matz (Yukihiro Matsumoto) about 14 years ago

=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 writes:

|On Mon, Feb 1, 2010 at 10:44 PM, Yusuke ENDOH 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

Actions #7

Updated by matz (Yukihiro Matsumoto) about 14 years ago

=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 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

Actions #8

Updated by keiju (Keiju Ishitsuka) almost 14 years ago

  • 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

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0