[Ironruby-core] Code Review: Better error messages

Tomas Matousek Tomas.Matousek at microsoft.com
Sun Jan 25 01:57:39 EST 2009


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?




More information about the Ironruby-core mailing list