|
From: | John Snow |
Subject: | Re: [PATCH v2 4/8] qapi/error: Change assertion |
Date: | Thu, 15 Apr 2021 11:44:24 -0400 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
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 = coldef __str__(self):+ assert self.info is not None loc = str(self.info) if self.col is not None: assert self.info.line is not NonePlease 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 NoneWe 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
[Prev in Thread] | Current Thread | [Next in Thread] |