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

[John Snow]

On Thu, Jan 11, 2024 at 4:33 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> 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.

git blame should cover it for the curious; otherwise if someone
decides to simplify it they'll find out quickly enough when the test
chirps. (Oh, assuming I actually get this into a test suite soon...)

--js

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

Kay, coming right up.

--js