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

[John Snow]

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.