Re: [PATCH v2 4/8] qapi/error: Change assertion

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 3/30/21 1:18 PM, John Snow wrote:
>
> Realizing now that this commit topic is wrong :)
>
> A prior version modified the assertion, I decided it was less churn to 
> simply add one.
>
> I think ideally we'd have no assertions here and we'd rely on the type 
> hints, but I don't think I can prove that this is safe until after part 
> 6, so I did this for now instead.
>
>> Eventually, we'll be able to prove that 'info.line' must be an int and
>> is never None at static analysis time, and this assert can go
>> away. Until then, it's a type error to assume that self.info is not
>> None.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/error.py | 1 +
>>   1 file changed, 1 insertion(+)
>> 
>> diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
>> index d179a3bd0c..d0bc7af6e7 100644
>> --- a/scripts/qapi/error.py
>> +++ b/scripts/qapi/error.py
>> @@ -25,6 +25,7 @@ def __init__(self, info, msg, col=None):
>>           self.col = col
>>   
>>       def __str__(self):
>> +        assert self.info is not None
>>           loc = str(self.info)
>>           if self.col is not None:
>>               assert self.info.line is not None
>> 

Please show us the revised commit message.  I'm asking because the
patch's purpose isn't quite clear to me.  Peeking ahead at PATCH 7, I
see that info becomes Optional[QAPISourceInfo].  This means something
passes None for info (else you wouldn't make it optional).  None info
Works (in the sense of "doesn't crash") as long as col is also None.
After the patch, it doesn't.  I'm not saying that's bad, only that I
don't quite understand what you're trying to accomplish here.