| Message ID | 20200922210101.4081073-37-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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
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.
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>
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.
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> >
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 --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
Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/visit.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)