Bug #2495
closedMatrix: Vector#each2 should check its argument
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
Updated by mame (Yusuke Endoh) almost 15 years ago
=begin
Hi,
2009/12/19 Marc-Andre Lafortune redmine@ruby-lang.org:
Bug #2495: Matrix: Vector#each2 should check its argument
http://redmine.ruby-lang.org/issues/show/2495Author: 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:ineach2': 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:ineach2': 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 mame@tsg.ne.jp
=end
Updated by mame (Yusuke Endoh) almost 15 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 mame@tsg.ne.jp
=end
Updated by marcandre (Marc-Andre Lafortune) almost 15 years ago
- Status changed from Rejected to Open
=begin
Hi
On Sun, Jan 31, 2010 at 10:12 AM, Yusuke Endoh redmine@ruby-lang.org 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
Updated by mame (Yusuke Endoh) almost 15 years ago
=begin
Hi,
2010/2/2 Marc-Andre Lafortune redmine@ruby-lang.org:
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 mame@tsg.ne.jp
=end
Updated by marcandre (Marc-Andre Lafortune) over 14 years ago
=begin
Hi,
On Mon, Feb 1, 2010 at 10:44 PM, Yusuke ENDOH mame@tsg.ne.jp 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
-
endi+=1
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
Updated by matz (Yukihiro Matsumoto) over 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 redmine@ruby-lang.org writes:
|On Mon, Feb 1, 2010 at 10:44 PM, Yusuke ENDOH mame@tsg.ne.jp 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
Updated by matz (Yukihiro Matsumoto) over 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 matz@ruby-lang.org 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
Updated by keiju (Keiju Ishitsuka) over 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