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

[Cleber Rosa]

On Thu, Sep 24, 2020 at 03:36:23PM -0400, John Snow wrote:
> 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>
> > 

I did not attempt to learn the type names by heart (mental sanity
first) but I get the big picture.  Thanks!

- Cleber.

signature.asc
Description: PGP signature