| Message ID | 20240313044127.49089-6-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
John Snow <jsnow@redhat.com> writes: > Include entities don't have names, but we generally expect "entities" to > have names. Reclassify all entities with names as *definitions*, leaving > the nameless include entities as QAPISchemaEntity instances. > > This is primarily to help simplify typing around expectations of what > callers expect for properties of an "entity". > > Suggested-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/schema.py | 144 +++++++++++++++++++++++------------------ > 1 file changed, 82 insertions(+), 62 deletions(-) > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 117f0f78f0c..da273c1649d 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py [...] > @@ -107,6 +90,46 @@ def _set_module(self, schema, info): > def set_module(self, schema): > self._set_module(schema, self.info) > > + def visit(self, visitor): > + # pylint: disable=unused-argument > + assert self._checked > + > + > +class QAPISchemaDefinition(QAPISchemaEntity): > + meta: Optional[str] = None > + > + def __init__(self, name: str, info, doc, ifcond=None, features=None): > + assert isinstance(name, str) > + super().__init__(info) > + for f in features or []: > + assert isinstance(f, QAPISchemaFeature) > + f.set_defined_in(name) > + self.name = name > + self.doc = doc > + self._ifcond = ifcond or QAPISchemaIfCond() > + self.features = features or [] > + > + def __repr__(self): > + return "<%s:%s at 0x%x>" % (type(self).__name__, self.name, > + id(self)) > + > + def c_name(self): > + return c_name(self.name) > + > + def check(self, schema): > + assert not self._checked > + seen = {} > + for f in self.features: > + f.check_clash(self.info, seen) > + super().check(schema) v3 called super().check() first. Why did you move it? I'm asking just to make sure I'm not missing something subtle. > + > + def connect_doc(self, doc=None): > + super().connect_doc(doc) > + doc = doc or self.doc > + if doc: > + for f in self.features: > + doc.connect_feature(f) > + > @property > def ifcond(self): > assert self._checked [...]
On Thu, Mar 14, 2024, 5:12 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > Include entities don't have names, but we generally expect "entities" to > > have names. Reclassify all entities with names as *definitions*, leaving > > the nameless include entities as QAPISchemaEntity instances. > > > > This is primarily to help simplify typing around expectations of what > > callers expect for properties of an "entity". > > > > Suggested-by: Markus Armbruster <armbru@redhat.com> > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > scripts/qapi/schema.py | 144 +++++++++++++++++++++++------------------ > > 1 file changed, 82 insertions(+), 62 deletions(-) > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index 117f0f78f0c..da273c1649d 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > [...] > > > @@ -107,6 +90,46 @@ def _set_module(self, schema, info): > > def set_module(self, schema): > > self._set_module(schema, self.info) > > > > + def visit(self, visitor): > > + # pylint: disable=unused-argument > > + assert self._checked > > + > > + > > +class QAPISchemaDefinition(QAPISchemaEntity): > > + meta: Optional[str] = None > > + > > + def __init__(self, name: str, info, doc, ifcond=None, > features=None): > > + assert isinstance(name, str) > > + super().__init__(info) > > + for f in features or []: > > + assert isinstance(f, QAPISchemaFeature) > > + f.set_defined_in(name) > > + self.name = name > > + self.doc = doc > > + self._ifcond = ifcond or QAPISchemaIfCond() > > + self.features = features or [] > > + > > + def __repr__(self): > > + return "<%s:%s at 0x%x>" % (type(self).__name__, self.name, > > + id(self)) > > + > > + def c_name(self): > > + return c_name(self.name) > > + > > + def check(self, schema): > > + assert not self._checked > > + seen = {} > > + for f in self.features: > > + f.check_clash(self.info, seen) > > + super().check(schema) > > v3 called super().check() first. Why did you move it? I'm asking just > to make sure I'm not missing something subtle. > I don't believe you are, I think it was just ... something I didn't notice when rebasing, since I merged this patch by hand. I recall having to insert the super call manually, and I guess my preferences are nondeterministic. > > + > > + def connect_doc(self, doc=None): > > + super().connect_doc(doc) > > + doc = doc or self.doc > > + if doc: > > + for f in self.features: > > + doc.connect_feature(f) > > + > > @property > > def ifcond(self): > > assert self._checked > > [...] > >
John Snow <jsnow@redhat.com> writes: > On Thu, Mar 14, 2024, 5:12 AM Markus Armbruster <armbru@redhat.com> wrote: > >> John Snow <jsnow@redhat.com> writes: >> >> > Include entities don't have names, but we generally expect "entities" to >> > have names. Reclassify all entities with names as *definitions*, leaving >> > the nameless include entities as QAPISchemaEntity instances. >> > >> > This is primarily to help simplify typing around expectations of what >> > callers expect for properties of an "entity". >> > >> > Suggested-by: Markus Armbruster <armbru@redhat.com> >> > Signed-off-by: John Snow <jsnow@redhat.com> >> > --- >> > scripts/qapi/schema.py | 144 +++++++++++++++++++++++------------------ >> > 1 file changed, 82 insertions(+), 62 deletions(-) >> > >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >> > index 117f0f78f0c..da273c1649d 100644 >> > --- a/scripts/qapi/schema.py >> > +++ b/scripts/qapi/schema.py >> >> [...] >> >> > @@ -107,6 +90,46 @@ def _set_module(self, schema, info): >> > def set_module(self, schema): >> > self._set_module(schema, self.info) >> > >> > + def visit(self, visitor): >> > + # pylint: disable=unused-argument >> > + assert self._checked >> > + >> > + >> > +class QAPISchemaDefinition(QAPISchemaEntity): >> > + meta: Optional[str] = None >> > + >> > + def __init__(self, name: str, info, doc, ifcond=None, features=None): >> > + assert isinstance(name, str) >> > + super().__init__(info) >> > + for f in features or []: >> > + assert isinstance(f, QAPISchemaFeature) >> > + f.set_defined_in(name) >> > + self.name = name >> > + self.doc = doc >> > + self._ifcond = ifcond or QAPISchemaIfCond() >> > + self.features = features or [] >> > + >> > + def __repr__(self): >> > + return "<%s:%s at 0x%x>" % (type(self).__name__, self.name, >> > + id(self)) >> > + >> > + def c_name(self): >> > + return c_name(self.name) >> > + >> > + def check(self, schema): >> > + assert not self._checked >> > + seen = {} >> > + for f in self.features: >> > + f.check_clash(self.info, seen) >> > + super().check(schema) >> >> v3 called super().check() first. Why did you move it? I'm asking just >> to make sure I'm not missing something subtle. >> > > I don't believe you are, I think it was just ... something I didn't notice > when rebasing, since I merged this patch by hand. I recall having to insert > the super call manually, and I guess my preferences are nondeterministic. I'd like to move it back, for consistency with its subtypes' .check(), which all call super().check() early. Okay? >> > + >> > + def connect_doc(self, doc=None): >> > + super().connect_doc(doc) >> > + doc = doc or self.doc >> > + if doc: >> > + for f in self.features: >> > + doc.connect_feature(f) >> > + >> > @property >> > def ifcond(self): >> > assert self._checked >> >> [...] >> >>
On Thu, Mar 14, 2024, 10:04 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > On Thu, Mar 14, 2024, 5:12 AM Markus Armbruster <armbru@redhat.com> > wrote: > > > >> John Snow <jsnow@redhat.com> writes: > >> > >> > Include entities don't have names, but we generally expect "entities" > to > >> > have names. Reclassify all entities with names as *definitions*, > leaving > >> > the nameless include entities as QAPISchemaEntity instances. > >> > > >> > This is primarily to help simplify typing around expectations of what > >> > callers expect for properties of an "entity". > >> > > >> > Suggested-by: Markus Armbruster <armbru@redhat.com> > >> > Signed-off-by: John Snow <jsnow@redhat.com> > >> > --- > >> > scripts/qapi/schema.py | 144 > +++++++++++++++++++++++------------------ > >> > 1 file changed, 82 insertions(+), 62 deletions(-) > >> > > >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > >> > index 117f0f78f0c..da273c1649d 100644 > >> > --- a/scripts/qapi/schema.py > >> > +++ b/scripts/qapi/schema.py > >> > >> [...] > >> > >> > @@ -107,6 +90,46 @@ def _set_module(self, schema, info): > >> > def set_module(self, schema): > >> > self._set_module(schema, self.info) > >> > > >> > + def visit(self, visitor): > >> > + # pylint: disable=unused-argument > >> > + assert self._checked > >> > + > >> > + > >> > +class QAPISchemaDefinition(QAPISchemaEntity): > >> > + meta: Optional[str] = None > >> > + > >> > + def __init__(self, name: str, info, doc, ifcond=None, > features=None): > >> > + assert isinstance(name, str) > >> > + super().__init__(info) > >> > + for f in features or []: > >> > + assert isinstance(f, QAPISchemaFeature) > >> > + f.set_defined_in(name) > >> > + self.name = name > >> > + self.doc = doc > >> > + self._ifcond = ifcond or QAPISchemaIfCond() > >> > + self.features = features or [] > >> > + > >> > + def __repr__(self): > >> > + return "<%s:%s at 0x%x>" % (type(self).__name__, self.name, > >> > + id(self)) > >> > + > >> > + def c_name(self): > >> > + return c_name(self.name) > >> > + > >> > + def check(self, schema): > >> > + assert not self._checked > >> > + seen = {} > >> > + for f in self.features: > >> > + f.check_clash(self.info, seen) > >> > + super().check(schema) > >> > >> v3 called super().check() first. Why did you move it? I'm asking just > >> to make sure I'm not missing something subtle. > >> > > > > I don't believe you are, I think it was just ... something I didn't > notice > > when rebasing, since I merged this patch by hand. I recall having to > insert > > the super call manually, and I guess my preferences are nondeterministic. > > I'd like to move it back, for consistency with its subtypes' .check(), > which all call super().check() early. Okay? > Yes, please do. (You *really* don't have to ask, but I understand you are being courteous to a fault. It's your module, though! I argue a lot but it *is* yours. Run it through the armbru delinter as much as you'd like.) > >> > + > >> > + def connect_doc(self, doc=None): > >> > + super().connect_doc(doc) > >> > + doc = doc or self.doc > >> > + if doc: > >> > + for f in self.features: > >> > + doc.connect_feature(f) > >> > + > >> > @property > >> > def ifcond(self): > >> > assert self._checked > >> > >> [...] > >> > >> > >
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 117f0f78f0c..da273c1649d 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -55,14 +55,13 @@ def is_present(self): class QAPISchemaEntity: - meta: Optional[str] = None + """ + A schema entity. - def __init__(self, name: str, info, doc, ifcond=None, features=None): - assert name is None or isinstance(name, str) - for f in features or []: - assert isinstance(f, QAPISchemaFeature) - f.set_defined_in(name) - self.name = name + This is either a directive, such as include, or a definition. + The latter uses sub-class `QAPISchemaDefinition`. + """ + def __init__(self, info): self._module = None # For explicitly defined entities, info points to the (explicit) # definition. For builtins (and their arrays), info is None. @@ -70,33 +69,17 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None): # triggered the implicit definition (there may be more than one # such place). self.info = info - self.doc = doc - self._ifcond = ifcond or QAPISchemaIfCond() - self.features = features or [] self._checked = False def __repr__(self): - if self.name is None: - return "<%s at 0x%x>" % (type(self).__name__, id(self)) - return "<%s:%s at 0x%x>" % (type(self).__name__, self.name, - id(self)) - - def c_name(self): - return c_name(self.name) + return "<%s at 0x%x>" % (type(self).__name__, id(self)) def check(self, schema): # pylint: disable=unused-argument - assert not self._checked - seen = {} - for f in self.features: - f.check_clash(self.info, seen) self._checked = True def connect_doc(self, doc=None): - doc = doc or self.doc - if doc: - for f in self.features: - doc.connect_feature(f) + pass def _set_module(self, schema, info): assert self._checked @@ -107,6 +90,46 @@ def _set_module(self, schema, info): def set_module(self, schema): self._set_module(schema, self.info) + def visit(self, visitor): + # pylint: disable=unused-argument + assert self._checked + + +class QAPISchemaDefinition(QAPISchemaEntity): + meta: Optional[str] = None + + def __init__(self, name: str, info, doc, ifcond=None, features=None): + assert isinstance(name, str) + super().__init__(info) + for f in features or []: + assert isinstance(f, QAPISchemaFeature) + f.set_defined_in(name) + self.name = name + self.doc = doc + self._ifcond = ifcond or QAPISchemaIfCond() + self.features = features or [] + + def __repr__(self): + return "<%s:%s at 0x%x>" % (type(self).__name__, self.name, + id(self)) + + def c_name(self): + return c_name(self.name) + + def check(self, schema): + assert not self._checked + seen = {} + for f in self.features: + f.check_clash(self.info, seen) + super().check(schema) + + def connect_doc(self, doc=None): + super().connect_doc(doc) + doc = doc or self.doc + if doc: + for f in self.features: + doc.connect_feature(f) + @property def ifcond(self): assert self._checked @@ -115,10 +138,6 @@ def ifcond(self): def is_implicit(self): return not self.info - def visit(self, visitor): - # pylint: disable=unused-argument - assert self._checked - def describe(self): assert self.meta return "%s '%s'" % (self.meta, self.name) @@ -218,7 +237,7 @@ def visit(self, visitor): class QAPISchemaInclude(QAPISchemaEntity): def __init__(self, sub_module, info): - super().__init__(None, info, None) + super().__init__(info) self._sub_module = sub_module def visit(self, visitor): @@ -226,7 +245,7 @@ def visit(self, visitor): visitor.visit_include(self._sub_module.name, self.info) -class QAPISchemaType(QAPISchemaEntity): +class QAPISchemaType(QAPISchemaDefinition): # Return the C type for common use. # For the types we commonly box, this is a pointer type. def c_type(self): @@ -797,7 +816,7 @@ def __init__(self, name, info, typ, ifcond=None): super().__init__(name, info, typ, False, ifcond) -class QAPISchemaCommand(QAPISchemaEntity): +class QAPISchemaCommand(QAPISchemaDefinition): meta = 'command' def __init__(self, name, info, doc, ifcond, features, @@ -868,7 +887,7 @@ def visit(self, visitor): self.coroutine) -class QAPISchemaEvent(QAPISchemaEntity): +class QAPISchemaEvent(QAPISchemaDefinition): meta = 'event' def __init__(self, name, info, doc, ifcond, features, arg_type, boxed): @@ -939,23 +958,24 @@ def __init__(self, fname): self.check() def _def_entity(self, ent): + self._entity_list.append(ent) + + def _def_definition(self, defn): # Only the predefined types are allowed to not have info - assert ent.info or self._predefining - self._entity_list.append(ent) - if ent.name is None: - return + assert defn.info or self._predefining + self._def_entity(defn) # TODO reject names that differ only in '_' vs. '.' vs. '-', # because they're liable to clash in generated C. - other_ent = self._entity_dict.get(ent.name) - if other_ent: - if other_ent.info: - where = QAPISourceError(other_ent.info, "previous definition") + other_defn = self._entity_dict.get(defn.name) + if other_defn: + if other_defn.info: + where = QAPISourceError(other_defn.info, "previous definition") raise QAPISemError( - ent.info, - "'%s' is already defined\n%s" % (ent.name, where)) + defn.info, + "'%s' is already defined\n%s" % (defn.name, where)) raise QAPISemError( - ent.info, "%s is already defined" % other_ent.describe()) - self._entity_dict[ent.name] = ent + defn.info, "%s is already defined" % other_defn.describe()) + self._entity_dict[defn.name] = defn def lookup_entity(self, name, typ=None): ent = self._entity_dict.get(name) @@ -997,7 +1017,7 @@ def _def_include(self, expr: QAPIExpression): QAPISchemaInclude(self._make_module(include), expr.info)) def _def_builtin_type(self, name, json_type, c_type): - self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type)) + self._def_definition(QAPISchemaBuiltinType(name, json_type, c_type)) # Instantiating only the arrays that are actually used would # be nice, but we can't as long as their generated code # (qapi-builtin-types.[ch]) may be shared by some other @@ -1023,15 +1043,15 @@ def _def_predefineds(self): self._def_builtin_type(*t) self.the_empty_object_type = QAPISchemaObjectType( 'q_empty', None, None, None, None, None, [], None) - self._def_entity(self.the_empty_object_type) + self._def_definition(self.the_empty_object_type) qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool'] qtype_values = self._make_enum_members( [{'name': n} for n in qtypes], None) - self._def_entity(QAPISchemaEnumType('QType', None, None, None, None, - qtype_values, 'QTYPE')) + self._def_definition(QAPISchemaEnumType( + 'QType', None, None, None, None, qtype_values, 'QTYPE')) def _make_features(self, features, info): if features is None: @@ -1053,7 +1073,8 @@ def _make_enum_members(self, values, info): def _make_array_type(self, element_type, info): name = element_type + 'List' # reserved by check_defn_name_str() if not self.lookup_type(name): - self._def_entity(QAPISchemaArrayType(name, info, element_type)) + self._def_definition(QAPISchemaArrayType( + name, info, element_type)) return name def _make_implicit_object_type(self, name, info, ifcond, role, members): @@ -1068,7 +1089,7 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members): # later. pass else: - self._def_entity(QAPISchemaObjectType( + self._def_definition(QAPISchemaObjectType( name, info, None, ifcond, None, None, members, None)) return name @@ -1079,7 +1100,7 @@ def _def_enum_type(self, expr: QAPIExpression): ifcond = QAPISchemaIfCond(expr.get('if')) info = expr.info features = self._make_features(expr.get('features'), info) - self._def_entity(QAPISchemaEnumType( + self._def_definition(QAPISchemaEnumType( name, info, expr.doc, ifcond, features, self._make_enum_members(data, info), prefix)) @@ -1107,7 +1128,7 @@ def _def_struct_type(self, expr: QAPIExpression): info = expr.info ifcond = QAPISchemaIfCond(expr.get('if')) features = self._make_features(expr.get('features'), info) - self._def_entity(QAPISchemaObjectType( + self._def_definition(QAPISchemaObjectType( name, info, expr.doc, ifcond, features, base, self._make_members(data, info), None)) @@ -1137,7 +1158,7 @@ def _def_union_type(self, expr: QAPIExpression): info) for (key, value) in data.items()] members: List[QAPISchemaObjectTypeMember] = [] - self._def_entity( + self._def_definition( QAPISchemaObjectType(name, info, expr.doc, ifcond, features, base, members, QAPISchemaVariants( @@ -1156,7 +1177,7 @@ def _def_alternate_type(self, expr: QAPIExpression): info) for (key, value) in data.items()] tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False) - self._def_entity( + self._def_definition( QAPISchemaAlternateType( name, info, expr.doc, ifcond, features, QAPISchemaVariants(None, info, tag_member, variants))) @@ -1181,11 +1202,10 @@ def _def_command(self, expr: QAPIExpression): if isinstance(rets, list): assert len(rets) == 1 rets = self._make_array_type(rets[0], info) - self._def_entity(QAPISchemaCommand(name, info, expr.doc, ifcond, - features, data, rets, - gen, success_response, - boxed, allow_oob, allow_preconfig, - coroutine)) + self._def_definition( + QAPISchemaCommand(name, info, expr.doc, ifcond, features, data, + rets, gen, success_response, boxed, allow_oob, + allow_preconfig, coroutine)) def _def_event(self, expr: QAPIExpression): name = expr['event'] @@ -1198,8 +1218,8 @@ def _def_event(self, expr: QAPIExpression): data = self._make_implicit_object_type( name, info, ifcond, 'arg', self._make_members(data, info)) - self._def_entity(QAPISchemaEvent(name, info, expr.doc, ifcond, - features, data, boxed)) + self._def_definition(QAPISchemaEvent(name, info, expr.doc, ifcond, + features, data, boxed)) def _def_exprs(self, exprs): for expr in exprs:
Include entities don't have names, but we generally expect "entities" to have names. Reclassify all entities with names as *definitions*, leaving the nameless include entities as QAPISchemaEntity instances. This is primarily to help simplify typing around expectations of what callers expect for properties of an "entity". Suggested-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/schema.py | 144 +++++++++++++++++++++++------------------ 1 file changed, 82 insertions(+), 62 deletions(-)