| Message ID | 20231116014350.653792-20-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
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
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.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)) 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) 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 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 +) + + 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 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
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: ... 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. 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. 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. 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(-)