[v5,34/36] qapi/visit.py: assert tag_member contains a QAPISchemaEnumType

Commit Message

John Snow Oct. 5, 2020, 7:51 p.m. UTC

This is true by design, but not presently able to be expressed in the
type system. An assertion helps mypy understand our constraints.

Signed-off-by: John Snow <[e-mail]>
Reviewed-by: Eduardo Habkost <[e-mail]>
Reviewed-by: Cleber Rosa <[e-mail]>
---
 scripts/qapi/visit.py | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Markus Armbruster Oct. 7, 2020, 12:43 p.m. UTC | #1

John Snow <[e-mail]> writes:

> This is true by design, but not presently able to be expressed in the
> type system. An assertion helps mypy understand our constraints.
>
> Signed-off-by: John Snow <[e-mail]>
> Reviewed-by: Eduardo Habkost <[e-mail]>
> Reviewed-by: Cleber Rosa <[e-mail]>
> ---
>  scripts/qapi/visit.py | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 14f30c228b7..4f11fd325b8 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -22,7 +22,7 @@
>      mcgen,
>  )
>  from .gen import QAPISchemaModularCVisitor, ifcontext
> -from .schema import QAPISchemaObjectType
> +from .schema import QAPISchemaEnumType, QAPISchemaObjectType
>  
>  
>  def gen_visit_decl(name, scalar=False):
> @@ -84,15 +84,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'm curious: do you need the local variable to make the assertion stick?

> +
>          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

John Snow Oct. 7, 2020, 4:40 p.m. UTC | #2

On 10/7/20 8:43 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> This is true by design, but not presently able to be expressed in the
>> type system. An assertion helps mypy understand our constraints.
>>
>> Signed-off-by: John Snow <[e-mail]>
>> Reviewed-by: Eduardo Habkost <[e-mail]>
>> Reviewed-by: Cleber Rosa <[e-mail]>
>> ---
>>   scripts/qapi/visit.py | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>> index 14f30c228b7..4f11fd325b8 100644
>> --- a/scripts/qapi/visit.py
>> +++ b/scripts/qapi/visit.py
>> @@ -22,7 +22,7 @@
>>       mcgen,
>>   )
>>   from .gen import QAPISchemaModularCVisitor, ifcontext
>> -from .schema import QAPISchemaObjectType
>> +from .schema import QAPISchemaEnumType, QAPISchemaObjectType
>>   
>>   
>>   def gen_visit_decl(name, scalar=False):
>> @@ -84,15 +84,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'm curious: do you need the local variable to make the assertion stick?
> 

No, but it only sticks to the binding and not the data. i.e. assertions 
to downcast work on the *name*.

(This comes up somewhere in the schema.py patches where I make a change 
that looks completely pointless, but it makes mypy happy.)

I could have left it alone. I just saw a lot of repeated multi-dots and 
habitually created a temporary local for the purpose.

>> +
>>           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

Markus Armbruster Oct. 8, 2020, 9:06 a.m. UTC | #3

John Snow <[e-mail]> writes:

> On 10/7/20 8:43 AM, Markus Armbruster wrote:
>> John Snow <[e-mail]> writes:
>> 
>>> This is true by design, but not presently able to be expressed in the
>>> type system. An assertion helps mypy understand our constraints.
>>>
>>> Signed-off-by: John Snow <[e-mail]>
>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>> ---
>>>   scripts/qapi/visit.py | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>>> index 14f30c228b7..4f11fd325b8 100644
>>> --- a/scripts/qapi/visit.py
>>> +++ b/scripts/qapi/visit.py
>>> @@ -22,7 +22,7 @@
>>>       mcgen,
>>>   )
>>>   from .gen import QAPISchemaModularCVisitor, ifcontext
>>> -from .schema import QAPISchemaObjectType
>>> +from .schema import QAPISchemaEnumType, QAPISchemaObjectType
>>>     
>>>   def gen_visit_decl(name, scalar=False):
>>> @@ -84,15 +84,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'm curious: do you need the local variable to make the assertion
>> stick?
>> 
>
> No, but it only sticks to the binding and not the
> data. i.e. assertions to downcast work on the *name*.

Would it stick to variants.tag_member, too?

I'm asking to learn, not to find a reason to make you change your patch.

> (This comes up somewhere in the schema.py patches where I make a
> change that looks completely pointless, but it makes mypy happy.)
>
> I could have left it alone. I just saw a lot of repeated multi-dots
> and habitually created a temporary local for the purpose.

Matter of taste.  Long chains of dots make the code hard to read because
they are so long.  Temporary variable make it hard to read because you
have to remember what they mean.  Tradeoff.  I come up with cases I find
hard to decide all too often.

In case the local variable isn't needed for mypy: when you throw in
something that isn't needed for the patch's stated purpose, it's best to
mention it in the commit message, because not mentioning it is a review
comment magnet :)

Put yourself in the reviewers shoes.  Your lovingly crafted commit
message puts him into a positive mood.  He nods along while reading your
obvious patch at a good pace.  And then he runs smack into the
unexpected unrelated part, and stops: oh, what's going on here?  Back up
some and read more slowly to make sure I understand.

>>> +
>>>           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

John Snow Oct. 8, 2020, 3:49 p.m. UTC | #4

On 10/8/20 5:06 AM, Markus Armbruster wrote:
> John Snow <[e-mail]> writes:
> 
>> On 10/7/20 8:43 AM, Markus Armbruster wrote:
>>> John Snow <[e-mail]> writes:
>>>
>>>> This is true by design, but not presently able to be expressed in the
>>>> type system. An assertion helps mypy understand our constraints.
>>>>
>>>> Signed-off-by: John Snow <[e-mail]>
>>>> Reviewed-by: Eduardo Habkost <[e-mail]>
>>>> Reviewed-by: Cleber Rosa <[e-mail]>
>>>> ---
>>>>    scripts/qapi/visit.py | 12 +++++++-----
>>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
>>>> index 14f30c228b7..4f11fd325b8 100644
>>>> --- a/scripts/qapi/visit.py
>>>> +++ b/scripts/qapi/visit.py
>>>> @@ -22,7 +22,7 @@
>>>>        mcgen,
>>>>    )
>>>>    from .gen import QAPISchemaModularCVisitor, ifcontext
>>>> -from .schema import QAPISchemaObjectType
>>>> +from .schema import QAPISchemaEnumType, QAPISchemaObjectType
>>>>      
>>>>    def gen_visit_decl(name, scalar=False):
>>>> @@ -84,15 +84,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'm curious: do you need the local variable to make the assertion
>>> stick?
>>>
>>
>> No, but it only sticks to the binding and not the
>> data. i.e. assertions to downcast work on the *name*.
> 
> Would it stick to variants.tag_member, too?
> 

As long as variants isn't re-bound in that scope, it should stick within 
that scope, yeah. (I think. Didn't test. It's my expectation.)

> I'm asking to learn, not to find a reason to make you change your patch.
> 
>> (This comes up somewhere in the schema.py patches where I make a
>> change that looks completely pointless, but it makes mypy happy.)
>>
>> I could have left it alone. I just saw a lot of repeated multi-dots
>> and habitually created a temporary local for the purpose.
> 
> Matter of taste.  Long chains of dots make the code hard to read because
> they are so long.  Temporary variable make it hard to read because you
> have to remember what they mean.  Tradeoff.  I come up with cases I find
> hard to decide all too often.
> 
> In case the local variable isn't needed for mypy: when you throw in
> something that isn't needed for the patch's stated purpose, it's best to
> mention it in the commit message, because not mentioning it is a review
> comment magnet :)
> 
> Put yourself in the reviewers shoes.  Your lovingly crafted commit
> message puts him into a positive mood.  He nods along while reading your
> obvious patch at a good pace.  And then he runs smack into the
> unexpected unrelated part, and stops: oh, what's going on here?  Back up
> some and read more slowly to make sure I understand.
> 

Yeah, understood. Well, part of it is knowing the review style of your 
reviewer too. It's been a while since we've personally swapped patches.

The actual process for much of this was: "There is an error! let me fix 
that error."

And then I do that, but I do so using idioms and patterns I'm familiar 
or comfortable with. And then post-hoc, not 100% of them were 
necessarily required, strictly, to fix the problem. Most of this series 
was written "one file at a time", and then split out post-fact into 
little per-warning changes.

I didn't even send part 1 until I finished typing the *entire* package, 
to make sure that things were as correct as possible from the very first 
commit.

I often don't really even notice that little changes aren't strictly 
necessary until they're challenged, it's usually not a conscious choice 
to try and sneak stuff in.

Still always trying to find a balance between "Easy to maintain and 
iterate" and "easy to review." Tough line for me.

>>>> +
>>>>            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

Markus Armbruster Oct. 9, 2020, 7:24 a.m. UTC | #5

John Snow <[e-mail]> writes:

[...]
> Still always trying to find a balance between "Easy to maintain and
> iterate" and "easy to review." Tough line for me.

I assure you it's a tough line for everyone.

[...]

Patch

diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 14f30c228b7..4f11fd325b8 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -22,7 +22,7 @@ 
     mcgen,
 )
 from .gen import QAPISchemaModularCVisitor, ifcontext
-from .schema import QAPISchemaObjectType
+from .schema import QAPISchemaEnumType, QAPISchemaObjectType
 
 
 def gen_visit_decl(name, scalar=False):
@@ -84,15 +84,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