John Snow <js...@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 <js...@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
