Add more descriptive error messages to init methods in Ruby (#5659)

* add more descriptive error messages to init methods

* add type errors test to Makefile.am

Makefile.am

@@ -950,6 +950,7 @@ ruby_EXTRA_DIST=                                                             \
   ruby/tests/test_ruby_package.proto                                         \
   ruby/tests/generated_code_test.rb                                          \
   ruby/tests/well_known_types_test.rb                                        \
+  ruby/tests/type_errors.rb                                                  \
   ruby/travis-test.sh
 
 js_EXTRA_DIST=                                                         \

ruby/ext/google/protobuf_c/map.c

@@ -82,7 +82,7 @@ static VALUE table_key(Map* self, VALUE key,
     case UPB_TYPE_INT64:
     case UPB_TYPE_UINT32:
     case UPB_TYPE_UINT64:
-      native_slot_set(self->key_type, Qnil, buf, key);
+      native_slot_set("", self->key_type, Qnil, buf, key);
       *out_key = buf;
       *out_length = native_slot_size(self->key_type);
       break;
@@ -396,7 +396,7 @@ VALUE Map_index_set(VALUE _self, VALUE key, VALUE value) {
   key = table_key(self, key, keybuf, &keyval, &length);
 
   mem = value_memory(&v);
-  native_slot_set(self->value_type, self->value_type_class, mem, value);
+  native_slot_set("", self->value_type, self->value_type_class, mem, value);
 
   // Replace any existing value by issuing a 'remove' operation first.
   upb_strtable_remove2(&self->table, keyval, length, NULL);

ruby/ext/google/protobuf_c/message.c

@@ -315,7 +315,8 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
 
     if (TYPE(val) != T_HASH) {
       rb_raise(rb_eArgError,
-               "Expected Hash object as initializer value for map field '%s'.", name);
+               "Expected Hash object as initializer value for map field '%s' (given %s).",
+               name, rb_class2name(CLASS_OF(val)));
     }
     map = layout_get(self->descriptor->layout, Message_data(self), f);
     Map_merge_into_self(map, val);
@@ -324,7 +325,8 @@ int Message_initialize_kwarg(VALUE key, VALUE val, VALUE _self) {
 
     if (TYPE(val) != T_ARRAY) {
       rb_raise(rb_eArgError,
-               "Expected array as initializer value for repeated field '%s'.", name);
+               "Expected array as initializer value for repeated field '%s' (given %s).",
+               name, rb_class2name(CLASS_OF(val)));
     }
     ary = layout_get(self->descriptor->layout, Message_data(self), f);
     for (int i = 0; i < RARRAY_LEN(val); i++) {

ruby/ext/google/protobuf_c/protobuf.h

@@ -337,14 +337,16 @@ VALUE Builder_finalize_to_pool(VALUE _self, VALUE pool_rb);
 #define NATIVE_SLOT_MAX_SIZE sizeof(uint64_t)
 
 size_t native_slot_size(upb_fieldtype_t type);
-void native_slot_set(upb_fieldtype_t type,
+void native_slot_set(const char* name,
+                     upb_fieldtype_t type,
                      VALUE type_class,
                      void* memory,
                      VALUE value);
 // Atomically (with respect to Ruby VM calls) either update the value and set a
 // oneof case, or do neither. If |case_memory| is null, then no case value is
 // set.
-void native_slot_set_value_and_case(upb_fieldtype_t type,
+void native_slot_set_value_and_case(const char* name,
+                                    upb_fieldtype_t type,
                                     VALUE type_class,
                                     void* memory,
                                     VALUE value,
@@ -360,7 +362,7 @@ void native_slot_deep_copy(upb_fieldtype_t type, void* to, void* from);
 bool native_slot_eq(upb_fieldtype_t type, void* mem1, void* mem2);
 
 VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value);
-void native_slot_check_int_range_precision(upb_fieldtype_t type, VALUE value);
+void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE value);
 
 extern rb_encoding* kRubyStringUtf8Encoding;
 extern rb_encoding* kRubyStringASCIIEncoding;

ruby/ext/google/protobuf_c/repeated_field.c

@@ -178,7 +178,7 @@ VALUE RepeatedField_index_set(VALUE _self, VALUE _index, VALUE val) {
   }
 
   memory = RepeatedField_memoryat(self, index, element_size);
-  native_slot_set(field_type, field_type_class, memory, val);
+  native_slot_set("", field_type, field_type_class, memory, val);
   return Qnil;
 }
 
@@ -217,7 +217,7 @@ VALUE RepeatedField_push(VALUE _self, VALUE val) {
 
   RepeatedField_reserve(self, self->size + 1);
   memory = (void *) (((uint8_t *)self->elements) + self->size * element_size);
-  native_slot_set(field_type, self->field_type_class, memory, val);
+  native_slot_set("", field_type, self->field_type_class, memory, val);
   // native_slot_set may raise an error; bump size only after set.
   self->size++;
   return _self;

ruby/ext/google/protobuf_c/storage.c

@@ -65,9 +65,10 @@ static bool is_ruby_num(VALUE value) {
           TYPE(value) == T_BIGNUM);
 }
 
-void native_slot_check_int_range_precision(upb_fieldtype_t type, VALUE val) {
+void native_slot_check_int_range_precision(const char* name, upb_fieldtype_t type, VALUE val) {
   if (!is_ruby_num(val)) {
-    rb_raise(cTypeError, "Expected number type for integral field.");
+    rb_raise(cTypeError, "Expected number type for integral field '%s' (given %s).",
+             name, rb_class2name(CLASS_OF(val)));
   }
 
   // NUM2{INT,UINT,LL,ULL} macros do the appropriate range checks on upper
@@ -77,13 +78,15 @@ void native_slot_check_int_range_precision(upb_fieldtype_t type, VALUE val) {
     double dbl_val = NUM2DBL(val);
     if (floor(dbl_val) != dbl_val) {
       rb_raise(rb_eRangeError,
-               "Non-integral floating point value assigned to integer field.");
+               "Non-integral floating point value assigned to integer field '%s' (given %s).",
+               name, rb_class2name(CLASS_OF(val)));
     }
   }
   if (type == UPB_TYPE_UINT32 || type == UPB_TYPE_UINT64) {
     if (NUM2DBL(val) < 0) {
       rb_raise(rb_eRangeError,
-               "Assigning negative value to unsigned integer field.");
+               "Assigning negative value to unsigned integer field '%s' (given %s).",
+               name, rb_class2name(CLASS_OF(val)));
     }
   }
 }
@@ -108,12 +111,14 @@ VALUE native_slot_encode_and_freeze_string(upb_fieldtype_t type, VALUE value) {
   return value;
 }
 
-void native_slot_set(upb_fieldtype_t type, VALUE type_class,
+void native_slot_set(const char* name, 
+                     upb_fieldtype_t type, VALUE type_class,
                      void* memory, VALUE value) {
-  native_slot_set_value_and_case(type, type_class, memory, value, NULL, 0);
+  native_slot_set_value_and_case(name, type, type_class, memory, value, NULL, 0);
 }
 
-void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
+void native_slot_set_value_and_case(const char* name, 
+                                    upb_fieldtype_t type, VALUE type_class,
                                     void* memory, VALUE value,
                                     uint32_t* case_memory,
                                     uint32_t case_number) {
@@ -124,13 +129,15 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
   switch (type) {
     case UPB_TYPE_FLOAT:
       if (!is_ruby_num(value)) {
-        rb_raise(cTypeError, "Expected number type for float field.");
+        rb_raise(cTypeError, "Expected number type for float field '%s' (given %s).",
+                 name, rb_class2name(CLASS_OF(value)));
       }
       DEREF(memory, float) = NUM2DBL(value);
       break;
     case UPB_TYPE_DOUBLE:
       if (!is_ruby_num(value)) {
-        rb_raise(cTypeError, "Expected number type for double field.");
+        rb_raise(cTypeError, "Expected number type for double field '%s' (given %s).",
+                 name, rb_class2name(CLASS_OF(value)));
       }
       DEREF(memory, double) = NUM2DBL(value);
       break;
@@ -141,7 +148,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
       } else if (value == Qfalse) {
         val = 0;
       } else {
-        rb_raise(cTypeError, "Invalid argument for boolean field.");
+        rb_raise(cTypeError, "Invalid argument for boolean field '%s' (given %s).",
+                 name, rb_class2name(CLASS_OF(value)));
       }
       DEREF(memory, int8_t) = val;
       break;
@@ -150,7 +158,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
       if (CLASS_OF(value) == rb_cSymbol) {
         value = rb_funcall(value, rb_intern("to_s"), 0);
       } else if (CLASS_OF(value) != rb_cString) {
-        rb_raise(cTypeError, "Invalid argument for string field.");
+        rb_raise(cTypeError, "Invalid argument for string field '%s' (given %s).",
+                 name, rb_class2name(CLASS_OF(value)));
       }
 
       DEREF(memory, VALUE) = native_slot_encode_and_freeze_string(type, value);
@@ -158,7 +167,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
 
     case UPB_TYPE_BYTES: {
       if (CLASS_OF(value) != rb_cString) {
-        rb_raise(cTypeError, "Invalid argument for string field.");
+        rb_raise(cTypeError, "Invalid argument for bytes field '%s' (given %s).",
+                 name, rb_class2name(CLASS_OF(value)));
       }
 
       DEREF(memory, VALUE) = native_slot_encode_and_freeze_string(type, value);
@@ -169,8 +179,8 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
         value = Qnil;
       } else if (CLASS_OF(value) != type_class) {
         rb_raise(cTypeError,
-                 "Invalid type %s to assign to submessage field.",
-                 rb_class2name(CLASS_OF(value)));
+                 "Invalid type %s to assign to submessage field '%s'.",
+                rb_class2name(CLASS_OF(value)), name);
       }
       DEREF(memory, VALUE) = value;
       break;
@@ -181,18 +191,18 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
         value = rb_funcall(value, rb_intern("to_sym"), 0);
       } else if (!is_ruby_num(value) && TYPE(value) != T_SYMBOL) {
         rb_raise(cTypeError,
-                 "Expected number or symbol type for enum field.");
+                 "Expected number or symbol type for enum field '%s'.", name);
       }
       if (TYPE(value) == T_SYMBOL) {
         // Ensure that the given symbol exists in the enum module.
         VALUE lookup = rb_funcall(type_class, rb_intern("resolve"), 1, value);
         if (lookup == Qnil) {
-          rb_raise(rb_eRangeError, "Unknown symbol value for enum field.");
+          rb_raise(rb_eRangeError, "Unknown symbol value for enum field '%s'.", name);
         } else {
           int_val = NUM2INT(lookup);
         }
       } else {
-        native_slot_check_int_range_precision(UPB_TYPE_INT32, value);
+        native_slot_check_int_range_precision(name, UPB_TYPE_INT32, value);
         int_val = NUM2INT(value);
       }
       DEREF(memory, int32_t) = int_val;
@@ -202,7 +212,7 @@ void native_slot_set_value_and_case(upb_fieldtype_t type, VALUE type_class,
     case UPB_TYPE_INT64:
     case UPB_TYPE_UINT32:
     case UPB_TYPE_UINT64:
-      native_slot_check_int_range_precision(type, value);
+      native_slot_check_int_range_precision(name, type, value);
       switch (type) {
       case UPB_TYPE_INT32:
         DEREF(memory, int32_t) = NUM2INT(value);
@@ -658,8 +668,9 @@ void layout_clear(MessageLayout* layout,
 
     DEREF(memory, VALUE) = ary;
   } else {
-    native_slot_set(upb_fielddef_type(field), field_type_class(field),
-                      memory, layout_get_default(field));
+    native_slot_set(upb_fielddef_name(field), 
+                    upb_fielddef_type(field), field_type_class(field),
+                    memory, layout_get_default(field));
   }
 }
 
@@ -816,6 +827,7 @@ void layout_set(MessageLayout* layout,
       // use native_slot_set_value_and_case(), which ensures that both the value
       // and case number are altered atomically (w.r.t. the Ruby VM).
       native_slot_set_value_and_case(
+          upb_fielddef_name(field),
           upb_fielddef_type(field), field_type_class(field),
           memory, val,
           oneof_case, upb_fielddef_number(field));
@@ -827,8 +839,9 @@ void layout_set(MessageLayout* layout,
     check_repeated_field_type(val, field);
     DEREF(memory, VALUE) = val;
   } else {
-    native_slot_set(upb_fielddef_type(field), field_type_class(field), memory,
-		    val);
+    native_slot_set(upb_fielddef_name(field), 
+                    upb_fielddef_type(field), field_type_class(field),
+                    memory, val);
   }
 
   if (layout->fields[upb_fielddef_index(field)].hasbit !=

ruby/tests/basic.rb

@@ -154,12 +154,12 @@ module BasicTest
       e = assert_raise ArgumentError do
         MapMessage.new(:map_string_int32 => "hello")
       end
-      assert_equal e.message, "Expected Hash object as initializer value for map field 'map_string_int32'."
+      assert_equal e.message, "Expected Hash object as initializer value for map field 'map_string_int32' (given String)."
 
       e = assert_raise ArgumentError do
         TestMessage.new(:repeated_uint32 => "hello")
       end
-      assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32'."
+      assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32' (given String)."
     end
 
     def test_map_field

ruby/tests/basic_proto2.rb

@@ -207,7 +207,7 @@ module BasicTestProto2
       e = assert_raise ArgumentError do
         TestMessage.new(:repeated_uint32 => "hello")
       end
-      assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32'."
+      assert_equal e.message, "Expected array as initializer value for repeated field 'repeated_uint32' (given String)."
     end
 
 

ruby/tests/type_errors.rb

+#!/usr/bin/ruby
+
+# generated_code.rb is in the same directory as this test.
+$LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__)))
+
+require 'test/unit'
+require 'google/protobuf/well_known_types'
+require 'generated_code_pb'
+
+class TestTypeErrors < Test::Unit::TestCase
+  def test_bad_string
+    check_error Google::Protobuf::TypeError,
+                "Invalid argument for string field 'optional_string' (given Integer)." do
+      A::B::C::TestMessage.new(optional_string: 4)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Invalid argument for string field 'oneof_string' (given Integer)." do
+      A::B::C::TestMessage.new(oneof_string: 4)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_string' (given String)." do
+      A::B::C::TestMessage.new(repeated_string: '4')
+    end
+  end
+
+  def test_bad_float
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for float field 'optional_float' (given TrueClass)." do
+      A::B::C::TestMessage.new(optional_float: true)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for float field 'oneof_float' (given TrueClass)." do
+      A::B::C::TestMessage.new(oneof_float: true)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_float' (given String)." do
+      A::B::C::TestMessage.new(repeated_float: 'true')
+    end
+  end
+
+  def test_bad_double
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for double field 'optional_double' (given Symbol)." do
+      A::B::C::TestMessage.new(optional_double: :double)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for double field 'oneof_double' (given Symbol)." do
+      A::B::C::TestMessage.new(oneof_double: :double)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_double' (given FalseClass)." do
+      A::B::C::TestMessage.new(repeated_double: false)
+    end
+  end
+
+  def test_bad_bool
+    check_error Google::Protobuf::TypeError,
+                "Invalid argument for boolean field 'optional_bool' (given Float)." do
+      A::B::C::TestMessage.new(optional_bool: 4.4)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Invalid argument for boolean field 'oneof_bool' (given Float)." do
+      A::B::C::TestMessage.new(oneof_bool: 4.4)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_bool' (given String)." do
+      A::B::C::TestMessage.new(repeated_bool: 'hi')
+    end
+  end
+
+  def test_bad_int
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for integral field 'optional_int32' (given String)." do
+      A::B::C::TestMessage.new(optional_int32: 'hi')
+    end
+    check_error RangeError,
+                "Non-integral floating point value assigned to integer field 'optional_int64' (given Float)." do
+      A::B::C::TestMessage.new(optional_int64: 2.4)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for integral field 'optional_uint32' (given Symbol)." do
+      A::B::C::TestMessage.new(optional_uint32: :thing)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for integral field 'optional_uint64' (given FalseClass)." do
+      A::B::C::TestMessage.new(optional_uint64: false)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for integral field 'oneof_int32' (given Symbol)." do
+      A::B::C::TestMessage.new(oneof_int32: :hi)
+    end
+    check_error RangeError,
+                "Non-integral floating point value assigned to integer field 'oneof_int64' (given Float)." do
+      A::B::C::TestMessage.new(oneof_int64: 2.4)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Expected number type for integral field 'oneof_uint32' (given String)." do
+      A::B::C::TestMessage.new(oneof_uint32: 'x')
+    end
+    check_error RangeError,
+                "Non-integral floating point value assigned to integer field 'oneof_uint64' (given Float)." do
+      A::B::C::TestMessage.new(oneof_uint64: 1.1)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_int32' (given Symbol)." do
+      A::B::C::TestMessage.new(repeated_int32: :hi)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_int64' (given Float)." do
+      A::B::C::TestMessage.new(repeated_int64: 2.4)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_uint32' (given String)." do
+      A::B::C::TestMessage.new(repeated_uint32: 'x')
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_uint64' (given Float)." do
+      A::B::C::TestMessage.new(repeated_uint64: 1.1)
+    end
+  end
+
+  def test_bad_enum
+    check_error RangeError,
+                "Unknown symbol value for enum field 'optional_enum'." do
+      A::B::C::TestMessage.new(optional_enum: 'enum')
+    end
+    check_error RangeError,
+                "Unknown symbol value for enum field 'oneof_enum'." do
+      A::B::C::TestMessage.new(oneof_enum: '')
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_enum' (given String)." do
+      A::B::C::TestMessage.new(repeated_enum: '')
+    end
+  end
+
+  def test_bad_bytes
+    check_error Google::Protobuf::TypeError,
+                "Invalid argument for bytes field 'optional_bytes' (given Float)." do
+      A::B::C::TestMessage.new(optional_bytes: 22.22)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Invalid argument for bytes field 'oneof_bytes' (given Symbol)." do
+      A::B::C::TestMessage.new(oneof_bytes: :T22)
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_bytes' (given Symbol)." do
+      A::B::C::TestMessage.new(repeated_bytes: :T22)
+    end
+  end
+
+  def test_bad_msg
+    check_error Google::Protobuf::TypeError,
+                "Invalid type Integer to assign to submessage field 'optional_msg'." do
+      A::B::C::TestMessage.new(optional_msg: 2)
+    end
+    check_error Google::Protobuf::TypeError,
+                "Invalid type String to assign to submessage field 'oneof_msg'." do
+      A::B::C::TestMessage.new(oneof_msg: '2')
+    end
+    check_error ArgumentError,
+                "Expected array as initializer value for repeated field 'repeated_msg' (given String)." do
+      A::B::C::TestMessage.new(repeated_msg: '2')
+    end
+  end
+
+  def check_error(type, message)
+    err = assert_raises type do
+      yield
+    end
+    assert_equal message, err.message
+  end
+end