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 164 matching lines @@
     return Code().Append(s)
 
   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) {')
-    )
+      .Append('bool %(namespace)s::Populate('))
+    if self._namespace.generate_error_messages:
+      c.Sblock('    const base::Value& value, %(name)s* out, '
+               'std::string* error) {')
+    else:
+      c.Sblock('    const base::Value& value, %(name)s* out) {')

[not at google - send to devlin, 2013/06/11 18:33:43]

I looked at h_generator first so are making comments in here assuming you've
read those first.

Same deal though, make parameters a list, etc. Maybe this should actually be a
method on cc_generator.py (I kinda wish these extended a base class, but only
kind of; it's not really that complicated):

def _GenerateParamsWithError(self, *params):
  if self._GenreateErrorMessages():
    params = params[:] + ['std::string* error']
  return ', '.join(params)

see how that looks.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

+
     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('}')
         )
+      if self._namespace.generate_error_messages:
+        self._GenerateError(c,

[not at google - send to devlin, 2013/06/11 18:33:43]

instead of passing |c| in, make _GenerateError return a Code object and Concat
it.

c.Concat(self._GenerateError(...))

same for all other calls.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

+                            'type-check',
+                            '"unexpected type, got " << value.GetType()')

[not at google - send to devlin, 2013/06/11 18:33:43]

value.GetType() returns an int but we should show a nice meaningful string. add
a method into tools/json_schema_compiler/util.{h,cc} but see if one already
exists in chrome somewhere. If it does maybe we should move it into value.h or
something.

That said I don't think this error message is actually that useful. We want to
associate errors with keys, not with their values. I.e. if we have

type: {
  id: Port,
  type: object
}

myObject: {
  properties: {
    myParam: {
      $ref: Port
    }
  }
}

and somebody tries to do like

{ myParam: 10 }

we want to say

"myParam": expecting object got int

not

unexpected type, got int

that may require a bit of restructuring, since there won't always be a key,
maybe it's a top-level type. the quickest way would probably be to pass the key
name into here and if it's non-None show that, if it's None just show "expecting
object got int".

a nicer one would be to return the error somehow and have the caller decide how
to show it, but in the generated code it may actually be too late to make this
decision, if these already 'return false'. maybe we could have like, for all of
these,

type_populate_code, error = self._GenerateTypePopulate(....)
if error:
  if self._ShouldGenerateErrorMessages():
    type_populate_code.Concat(self._GenerateErrorMessage(...))
  type_populate_code.Append('return false')
c.Append(type_populate_code)

but that may be impractical depending on how all of this is structured.

in any case - same comment throughout this file, where appropriate.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done (kinda) - I'm not completely sure how to get the javascript variable names
from the schema compiler.

       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)) {')
+      if self._namespace.generate_error_messages:
+        self._GenerateError(c,
+                            'type-check',
+                            '"expected dictionary, got " << value.GetType()',
+                            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)) {')
+      if self._namespace.generate_error_messages:
+        self._GenerateError(c,
+                            'get-from-dict',
+                            '"key not found: %(key)s"',

[not at google - send to devlin, 2013/06/11 18:33:43]

'"%(key)s" is required'

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

+                            True)
+      (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) {' % (
+    c.Append('// static')
+    if self._namespace.generate_error_messages:
+      c.Append('scoped_ptr<%s> %s::FromValue(const base::Value& value, '
+                   'std::string* error) {' % (classname, cpp_namespace))
+    else:
+      c.Append('scoped_ptr<%s> %s::FromValue(const base::Value& value) {' % (
           classname, cpp_namespace))
-      .Append('  scoped_ptr<%s> out(new %s());' % (classname, classname))
-      .Append('  if (!Populate(value, out.get()))')
-      .Append('    return scoped_ptr<%s>();' % classname)
+    c.Append('  scoped_ptr<%s> out(new %s());' % (classname, classname))
+    if self._namespace.generate_error_messages:
+      c.Append('  if (!Populate(value, out.get(), error))')
+    else:
+      c.Append('  if (!Populate(value, out.get()))')
+    (c.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()
     """
     if type_.property_type == PropertyType.OBJECT:
@@ skipping 180 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) {')
+    if self._namespace.generate_error_messages:
+      self._GenerateError(c,
+          'params',
+          '"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> '
+    c.Append('// static')
+    if self._namespace.generate_error_messages:
+      c.Sblock('scoped_ptr<Params> Params::Create('
+               'const base::ListValue& args, std::string* error) {')
+    else:
+      c.Sblock('scoped_ptr<Params> '
                   'Params::Create(const base::ListValue& args) {')
-      .Concat(self._GenerateParamsCheck(function, 'args'))
-      .Append('scoped_ptr<Params> params(new Params());')
-    )
+    (c.Concat(self._GenerateParamsCheck(function, 'args'))
+      .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 {')
+        if self._namespace.generate_error_messages:
+          self._GenerateError(c,
+                              'params',
+                              '"missing non-optional value: %(value_var)s"')

[not at google - send to devlin, 2013/06/11 18:33:43]

'"%s" is required'

[Aaron Jacobs, 2013/06/14 02:00:37]

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')))
+        if self._namespace.generate_error_messages:
+          self._GenerateError(c,
+              'get-as-type',
+              '"expected %s, got " << %s->GetType()' %

[not at google - send to devlin, 2013/06/11 18:33:43]

same comments re key name

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

+                  (self._type_helper.GetCppType(type_), src_var), True)
+        (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)))
+        if self._namespace.generate_error_messages:
+          self._GenerateError(c,
+              'get-as-type',
+              '"expected %s, got " << %s->GetType()' %
+                  (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('  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('if (!%(src_var)s->GetAsDictionary(&dictionary)) {'))
+        if self._namespace.generate_error_messages:
+          self._GenerateError(c,
+              'get-as-dict',
+              '"expected dictionary, got " << %(src_var)s->GetType()', True)
+        (c.Append('  return %(failure_value)s;')
+          .Append('}')
+          .Append('scoped_ptr<%(cpp_type)s> temp(new %(cpp_type)s());'))
+        if self._namespace.generate_error_messages:
+          c.Append('if (!%(cpp_type)s::Populate('
+                       '*dictionary, temp.get(), error)) {')
+        else:
+          c.Append('if (!%(cpp_type)s::Populate(*dictionary, temp.get())) {')
+        (c.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('  return %(failure_value)s;')
-          .Append('if (!%(cpp_type)s::Populate(*dictionary, &%(dst_var)s))')
-          .Append('  return %(failure_value)s;')
+          .Append('if (!%(src_var)s->GetAsDictionary(&dictionary)) {'))
+        if self._namespace.generate_error_messages:
+          self._GenerateError(c,
+              'get-as-dict',
+              '"expected dictionary, got " << %(src_var)s->GetType()', True)
+        (c.Append('  return %(failure_value)s;')
+          .Append('}'))
+        if self._namespace.generate_error_messages:
+          c.Append('if (!%(cpp_type)s::Populate('
+                       '*dictionary, &%(dst_var)s, error)) {')
+        else:
+          c.Append('if (!%(cpp_type)s::Populate(*dictionary, &%(dst_var)s)) {')
+        (c.Append('  return %(failure_value)s;')
+          .Append('}')

[not at google - send to devlin, 2013/06/11 18:33:43]

same comments; and is there a way to avoid so much repetition here?

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

         )
     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)) {'))
+      if self._namespace.generate_error_messages:
+        self._GenerateError(c,
+            'get-as-list',
+            '"expected list, got " << %(src_var)s->GetType()', True)
+      (c.Append('  return %(failure_value)s;')

[not at google - send to devlin, 2013/06/11 18:33:43]

as well as saying what the key is, it would be nice to say what index the error
was at; but that probably means cooperating with the C++ utility.

+        .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;')
+        if self._namespace.generate_error_messages:
+          self._GenerateError(c,
+                              'populate-array',
+                              '"unable to populate array"',

[not at google - send to devlin, 2013/06/11 18:33:43]

yeah; bummer. maybe those methods need to be changed to take an error. ugh! oh
well.

+                              True)
+        (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('  return %(failure_value)s;')
+        c.Append('scoped_ptr<%(cpp_type)s> temp(new %(cpp_type)s());')
+        if self._namespace.generate_error_messages:
+          c.Append('if (!%(cpp_type)s::Populate('
+                       '*%(src_var)s, temp.get(), error))')
+        else:
+          c.Append('if (!%(cpp_type)s::Populate(*%(src_var)s, temp.get()))')
+        (c.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;')
-        )
+        if self._namespace.generate_error_messages:
+          c.Append('if (!%(cpp_type)s::Populate('
+                       '*%(src_var)s, &%(dst_var)s, error))')
+        else:
+          c.Append('if (!%(cpp_type)s::Populate(*%(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)) {')
+      if self._namespace.generate_error_messages:
+        self._GenerateError(c,
+            'type-check',
+            '"expected %(value_type)s, got " << %(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)))
+    if self._namespace.generate_error_messages:
+      self._GenerateError(c,
+          'get-as-string',
+          '"expected std::string, got " << %s->GetType()' % 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_))))
+    if self._namespace.generate_error_messages:
+      self._GenerateError(c,
+          'get-from-string',
+          '"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, c, location, body, indent = False):

[not at google - send to devlin, 2013/06/11 18:33:43]

- return the code don't append to an argument
- control the indentation level from the caller not here

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

+    """Generates an error message pertaining to population failure, and appends
+    it to |c|.
+
+    E.g...
+    (get-from-dict): 'key not found: content' at
+        gen/chrome/common/extensions/api/omnibox.cc:241
+    """

[not at google - send to devlin, 2013/06/11 18:33:43]

assert self._ShouldGenerateErrors()

+    padding = '';
+    if indent:
+      padding = '  ';
+    (c.Append(padding + 'if (error) {')
+      .Append(padding + '  std::stringstream ss;')

[not at google - send to devlin, 2013/06/11 18:33:43]

just use string concatenation

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

+      .Append(padding + '  ss << "' + location + ': \'" << ' + body +
+                        ' << "\' at " << __FILE__ << ":" << (__LINE__ - 3);')

[not at google - send to devlin, 2013/06/11 18:33:43]

this is all unlikely to be of interest to an extension developer. I think all
this method really needs to take is |body|.

[Aaron Jacobs, 2013/06/14 02:00:37]

Done.

+      .Append(padding + '  *error = ss.str();')
+      .Append(padding + '}'))