tfpt review "/shelveset:error;REDMOND\sborde" Microsoft.Scripting: Change in DefaultBinder is to make the binder exception processing logic be extensible so that languages can customize the type of exception thrown. IronRuby: Improves error reporting in IronRuby by checking the results of the bind failure. This allowed many Math tests to be enabled. Also, added a DefaultProtocol for to_f. This allowed more Math tests to be enabled as you can now pass the string "1.0" to a function expecting a "Float". I have added test cases for the Float conversion rules in core\math\acos_spec.rb. I have not copied this to the other specs as that would be hard to maintain. We will look at factoring such conversion checks into a utility library so that they will be much easier to use. Fixes some String bugs - String.#inspect on string subtype should return a string, and passing wrong argument types to Strings#[]= should cause TypeError instead of ArgumentError. This is where I started and it put me in the direction of the fixes above. Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext? -------------- next part -------------- A non-text attachment was scrubbed... Name: error.diff Type: application/octet-stream Size: 76269 bytes Desc: error.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20090123/fa79af49/attachment-0001.obj>
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?
Jim, using should_not_receive sounds like a good idea. Will do the change.
But we still need all those specs including the one using hijack_String_to_f. It
is not testing method dispatch. Ruby method dispatch only controls which method
body will be called. It does not do argument conversions. That is handled by the
target method which is free to use to_f to convert the argument to a float, but
is not required to. The specs below will ensure that the library method does
follow the right convention/protocol for converting the argument to a float.
Does that make sense?
Also, I had this comment:
/// <summary>
/// An overload with a block should get precedence. However,
Microsoft.Scripting.Action.Calls.MethodBinder
/// gives precedence to overloads with a matching number of arguments as the
actual arguments. Consider Object#eval
/// which effectively has the following two overloads in the Ruby sense.
/// obj.instance_eval(string [, filename [, lineno]] ) => obj
/// obj.instance_eval {| | block } => obj
/// Consider this call:
/// obj.instance_eval(2, 3) { }
/// This should match with the second Ruby overload, and report an ArgumentError
because of the mismatch in the number of arguments.
/// However, the MethodBinder will prefer the first Ruby overload as it has a
matching number of arguments, and will report
/// a type-mismatch instead.
/// Filterting down the overload list upfront yields better errors.
/// </summary>
Tomas pointed out that this filtering was incorrect. Tomas, it did cause test
failures (My last test pass had this filtering done in error-handling after the
MethodBinder did its stuff).
However, overloads do get precedence in some cases like the instance_eval
example in the comment above, and also with Array#fill as shown below. There is
a RubySpec test for Array#fill, but we have a tag because we get it wrong. I
agree with Tomas that this will need to be handled more carefully, and we might
need to have an attribute on the library methods indicating whether the block
gets precedence, or whether the argument count gets precedence in selecting the
method overload. I will undo my change for now.
http://ruby-doc.org/core/classes/Array.html
array.fill(obj) ? array
array.fill(obj, start [, length]) ? array
array.fill(obj, range ) ? array
array.fill {|index| block } ? array
array.fill(start [, length] ) {|index| block } ? array
array.fill(range) {|index| block } ? array
MRI
irb(main):005:0> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
ArgumentError: wrong number of arguments (3 for 2)
IronRuby>>> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
=> [1, 1, 1]
Thanks,
Shri
-----Original Message-----
From: Jim Deville
Sent: Friday, January 23, 2009 3:05 PM
To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?
The question is whether we need to copy the exact error reporting for argument
mismatch, or whether best effort approach isn''t good enough. The
advantage of the current behavior is a simpler binder. We can certainly add
attributes that will prioritize some overloads but it would make the binder more
complicated. I don''t see a scenario where it makes sense for an app to
catch ArgumentErrors and depend on it. Array#fill behavior might also be
considered a bug in MRI.
Tomas
-----Original Message-----
http://ruby-doc.org/core/classes/Array.html
array.fill(obj) ? array
array.fill(obj, start [, length]) ? array
array.fill(obj, range ) ? array
array.fill {|index| block } ? array
array.fill(start [, length] ) {|index| block } ? array
array.fill(range) {|index| block } ? array
MRI
irb(main):005:0> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
ArgumentError: wrong number of arguments (3 for 2)
IronRuby>>> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
=> [1, 1, 1]
Thanks,
Shri
-----Original Message-----
From: Jim Deville
Sent: Friday, January 23, 2009 3:05 PM
To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?
Agreed, this is a negative scenario and exact error reporting is not a priority
in such contrived scenarios.
-----Original Message-----
From: Tomas Matousek
Sent: Saturday, January 24, 2009 3:03 PM
To: Shri Borde; Jim Deville; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
The question is whether we need to copy the exact error reporting for argument
mismatch, or whether best effort approach isn''t good enough. The
advantage of the current behavior is a simpler binder. We can certainly add
attributes that will prioritize some overloads but it would make the binder more
complicated. I don''t see a scenario where it makes sense for an app to
catch ArgumentErrors and depend on it. Array#fill behavior might also be
considered a bug in MRI.
Tomas
-----Original Message-----
http://ruby-doc.org/core/classes/Array.html
array.fill(obj) ? array
array.fill(obj, start [, length]) ? array
array.fill(obj, range ) ? array
array.fill {|index| block } ? array
array.fill(start [, length] ) {|index| block } ? array
array.fill(range) {|index| block } ? array
MRI
irb(main):005:0> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
ArgumentError: wrong number of arguments (3 for 2)
IronRuby>>> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
=> [1, 1, 1]
Thanks,
Shri
-----Original Message-----
From: Jim Deville
Sent: Friday, January 23, 2009 3:05 PM
To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?
Tomas, correct me if I''m wrong, but Ruby uses a message based method
dispatch. When to_f is called, the message to_f gets sent to the object, and
then normal method resolution occurs. There is no way to bypass that resolution.
You can''t call String#to_f instead of the singleton class''s
to_f. You call to_f on the object, and it sends the message. That is why I argue
those tests are the same. If we have the ability, or the possibility of
something different, then something is wrong with our method dispatch an method
resolution. That should be exposed in the method tests in Language specs. This
spec doesn''t need both the case of a classes instance method and a
singleton instance method.
JD
-----Original Message-----
From: Shri Borde
Sent: Saturday, January 24, 2009 2:36 PM
To: Jim Deville; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
Jim, using should_not_receive sounds like a good idea. Will do the change.
But we still need all those specs including the one using hijack_String_to_f. It
is not testing method dispatch. Ruby method dispatch only controls which method
body will be called. It does not do argument conversions. That is handled by the
target method which is free to use to_f to convert the argument to a float, but
is not required to. The specs below will ensure that the library method does
follow the right convention/protocol for converting the argument to a float.
Does that make sense?
Also, I had this comment:
/// <summary>
/// An overload with a block should get precedence. However,
Microsoft.Scripting.Action.Calls.MethodBinder
/// gives precedence to overloads with a matching number of arguments as the
actual arguments. Consider Object#eval
/// which effectively has the following two overloads in the Ruby sense.
/// obj.instance_eval(string [, filename [, lineno]] ) => obj
/// obj.instance_eval {| | block } => obj
/// Consider this call:
/// obj.instance_eval(2, 3) { }
/// This should match with the second Ruby overload, and report an ArgumentError
because of the mismatch in the number of arguments.
/// However, the MethodBinder will prefer the first Ruby overload as it has a
matching number of arguments, and will report
/// a type-mismatch instead.
/// Filterting down the overload list upfront yields better errors.
/// </summary>
Tomas pointed out that this filtering was incorrect. Tomas, it did cause test
failures (My last test pass had this filtering done in error-handling after the
MethodBinder did its stuff).
However, overloads do get precedence in some cases like the instance_eval
example in the comment above, and also with Array#fill as shown below. There is
a RubySpec test for Array#fill, but we have a tag because we get it wrong. I
agree with Tomas that this will need to be handled more carefully, and we might
need to have an attribute on the library methods indicating whether the block
gets precedence, or whether the argument count gets precedence in selecting the
method overload. I will undo my change for now.
http://ruby-doc.org/core/classes/Array.html
array.fill(obj) ? array
array.fill(obj, start [, length]) ? array
array.fill(obj, range ) ? array
array.fill {|index| block } ? array
array.fill(start [, length] ) {|index| block } ? array
array.fill(range) {|index| block } ? array
MRI
irb(main):005:0> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
ArgumentError: wrong number of arguments (3 for 2)
IronRuby>>> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
=> [1, 1, 1]
Thanks,
Shri
-----Original Message-----
From: Jim Deville
Sent: Friday, January 23, 2009 3:05 PM
To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?
I believe Jim is right. Library methods performing conversions (or other
operations) do so either by calling internal C methods or using regular Ruby
method dispatch. The former can''t be detected from Ruby except for
using a tracing proc (set_trace_func). Method dispatch goes to the singleton of
the object first and then to its class. So the use of should_not_receive ensures
that to_f is not invoked on the given object.
Tomas
-----Original Message-----
From: Jim Deville
Sent: Saturday, January 24, 2009 4:26 PM
To: Shri Borde; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
Tomas, correct me if I''m wrong, but Ruby uses a message based method
dispatch. When to_f is called, the message to_f gets sent to the object, and
then normal method resolution occurs. There is no way to bypass that resolution.
You can''t call String#to_f instead of the singleton class''s
to_f. You call to_f on the object, and it sends the message. That is why I argue
those tests are the same. If we have the ability, or the possibility of
something different, then something is wrong with our method dispatch an method
resolution. That should be exposed in the method tests in Language specs. This
spec doesn''t need both the case of a classes instance method and a
singleton instance method.
JD
-----Original Message-----
From: Shri Borde
Sent: Saturday, January 24, 2009 2:36 PM
To: Jim Deville; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
Jim, using should_not_receive sounds like a good idea. Will do the change.
But we still need all those specs including the one using hijack_String_to_f. It
is not testing method dispatch. Ruby method dispatch only controls which method
body will be called. It does not do argument conversions. That is handled by the
target method which is free to use to_f to convert the argument to a float, but
is not required to. The specs below will ensure that the library method does
follow the right convention/protocol for converting the argument to a float.
Does that make sense?
Also, I had this comment:
/// <summary>
/// An overload with a block should get precedence. However,
Microsoft.Scripting.Action.Calls.MethodBinder
/// gives precedence to overloads with a matching number of arguments as the
actual arguments. Consider Object#eval
/// which effectively has the following two overloads in the Ruby sense.
/// obj.instance_eval(string [, filename [, lineno]] ) => obj
/// obj.instance_eval {| | block } => obj
/// Consider this call:
/// obj.instance_eval(2, 3) { }
/// This should match with the second Ruby overload, and report an ArgumentError
because of the mismatch in the number of arguments.
/// However, the MethodBinder will prefer the first Ruby overload as it has a
matching number of arguments, and will report
/// a type-mismatch instead.
/// Filterting down the overload list upfront yields better errors.
/// </summary>
Tomas pointed out that this filtering was incorrect. Tomas, it did cause test
failures (My last test pass had this filtering done in error-handling after the
MethodBinder did its stuff).
However, overloads do get precedence in some cases like the instance_eval
example in the comment above, and also with Array#fill as shown below. There is
a RubySpec test for Array#fill, but we have a tag because we get it wrong. I
agree with Tomas that this will need to be handled more carefully, and we might
need to have an attribute on the library methods indicating whether the block
gets precedence, or whether the argument count gets precedence in selecting the
method overload. I will undo my change for now.
http://ruby-doc.org/core/classes/Array.html
array.fill(obj) ? array
array.fill(obj, start [, length]) ? array
array.fill(obj, range ) ? array
array.fill {|index| block } ? array
array.fill(start [, length] ) {|index| block } ? array
array.fill(range) {|index| block } ? array
MRI
irb(main):005:0> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
ArgumentError: wrong number of arguments (3 for 2)
IronRuby>>> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
=> [1, 1, 1]
Thanks,
Shri
-----Original Message-----
From: Jim Deville
Sent: Friday, January 23, 2009 3:05 PM
To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?
The test below actually shows that the singleton to_f is not called. Similarly,
a string like "1.0" gets converted to a Float even after undef-ing
String#to_f. So the library methods are *not* calling to_f on strings at all,
but are directly converting the string contents to a Float value.
The question is not whether should_not_receive ensures that to_f is not invoked
(I agree that we should use this API). The question is whether we need the
testing of all these cases for the default protocol for converting Float
arguments, or if its redundant with language tests testing method dispatch. The
language tests will cover the conversions done in the libraries.
-----Original Message-----
From: Tomas Matousek
Sent: Saturday, January 24, 2009 10:58 PM
To: Jim Deville; Shri Borde; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
I believe Jim is right. Library methods performing conversions (or other
operations) do so either by calling internal C methods or using regular Ruby
method dispatch. The former can''t be detected from Ruby except for
using a tracing proc (set_trace_func). Method dispatch goes to the singleton of
the object first and then to its class. So the use of should_not_receive ensures
that to_f is not invoked on the given object.
Tomas
-----Original Message-----
From: Jim Deville
Sent: Saturday, January 24, 2009 4:26 PM
To: Shri Borde; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
Tomas, correct me if I''m wrong, but Ruby uses a message based method
dispatch. When to_f is called, the message to_f gets sent to the object, and
then normal method resolution occurs. There is no way to bypass that resolution.
You can''t call String#to_f instead of the singleton class''s
to_f. You call to_f on the object, and it sends the message. That is why I argue
those tests are the same. If we have the ability, or the possibility of
something different, then something is wrong with our method dispatch an method
resolution. That should be exposed in the method tests in Language specs. This
spec doesn''t need both the case of a classes instance method and a
singleton instance method.
JD
-----Original Message-----
From: Shri Borde
Sent: Saturday, January 24, 2009 2:36 PM
To: Jim Deville; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
Jim, using should_not_receive sounds like a good idea. Will do the change.
But we still need all those specs including the one using hijack_String_to_f. It
is not testing method dispatch. Ruby method dispatch only controls which method
body will be called. It does not do argument conversions. That is handled by the
target method which is free to use to_f to convert the argument to a float, but
is not required to. The specs below will ensure that the library method does
follow the right convention/protocol for converting the argument to a float.
Does that make sense?
Also, I had this comment:
/// <summary>
/// An overload with a block should get precedence. However,
Microsoft.Scripting.Action.Calls.MethodBinder
/// gives precedence to overloads with a matching number of arguments as the
actual arguments. Consider Object#eval
/// which effectively has the following two overloads in the Ruby sense.
/// obj.instance_eval(string [, filename [, lineno]] ) => obj
/// obj.instance_eval {| | block } => obj
/// Consider this call:
/// obj.instance_eval(2, 3) { }
/// This should match with the second Ruby overload, and report an ArgumentError
because of the mismatch in the number of arguments.
/// However, the MethodBinder will prefer the first Ruby overload as it has a
matching number of arguments, and will report
/// a type-mismatch instead.
/// Filterting down the overload list upfront yields better errors.
/// </summary>
Tomas pointed out that this filtering was incorrect. Tomas, it did cause test
failures (My last test pass had this filtering done in error-handling after the
MethodBinder did its stuff).
However, overloads do get precedence in some cases like the instance_eval
example in the comment above, and also with Array#fill as shown below. There is
a RubySpec test for Array#fill, but we have a tag because we get it wrong. I
agree with Tomas that this will need to be handled more carefully, and we might
need to have an attribute on the library methods indicating whether the block
gets precedence, or whether the argument count gets precedence in selecting the
method overload. I will undo my change for now.
http://ruby-doc.org/core/classes/Array.html
array.fill(obj) ? array
array.fill(obj, start [, length]) ? array
array.fill(obj, range ) ? array
array.fill {|index| block } ? array
array.fill(start [, length] ) {|index| block } ? array
array.fill(range) {|index| block } ? array
MRI
irb(main):005:0> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
ArgumentError: wrong number of arguments (3 for 2)
IronRuby>>> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
=> [1, 1, 1]
Thanks,
Shri
-----Original Message-----
From: Jim Deville
Sent: Friday, January 23, 2009 3:05 PM
To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?
I feel that it is redundant.
JD
-----Original Message-----
From: Shri Borde
Sent: Saturday, January 24, 2009 11:21 PM
To: Tomas Matousek; Jim Deville; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
The test below actually shows that the singleton to_f is not called. Similarly,
a string like "1.0" gets converted to a Float even after undef-ing
String#to_f. So the library methods are *not* calling to_f on strings at all,
but are directly converting the string contents to a Float value.
The question is not whether should_not_receive ensures that to_f is not invoked
(I agree that we should use this API). The question is whether we need the
testing of all these cases for the default protocol for converting Float
arguments, or if its redundant with language tests testing method dispatch. The
language tests will cover the conversions done in the libraries.
-----Original Message-----
From: Tomas Matousek
Sent: Saturday, January 24, 2009 10:58 PM
To: Jim Deville; Shri Borde; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
I believe Jim is right. Library methods performing conversions (or other
operations) do so either by calling internal C methods or using regular Ruby
method dispatch. The former can''t be detected from Ruby except for
using a tracing proc (set_trace_func). Method dispatch goes to the singleton of
the object first and then to its class. So the use of should_not_receive ensures
that to_f is not invoked on the given object.
Tomas
-----Original Message-----
From: Jim Deville
Sent: Saturday, January 24, 2009 4:26 PM
To: Shri Borde; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
Tomas, correct me if I''m wrong, but Ruby uses a message based method
dispatch. When to_f is called, the message to_f gets sent to the object, and
then normal method resolution occurs. There is no way to bypass that resolution.
You can''t call String#to_f instead of the singleton class''s
to_f. You call to_f on the object, and it sends the message. That is why I argue
those tests are the same. If we have the ability, or the possibility of
something different, then something is wrong with our method dispatch an method
resolution. That should be exposed in the method tests in Language specs. This
spec doesn''t need both the case of a classes instance method and a
singleton instance method.
JD
-----Original Message-----
From: Shri Borde
Sent: Saturday, January 24, 2009 2:36 PM
To: Jim Deville; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
Jim, using should_not_receive sounds like a good idea. Will do the change.
But we still need all those specs including the one using hijack_String_to_f. It
is not testing method dispatch. Ruby method dispatch only controls which method
body will be called. It does not do argument conversions. That is handled by the
target method which is free to use to_f to convert the argument to a float, but
is not required to. The specs below will ensure that the library method does
follow the right convention/protocol for converting the argument to a float.
Does that make sense?
Also, I had this comment:
/// <summary>
/// An overload with a block should get precedence. However,
Microsoft.Scripting.Action.Calls.MethodBinder
/// gives precedence to overloads with a matching number of arguments as the
actual arguments. Consider Object#eval
/// which effectively has the following two overloads in the Ruby sense.
/// obj.instance_eval(string [, filename [, lineno]] ) => obj
/// obj.instance_eval {| | block } => obj
/// Consider this call:
/// obj.instance_eval(2, 3) { }
/// This should match with the second Ruby overload, and report an ArgumentError
because of the mismatch in the number of arguments.
/// However, the MethodBinder will prefer the first Ruby overload as it has a
matching number of arguments, and will report
/// a type-mismatch instead.
/// Filterting down the overload list upfront yields better errors.
/// </summary>
Tomas pointed out that this filtering was incorrect. Tomas, it did cause test
failures (My last test pass had this filtering done in error-handling after the
MethodBinder did its stuff).
However, overloads do get precedence in some cases like the instance_eval
example in the comment above, and also with Array#fill as shown below. There is
a RubySpec test for Array#fill, but we have a tag because we get it wrong. I
agree with Tomas that this will need to be handled more carefully, and we might
need to have an attribute on the library methods indicating whether the block
gets precedence, or whether the argument count gets precedence in selecting the
method overload. I will undo my change for now.
http://ruby-doc.org/core/classes/Array.html
array.fill(obj) ? array
array.fill(obj, start [, length]) ? array
array.fill(obj, range ) ? array
array.fill {|index| block } ? array
array.fill(start [, length] ) {|index| block } ? array
array.fill(range) {|index| block } ? array
MRI
irb(main):005:0> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
ArgumentError: wrong number of arguments (3 for 2)
IronRuby>>> [1,2].fill(1,1,2) {|i| puts "index=#{i}" }
=> [1, 1, 1]
Thanks,
Shri
-----Original Message-----
From: Jim Deville
Sent: Friday, January 23, 2009 3:05 PM
To: Shri Borde; IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: Better error messages
it "coerces string argument with Float() without calling singleton
to_f" do
s = MathSpecs.string_with_singleton_to_f("0.5", 0.5)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "coerces string argument with Float() without calling
String#to_f" do
MathSpecs.hijack_String_to_f
begin
Math.acos("0.5").should be_close(Math.acos(0.5), TOLERANCE)
ensure
MathSpecs.reset_String_to_f
end
end
These can be rewritten with mocks instead of specialized classes.
it "coerces string argument with Float() without calling singleton
to_f" do
s = "0.5"
s.should_not_receive(:to_f)
Math.acos(s).should be_close(Math.acos(0.5), TOLERANCE)
End
The second spec is really testing method dispatch, and otherwise is the same
thing as this one. Same with:
it "raises an ArgumentError if the singleton string cannot be directly
coerced with Float()" do
s = MathSpecs.string_with_singleton_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
end
it "raises an ArgumentError if the string subclass cannot be directly
coerced with Float()" do
s = MathSpecs.string_subclass_with_to_f("test", 0.5)
lambda { Math.acos(s) }.should raise_error(ArgumentError)
ScratchPad.recorded.should == nil # to_f should not be called
End
JD
-----Original Message-----
From: Shri Borde
Sent: Friday, January 23, 2009 2:04 PM
To: IronRuby External Code Reviewers; DLR Code Reviews
Cc: ironruby-core at rubyforge.org
Subject: Code Review: Better error messages
tfpt review "/shelveset:error;REDMOND\sborde"
Microsoft.Scripting:
Change in DefaultBinder is to make the binder exception processing logic be
extensible so that languages can customize the type of exception thrown.
IronRuby:
Improves error reporting in IronRuby by checking the results of the bind
failure. This allowed many Math tests to be enabled.
Also, added a DefaultProtocol for to_f. This allowed more Math tests to be
enabled as you can now pass the string "1.0" to a function expecting a
"Float". I have added test cases for the Float conversion rules in
core\math\acos_spec.rb. I have not copied this to the other specs as that would
be hard to maintain. We will look at factoring such conversion checks into a
utility library so that they will be much easier to use.
Fixes some String bugs - String.#inspect on string subtype should return a
string, and passing wrong argument types to Strings#[]= should cause TypeError
instead of ArgumentError. This is where I started and it put me in the direction
of the fixes above.
Tomas: In RubyBinder.GetTypeName, is there a better way to get to RubyContext?