Re: [PATCH v2 36/38] qapi/visit.py: assert tag_member contains a QAPISchemaEnumType

[John Snow]

On 9/24/20 3:10 PM, Cleber Rosa wrote:
> On Tue, Sep 22, 2020 at 05:00:59PM -0400, John Snow wrote:
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/visit.py | 15 ++++++++++-----
> >   1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 4edaee33e3..180c140180 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -22,7 +22,10 @@
> >       indent,
> >   )
> >   from .gen import QAPISchemaModularCVisitor, ifcontext
> > -from .schema import QAPISchemaObjectType
> > +from .schema import (
> > +    QAPISchemaEnumType,
> > +    QAPISchemaObjectType,
> > +)
> >   
> >   
> >   def gen_visit_decl(name, scalar=False):
> > @@ -84,15 +87,17 @@ def gen_visit_object_members(name, base, members, variants):
> >           ret += gen_endif(memb.ifcond)
> >   
> >       if variants:
> > +        tag_member = variants.tag_member
> > +        assert isinstance(tag_member.type, QAPISchemaEnumType)
> > +
>
> I'd be interested in knowing why this wasn't left to be handled by the
> type checking only.  Anyway,

QAPISchemaVariants is a container type that is used to house a number of 
QAPISchemaVariant types; but it (can) also take a tag_member to identify 
one of the fields in the variants that can be used to differentiate them.

Now, we assert that tag_member must be a QAPISchemaObjectTypeMember. 
QAPISchemaVariant is also a QAPISchemaObjectTypeMember.

a QAPISchemaObjectTypeMember is a QAPISchemaMember. a QAPISchemaMember 
describes one 'member' of either an enum, a features list, or an object 
member.

Now, the QAPISchemaObjectTypeMember (and not the QAPISchemaMember!) 
serves as a container for a QAPISchemaType -- this is a wrapper type, 
effectively. That contained type can be *anything*, because object 
members can be *anything*.

Oops, but if we zoom back out, we are only able to constrain tag_member 
to being a QAPISchemaObjectTypeMember, we have no expressive power over 
its contained type.

That's why you need the assertion here; because of a loss of specificity 
when we declare tag_member.


"Wow, John, it sounds like you should use a Generic type to be able to 
describe the inner type of a QAPISchemaObjectTypeMember?"

Uh, yup, you're right! I should. But it's complicated, because 
QAPISchemaMember does not have a contained type. Further, the contained 
type of a Member may or may not be known at construction time right now.

It's fixable, and probably involves adding something like a "string 
constant" dummy type to allow QAPISchemaMember to have a contained type.

"Hey, all of that sounds very messy. What if we just added in a few 
assertions for right now while we get the preliminary types in, and then 
we can make adjustments based on what makes the code prettier?"

Sounds like a plan, hypothetical quote person.

> Reviewed-by: Cleber Rosa <crosa@redhat.com>