Improvements to JSON Schema Compiler's Dart Generation

tools/json_schema_compiler/dart_generator.py

Index: tools/json_schema_compiler/dart_generator.py
diff --git a/tools/json_schema_compiler/dart_generator.py b/tools/json_schema_compiler/dart_generator.py
index 9503ef337f1ba6845f3938de901783ddca474dc8..e97048f863e12fa40157cc1de987a0c5cfb54eb1 100644
--- a/tools/json_schema_compiler/dart_generator.py
+++ b/tools/json_schema_compiler/dart_generator.py
@@ -43,11 +43,17 @@ class _Generator(object):
             type_path = '.'.join(filename.split('.')[1:-1])
             self._type_overrides[type_path] = f.read()
 
+    # TODO(sashab): Add all inline type definitions to the global Types
+    # dictionary.
+    # List types do not apply - their types are automatically generated instead
+    # of the list type.
+
   def Generate(self):
     """Generates a Code object with the .dart for the entire namespace.
     """
     c = Code()
-    (c.Append(LICENSE)
+    (c.Append(LICENSE.strip())

[not at google - send to devlin, 2013/02/07 01:16:41]

Why strip()?

P.S. you could make LICENCE a Code() object and then use Cblock here
(automatically adds the blank line) - if you want. I don't mind either way.

[sashab, 2013/02/08 01:53:34]

The problem is getting the LICENSE (and the triple quotes) to fit inside the
code. If you look at where its defined, its hard to fit it to 80 chars since it
barely fits within 80 chars itself.

Managed to get it to fit, though, removing the need for .strip().

+      .Append()
       .Append('// Generated from namespace: %s' % self._namespace.name)
       .Append()
       .Append('part of chrome;'))
@@ -58,8 +64,11 @@ class _Generator(object):
         .Append(' * Types')
         .Append(' */')
       )
-    for type_name in self._types:
-      c.Concat(self._GenerateType(self._types[type_name]))
+    for type_ in self._types.values():
+      # Check for custom dart for this whole type.
+      c.Append()
+      if not self._ConcatOverride(c, [type_.name], doc_property=type_):

[not at google - send to devlin, 2013/02/07 01:16:41]

Without looking at the implementation of ConcatOverride: can it return a Code
object rather than accepting one to concat onto? Return None if it doesn't? I
think I meant to make this comment last time around but forgot.

You could be super tricky like

override = self._GetOverride([type_.name], doc_property=true)
c.Cblock(override if override is not None else self._GenerateType(type_))

but wow ternarys are ugly. Whatever, use your judgement.

[sashab, 2013/02/08 01:53:34]

This is pretty much what I wanted to do before. Not super-tricky, but I wanted
to get a code object and then be like "if it exists, add it on, blah blah"...
But it seemed much simpler to just return T/F.

Changed it to return a code object, arguably more readable. Refactored the way
getters/setters work to use the nice ternary operators too =)

+        c.Concat(self._GenerateType(type_))

[not at google - send to devlin, 2013/02/07 01:16:41]

I suggest using Cblock() here rather than the c.Append() before. In fixing
whitespace things in the C++ compiler I've found that the easiest way to get
things right is for Code to generate without any blank lines before/after and
let the Concat'ters of the code add any spacing (usually with Cblock if they
desire that).

Since I presume that you want a blank line between each type definition, use
Cblock here. If you always want a blank line between /** Types */ and the first
type, do it there (up on line 65).

[sashab, 2013/02/08 01:53:34]

Don't think I need a blank line between /** Types */ and the first type. The
problem with CBlock() is a lot of the places I'm using .Append() need strings,
and CBlock() wants a code object.

 
     if self._namespace.events:
       (c.Append()
@@ -87,10 +96,18 @@ class _Generator(object):
     object that extends ChromeObject. All parameters are specifiable as named
     arguments in the constructor, and all methods are wrapped with getters and
     setters that hide the JS() implementation.
+
+    List objects with inline types are created with a definition for the inline
+    type instead.
     """
+    if type_.property_type is PropertyType.ARRAY:
+      # This is an inline type definition: generate that type instead.
+      type_ = type_.item_type
+      # Save this to global types, just so future functions don't get confused.
+      self._types[type_.name] = type_

[not at google - send to devlin, 2013/02/07 01:16:41]

This modifies the original model (since types_ is just a reference into the
model). That probably isn't what you want to do.

Why is this necessary anyway?

[sashab, 2013/02/08 01:53:34]

A little complicated... Basically for objects that are actually arrays. In this
case, we want a wrapper for the actual object, not the array. May not be needed
anymore so removed.

+
     c = Code()
-    (c.Append()
-      .Concat(self._GenerateDocumentation(type_))
+    (c.Concat(self._GenerateDocumentation(type_))
       .Sblock('class %(type_name)s extends ChromeObject {')
     )
 
@@ -134,15 +151,15 @@ class _Generator(object):
 
       # Check for custom dart for this whole property.
       c.Append()
-      if not self._ConcatOverride(c, type_, prop, add_doc=True):
+      if not self._ConcatOverride(c, [type_.name, prop.name],
+                                  doc_property=prop):
         # Add the getter.
-        if not self._ConcatOverride(c, type_, prop, key_suffix='.get',
-                                       add_doc=True):
+        if not self._ConcatOverride(c, [type_.name, prop.name, '.get'],
+                                    doc_property=prop):
           # Add the documentation for this property.
           c.Concat(self._GenerateDocumentation(prop))
 
-          if (self._IsBaseType(prop.type_)
-              or self._IsListOfBaseTypes(prop.type_)):
+          if (self._IsBaseType(prop.type_, list_as_base=True)):
             c.Append("%s get %s => JS('%s', '#.%s', this._jsObject);" %
                 (type_name, prop.name, type_name, prop.name))
           elif self._IsSerializableObjectType(prop.type_):
@@ -150,13 +167,18 @@ class _Generator(object):
                      "this._jsObject));"
               % (type_name, prop.name, type_name, prop.name))
           elif self._IsListOfSerializableObjects(prop.type_):
-            (c.Sblock('%s get %s {' % (type_name, prop.name))
-              .Append('%s __proxy_%s = new %s();' % (type_name, prop.name,
-                                                     type_name))
+            array_type, array_type_name = prop.type_, type_name
+            if prop.type_.property_type is not PropertyType.ARRAY:
+              # This is a reference to an array object, not the object itself.
+              array_type = self._types[prop.type_.ref_type]

[not at google - send to devlin, 2013/02/07 01:16:41]

I think you should re-use CppTypeGenerator rather than reimplementing the ref
logic.

And by "reuse" I mean pull out the initialization logic, FindType, and FollowRef
(make FindType public) into a new class like TypeGraph. Construct TypeGraph
rather than CppTypeGenerator in the compiler, pass it everywhere, and the h/c
generators can construct the CppTypeGenerator themselves from an injected
TypeGraph.

If you read through the c/h generators and look for FollowRef you'll see an
idiom where methods right at the start do something like

concrete_type = type_helper_.FollowRef(type_)

and then deal with the concrete type rather than the ref type. That won't always
be the right thing to do obviously - often you'll want to know that it is
actually a ref - but when generating the instructions typically you only care
about the concrete type.

I think many places where you're looking at REF would be moot if you used that
idiom.

[sashab, 2013/02/08 01:53:34]

May no longer be needed; temporarily removed.

+              array_type_name =self._GetDartType(array_type)
+            (c.Sblock('%s get %s {' % (array_type_name, prop.name))
+              .Append('%s __proxy_%s = new %s();' % (array_type_name, prop.name,
+                                                     array_type_name))
               .Sblock("for (var o in JS('List', '#.%s', this._jsObject)) {" %
                       prop.name)
               .Append('__proxy_%s.add(new %s._proxy(o));' % (prop.name,
-                   self._GetDartType(prop.type_.item_type)))
+                   self._GetDartType(array_type.item_type)))
               .Eblock('}')
               .Append('return __proxy_%s;' % prop.name)
               .Eblock('}')
@@ -173,7 +195,7 @@ class _Generator(object):
 
         # Add the setter.
         c.Append()
-        if not self._ConcatOverride(c, type_, prop, key_suffix='.set'):
+        if not self._ConcatOverride(c, [type_.name, prop.name, '.set']):
           wrapped_name = prop.name
           if not self._IsBaseType(prop.type_):
             wrapped_name = 'convertArgument(%s)' % prop.name
@@ -194,7 +216,10 @@ class _Generator(object):
         .Append(' */')
       )
     for prop in methods:
-      c.Concat(self._GenerateFunction(prop.type_.function))
+      # Check if there's an override for this method.
+      if not self._ConcatOverride(c, [type_.name, prop.name],
+                                    doc_property=prop):
+        c.Concat(self._GenerateFunction(prop.type_.function))
 
     (c.Eblock('}')
       .Substitute({
@@ -223,9 +248,7 @@ class _Generator(object):
     """Returns the Code object for the given function.
     """
     c = Code()
-    (c.Append()
-      .Concat(self._GenerateDocumentation(f))
-    )
+    c.Concat(self._GenerateDocumentation(f))
 
     if not self._NeedsProxiedCallback(f):
         c.Append("%s => %s;" % (self._GenerateFunctionSignature(f),
@@ -252,7 +275,7 @@ class _Generator(object):
     # their members (by copying out each member and proxying it).
     lists_to_proxy = []
     for p in f.params:
-      if self._IsBaseType(p.type_) or self._IsListOfBaseTypes(p.type_):
+      if self._IsBaseType(p.type_, list_as_base=True):
         proxied_params.append(p.name)
       elif self._IsSerializableObjectType(p.type_):
         proxied_params.append('new %s._proxy(%s)' % (
@@ -263,6 +286,11 @@ class _Generator(object):
       elif self._IsObjectType(p.type_):
         # TODO(sashab): Find a way to build generic JS objects back in Dart.
         proxied_params.append('%s' % p.name)
+      elif p.type_.property_type is PropertyType.ARRAY:
+        # TODO(sashab): This might be okay - what if this is a list of
+        # FileEntry elements? In this case, a basic list will proxy the objects
+        # fine.
+        proxied_params.append('%s' % p.name)
       else:
         raise Exception(
             "Cannot automatically create proxy; can't wrap %s, type %s" % (
@@ -425,7 +453,10 @@ class _Generator(object):
         .Append(' */')
       )
     for function in self._namespace.functions.values():
-      c.Concat(self._GenerateFunction(function))
+      # Check for custom dart for this whole property.
+      c.Append()
+      if not self._ConcatOverride(c, [function.name], doc_property=function):
+        c.Concat(self._GenerateFunction(function))
 
     # Add the constructor.
     (c.Append()
@@ -459,22 +490,17 @@ class _Generator(object):
                'name': prop.simple_name
            }
 
-  def _GenerateFunctionSignature(self, function):
+  def _GenerateFunctionSignature(self, function, convert_optional=False):
     """Given a function object, returns the signature for that function.
     Recursively generates the signature for callbacks.
     Returns a String for the given function.
 
-    If prepend_this is True, adds "this." to the function's name.
+    If convert_optional is True, changes optional parameters to be required.
 
     e.g.
       void onClosed()
       bool isOpen([String type])
       void doSomething(bool x, void callback([String x]))
-
-    e.g. If prepend_this is True:
-      void this.onClosed()
-      bool this.isOpen([String type])
-      void this.doSomething(bool x, void callback([String x]))
     """
     sig = '%(return_type)s %(name)s(%(params)s)'
 
@@ -487,7 +513,8 @@ class _Generator(object):
         'return_type': return_type,
         'name': function.simple_name,
         'params': self._GenerateParameterList(function.params,
-                                              function.callback)
+                                              function.callback,
+                                              convert_optional=convert_optional)
     }
 
   def _GenerateParameterList(self,
@@ -518,7 +545,7 @@ class _Generator(object):
 
     # Add the callback, if it exists.
     if callback:
-      c_sig = self._GenerateFunctionSignature(callback)
+      c_sig = self._GenerateFunctionSignature(callback, convert_optional=True)
       if callback.optional:
         params_opt.append(c_sig)
       else:
@@ -544,26 +571,22 @@ class _Generator(object):
     # prevents a leading comma, e.g. '(, [a, b])'.
     return ', '.join(p for p in param_sets if p)
 
-  def _ConcatOverride(self, c, type_, prop, key_suffix='', add_doc=False):
-    """Given a particular type and property to find in the custom dart
-    overrides, checks whether there is an override for that key.
-    If there is, appends the override code, and returns True.
-    If not, returns False.
-
-    |key_suffix| will be added to the end of the key before searching, e.g.
-    '.set' or '.get' can be used for setters and getters respectively.
+  def _ConcatOverride(self, c, key_chain, doc_property=False):

[not at google - send to devlin, 2013/02/07 01:16:41]

From what I can tell, this isn't always a property. In fact reading the code I
assumed it was some flag about whether to document properties.

How about like "document_as" or something.

Also =False as the default argument seems wrong. Should be None.

[sashab, 2013/02/08 01:53:34]

Done.

+    """Given a list of keys, joins them with periods and searches for them in
+    the custom dart overrides.
+    If there is an override for that key, appends the override code and returns
+    True. If not, returns False.
 
-    If add_doc is given, adds the documentation for this property before the
-    override code.
+    If doc_property is given, adds the documentation for this property before
+    the override code.
     """
-    contents = self._type_overrides.get('%s.%s%s' % (type_.name, prop.name,
-                                     key_suffix))
+    contents = self._type_overrides.get('.'.join(key_chain))
     if contents is None:
       return False
 
-    if prop is not None:
-      c.Concat(self._GenerateDocumentation(prop))
-    for line in contents.split('\n'):
+    if doc_property is not None:
+      c.Concat(self._GenerateDocumentation(doc_property))
+    for line in contents.strip().split('\n'):

[not at google - send to devlin, 2013/02/07 01:16:41]

why strip()?

[sashab, 2013/02/08 01:53:34]

Good question. File contents always contain a trailing newline. Since the
documentation should be straight before the function definition, all newlines
here must be removed (hence removing all the blank lines). Maybe .strip('\n') is
better?

       c.Append(line)
     return True
 
@@ -575,10 +598,14 @@ class _Generator(object):
   def _IsSerializableObjectType(self, type_):
     """Given a model.Type, returns whether this type is a serializable object.
     Serializable objects are custom types defined in this namespace.
+
+    If this object is a reference to something not in this namespace, assumes
+    its a serializable object.
     """
-    if (type_.property_type == PropertyType.REF
-        and type_.ref_type in self._types):
-      return self._IsObjectType(self._types[type_.ref_type])
+    if type_.property_type is PropertyType.REF:
+      if type_.ref_type in self._types:
+        return self._IsObjectType(self._types[type_.ref_type])
+      return True
     if (type_.property_type == PropertyType.OBJECT
         and type_.instance_of in self._types):
       return self._IsObjectType(self._types[type_.instance_of])
@@ -592,10 +619,19 @@ class _Generator(object):
 
   def _IsListOfSerializableObjects(self, type_):
     """Given a model.Type, returns whether this type is a list of serializable
-    objects (PropertyType.REF types).
+    objects (or regular objects, if this list is treated as a type - in this
+    case, the item type was defined inline).
+
+    If this type is a reference to something not in this namespace, assumes
+    it is not a list of serializable objects.
     """
+    if type_.property_type is PropertyType.REF:
+      if type_.ref_type in self._types:
+        return self._IsListOfSerializableObjects(self._types[type_.ref_type])
+      return False
     return (type_.property_type is PropertyType.ARRAY and
-            type_.item_type.property_type is PropertyType.REF)
+        (self._IsSerializableObjectType(type_.item_type) or
+         (self._IsObjectType(type_.item_type) and type_.name in self._types)))
 
   def _IsListOfBaseTypes(self, type_):
     """Given a model.Type, returns whether this type is a list of base type
@@ -604,12 +640,23 @@ class _Generator(object):
     return (type_.property_type is PropertyType.ARRAY and
             self._IsBaseType(type_.item_type))
 
-  def _IsBaseType(self, type_):
+  def _IsBaseType(self, type_, list_as_base=False):

[not at google - send to devlin, 2013/02/07 01:16:41]

list_as_base isn't descriptive on its own, I had to come look a this method to
see what all the list_as_base things were about. It's also a pretty specialised
thing to be passing a generic method like this.

I'm not really sure what should be the default (true or false) though true makes
a bit more sense to me - so how about a parameter "follow" or "unwrap"
(follow=False/unwrap=False) and invert the conditions.

Does the same problem exist for other types that wrap other types? Notably ref
types (should those be treated the same was as lists?). At times when you don't
want to unwrap, do you still want to dig down into choices?

I guess my understanding of the code that's being generated isn't deep enough to
understand the problem that this solves.

[sashab, 2013/02/08 01:53:34]

Before, lists had to be wrapped to be generated correctly. In this new version,
lists don't need to be wrapped, so they are technically a "base type". Choices
will remain a base type until we come up with a nice way to handle them using
Dart classes (I added a note about that).

     """Given a model.type_, returns whether this type is a base type
     (string, number or boolean).
+
+    If type_ is a Choices object, returns True if all possible choices are base
+    types.
+
+    If list_as_base is True, counts List as a base type.
     """
-    return (self._GetDartType(type_) in
-            ['bool', 'num', 'int', 'double', 'String'])
+    return (
+        (self._GetDartType(type_) in ['bool', 'num', 'int', 'double', 'String'])
+        or (type_.property_type is PropertyType.ARRAY
+           and self._IsBaseType(type_.item_type, list_as_base=list_as_base))
+        or (type_.property_type is PropertyType.CHOICES
+           and all(self._IsBaseType(t, list_as_base=list_as_base)
+                   for t in type_.choices))
+    )
 
   def _GetDartType(self, type_):
     """Given a model.Type object, returns its type as a Dart string.
@@ -619,6 +666,8 @@ class _Generator(object):
 
     prop_type = type_.property_type
     if prop_type is PropertyType.REF:
+      if type_.ref_type in self._types:
+        return self._GetDartType(self._types[type_.ref_type])
       # TODO(sashab): If the type is foreign, it might have to be imported.
       return StripNamespace(type_.ref_type)
     elif prop_type is PropertyType.BOOLEAN:
@@ -634,12 +683,20 @@ class _Generator(object):
     elif prop_type is PropertyType.ENUM:
       return 'String'
     elif prop_type is PropertyType.CHOICES:
-      # TODO: What is a Choices type? Is it closer to a Map Dart object?
+      # TODO: Think of a nice way to generate code for Choices objects in Dart.
       return 'Object'
     elif prop_type is PropertyType.ANY:
       return 'Object'
     elif prop_type is PropertyType.OBJECT:
-      return type_.instance_of or 'Object'
+      if type_.instance_of is not None:
+        return type_.instance_of
+      if type(type_.parent) is not Function:
+        return type_.name
+      # If we're here, the parent's type is a Function. So it's name will be the
+      # name of the function parameter (its an inline type definition).
+      # TODO(sashab): When there is a nice way to generate code for inline
+      # types, change this to reflect the type's new name.
+      return 'Object'
     elif prop_type is PropertyType.FUNCTION:
       return 'Function'
     elif prop_type is PropertyType.ARRAY: