diff mbox series

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

Message ID 20200922210101.4081073-37-jsnow@redhat.com
State New
Headers show
Series qapi: static typing conversion, pt1 | expand

Commit Message

John Snow Sept. 22, 2020, 9 p.m. UTC
Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/visit.py | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Comments

Eduardo Habkost Sept. 23, 2020, 7:15 p.m. UTC | #1
On Tue, Sep 22, 2020 at 05:00:59PM -0400, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>


This for making mypy happy, correct?  An explanation in the commit
message would be nice.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


-- 
Eduardo
John Snow Sept. 23, 2020, 10:13 p.m. UTC | #2
On 9/23/20 3:15 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:00:59PM -0400, John Snow wrote:

>> Signed-off-by: John Snow <jsnow@redhat.com>

> 

> This for making mypy happy, correct?  An explanation in the commit

> message would be nice.

> 

> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> 


Yes, it's for mypy -- but it's a runtime visible change. Technically our 
type system isn't mature enough to express this constraint natively, so 
it's being carried around as developer knowledge.

This formalizes that knowledge, albeit in a very crude way.

I've amended the commit msg.
Cleber Rosa Sept. 24, 2020, 7:10 p.m. UTC | #3
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,

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Cleber Rosa Sept. 24, 2020, 7:12 p.m. UTC | #4
On Wed, Sep 23, 2020 at 06:13:30PM -0400, John Snow wrote:
> On 9/23/20 3:15 PM, Eduardo Habkost wrote:

> > On Tue, Sep 22, 2020 at 05:00:59PM -0400, John Snow wrote:

> > > Signed-off-by: John Snow <jsnow@redhat.com>

> > 

> > This for making mypy happy, correct?  An explanation in the commit

> > message would be nice.

> > 

> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

> > 

> 

> Yes, it's for mypy -- but it's a runtime visible change. Technically our

> type system isn't mature enough to express this constraint natively, so it's

> being carried around as developer knowledge.

> 

> This formalizes that knowledge, albeit in a very crude way.

> 

> I've amended the commit msg.


OK, this answers my previous question about why it was handled as an
assert.

- Cleber.
John Snow Sept. 24, 2020, 7:36 p.m. UTC | #5
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>

>
Cleber Rosa Sept. 24, 2020, 11:52 p.m. UTC | #6
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.
diff mbox series

Patch

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)
+
         ret += mcgen('''
     switch (obj->%(c_name)s) {
 ''',
-                     c_name=c_name(variants.tag_member.name))
+                     c_name=c_name(tag_member.name))
 
         for var in variants.variants:
-            case_str = c_enum_const(variants.tag_member.type.name,
-                                    var.name,
-                                    variants.tag_member.type.prefix)
+            case_str = c_enum_const(tag_member.type.name, var.name,
+                                    tag_member.type.prefix)
             ret += gen_if(var.ifcond)
             if var.type.name == 'q_empty':
                 # valid variant and nothing to do