Anton Nefedov <anton.nefe...@virtuozzo.com> writes:

> On 14/6/2018 10:19 AM, Markus Armbruster wrote:
>> Anton Nefedov <anton.nefe...@virtuozzo.com> writes:
>>
>>> It often happens that just a few discriminator values imply extra data in
>>> a flat union. Existing checks did not make possible to leave other values
>>> uncovered. Such cases had to be worked around by either stating a dummy
>>> (empty) type or introducing another (subset) discriminator enumeration.
>>>
>>> Both options create redundant entities in qapi files for little profit.
>>>
>>> With this patch it is not necessary anymore to add designated union
>>> fields for every possible value of a discriminator enumeration.
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefe...@virtuozzo.com>
>>> Reviewed-by: Eric Blake <ebl...@redhat.com>
[...]
>>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>>> index 2462fc0..0f3d89b 100644
>>> --- a/scripts/qapi/common.py
>>> +++ b/scripts/qapi/common.py
>>> @@ -779,13 +779,6 @@ def check_union(expr, info):
>>>                                      "enum '%s'"
>>>                                      % (key, enum_define['enum']))
>>>   -    # If discriminator is user-defined, ensure all values are
>>> covered
>>> -    if enum_define:
>>> -        for value in enum_define['data']:
>>> -            if value not in members.keys():
>>> -                raise QAPISemError(info, "Union '%s' data missing '%s' 
>>> branch"
>>> -                                   % (name, value))
>>> -
>>>     def check_alternate(expr, info):
>>>       name = expr['alternate']
>>> @@ -1644,6 +1637,10 @@ class QAPISchema(object):
>>>           if tag_name:
>>>               variants = [self._make_variant(key, value)
>>>                           for (key, value) in data.items()]
>>> +            # branches that are not explicitly covered get an empty type
>>> +            variants += [self._make_variant(key, 'q_empty')
>>> +                         for key in 
>>> discriminator_find_enum_define(expr)['data']
>>
>> Long line, flagged by pycodestyle --diff.
>>
>
> Done.
>
>>> +                         if key not in data.keys()]
>>>               members = []
>>>           else:
>>>               variants = [self._make_simple_variant(key, value, info)
>>
>> The way you desugar the union is simple enough, but it uses
>> discriminator_find_enum_define() outside check_exprs(), which I'd rather
>> avoid.  Not something you could be reasonably expected to know.
>>
>> We could desugar in QAPISchemaObjectTypeVariants.check(), where we have
>> self.tag_member.type.values.
>>
>>     @@ -1346,10 +1346,17 @@ class QAPISchemaObjectTypeVariants(object):
>>                  v.set_owner(name)
>>
>>          def check(self, schema, seen):
>>     -        if not self.tag_member:    # flat union
>>     +        if not self.tag_member: # flat union
>>                  self.tag_member = seen[c_name(self._tag_name)]
>>                  assert self._tag_name == self.tag_member.name
>>              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>>     +        if self._tag_name:      # flat union
>>     +            cases = set([v.name for v in self.variants])
>>     +            for val in self.tag_member.type.values:
>>     +                if val.name not in cases:
>>     +                    v = QAPISchemaObjectTypeVariant(val.name, 'q_empty')
>>     +                    v.set_owner(self.tag_member.owner)
>>     +                    self.variants.append(v)
>>              for v in self.variants:
>>                  v.check(schema)
>>                  # Union names must match enum values; alternate names are
>>
>> Not so simple anymore.
>>
>> Simpler: desugar in check_union(), i.e. replace your two hunks by just
>>
>>     @@ -783,8 +783,7 @@ def check_union(expr, info):
>>          if enum_define:
>>              for value in enum_define['data']:
>>                  if value not in members.keys():
>>     -                raise QAPISemError(info, "Union '%s' data missing '%s' 
>> branch"
>>     -                                   % (name, value))
>>     +                members[value] = 'q_empty'
>>
>>
>>      def check_alternate(expr, info):
>>
>> What do you think?
>>
>
> it didn't feel right aesthetically to normalize by modifying the parsed
> expressions especially at check_exprs() stage. But maybe using
> discriminator_find_enum_define() is more sloppy, I can't quite see

The issue isn't sloppiness, it's adding to our technical debt.  To
explain this, I need to relate a bit of history.

In the beginning, we did everything with syntax trees.  Commit
ac88219a6c7 (Sep 2015) spliced in an intermediate representation.  Its
commit message explains:

    The QAPI code generators work with a syntax tree (nested dictionaries)
    plus a few symbol tables (also dictionaries) on the side.

    They have clearly outgrown these simple data structures.  There's lots
    of rummaging around in dictionaries, and information is recomputed on
    the fly.  For the work I'm going to do, I want more clearly defined
    and more convenient interfaces.
    
    Going forward, I also want less coupling between the back-ends and the
    syntax tree, to make messing with the syntax easier.

We accepted some technical debt back then.  Same commit message:

    Shortcut: the semantic analysis still relies on existing check_exprs()
    to do the actual semantic checking.  All this code needs to move into
    the classes.  Mark as TODO.

It's this one:

    #
    # Semantic analysis of schema expressions
    # TODO fold into QAPISchema
    # TODO catching name collisions in generated code would be nice
    #

check_exprs() is the only entry point into the code under this TODO.
Your patch adds another one.  I don't like that.

Nobody expects you to deduce this from the code by yourself :)

Now, I agree hacking up the syntax tree isn't particularly clean,
either.  Perhaps doing it in QAPISchemaObjectTypeVariants.check() is
best after all.  In case you feel unsure about making that change, I can
take your solution, and address its technical debt issue in a follow-up
patch.

[...]

Reply via email to