[Ironruby-core] Code Review: Better error messages

Shri Borde Shri.Borde at microsoft.com
Sat Jan 24 17:35:45 EST 2009


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?




More information about the Ironruby-core mailing list