diff mbox series

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

Message ID 20240315152301.3621858-25-armbru@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi: statically type schema.py | expand

Commit Message

Markus Armbruster March 15, 2024, 3:23 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé March 15, 2024, 4:39 p.m. UTC | #1
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>
John Snow March 19, 2024, 3:30 p.m. UTC | #2
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>

>
Markus Armbruster March 19, 2024, 4:02 p.m. UTC | #3
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!
John Snow March 19, 2024, 4:06 p.m. UTC | #4
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 mbox series

Patch

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.