| Message ID | 20240315152301.3621858-25-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
On 15/3/24 16:23, Markus Armbruster wrote: > Entities with names starting with q_obj_ are implicit object types. > Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity() > can only return a QAPISchemaObjectType. Assert that. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi/schema.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster <armbru@redhat.com> wrote: > Entities with names starting with q_obj_ are implicit object types. > Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity() > can only return a QAPISchemaObjectType. Assert that. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi/schema.py | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index e52930a48a..a6180f93c6 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -1297,8 +1297,9 @@ 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.lookup_entity(name) > if typ: > + assert(isinstance(typ, QAPISchemaObjectType)) > # The implicit object type has multiple users. This can > # only be a duplicate definition, which will be flagged > # later. > -- > 2.44.0 > Seems obviously fine, though I don't suppose this narrowing will be "remembered" by the type system. Do we care? Reviewed-by: John Snow <jsnow@redhat.com> >
John Snow <jsnow@redhat.com> writes: > On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster <armbru@redhat.com> wrote: > >> Entities with names starting with q_obj_ are implicit object types. >> Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity() >> can only return a QAPISchemaObjectType. Assert that. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi/schema.py | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >> index e52930a48a..a6180f93c6 100644 >> --- a/scripts/qapi/schema.py >> +++ b/scripts/qapi/schema.py >> @@ -1297,8 +1297,9 @@ 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.lookup_entity(name) >> if typ: >> + assert(isinstance(typ, QAPISchemaObjectType)) >> # The implicit object type has multiple users. This can >> # only be a duplicate definition, which will be flagged >> # later. >> -- >> 2.44.0 >> > > Seems obviously fine, though I don't suppose this narrowing will be > "remembered" by the type system. Do we care? mypy passes without it. It's for catching programming errors and helping the reader along. The former are unlikely, and the latter is debatable, but when in doubt, assert. > Reviewed-by: John Snow <jsnow@redhat.com> Thanks!
On Tue, Mar 19, 2024, 12:02 PM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster <armbru@redhat.com> > wrote: > > > >> Entities with names starting with q_obj_ are implicit object types. > >> Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity() > >> can only return a QAPISchemaObjectType. Assert that. > >> > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- > >> scripts/qapi/schema.py | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > >> index e52930a48a..a6180f93c6 100644 > >> --- a/scripts/qapi/schema.py > >> +++ b/scripts/qapi/schema.py > >> @@ -1297,8 +1297,9 @@ 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.lookup_entity(name) > >> if typ: > >> + assert(isinstance(typ, QAPISchemaObjectType)) > >> # The implicit object type has multiple users. This can > >> # only be a duplicate definition, which will be flagged > >> # later. > >> -- > >> 2.44.0 > >> > > > > Seems obviously fine, though I don't suppose this narrowing will be > > "remembered" by the type system. Do we care? > > mypy passes without it. It's for catching programming errors and > helping the reader along. The former are unlikely, and the latter is > debatable, but when in doubt, assert. > mmhmm. I was just wondering if we could tighten the typing of typ itself, or if it conflicted with legitimate broader uses. it happens a lot in qapi that we're regulated by broader base types in parent classes etc. If we CAN tighten the variable/field (without runtime code changes), we should do so. If we can't, this patch is 100% totally fine as is. > > Reviewed-by: John Snow <jsnow@redhat.com> > > Thanks! > >
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index e52930a48a..a6180f93c6 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -1297,8 +1297,9 @@ 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.lookup_entity(name) if typ: + assert(isinstance(typ, QAPISchemaObjectType)) # The implicit object type has multiple users. This can # only be a duplicate definition, which will be flagged # later.
Entities with names starting with q_obj_ are implicit object types. Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity() can only return a QAPISchemaObjectType. Assert that. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi/schema.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)