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

[Kevin Wolf <kwolf@redhat.com>]

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".

> "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?

> 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.

> > +
> > +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?

> 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 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...

Kevin