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

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7596 bytes --]

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.


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


> 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.)

--js

>

[-- Attachment #2: Type: text/html, Size: 12313 bytes --]