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 Thu, Nov 23, 2023, 8:03 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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.
>>
>
> I think my expectations match yours: "x and y" should return either x or y,
> so the resulting type would naively be Union[X | Y], which would indeed be
> Union[QAPISourceInfo | None | str], but:
>
> If QAPISourceInfo is *false-y*, but not None, it'd be possible for the
> expression to yield a QAPISourceInfo. mypy does not understand that
> QAPISourceInfo can never be false-y.
>
> (That I know of. Maybe there's a trick to annotate it. I like your solution
> below better anyway, just curious about the exact nature of this
> limitation.)
>
>
>> > 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.
>>
>
> Mmhmm.
>
>
>> > 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.
>
> Right, okay. I just couldn't guarantee it statically. I knew this patch was
> a little bananas, sorry for tossing you the stinkbomb.

No need to be sorry!  Feels like an efficient way to collaborate with
me.

>> 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)
>>
>
> Yep.
>
>          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.
>
> Sounds good to me!

Does it need a comment explaining the somewhat awkward coding?  Probably
not.

>> 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?
>>
>
> I'm on my mobile again, but at a glance I like it. Except that I'm a little
> reluctant to allow what to be None if this is the *only* caller known to
> possibly do it, and only in a circumstance that we assert elsewhere that it
> should never happen.
>
> Can we do:
>
> what = self.info.defn_meta if self.info else None
> assert what [is not None]  # Depending on taste
>
> instead?
>
> No sem error, no new unit test needed, assertion provides the correct frame
> of mind (programmer error), stronger typing on resolve_type.
>
> (I really love eliminating None when I can as a rule because I like how
> much more it tells you about the nature of all callers, it's a much
> stronger decree. Worth pursuing where you can, IMO, but I'm not gonna die
> on the hill for a patch like this - just sharing my tendencies for
> discussion.)

Suggest you post the patch, so I can see it more easily in context.