Add optional schema compiler error messages

tools/json_schema_compiler/cc_generator.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 from code import Code
 from model import PropertyType, Type
 import cpp_util
 import model
 import schema_util
 import sys
@@ skipping 13 matching lines @@
   """A .cc generator for a namespace.
   """
   def __init__(self, namespace, cpp_type_generator, cpp_namespace):
     self._namespace = namespace
     self._type_helper = cpp_type_generator
     self._cpp_namespace = cpp_namespace
     self._target_namespace = (
         self._type_helper.GetCppNamespaceName(self._namespace))
     self._util_cc_helper = (
         util_cc_helper.UtilCCHelper(self._type_helper))
+    self._generate_error_messages = namespace.compiler_options.get(
+        'generate_error_messages', False)
 
   def Generate(self):
     """Generates a Code object with the .cc for a single namespace.
     """
     c = Code()
     (c.Append(cpp_util.CHROMIUM_LICENSE)
       .Append()
       .Append(cpp_util.GENERATED_FILE_MESSAGE % self._namespace.source_file)
       .Append()
       .Append(self._util_cc_helper.GetIncludePath())
@@ skipping 132 matching lines @@
 
   def _GenerateTypePopulate(self, cpp_namespace, type_):
     """Generates the function for populating a type given a pointer to it.
 
     E.g for type "Foo", generates Foo::Populate()
     """
     classname = cpp_util.Classname(schema_util.StripNamespace(type_.name))
     c = Code()
     (c.Append('// static')
       .Append('bool %(namespace)s::Populate(')
-      .Sblock('    const base::Value& value, %(name)s* out) {')
-    )
+      .Sblock('    %s) {' % self._GenerateParams(
+          ['const base::Value& value', '%(name)s* out'])))
+
     if type_.property_type == PropertyType.CHOICES:
       for choice in type_.choices:
         value_type = cpp_util.GetValueType(self._type_helper.FollowRef(choice))
         (c.Sblock('if (value.IsType(%s)) {' % value_type)
             .Concat(self._GeneratePopulateVariableFromValue(
                 choice,
                 '(&value)',
                 'out->as_%s' % choice.unix_name,
                 'false',
                 is_ptr=True))
             .Append('return true;')
           .Eblock('}')
         )
+      c.Concat(self._GenerateError(
+          '"unexpected type, got " + '
+          'json_schema_compiler::util::ValueTypeToString(value.GetType())'))

[not at google - send to devlin, 2013/06/14 21:48:39]

can you use the same style here (and everywhere), where instead of

c.Append(...)
c.Append(...)
c.Concat(...)

there is

(c.Append(..)
  .Append(...)
  .Concat(...))

and also: "unexpected type" -> "unexpected type choice, got ..." in lieu of
actually doing this properly and saying "unexpected type, got $type but
expecting one of [$types]", which I don't mind us not doing since choices is
quite rare.

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

       c.Append('return false;')
     elif type_.property_type == PropertyType.OBJECT:
-      (c.Append('if (!value.IsType(base::Value::TYPE_DICTIONARY))')
-        .Append('  return false;')
-      )
+      c.Append('if (!value.IsType(base::Value::TYPE_DICTIONARY)) {')
+      c.Concat(self._GenerateError(
+          '"expected dictionary, got " + '
+          'json_schema_compiler::util::ValueTypeToString(value.GetType())',

[not at google - send to devlin, 2013/06/14 21:48:39]

and also there are wrapper for all these in util_cc_helper.py, we should use
that

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+          True))
+      (c.Append('  return false;')
+        .Append('}'))
+
       if type_.properties or type_.additional_properties is not None:
         c.Append('const base::DictionaryValue* dict = '
                      'static_cast<const base::DictionaryValue*>(&value);')
       for prop in type_.properties.values():
         c.Concat(self._InitializePropertyToDefault(prop, 'out'))
       for prop in type_.properties.values():
         c.Concat(self._GenerateTypePopulateProperty(prop, 'dict', 'out'))
       if type_.additional_properties is not None:
         if type_.additional_properties.property_type == PropertyType.ANY:
           c.Append('out->additional_properties.MergeDictionary(dict);')
@@ skipping 30 matching lines @@
           'if (%(src)s->GetWithoutPathExpansion("%(key)s", &%(value_var)s)) {')
         .Concat(self._GeneratePopulatePropertyFromValue(
             prop, value_var, dst, 'false')))
       underlying_type = self._type_helper.FollowRef(prop.type_)
       if underlying_type.property_type == PropertyType.ENUM:
         (c.Append('} else {')
           .Append('%%(dst)s->%%(name)s = %s;' %
               self._type_helper.GetEnumNoneValue(prop.type_)))
       c.Eblock('}')
     else:
-      (c.Append(
-          'if (!%(src)s->GetWithoutPathExpansion("%(key)s", &%(value_var)s))')
-        .Append('  return false;')
+      c.Append(
+          'if (!%(src)s->GetWithoutPathExpansion("%(key)s", &%(value_var)s)) {')
+      c.Concat(self._GenerateError('"%%(key)s is required"', True))

[not at google - send to devlin, 2013/06/14 21:48:39]

let's standardise on something. single quotes - I think that looks better - so
this should be (oh dear) '"\'%%(key)s\' is required"'.

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+      (c.Append('  return false;')
+        .Append('}')
         .Concat(self._GeneratePopulatePropertyFromValue(
             prop, value_var, dst, 'false'))
       )
     c.Append()
     c.Substitute({
       'value_var': value_var,
       'key': prop.name,
       'src': src,
       'dst': dst,
       'name': prop.unix_name
     })
     return c
 
   def _GenerateTypeFromValue(self, cpp_namespace, type_):
     classname = cpp_util.Classname(schema_util.StripNamespace(type_.name))
     c = Code()
     (c.Append('// static')
-      .Append('scoped_ptr<%s> %s::FromValue(const base::Value& value) {' % (
-          classname, cpp_namespace))
+      .Append('scoped_ptr<%s> %s::FromValue(%s) {' % (classname,
+        cpp_namespace, self._GenerateParams(['const base::Value& value'])))
       .Append('  scoped_ptr<%s> out(new %s());' % (classname, classname))
-      .Append('  if (!Populate(value, out.get()))')
+      .Append('  if (!Populate(%s))' % self._GenerateArgs(
+          ['value', 'out.get()']))
       .Append('    return scoped_ptr<%s>();' % classname)
       .Append('  return out.Pass();')
       .Append('}')
     )
     return c
 
   def _GenerateTypeToValue(self, cpp_namespace, type_):
     """Generates a function that serializes the type into a base::Value.
     E.g. for type "Foo" generates Foo::ToValue()
     """
@@ skipping 181 matching lines @@
   def _GenerateParamsCheck(self, function, var):
     """Generates a check for the correct number of arguments when creating
     Params.
     """
     c = Code()
     num_required = 0
     for param in function.params:
       if not param.optional:
         num_required += 1
     if num_required == len(function.params):
-      c.Append('if (%(var)s.GetSize() != %(total)d)')
+      c.Append('if (%(var)s.GetSize() != %(total)d) {')
     elif not num_required:
-      c.Append('if (%(var)s.GetSize() > %(total)d)')
+      c.Append('if (%(var)s.GetSize() > %(total)d) {')
     else:
       c.Append('if (%(var)s.GetSize() < %(required)d'
-          ' || %(var)s.GetSize() > %(total)d)')
+          ' || %(var)s.GetSize() > %(total)d) {')
+    c.Concat(self._GenerateError(
+        '"expected %%(total)d arguments, got " + %%(var)s.GetSize()',
+        True))
     c.Append('  return scoped_ptr<Params>();')
+    c.Append('}')
     c.Substitute({
         'var': var,
         'required': num_required,
         'total': len(function.params),
     })
     return c
 
   def _GenerateFunctionParamsCreate(self, function):
     """Generate function to create an instance of Params. The generated
     function takes a base::ListValue of arguments.
 
     E.g for function "Bar", generate Bar::Params::Create()
     """
     c = Code()
     (c.Append('// static')
-      .Sblock('scoped_ptr<Params> '
-                  'Params::Create(const base::ListValue& args) {')
+      .Sblock('scoped_ptr<Params> Params::Create(%s) {' % self._GenerateParams(
+        ['const base::ListValue& args']))
       .Concat(self._GenerateParamsCheck(function, 'args'))
-      .Append('scoped_ptr<Params> params(new Params());')
-    )
+      .Append('scoped_ptr<Params> params(new Params());'))
 
     for param in function.params:
       c.Concat(self._InitializePropertyToDefault(param, 'params'))
 
     for i, param in enumerate(function.params):
       # Any failure will cause this function to return. If any argument is
       # incorrect or missing, those following it are not processed. Note that
       # for optional arguments, we allow missing arguments and proceed because
       # there may be other arguments following it.
       failure_value = 'scoped_ptr<Params>()'
       c.Append()
       value_var = param.unix_name + '_value'
       (c.Append('const base::Value* %(value_var)s = NULL;')
         .Append('if (args.Get(%(i)s, &%(value_var)s) &&')
         .Sblock('    !%(value_var)s->IsType(base::Value::TYPE_NULL)) {')
         .Concat(self._GeneratePopulatePropertyFromValue(
             param, value_var, 'params', failure_value))
         .Eblock('}')
       )
       if not param.optional:
-        (c.Sblock('else {')
-          .Append('return %s;' % failure_value)
-          .Eblock('}')
-        )
+        c.Sblock('else {')
+        c.Concat(self._GenerateError('"%%(value_var)s is required"'))

[not at google - send to devlin, 2013/06/14 21:48:39]

ditto, single quotes around the key in here

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+        (c.Append('return %s;' % failure_value)
+          .Eblock('}'))
       c.Substitute({'value_var': value_var, 'i': i})
     (c.Append()
       .Append('return params.Pass();')
       .Eblock('}')
       .Append()
     )
 
     return c
 
   def _GeneratePopulatePropertyFromValue(self,
@@ skipping 24 matching lines @@
     |failure_value|.
     """
     c = Code()
     c.Sblock('{')
 
     underlying_type = self._type_helper.FollowRef(type_)
 
     if underlying_type.property_type.is_fundamental:
       if is_ptr:
         (c.Append('%(cpp_type)s temp;')
-          .Append('if (!%s)' % cpp_util.GetAsFundamentalValue(
-                      self._type_helper.FollowRef(type_), src_var, '&temp'))
-          .Append('  return %(failure_value)s;')
+          .Append('if (!%s) {' % cpp_util.GetAsFundamentalValue(
+                      self._type_helper.FollowRef(type_), src_var, '&temp')))
+        c.Concat(self._GenerateError('"\'%s\': expected %s, got " + '

[not at google - send to devlin, 2013/06/14 21:48:39]

ditto, and the inside-parens thing all through here

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+            'json_schema_compiler::util::ValueTypeToString(%s->GetType())' %
+            (src_var, self._type_helper.GetCppType(type_), src_var), True))

[not at google - send to devlin, 2013/06/14 21:48:39]

src_var isn't right, that's a c++ variable name. we want the key name in the
schema.

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+        (c.Append('  return %(failure_value)s;')
+          .Append('}')
           .Append('%(dst_var)s.reset(new %(cpp_type)s(temp));')
         )
       else:
-        (c.Append('if (!%s)' % cpp_util.GetAsFundamentalValue(
+        (c.Append('if (!%s) {' % cpp_util.GetAsFundamentalValue(
                       self._type_helper.FollowRef(type_),
                       src_var,
-                      '&%s' % dst_var))
-          .Append('  return %(failure_value)s;')
+                      '&%s' % dst_var)))
+        c.Concat(self._GenerateError('"\'%s\': expected %s, got " + '
+            'json_schema_compiler::util::ValueTypeToString(%s->GetType())' %
+            (src_var, self._type_helper.GetCppType(type_), src_var), True))
+        (c.Append('  return %(failure_value)s;')
+          .Append('}')
         )
     elif underlying_type.property_type == PropertyType.OBJECT:
       if is_ptr:
         (c.Append('const base::DictionaryValue* dictionary = NULL;')
-          .Append('if (!%(src_var)s->GetAsDictionary(&dictionary))')
+          .Append('if (!%(src_var)s->GetAsDictionary(&dictionary)) {'))
+        c.Concat(self._GenerateError(
+            '"\'%%(src_var)s\': expected dictionary, got " + '
+            'json_schema_compiler::util::ValueTypeToString('
+            '%%(src_var)s->GetType())', True))
+        (c.Append('  return %(failure_value)s;')
+          .Append('}')
+          .Append('scoped_ptr<%(cpp_type)s> temp(new %(cpp_type)s());')
+          .Append('if (!%%(cpp_type)s::Populate(%s)) {' % self._GenerateArgs(
+            ['*dictionary', 'temp.get()']))
           .Append('  return %(failure_value)s;')
-          .Append('scoped_ptr<%(cpp_type)s> temp(new %(cpp_type)s());')
-          .Append('if (!%(cpp_type)s::Populate(*dictionary, temp.get()))')
-          .Append('  return %(failure_value)s;')
+          .Append('}')
           .Append('%(dst_var)s = temp.Pass();')
         )
       else:
         (c.Append('const base::DictionaryValue* dictionary = NULL;')
-          .Append('if (!%(src_var)s->GetAsDictionary(&dictionary))')
+          .Append('if (!%(src_var)s->GetAsDictionary(&dictionary)) {'))
+        c.Concat(self._GenerateError(
+            '"\'%%(src_var)s\': expected dictionary, got " + '
+            'json_schema_compiler::util::ValueTypeToString('
+            '%%(src_var)s->GetType())', True))
+        (c.Append('  return %(failure_value)s;')
+          .Append('}')
+          .Append('if (!%%(cpp_type)s::Populate(%s)) {' % self._GenerateArgs(
+            ['*dictionary', '&%(dst_var)s']))
           .Append('  return %(failure_value)s;')
-          .Append('if (!%(cpp_type)s::Populate(*dictionary, &%(dst_var)s))')
-          .Append('  return %(failure_value)s;')
+          .Append('}')
         )
     elif underlying_type.property_type == PropertyType.FUNCTION:
       if is_ptr:
         c.Append('%(dst_var)s.reset(new base::DictionaryValue());')
     elif underlying_type.property_type == PropertyType.ANY:
       c.Append('%(dst_var)s.reset(%(src_var)s->DeepCopy());')
     elif underlying_type.property_type == PropertyType.ARRAY:
       # util_cc_helper deals with optional and required arrays
       (c.Append('const base::ListValue* list = NULL;')
-        .Append('if (!%(src_var)s->GetAsList(&list))')
-        .Append('  return %(failure_value)s;'))
+        .Append('if (!%(src_var)s->GetAsList(&list)) {'))
+      c.Concat(self._GenerateError(
+          '"\'%%(src_var)s\': expected list, got " + '
+          'json_schema_compiler::util::ValueTypeToString('
+          '%%(src_var)s->GetType())', True))
+      (c.Append('  return %(failure_value)s;')
+        .Append('}'))
       item_type = underlying_type.item_type
       if item_type.property_type == PropertyType.ENUM:
         c.Concat(self._GenerateListValueToEnumArrayConversion(
                      item_type,
                      'list',
                      dst_var,
                      failure_value,
                      is_ptr=is_ptr))
       else:
-        (c.Append('if (!%s)' % self._util_cc_helper.PopulateArrayFromList(
+        c.Append('if (!%s) {' % self._util_cc_helper.PopulateArrayFromList(
               underlying_type,
               'list',
               dst_var,
               is_ptr))
-          .Append('  return %(failure_value)s;')
+        c.Concat(self._GenerateError('"unable to populate array"', True))

[not at google - send to devlin, 2013/06/14 21:48:39]

key can be known here, right?

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+        (c.Append('  return %(failure_value)s;')
+          .Append('}')
         )
     elif underlying_type.property_type == PropertyType.CHOICES:
       if is_ptr:
         (c.Append('scoped_ptr<%(cpp_type)s> temp(new %(cpp_type)s());')
-          .Append('if (!%(cpp_type)s::Populate(*%(src_var)s, temp.get()))')
+          .Append('if (!%%(cpp_type)s::Populate(%s))' % self._GenerateArgs(
+            ['*%(src_var)s', 'temp.get()']))
           .Append('  return %(failure_value)s;')
           .Append('%(dst_var)s = temp.Pass();')
         )
       else:
-        (c.Append('if (!%(cpp_type)s::Populate(*%(src_var)s, &%(dst_var)s))')
-          .Append('  return %(failure_value)s;')
-        )
+        c.Append('if (!%%(cpp_type)s::Populate(%s))' % self._GenerateArgs(
+            ['*%(src_var)s', '&%(dst_var)s']))
+        c.Append('  return %(failure_value)s;')
     elif underlying_type.property_type == PropertyType.ENUM:
       c.Concat(self._GenerateStringToEnumConversion(type_,
                                                     src_var,
                                                     dst_var,
                                                     failure_value))
     elif underlying_type.property_type == PropertyType.BINARY:
-      (c.Append('if (!%(src_var)s->IsType(%(value_type)s))')
-        .Append('  return %(failure_value)s;')
+      c.Append('if (!%(src_var)s->IsType(%(value_type)s)) {')
+      c.Concat(self._GenerateError(
+          '"\'%%(src_var)s\': expected %(value_type)s, got " + '
+          'json_schema_compiler::util::ValueTypeToString('
+          '%(src_var)s->GetType())', True))
+      (c.Append('  return %(failure_value)s;')
+        .Append('}')
         .Append('const base::BinaryValue* binary_value =')
         .Append('    static_cast<const base::BinaryValue*>(%(src_var)s);')
        )
       if is_ptr:
         (c.Append('%(dst_var)s.reset(')
           .Append('    new std::string(binary_value->GetBuffer(),')
           .Append('                    binary_value->GetSize()));')
          )
       else:
         (c.Append('%(dst_var)s.assign(binary_value->GetBuffer(),')
@@ skipping 49 matching lines @@
                                       src_var,
                                       dst_var,
                                       failure_value):
     """Returns Code that converts a string type in |src_var| to an enum with
     type |type_| in |dst_var|. In the generated code, if |src_var| is not
     a valid enum name then the function will return |failure_value|.
     """
     c = Code()
     enum_as_string = '%s_as_string' % type_.unix_name
     (c.Append('std::string %s;' % enum_as_string)
-      .Append('if (!%s->GetAsString(&%s))' % (src_var, enum_as_string))
-      .Append('  return %s;' % failure_value)
+      .Append('if (!%s->GetAsString(&%s)) {' % (src_var, enum_as_string)))
+    c.Concat(self._GenerateError(
+        '"\'%s\': expected std::string, got " + '

[not at google - send to devlin, 2013/06/14 21:48:39]

just "string" not "std::string"

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+        'json_schema_compiler::util::ValueTypeToString('
+        '%s->GetType())' % (src_var, src_var), True))
+    (c.Append('  return %s;' % failure_value)
+      .Append('}')
       .Append('%s = Parse%s(%s);' % (dst_var,
                                      self._type_helper.GetCppType(type_),
                                      enum_as_string))
-      .Append('if (%s == %s)' % (dst_var,
-                                 self._type_helper.GetEnumNoneValue(type_)))
-     .Append('  return %s;' % failure_value)
+      .Append('if (%s == %s) {' % (dst_var,
+                                 self._type_helper.GetEnumNoneValue(type_))))
+    c.Concat(self._GenerateError(
+        '"got bad enum value from variable \'%s\'"' % dst_var, True))
+    (c.Append('  return %s;' % failure_value)
+      .Append('}')
     )
     return c
 
   def _GeneratePropertyFunctions(self, namespace, params):
     """Generates the member functions for a list of parameters.
     """
     return self._GenerateTypes(namespace, (param.type_ for param in params))
 
   def _GenerateTypes(self, namespace, types):
     """Generates the member functions for a list of types.
@@ skipping 97 matching lines @@
     """
     c = Code()
     underlying_type = self._type_helper.FollowRef(prop.type_)
     if (underlying_type.property_type == PropertyType.ENUM and
         prop.optional):
       c.Append('%s->%s = %s;' % (
         dst,
         prop.unix_name,
         self._type_helper.GetEnumNoneValue(prop.type_)))
     return c
+
+  def _GenerateError(self, body, indent = False):
+    """Generates an error message pertaining to population failure.
+
+    E.g 'expected bool, got int'
+    """
+    c = Code()
+    if not self._generate_error_messages:
+      return c
+    padding = '';
+    if indent:
+      padding = '  ';
+    (c.Append(padding + 'if (error)')
+      .Append(padding + '  *error = ' + body + ';'))

[not at google - send to devlin, 2013/06/14 21:48:39]

why do you need this padding logic? the caller can do it if necessary.

[Aaron Jacobs, 2013/06/15 00:47:18]

Done.

+    return c
+
+  def _GenerateParams(self, params):
+    """Builds the parameter list for a function, given an array of parameters.
+    """
+    if self._generate_error_messages:
+      params.append('std::string* error')
+    return ', '.join(params)
+
+  def _GenerateArgs(self, args):
+    """Builds the argument list for a function, given an array of arguments.
+    """
+    if self._generate_error_messages:
+      args.append('error')
+    return ', '.join(args)