Kevin Wolf <kw...@redhat.com> writes:
> Am 10.02.2021 um 10:17 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kw...@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 <kw...@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.
