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

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
> 

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.

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.


--js

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.

What do you think?

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?)

John Snow <jsnow@redhat.com> writes:

[...]

> (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?)

Have you tried git-range-diff?