[v5] qapi: statically type schema.py

[PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

Posted by Markus Armbruster 8 months ago

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

Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

Posted by John Snow 7 months, 4 weeks ago

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>

>

Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

Posted by Markus Armbruster 7 months, 4 weeks ago

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!

Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

Posted by John Snow 7 months, 4 weeks ago

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!
>
>

Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

Posted by Philippe Mathieu-Daudé 8 months ago

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>