Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel

From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Cleber Rosa" <crosa@redhat.com>,
	qemu-devel@nongnu.org, "Eduardo Habkost" <ehabkost@redhat.com>
Subject: Re: [PATCH 06/12] qapi/source: Add builtin null-object sentinel
Date: Thu, 17 Dec 2020 13:33:07 +0100	[thread overview]
Message-ID: <87a6uck3y4.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <d8f468e9-a17d-6ff7-4cf1-369085d8edd3@redhat.com> (John Snow's message of "Wed, 16 Dec 2020 12:53:29 -0500")

John Snow <jsnow@redhat.com> writes:

> On 12/16/20 4:22 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> We use None to represent an object that has no source information
>>> because it's a builtin. This complicates interface typing, since many
>>> interfaces expect that there is an info object available to print errors
>>> with.
>>>
>>> Introduce a special QAPISourceInfo that represents these built-ins so
>>> that if an error should so happen to occur relating to one of these
>>> builtins that we will be able to print its information, and interface
>>> typing becomes simpler: you will always have a source info object.
>> 
>> Two aspects mixed up:
>> 
>> 1. Represent "no source info" as special QAPISourceInfo instead of
>>     None
>> 
>>     This is what de-complicates interface typing.
>> 
>
> Yup.
>
>> 2. On error with "no source info", don't crash.
>> 
>>     I have my doubts on this one.
>> 
>>     Such an error means the QAPI code generator screwed up, at least in
>>     theory.  Crashing is only proper.  It gets the screwup fixed.
>> 
>>     In practice, errors due to interactions between built-in stuff and
>>     user-defined stuff could conceivably escape testing.  I can't
>>     remember such a case offhand.
>> 
>>     Will the "no source info" error be more useful than a crash?
>>     Possibly.  Will it get the screwup fixed?  Maybe not.
>> 
>> Can we separate the two aspects?
>> 
>
> We can add an intentional assertion, if you'd like, that makes such 
> cases obvious -- but if we are already in the error printer, QAPI is 
> likely already in the process of crashing and printing an error.
>
> So, Is this really an issue?

A patch limited to the first aspect merely tweaks an implementation
detail.

As soon as we include the second aspect, we get to debate how to handle
programming errors, and maybe whether any of the errors involving a
QAPISourceInfo.builtin() are *not* programming errors.

I'd prefer this series to remain focused on "enabling strict optional
checking in mypy for everything we have typed so far".

>>>
>>> This object will evaluate as False, so "if info" is a valid idiomatic
>>> construct.
>> 
>> Suggest s/is a valid/remains a valid/.
>> 
>> Not 100% sure we'll want to keep this idiom, but now is not the time to
>> worry about that.
>> 
>
> OK.
>
>>>
>>> NB: It was intentional to not allow empty constructors or similar to
>>> create "empty" source info objects; callers must explicitly invoke
>>> 'builtin()' to pro-actively opt into using the sentinel. This should
>>> prevent use-by-accident.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/source.py | 20 +++++++++++++++++++-
>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>>> index d7a79a9b8aee..64af7318cb67 100644
>>> --- a/scripts/qapi/source.py
>>> +++ b/scripts/qapi/source.py
>>> @@ -11,7 +11,12 @@
>>>   
>>>   import copy
>>>   import sys
>>> -from typing import List, Optional, TypeVar
>>> +from typing import (
>>> +    List,
>>> +    Optional,
>>> +    Type,
>>> +    TypeVar,
>>> +)
>>>   
>>>   
>>>   class QAPISchemaPragma:
>>> @@ -41,6 +46,17 @@ def __init__(self, fname: str, line: int,
>>>           self.defn_meta: Optional[str] = None
>>>           self.defn_name: Optional[str] = None
>>>   
>>> +    @classmethod
>>> +    def builtin(cls: Type[T]) -> T:
>>> +        """
>>> +        Create a SourceInfo object corresponding to a builtin definition.
>> 
>> Let's spell it built-in for consistency with existing comments.
>> 
>> Could perhaps shorten "a SourceInfo object" to "an instance".
>> 
>
> OK.
>
>>> +        """
>>> +        return cls('', -1, None)
>> 
>> No users?  Peeking ahead... aha, they are in Patch 08.  Any particular
>> reason for putting PATCH 07 between the two?  Could PATCH 08 be squashed
>> into this one?
>> 
>
> Too much soup in one pot: this patch highlights the "trick" and the 
> subsequent patch shows the adoption of it. Seemed safe.
>
> Goofy ordering, though. I've pushed the genc/genh patch downwards 
> instead; you can squash them on commit if you'd like.
>
>>> +
>>> +    def __bool__(self) -> bool:
>>> +        # "if info: ..." is false if info is the builtin sentinel.
>>> +        return bool(self.fname)
>> 
>> Nitpicking...  "The builtin sentinel" suggests there is just one.  PATCH
>> 08 creates several.  I don't mind, but let's say something like "if
>> @info corresponds to a built-in definition".
>> 
>
> Fair enough. I don't mind nitpicks on comments and docstrings so much if 
> it helps make things clearer for more people.
>
> (And they don't cause me rebase pain as much as other nitpicks ;)
>
>>> +
>>>       def set_defn(self, meta: str, name: str) -> None:
>>>           self.defn_meta = meta
>>>           self.defn_name = name
>>> @@ -73,4 +89,6 @@ def include_path(self) -> str:
>>>           return ret
>>>   
>>>       def __str__(self) -> str:
>>> +        if not bool(self):
>>> +            return '[builtin]'
>>>           return self.include_path() + self.in_defn() + self.loc()
>> 
>> Looks like we can separate the two aspects easily:
>> 
>>         def __str__(self) -> str:
>>    +        assert not bool(self)
>>             return self.include_path() + self.in_defn() + self.loc()
>> 
>
> Feels like abusing __str__ to prevent application logic we don't like 
> elsewhere and unrelated to this class; I am still leaning on "If we are 
> printing this, it's likely we're already crashing" unless you have news 
> to the contrary for me.

You're right, making __str__() fail is not nice.  It has other uses,
e.g. when messing around interactively.

Ways out:

1. Avoid abuse of __str__() by naming the thing differently.

2. Lift the assertion into the relevant caller(s).  Unfortunately, the
   relevant caller is another __str__(): QAPIError's.  Next level up:
   the except suite in main() and also test-qapi.py's test_and_diff().
   Keeping the stack backtrace useful might require care.

3. Find a simpler way to signal "oops, programming error".

   For a simple batch program like this one, crashing is a perfectly
   fine reaction to programming errors.  Most of the time, it's also the
   simplest one.  Simple is *good*.  *Especially* when it's something
   that should never happen.

   If reporting an error is genuinely simpler than crashing: simple is
   good.  But the error message should clearly express "this is a bug,
   please report it", like a crash does.

   Drawbacks: we'd have to relax our rule "no error without a test
   case", and we'd lose the stack backtrace.