Re: [PATCH 19/19] qapi/schema: refactor entity lookup helpers

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is not a clear win, but I was confused and I couldn't help myself.
>
> Before:
>
> lookup_entity(self, name: str, typ: Optional[type] = None
>               ) -> Optional[QAPISchemaEntity]: ...
>
> lookup_type(self, name: str) -> Optional[QAPISchemaType]: ...
>
> resolve_type(self, name: str, info: Optional[QAPISourceInfo],
>              what: Union[str, Callable[[Optional[QAPISourceInfo]], str]]
>              ) -> QAPISchemaType: ...
>
> After:
>
> get_entity(self, name: str) -> Optional[QAPISchemaEntity]: ...
> get_typed_entity(self, name: str, typ: Type[_EntityType]
>                  ) -> Optional[_EntityType]: ...
> lookup_type(self, name: str) -> QAPISchemaType: ...
> resolve_type(self, name: str, info: Optional[QAPISourceInfo],
>              what: Union[str, Callable[[Optional[QAPISourceInfo]], str]]
>              ) -> QAPISchemaType: ...

.resolve_type()'s type remains the same.

> In essence, any function that can return a None value becomes "get ..."
> to encourage association with the dict.get() function which has the same
> behavior. Any function named "lookup" or "resolve" by contrast is no
> longer allowed to return a None value.

.resolve_type() doesn't before the patch.

> This means that any callers to resolve_type or lookup_type don't have to
> check that the function worked, they can just assume it did.
>
> Callers to resolve_type will be greeted with a QAPISemError if something
> has gone wrong, as they have in the past. Callers to lookup_type will be
> greeted with a KeyError if the entity does not exist, or a TypeError if
> it does, but is the wrong type.

Talking about .resolve_type() so much suggests you're changing it.
You're not.

Here's my own summary of the change, just to make sure I got it:

1. Split .lookup_entity() into .get_entity() and .get_typed_entity().

   schema.lookup_entity(name) and schema.lookup_entity(name, None)
   become schema.get_entity(name).

   schema.lookup_entity(name, typ) where typ is not None becomes
   schema.get_typed_entity().

2. Tighten .get_typed_entity()'s type from Optional[QAPISchemaEntity] to
   Optional[_EntityType], where Entity is argument @typ.

3. Change .lookup_type()'s behavior for "not found" from "return None"
   to "throw KeyError if doesn't exist, throw TypeError if exists, but
   not a type".

Correct?

> get_entity and get_typed_entity remain for any callers who are
> specifically interested in the negative case. These functions have only
> a single caller each.

.get_entity()'s single caller being QAPIDocDirective.run(), and its
other single caller being QAPISchema._make_implicit_object_type() ;-P

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py     |  2 +-
>  scripts/qapi/introspect.py |  8 ++----
>  scripts/qapi/schema.py     | 52 ++++++++++++++++++++++++--------------
>  3 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 8f3b9997a15..96deadbf7fc 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -508,7 +508,7 @@ def run(self):
               vis = QAPISchemaGenRSTVisitor(self)
>              vis.visit_begin(schema)
>              for doc in schema.docs:
>                  if doc.symbol:
> -                    vis.symbol(doc, schema.lookup_entity(doc.symbol))
> +                    vis.symbol(doc, schema.get_entity(doc.symbol))

@vis is a QAPISchemaGenRSTVisitor, and vis.symbol is

    def symbol(self, doc, entity):
        [...]
        self._cur_doc = doc
        entity.visit(self)
        self._cur_doc = None

When you add type hints to qapidoc.py, parameter @entity will be
QAPISchemaEntity.  Type error since .get_entity() returns
Optional[QAPISchemaEntity].  I'm fine with addressing that when adding
types to qapidoc.py.

>                  else:
>                      vis.freeform(doc)
>              return vis.get_document_nodes()
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 42981bce163..67c7d89aae0 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -227,14 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>  
>          # Map the various integer types to plain int
>          if typ.json_type() == 'int':
> -            tmp = self._schema.lookup_type('int')
> -            assert tmp is not None
> -            typ = tmp
> +            typ = self._schema.lookup_type('int')
>          elif (isinstance(typ, QAPISchemaArrayType) and
>                typ.element_type.json_type() == 'int'):
> -            tmp = self._schema.lookup_type('intList')
> -            assert tmp is not None
> -            typ = tmp
> +            typ = self._schema.lookup_type('intList')
>          # Add type to work queue if new
>          if typ not in self._used_types:
>              self._used_types.append(typ)

Readability improvement here, due to tighter typing of .lookup_type():
it now returns QAPISchemaType instead of Optional[QAPISchemaType].

Before, lookup failure results in AssertionError.  Afterwards, it
results in KeyError or TypeError.  Fine.  Is it worth mentioning in the
commit message?  Genuine question!

> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index b5f377e68b8..5813136e78b 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -26,6 +26,8 @@
>      Dict,
>      List,
>      Optional,
> +    Type,
> +    TypeVar,
>      Union,
>      cast,
>  )
> @@ -767,7 +769,6 @@ def check(
>              # Here we do:
>              assert self.tag_member.defined_in
>              base_type = schema.lookup_type(self.tag_member.defined_in)
> -            assert base_type

Same change of errors as above.

>              if not base_type.is_implicit():
>                  base = "base type '%s'" % self.tag_member.defined_in
>              if not isinstance(self.tag_member.type, QAPISchemaEnumType):
> @@ -1111,6 +1112,12 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
>              self.arg_type, self.boxed)
>  
>  
> +# Used for type-dependent type lookup return values.
> +_EntityType = TypeVar(   # pylint: disable=invalid-name
> +    '_EntityType', bound=QAPISchemaEntity
> +)

Oh, the fanciness!

> +
> +
>  class QAPISchema:
>      def __init__(self, fname: str):
>          self.fname = fname
> @@ -1155,22 +1162,28 @@ def _def_entity(self, ent: QAPISchemaEntity) -> None:
>                  ent.info, "%s is already defined" % other_ent.describe())
>          self._entity_dict[ent.name] = ent
>  
> -    def lookup_entity(
> +    def get_entity(self, name: str) -> Optional[QAPISchemaEntity]:
> +        return self._entity_dict.get(name)
> +
> +    def get_typed_entity(
>          self,
>          name: str,
> -        typ: Optional[type] = None,
> -    ) -> Optional[QAPISchemaEntity]:
> -        ent = self._entity_dict.get(name)
> -        if typ and not isinstance(ent, typ):
> -            return None
> +        typ: Type[_EntityType]
> +    ) -> Optional[_EntityType]:
> +        ent = self.get_entity(name)
> +        if ent is not None and not isinstance(ent, typ):
> +            etype = type(ent).__name__
> +            ttype = typ.__name__
> +            raise TypeError(
> +                f"Entity '{name}' is of type '{etype}', not '{ttype}'."
> +            )
>          return ent
>  
> -    def lookup_type(self, name: str) -> Optional[QAPISchemaType]:
> -        typ = self.lookup_entity(name, QAPISchemaType)
> -        if typ is None:
> -            return None
> -        assert isinstance(typ, QAPISchemaType)
> -        return typ
> +    def lookup_type(self, name: str) -> QAPISchemaType:
> +        ent = self.get_typed_entity(name, QAPISchemaType)
> +        if ent is None:
> +            raise KeyError(f"Entity '{name}' is not defined.")
> +        return ent
>  
>      def resolve_type(
>          self,
> @@ -1178,13 +1191,14 @@ def resolve_type(
>          info: Optional[QAPISourceInfo],
>          what: Union[str, Callable[[Optional[QAPISourceInfo]], str]],
>      ) -> QAPISchemaType:
> -        typ = self.lookup_type(name)
> -        if not typ:
> +        try:
> +            return self.lookup_type(name)
> +        except (KeyError, TypeError) as err:
>              if callable(what):
>                  what = what(info)
>              raise QAPISemError(
> -                info, "%s uses unknown type '%s'" % (what, name))
> -        return typ
> +                info, "%s uses unknown type '%s'" % (what, name)
> +            ) from err

This is at best a wash for readability.

When something throws KeyError or TypeError unexpectedly, we
misinterpret the programming error as a semantic error in the schema.

>  
>      def _module_name(self, fname: str) -> str:
>          if QAPISchemaModule.is_system_module(fname):
> @@ -1279,7 +1293,7 @@ def _make_array_type(
>          self, element_type: str, info: Optional[QAPISourceInfo]
>      ) -> str:
>          name = element_type + 'List'    # reserved by check_defn_name_str()
> -        if not self.lookup_type(name):
> +        if not self.get_entity(name):
>              self._def_entity(QAPISchemaArrayType(name, info, element_type))
>          return name
>  
> @@ -1295,7 +1309,7 @@ def _make_implicit_object_type(
>              return None
>          # See also QAPISchemaObjectTypeMember.describe()
>          name = 'q_obj_%s-%s' % (name, role)
> -        typ = self.lookup_entity(name, QAPISchemaObjectType)
> +        typ = self.get_typed_entity(name, QAPISchemaObjectType)
>          if typ:
>              # The implicit object type has multiple users.  This can
>              # only be a duplicate definition, which will be flagged