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

[John Snow <jsnow@redhat.com>]

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?

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