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

[Markus Armbruster <armbru@redhat.com>]

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.

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?

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

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

> +        """
> +        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?

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

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