Re: [PATCH v6 02/11] qapi: wrap Sequence[str] in an object

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Mon, Aug 2, 2021 at 1:21 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Except for the special casing assert in _make_implicit_object_type(),
>> > which needs to handle schema objects, it's a mechanical change.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
[...]
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index d1d27ff7ee..5e44164bd1 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
[...]
>> > @@ -968,11 +974,13 @@ def _def_predefineds(self):
>> >      def _make_features(self, features, info):
>> >          if features is None:
>> >              return []
>> > -        return [QAPISchemaFeature(f['name'], info, f.get('if'))
>> > +        return [QAPISchemaFeature(f['name'], info,
>> > +                                  QAPISchemaIfCond(f.get('if')))
>> >                  for f in features]
>> >
>> >      def _make_enum_members(self, values, info):
>> > -        return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
>> > +        return [QAPISchemaEnumMember(v['name'], info,
>> > +                                     QAPISchemaIfCond(v.get('if')))
>> >                  for v in values]
>> >
>>
>> Two more instances of pattern #3, only here we wrap values we get from
>> the JSON parser.  These are either None or non-empty lists.
>>
>> >      def _make_implicit_enum_type(self, name, info, ifcond, values):
>> > @@ -1008,7 +1016,10 @@ def _make_implicit_object_type(self, name, info, 
>> > ifcond, role, members):
>>            if typ:
>>                # The implicit object type has multiple users.  This can
>>                # happen only for simple unions' implicit wrapper types.
>>                # Its ifcond should be the disjunction of its user's
>>                # ifconds.  Not implemented.  Instead, we always pass the
>>                # wrapped type's ifcond, which is trivially the same for all
>>                # users.  It's also necessary for the wrapper to compile.
>>                # But it's not tight: the disjunction need not imply it.  We
>>                # may end up compiling useless wrapper types.
>> >              # TODO kill simple unions or implement the disjunction
>> >
>> >              # pylint: disable=protected-access
>> > -            assert (ifcond or []) == typ._ifcond
>> > +            if isinstance(ifcond, QAPISchemaIfCond):
>> > +                assert ifcond.ifcond == typ._ifcond.ifcond
>> > +            else:
>> > +                assert ifcond == typ._ifcond
>> >          else:
>> >              self._def_entity(QAPISchemaObjectType(
>> >                  name, info, None, ifcond, None, None, members, None))
>>
>> This is the non-mechanical change mentioned in the commit message.
>>
>> Can you explain where the two cases come from?
>>
>>
> _make_simple_variant() calls _make_implicit_object_type() with
> self.lookup_type(typ).
>
> I think it could instead call with the ._ifcond value.
>
> To be done after?

We can't.

._make_implicit_object_type() is called by ._make_simple_variant(),
._def_union_type(), ._def_command(), and ._def_event().

The latter three all pass QAPISchemaIfCond(expr.get('if')).
Straightforward.

._make_simple_variant() passes self.lookup_type(typ), i.e. a
QAPISchemaType.  The condition is to be inherited from the type.

We can't pass ._ifcond, because it becomes valid only at .check().
Commit f9d1743b9b0 explains:

    * A QAPISchemaEntity's .ifcond becomes valid at .check().  This is due
      to arrays and simple unions.
    
      Most types get ifcond and info passed to their constructor.
    
      Array types don't: they get it from their element type, which
      becomes known only in .element_type.check().
    
      The implicit wrapper object types for simple union branches don't:
      they get it from the wrapped type, which might be an array.
    
      Ditch the idea to set .ifcond in .check().  Instead, turn it into a
      property and compute it on demand.  Safe because it's only used
      after the schema has been checked.
    
      Most types simply return the ifcond passed to their constructor.
    
      Array types forward to their .element_type instead, and the wrapper
      types forward to the wrapped type.

We really need to kill off "simple" unions.

I was tempted to accept your patch as is, but then my spider sense made
me dig deeper.

When an implicit object type has multiple users, we call
._make_implicit_object_type() multiple times.  The first call creates
the type, subsequent calls reuse it.  The assertion ensures we pass the
same condition to subsequent calls, so reuse is actually valid.

Assertion before the patch:

        typ = self.lookup_entity(name, QAPISchemaObjectType)

@typ is the QAPISchemaType created by the first call, if any.

        if typ:

This is a subsequent call, and @ifcond is its argument.

            [...]
            assert (ifcond or []) == typ._ifcond

typ._ifcond comes from the first call's @ifcond argument, which created
@typ and intialized typ._ifcond in QAPISchemaEntity.__init__():

        self._ifcond = ifcond or []

The assertion asserts "same condition argument", as intended.  It
carefully avoids use of potentially invalid .ifcond.  Good.

For simple union branch wrapper types, @ifcond is the QAPISchemaType
we're wrapping.  The assertion boils down to ifcond == typ._ifcond,
i.e. we assert we pass the same type.  Still good.

The comment before the assertion claims this is the only way to get
here:

            # The implicit object type has multiple users.  This can
            # happen only for simple unions' implicit wrapper types.

Not true, as tests/qapi-schema/redefined-command.json demonstrates: we
get here for unboxed redefined commands, too.

For implicit object types other than simple union branch wrapper types,
i.e. for anonymous union base types, command and event argument types,
@ifcond is either None or a non-empty array of condition strings.  Now
the assertion asserts we pass the same Optional[List[str]].  Quack!
This is *wrong*.  To demonstrate, feed it redefined-command.json
modified like this:

    # we reject commands defined more than once
    { 'command': 'foo', 'data': { 'one': 'str' } }
    { 'command': 'foo', 'data': { '*two': 'str' }, 'if': 'defined(FOO)' }

It trips the assertion.

Your patch wraps QAPISchemaIfCond() around non-empty arrays, and
replaces None by QAPISchemaIfCond([]).

Doesn't affect @ifcond arguments for simple union branch wrapper types.
The assertion continues to work as intended.

Does affect @ifcond arguments for the other implicit object types.
There, we now need to compare the wrapped value instead.  Thus, the
quackery doesn't work anymore, and we need something like

            if simple union wrapper type:
                assert (ifcond or []) == typ._ifcond # old assertion
            else:
                assert ifcond.ifcond == typ._ifcond.ifcond

Your patch uses an indirect way to determine the condition, negates it,
and simplifies the old assertion a bit:

            if isinstance(ifcond, QAPISchemaIfCond):
                assert ifcond.ifcond == typ._ifcond.ifcond
            else:
                assert ifcond == typ._ifcond

All fine, except the assertion remains just as wrong as it was :)

I'll send a patch to delete it, to be inserted before your series.

[...]