Re: [PATCH 5/6] qapi: Add support for aliases

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.02.2021 um 10:17 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Introduce alias definitions for object types (structs and unions). This
>> > allows using the same QAPI type and visitor for many syntax variations
>> > that exist in the external representation, like between QMP and the
>> > command line. It also provides a new tool for evolving the schema while
>> > maintaining backwards compatibility during a deprecation period.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  docs/devel/qapi-code-gen.txt           | 37 +++++++++++++++++++++++---
>> >  docs/sphinx/qapidoc.py                 |  2 +-
>> >  scripts/qapi/expr.py                   | 34 +++++++++++++++++++++--
>> >  scripts/qapi/schema.py                 | 27 +++++++++++++++----
>> >  scripts/qapi/types.py                  |  4 ++-
>> >  scripts/qapi/visit.py                  | 33 ++++++++++++++++++++---
>> >  tests/qapi-schema/test-qapi.py         |  7 ++++-
>> >  tests/qapi-schema/double-type.err      |  2 +-
>> >  tests/qapi-schema/unknown-expr-key.err |  2 +-
>> >  9 files changed, 130 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> > index 6906a06ad2..6da14d5275 100644
>> > --- a/docs/devel/qapi-code-gen.txt
>> > +++ b/docs/devel/qapi-code-gen.txt
>> > @@ -231,7 +231,8 @@ Syntax:
>> >                 'data': MEMBERS,
>> >                 '*base': STRING,
>> >                 '*if': COND,
>> > -               '*features': FEATURES }
>> > +               '*features': FEATURES,
>> > +               '*aliases': ALIASES }
>> >      MEMBERS = { MEMBER, ... }
>> >      MEMBER = STRING : TYPE-REF
>> >             | STRING : { 'type': TYPE-REF,
>> 
>> Missing: a forward reference, like we have for 'if' and 'features'.
>> Here's the obvious one:
>> 
>>    The optional 'if' member specifies a conditional.  See "Configuring
>>    the schema" below for more on this.
>> 
>>    The optional 'features' member specifies features.  See "Features"
>>    below for more on this.
>> 
>>   +The optional 'aliases' member specifies aliases.  See "Aliases" below
>>   +for more on this.
>> 
>> > @@ -286,13 +287,15 @@ Syntax:
>> >      UNION = { 'union': STRING,
>> >                'data': BRANCHES,
>> >                '*if': COND,
>> > -              '*features': FEATURES }
>> > +              '*features': FEATURES,
>> > +              '*aliases': ALIASES }
>> >            | { 'union': STRING,
>> >                'data': BRANCHES,
>> >                'base': ( MEMBERS | STRING ),
>> >                'discriminator': STRING,
>> >                '*if': COND,
>> > -              '*features': FEATURES }
>> > +              '*features': FEATURES,
>> > +              '*aliases': ALIASES }
>> >      BRANCHES = { BRANCH, ... }
>> >      BRANCH = STRING : TYPE-REF
>> >             | STRING : { 'type': TYPE-REF, '*if': COND }
>> 
>> Likewise.
>> 
>> > @@ -837,6 +840,34 @@ shows a conditional entity only when the condition is 
>> > satisfied in
>> >  this particular build.
>> >  
>> >  
>> > +=== Aliases ===
>> > +
>> > +Object types, including structs and unions, can contain alias
>> > +definitions.
>> > +
>> > +Aliases define alternative member names that may be used in the
>> > +external representation to provide a value for a member in the same
>> > +object or in a nested object.
>> 
>> "or one if its sub-objects"?  Not sure which is better.
>
> "nested object" feels a little clearer to me, but not that it's a big
> difference. If you feel "sub-object" is better, I can use that.
>
>> > +
>> > +Syntax:
>> > +    ALIAS = { '*alias': STRING,
>> > +              'source': [ STRING, ... ] }
>> 
>> You used non-terminal ALIASES above.  Please define it here.
>> 
>> I have doubts about the name 'alias'.  The alias is the complete thing,
>> and 'alias' is just one property of the complete thing.  I think 'name'
>> would be better.  Further evidence: below, you write "If 'alias' is
>> present" and "If 'alias' is not present".  I think both read better with
>> 'name' instead of 'alias'.
>
> Works for me.
>
>> > +
>> > +'source' is a list of member names representing the path to an object
>> > +member, starting from the type where the alias definition is
>> > +specified.
>> 
>> May 'source' be empty?  More on that below.
>
> No. Empty 'source' isn't the path to any object member, so it doesn't
> meet the requirement. If you prefer, we can explicitly specify a
> "non-empty list".

I think it's best to be tediously explicit here.

>> "where the definition is specified" feels a bit awkward, and "path"
>> slightly hand-wavy.  Let me try induction:
>> 
>>    'source' is a list of member names.  The first name is resolved in
>>    the same object.  Each subsequent member is resolved in the object
>>    named by the preceding member.
>> 
>> Differently awkward, I guess.
>
> Now you've left out what the purpose of it is. I think I'll combine your
> version with my first part ("'source' is a list of member names
> representing the path to an object member").
>
>> >              It may refer to another alias name.  It is allowed to use
>> > +a path that doesn't necessarily match an existing member in every
>> > +variant or even at all; in this case, the alias remains unused.
>> 
>> Aha!  Knowing this would've saved me some trouble in reviewing code.
>> 
>> I wrote on PATCH 1:
>> 
>>     I think updating the big comment in visitor.h for aliases would help.
>>     Let's postpone it until I've seen the rest of the series.
>> 
>> We can cover unused aliases right there.  Whether they also need to go
>> into contracts we'll see.
>
> Ok. I assume updating that big comment is still postponed because you
> saw the series, but didn't actually review all of it yet?

Writing documentation before I understand the code is probably not a
good use of my time, and my reviewer's time, too.  Getting there.

If you want to try, go right ahead.

>> What if only a *prefix* of 'source' matches?  E.g.
>> 
>>     'source': ['eins', 'zwei', 'drei']
>> 
>> and we have an object-valued member 'eins' (match), which has a member
>> 'zwei' (match), which is *not* an object.  Is that an error?  Is it
>> caught?
>
> This feels like a realistic case to me when 'eins' is a union type where
> some variants contain an object 'zwei' with a member 'drei' and others
> have 'zwei' as a non-object member.
>
> In this case, we want the alias not to match in the non-object 'zwei'
> case, but we do want it to match in another variant. So it is
> intentionally not an error.
>
> The QAPI generator could try to prove that there is at least one variant
> where the alias would actually be applied, but just leaving it unused
> when it doesn't match anywhere seemed good enough to me.

I see.

A typo can get your alias silently ignored.  A bit of a trap.  Testing
should catch this, of course.

Consider adding a comment in the QAPI generator along the lines "could
check this, but not sure it's worth our while", and a short note in
qapi-code-gen.txt to warn users about the trap.

>> > +
>> > +If 'alias' is present, then the single member referred to by 'source'
>> > +is made accessible with the name given in 'alias' in the type where
>> > +the alias definition is specified.
>> 
>> 'source' may not be empty here.  Correct?
>> 
>> If yes, please spell it out.
>
> Yes. Does spelling it out more explicitly in the description of 'source'
> suffice?

I think so, yes.

>> Double-checking I got it...  Say we have
>> 
>>     'alias': 'foo',
>>     'source': ['bar', 'baz']
>> 
>> where 'bar' is an object with a member 'baz'.
>> 
>> Then input "foo": FOOVAL gets interpreted like "bar": {"baz": FOOVAL}.
>> 
>> If input also contains "bar", we merge.  Duplicates are an error.
>> 
>> This is the case even when 'baz' is an object.  If you want to alias
>> member 'foo' of 'baz', you have to say
>> 
>>     'alias': 'foo',
>>     'source': ['bar', 'baz', 'foo']
>> 
>> Correct?
>
> Correct.
>
>> > +
>> > +If 'alias' is not present, then all members in the object referred to
>> > +by 'source' are made accessible in the type where the alias definition
>> > +is specified with the same name as they have in 'source'.
>> 
>> 'source' may not be empty here, either.  Correct?
>> 
>> If yes, please spell it out, and make sure the code catches it.
>
> Yes, as above. It's checked in check_aliases():
>
>         if not a['source']:
>             raise QAPISemError(info, "'source' must not be empty")
>
>> What if it resolve to a non-object?  Is that an error?  Is it caught?
>
> Same as above, it just doesn't match.
>
>> Continuing the double-checking...  Say we have
>> 
>>     # alias missing
>>     'source': ['gnu']
>> 
>> where 'gnu' is an object with a member 'gnat'.
>> 
>> Input "gnat": GNATVAL gets interpreted like "gnu": {"gnat": GNATVAL}.
>> 
>> Correct?
>
> Yes.
>
>> The document could use examples.  Feel free to steal mine.
>> 
>> I think we should talk about 'alias' first, and only then about
>> 'source'.  It matches their order in the schema, and also matches how I
>> think about aliases, namely "this name actually means that".  Here,
>> "this name" is 'alias', and "that" is 'source'.
>> 
>> > +
>> > +
>> 
>> Don't get deceived by my comments; this is a pretty good start.
>> 
>> I wish I had studied this part before PATCH 1.
>> 
>> >  === Documentation comments ===
>> >  
>> >  A multi-line comment that starts and ends with a '##' line is a
>> 
>> I intend to look at the remainder shortly.
>
> Ok. I'll prepare for a context switch to actually be able to address
> your comments on the other patches and to figure out what I had already
> addressed in my branch during your last review attempt.

I intend to look at the remainder of PATCH 5 this afternoon.

> I thought I had done a better than average job on documenting the code
> (at least compare to my other patches), but doesn't seem so...

Writing excellent documentation for code you just wrote is *hard*!  I
think yours was pretty good, actually.