Re: [PATCH 11/19] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On Wed, Nov 22, 2023 at 7:59 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > There's more conditionals in here than we can reasonably pack into a
>> > terse little statement, so break it apart into something more explicit.
>> >
>> > (When would a built-in array ever cause a QAPISemError? I don't know,
>> > maybe never - but the type system wasn't happy all the same.)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  scripts/qapi/schema.py | 11 +++++++++--
>> >  1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index 462acb2bb61..164d86c4064 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -384,9 +384,16 @@ def need_has_if_optional(self):
>> >
>> >      def check(self, schema):
>> >          super().check(schema)
>> > +
>> > +        if self.info:
>> > +            assert self.info.defn_meta  # guaranteed to be set by expr.py
>> > +            what = self.info.defn_meta
>> > +        else:
>> > +            what = 'built-in array'
>> > +
>> >          self._element_type = schema.resolve_type(
>> > -            self._element_type_name, self.info,
>> > -            self.info and self.info.defn_meta)
>> > +            self._element_type_name, self.info, what
>> > +        )
0>> >          assert not isinstance(self.element_type, QAPISchemaArrayType)
>> >
>> >      def set_module(self, schema):
>>
>> What problem are you solving here?
>>
>
> 1. "self.info and self.info.defn_meta" is the wrong type ifn't self.info

self.info is Optional[QAPISourceInfo].

When self.info, then self.info.defn_meta is is Optional[str].

Naive me expects self.info and self.info.defn_meta to be Optional[str].
Playing with mypy...  it seems to be Union[QAPISourceInfo, None, str].
Type inference too weak.

> 2. self.info.defn_meta is *also* not guaranteed by static types

Yes.  We know it's not None ("guaranteed to be set by expr.py"), but the
type system doesn't.

> ultimately: we need to assert self.info and self.info.defn_meta both;
> but it's possible (?) that we don't have self.info in the case that
> we're a built-in array, so I handle that.

This bring us back to the question in your commit message: "When would a
built-in array ever cause a QAPISemError?"  Short answer: never.

Long answer.  We're dealing with a *specific* QAPISemError here, namely
.resolve_type()'s "uses unknown type".  If this happens for a built-in
array, it's a programming error.

Let's commit such an error to see what happens: stick

        self._make_array_type('xxx', None)

Dies like this:

    Traceback (most recent call last):
      File "/work/armbru/qemu/scripts/qapi/main.py", line 94, in main
        generate(args.schema,
      File "/work/armbru/qemu/scripts/qapi/main.py", line 50, in generate
        schema = QAPISchema(schema_file)
                 ^^^^^^^^^^^^^^^^^^^^^^^
      File "/work/armbru/qemu/scripts/qapi/schema.py", line 938, in __init__
        self.check()
      File "/work/armbru/qemu/scripts/qapi/schema.py", line 1225, in check
        ent.check(self)
      File "/work/armbru/qemu/scripts/qapi/schema.py", line 373, in check
        self.element_type = schema.resolve_type(
                            ^^^^^^^^^^^^^^^^^^^^
      File "/work/armbru/qemu/scripts/qapi/schema.py", line 973, in resolve_type
        raise QAPISemError(
    qapi.error.QAPISemError: <exception str() failed>

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/work/armbru/qemu/scripts/qapi-gen.py", line 19, in <module>
        sys.exit(main.main())
                 ^^^^^^^^^^^
      File "/work/armbru/qemu/scripts/qapi/main.py", line 101, in main
        print(err, file=sys.stderr)
      File "/work/armbru/qemu/scripts/qapi/error.py", line 41, in __str__
        assert self.info is not None
               ^^^^^^^^^^^^^^^^^^^^^
    AssertionError

Same before and after your patch.  The patch's change of what=None to
what='built-in array' has no effect.

Here's a slightly simpler patch:

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 46004689f0..feb0023d25 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -479,7 +479,7 @@ def check(self, schema: QAPISchema) -> None:
         super().check(schema)
         self._element_type = schema.resolve_type(
             self._element_type_name, self.info,
-            self.info and self.info.defn_meta)
+            self.info.defn_meta if self.info else None)
         assert not isinstance(self.element_type, QAPISchemaArrayType)
 
     def set_module(self, schema: QAPISchema) -> None:
@@ -1193,7 +1193,7 @@ def resolve_type(
         self,
         name: str,
         info: Optional[QAPISourceInfo],
-        what: Union[str, Callable[[Optional[QAPISourceInfo]], str]],
+        what: Union[None, str, Callable[[Optional[QAPISourceInfo]], str]],
     ) -> QAPISchemaType:
         typ = self.lookup_type(name)
         if not typ:

The first hunk works around mypy's type inference weakness.  It rewrites

    A and B

as

    B if A else A

and then partially evaluates to

    B if A else None

exploiting the fact that falsy A can only be None.  It replaces this
patch.

The second hunk corrects .resolve_type()'s typing to accept what=None.
It's meant to be squashed into PATCH 16.

What do you think?