Implement python mojo bindings unions.

Implement python mojo bindings unions.

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/9b88deb16f58cd9a7a0e0ec10a9ef3331896a236

[azani, 2015-07-10 19:49:45]

azani@chromium.org changed reviewers:
+ qsr@chromium.org

[azani, 2015-07-10 19:50:21]

please take a look.

[qsr, 2015-07-10 21:37:34]

Monday and Tuesday are bank holidays in France, I won't be able to review this
before.

 In the meantime, it would be very good if there was validation tests for union.
Without those, all we are testing is that a language has coherent
serialization/deserialization but we have no guarantee that they are
compatibles.

 See: //mojo/public/interfaces/bindings/tests/data/validation

[qsr, 2015-07-16 08:32:55]

On 2015/07/10 21:37:34, qsr wrote:
> Monday and Tuesday are bank holidays in France, I won't be able to review this
> before.
> 
>  In the meantime, it would be very good if there was validation tests for
union.
> Without those, all we are testing is that a language has coherent
> serialization/deserialization but we have no guarantee that they are
> compatibles.
> 
>  See: //mojo/public/interfaces/bindings/tests/data/validation

Alex, ping? Are you looking at adding validation test? At this point, I really
think we should have those worked on if we want to have any confidence about
unions.

[qsr, 2015-07-16 09:35:26]

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/reflection.py (right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:111: dictionary['GetS'] =
classmethod(lambda s: serialization_object)
What is this? I didn't find where it was used? By anyway, this should not be
there. We do not want to add random method to the public interface of those
objects.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:151: if self._tag !=
self._tags[field.name]:
I might be wrong, but I think you do not need to put _tags in self. You can
instead just define it in the function and reference it by, as it will capture
the value. Kind of this:

def f():
  l = []
  def getL():
    return l
  return getL


g1 = f()
g2 = f()
g1() => []
g2() => []
g1().append(1)
g1() => [1]
g2() => []

Same for _tag_names

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:195: dictionary['tag'] =
property(GetTag, None)
What can I compare tag with? I think you are missing the generation of an enum
with all the possible values. You are not testing this either BTW.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:225: __slots__ = ('data', 'tag')
What is this used for.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:237: if
hasattr(field.field_type, 'union_type'):
I really don't like this.

Each time I see a CL for union, it is always a lot of exception/corner case etc.

The more I see it, the more I think we should not have inline serialization for
unions. We should always use a pointer. This would make the whole serialization
simpler.

Anyway, you should use isinstance(field.field_type, UnionType) if you want to
check type, and not the presence of a given field.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:240: struct.pack_into('<Q',
nested_union, 8, entry[2])
You should have a constant for this struct -> you do not want to recompute it
all the time.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_python_generator.py (right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_python_generator.py:215:
'union_field_desc': GetUnionFieldDescriptor,
union_field_descriptor

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/python_templates/module.py.tmpl
(right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module.py.tmpl:39: {% for
union in unions %}
Remove the space before the for loop, and add it after. Right now, if you have
multiple unions, they are not separated, and if you have none, you have 2 blank
lines.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module.py.tmpl:66: 
Why this blank line?

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl
(right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl:44: {%
   for field in union.fields %}
Identation is incorrect.

[azani, 2015-07-16 21:57:39]

ptal

I'm looking at what might be causing the test failures, but they look
unrelated-enough that the review should be able to keep going while I confirm.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/reflection.py (right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:111: dictionary['GetS'] =
classmethod(lambda s: serialization_object)
On 2015/07/16 09:35:25, qsr wrote:
> What is this? I didn't find where it was used? By anyway, this should not be
> there. We do not want to add random method to the public interface of those
> objects.

Sorry, that was here for debugging.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:151: if self._tag !=
self._tags[field.name]:
On 2015/07/16 09:35:25, qsr wrote:
> I might be wrong, but I think you do not need to put _tags in self. You can
> instead just define it in the function and reference it by, as it will capture
> the value. Kind of this:
> 
> def f():
>   l = []
>   def getL():
>     return l
>   return getL
> 
> 
> g1 = f()
> g2 = f()
> g1() => []
> g2() => []
> g1().append(1)
> g1() => [1]
> g2() => []
> 
> Same for _tag_names

Done.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:195: dictionary['tag'] =
property(GetTag, None)
On 2015/07/16 09:35:25, qsr wrote:
> What can I compare tag with? I think you are missing the generation of an enum
> with all the possible values. You are not testing this either BTW.

Done.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:225: __slots__ = ('data', 'tag')
On 2015/07/16 09:35:25, qsr wrote:
> What is this used for.

Sorry, left over from previous iteration.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:237: if
hasattr(field.field_type, 'union_type'):
On 2015/07/16 09:35:26, qsr wrote:
> I really don't like this.
> 
> Each time I see a CL for union, it is always a lot of exception/corner case
etc.
> 
> The more I see it, the more I think we should not have inline serialization
for
> unions. We should always use a pointer. This would make the whole
serialization
> simpler.

I can certainly see your point, but that would also incur an extra 50% space
overhead per union.

> 
> Anyway, you should use isinstance(field.field_type, UnionType) if you want to
> check type, and not the presence of a given field.

UnionType is defined in descriptor.py and descriptor.py imports
serialization.py. This makes it difficult to get a reference to UnionType.

I could do something like this:

def GetUnionType(self):
  if self._union_type:
    return self._union_type
  from mojo_bindings import descriptor
  self._union_type = descriptor.UnionType
  return self._union_type

That has its own bad smell (importing within a function) but I'm happy to defer
on which option is better.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:240: struct.pack_into('<Q',
nested_union, 8, entry[2])
On 2015/07/16 09:35:25, qsr wrote:
> You should have a constant for this struct -> you do not want to recompute it
> all the time.

Done.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/mojom_python_generator.py (right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/mojom_python_generator.py:215:
'union_field_desc': GetUnionFieldDescriptor,
On 2015/07/16 09:35:26, qsr wrote:
> union_field_descriptor

Done.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/python_templates/module.py.tmpl
(right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module.py.tmpl:39: {% for
union in unions %}
On 2015/07/16 09:35:26, qsr wrote:
> Remove the space before the for loop, and add it after. Right now, if you have
> multiple unions, they are not separated, and if you have none, you have 2
blank
> lines.

Done.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module.py.tmpl:66: 
On 2015/07/16 09:35:26, qsr wrote:
> Why this blank line?

Done.

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl
(right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl:44: {%
   for field in union.fields %}
On 2015/07/16 09:35:26, qsr wrote:
> Identation is incorrect.

Done.

[qsr, 2015-07-17 10:02:04]

> >  In the meantime, it would be very good if there was validation tests for
> union.
> > Without those, all we are testing is that a language has coherent
> > serialization/deserialization but we have no guarantee that they are
> > compatibles.
> > 
> >  See: //mojo/public/interfaces/bindings/tests/data/validation
> 
> Alex, ping? Are you looking at adding validation test? At this point, I really
> think we should have those worked on if we want to have any confidence about
> unions.

Alex, still interested to know what you are planning on validation?

[qsr, 2015-07-17 10:53:59]

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/140001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:237: if
hasattr(field.field_type, 'union_type'):
On 2015/07/16 21:57:38, azani wrote:
> On 2015/07/16 09:35:26, qsr wrote:
> > I really don't like this.
> > 
> > Each time I see a CL for union, it is always a lot of exception/corner case
> etc.
> > 
> > The more I see it, the more I think we should not have inline serialization
> for
> > unions. We should always use a pointer. This would make the whole
> serialization
> > simpler.
> 
> I can certainly see your point, but that would also incur an extra 50% space
> overhead per union.

 Well, first of all this is a 8 bytes overhead at worst, which is again a 50%
overhead at worst. It is 0 overhead if the content is an union. And if the
content is a struct or an array, the percentage overhead is way lower than that
because you need to consider the content of the struct or array that is pointed
to.
 In the end, I do not think such a small gain is worst the code complexity which
makes it very hard to have correct encoding/decoding and also significantly
raises the bar to write bindings for a new language.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/reflection.py (right):

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:141: dictionary['__slots__'] =
('_fields', '_tag', '_data')
Instead of keeping the _tag, you can keep a reference to the current field. This
will allows you to no keep tag_names in the closure.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:153: field.name,
tag_names[self._tag])
What do the 2nd and 3rd arguments do?

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:158: self._tag = tags[field.name]
Why is tags[field.name] different than field.index?

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:226: class
UnionSerializer(object):
This code needs comments. I find it really hard to understand what happens.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:240: if
hasattr(field.field_type, 'union_type'):
If you cannot do usual instanceof calls, then adds a is_union() method to the
type object. I really do not like this.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:248: return (16, union.tag,
entry, data), handles
I am clearly missing something.

Let say that you are trying to encode an union containing a struct. Then entry
will be the content of the struct. Let's say this struct has not pointers, so
data will be empty.

Then when you try to serialize this union, how will that work? From what I
understand, this will ends up in line 263, when you will encode the full contant
of the struct as an Uint64. What am I missing? I don't understand how this will
be transformed into the right pointer to the struct content.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl
(right):

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl:45:
{{field|union_field_descriptor}},
Alignemtn is still wrong. The issue was with the for. You want:
{% for field in union.fields %}
  {{field|union_field_descriptor}},
{% endfor %}

See enum_values for example.

[azani, 2015-07-17 20:38:16]

ptal

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/reflection.py (right):

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:141: dictionary['__slots__'] =
('_fields', '_tag', '_data')
On 2015/07/17 10:53:58, qsr wrote:
> Instead of keeping the _tag, you can keep a reference to the current field.
This
> will allows you to no keep tag_names in the closure.

Done.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:153: field.name,
tag_names[self._tag])
On 2015/07/17 10:53:58, qsr wrote:
> What do the 2nd and 3rd arguments do?

All the arguments are printed when you print the AttributeError instance and
they are available as a list if you look at the instance's "args" field.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:158: self._tag = tags[field.name]
On 2015/07/17 10:53:58, qsr wrote:
> Why is tags[field.name] different than field.index?

Done.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:226: class
UnionSerializer(object):
On 2015/07/17 10:53:58, qsr wrote:
> This code needs comments. I find it really hard to understand what happens.

I have added some comments. Let me know if they provide enough clarification.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:240: if
hasattr(field.field_type, 'union_type'):
On 2015/07/17 10:53:58, qsr wrote:
> If you cannot do usual instanceof calls, then adds a is_union() method to the
> type object. I really do not like this.

Done.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:248: return (16, union.tag,
entry, data), handles
On 2015/07/17 10:53:58, qsr wrote:
> I am clearly missing something.
> 
> Let say that you are trying to encode an union containing a struct. Then entry
> will be the content of the struct. Let's say this struct has not pointers, so
> data will be empty.
> 
> Then when you try to serialize this union, how will that work? From what I
> understand, this will ends up in line 263, when you will encode the full
contant
> of the struct as an Uint64. What am I missing? I don't understand how this
will
> be transformed into the right pointer to the struct content.

In your exampled Serialize is PointerType.Serialize at descriptor.py:201 and it
calls StructType.SerializePointer which is at descriptor.py:565.
SerializePointer will actually append the struct to data and return data_offset.
So in the case you describe above, entry is actually a pointer to the serialized
struct data and the serialize struct data got appended to data.

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/tools/bind...
File mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl
(right):

https://codereview.chromium.org/1218023006/diff/240001/mojo/public/tools/bind...
mojo/public/tools/bindings/generators/python_templates/module_macros.tmpl:45:
{{field|union_field_descriptor}},
On 2015/07/17 10:53:58, qsr wrote:
> Alignemtn is still wrong. The issue was with the for. You want:
> {% for field in union.fields %}
>   {{field|union_field_descriptor}},
> {% endfor %}
> 
> See enum_values for example.

Thanks for the clarification.

[qsr, 2015-07-20 21:29:59]

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/reflection.py (right):

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:141: dictionary['__slots__'] =
('_fields', '_cur_field', '_data')
_fields doesn't seem to be used.

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:285: field = self._fields[tag]
1) You should not be using private variable outside of the class. So you might
need to make fields a public variable :( I really do not find any good way to do
this here :(

Also, you need to handle the unknown case (when tag is outside of known fields)

https://codereview.chromium.org/1218023006/diff/260001/mojo/python/tests/bind...
File mojo/python/tests/bindings_unions_unittest.py (right):

https://codereview.chromium.org/1218023006/diff/260001/mojo/python/tests/bind...
mojo/python/tests/bindings_unions_unittest.py:160: def testUnionInMap(self):
Can you add a test for unknown case? Maybe define 2 unions with the same first
fields and one with another field and check what happens when you serialize one
and deserialize the other.

[azani, 2015-07-20 22:17:52]

ptal

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/reflection.py (right):

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/reflection.py:141: dictionary['__slots__'] =
('_fields', '_cur_field', '_data')
On 2015/07/20 21:29:59, qsr wrote:
> _fields doesn't seem to be used.

Done.

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/260001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:285: field = self._fields[tag]
On 2015/07/20 21:29:59, qsr wrote:
> 1) You should not be using private variable outside of the class. So you might
> need to make fields a public variable :( I really do not find any good way to
do
> this here :(
> 
> Also, you need to handle the unknown case (when tag is outside of known
fields)

Done.

https://codereview.chromium.org/1218023006/diff/260001/mojo/python/tests/bind...
File mojo/python/tests/bindings_unions_unittest.py (right):

https://codereview.chromium.org/1218023006/diff/260001/mojo/python/tests/bind...
mojo/python/tests/bindings_unions_unittest.py:160: def testUnionInMap(self):
On 2015/07/20 21:29:59, qsr wrote:
> Can you add a test for unknown case? Maybe define 2 unions with the same first
> fields and one with another field and check what happens when you serialize
one
> and deserialize the other.

Done.

[qsr, 2015-07-20 23:08:28]

https://codereview.chromium.org/1218023006/diff/280001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/280001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:295: if tag not in
self._fields:
Do the test pass? _fields is not in the union anymore, so unless I'm missing
something, I'm worried if test passes.

Also _fields is supposed to contain field object, while tag is an int, how can
this work?

[azani, 2015-07-20 23:12:13]

ptal

https://codereview.chromium.org/1218023006/diff/280001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/280001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:295: if tag not in
self._fields:
On 2015/07/20 23:08:27, qsr wrote:
> Do the test pass? _fields is not in the union anymore, so unless I'm missing
> something, I'm worried if test passes.
> 
> Also _fields is supposed to contain field object, while tag is an int, how can
> this work?

This is the UnionSerializer class. You're thinking of the MojoUnionType class.
UnionSerializer still has self._fields because it needs it here. The tests still
pass.

[qsr, 2015-07-20 23:16:16]

lgtm

https://codereview.chromium.org/1218023006/diff/280001/mojo/public/python/moj...
File mojo/public/python/mojo_bindings/serialization.py (right):

https://codereview.chromium.org/1218023006/diff/280001/mojo/public/python/moj...
mojo/public/python/mojo_bindings/serialization.py:295: if tag not in
self._fields:
On 2015/07/20 23:12:13, azani wrote:
> On 2015/07/20 23:08:27, qsr wrote:
> > Do the test pass? _fields is not in the union anymore, so unless I'm missing
> > something, I'm worried if test passes.
> > 
> > Also _fields is supposed to contain field object, while tag is an int, how
can
> > this work?
> 
> This is the UnionSerializer class. You're thinking of the MojoUnionType class.
> UnionSerializer still has self._fields because it needs it here. The tests
still
> pass.

Oh. Sorry you are perfectly right.

[azani, 2015-07-20 23:41:30]

Committed patchset #16 (id:300001) manually as
9b88deb16f58cd9a7a0e0ec10a9ef3331896a236 (presubmit successful).