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

[John Snow <jsnow@redhat.com>]

On 4/16/21 2:17 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 4/15/21 3:00 AM, Markus Armbruster wrote:
>>> 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.
>>>
>>
>> Sure.
>>
>> If you recall, I tried to enforce that QAPISourceInfo was *never* None
>> by creating a special value for QSI that represented "No Source
>> Info". Ultimately, that idea didn't go through and we solidified that
>> the 'info' parameter that gets passed around can sometimes be None.
>>
>> Hence, this property is Optional[QAPISourceInfo].
>>
>> Since I know you wanted to crash messily in the case that we allowed a
>> None-info to leak into a context like this, I added the new assertion
>> to make sure this remains the case.
>>
>> (since str(None) evaluates to "None", it seemed like there was already
>> the implicit assumption that info would never be None. Plus, if 'col'
>> is set, mypy notices that we are performing an unsafe check on
>> self.info.line, which had to be remedied.)
>>
>> Later in the series, after schema.py is typed, it may be possible to
>> remove these assertions as we may be able to rely on the strict typing
>> to prove that this situation can never occur. However, since schema.py
>> is not yet typed, we can't yet.
>>
>>
>>
>> Alright. So given that, I think what you'd like to see for a commit
>> message is:
>>
>> qapi/error: assert QAPISourceInfo is not None
>>
>> We implicitly assume that self.info will never be None, as the error
>> reporting function will not produce meaningful output in this case,
>> and will crash if self.col was set. Assert that self.info is not None
>> in order to formalize this assumption.
>>
>> We can not yet change the type of the initializer to prove that this
>> is provably true at static analysis time until the rest of this
>> library is fully typed.
> 
> Let's insert another paragraph to make the intent even clearer, and
> adjust the remainder for it:
> 
>    qapi/error: assert QAPISourceInfo is not None
> 
>    Built-in stuff is not parsed from a source file, and therefore have no
>    QAPISourceInfo.  If such None info was used for reporting an error,
>    built-in stuff would be broken.  Programming error.  Instead of
>    reporting a confusing error with bogus source location then, we better
>    crash.
> 
>    We currently crash only if self.col was set.  Assert that self.info is
>    not None in order to crash reliably.
> 
>    We can not yet change the type of the initializer to prove this cannot
>    happen at static analysis time before the remainder of the code is
>    fully typed.
> 
> Note I also replaced "this library" because I'm not sure what it means.
> 

Wiggly-brain, wiggly-mouth. I meant "package". I get these terms 
consistently wrong, and I need to really make a very direct effort to 
treat them precisely correctly in the future.

MODULE: A single Python file. It may be a script, or serve only as a 
library meant for consumption by other scripts.

PACKAGE: A collection of one or more Python modules. QAPI is a package 
because it's a folder with an __init__.py file, containing several modules.

LIBRARY: No formal definition; however, I think of it as a Python module 
that is meant primarily to be consumed by another script or entry-point. 
It has some implications for things like Python's hierarchical logging 
library where the distinction is important for how logger setup is 
handled. Modules that use the "if __name__ == '__main__'" stanza serve 
dual purpose as a script *and* library.


In this case, I meant "package". I think I accidentally avoid this term 
because I don't want to imply that it is a "distributed package" in the 
sense of a "PyPI package" and my brain skips a clock cycle and picks 
another random word.

Bad habit. :(

> What do you think?
> 
> 

TLDR: I took your phrasing wholesale. Thanks!

--js

(Random aside on patch submission process: I do dislike how when I 
change the topic of a commit, I lose out on git-backport-diff 
functionality. I wish I had a persistent ID for commits that survived 
beyond title changes. Sometimes I debate scripting adding some kind of 
Patch-GUID: <...> tag, but I don't know if that would look like 
undesirable clutter in the git log to everyone else. Maybe a "WAS: 
old-topic-name-here" in the comment section would suffice well enough?)