On Thu, Jan 11, 2024 at 4:33 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> John Snow <js...@redhat.com> writes:
>
> > On Thu, Nov 23, 2023, 8:03 AM Markus Armbruster <arm...@redhat.com> wrote:
> >
> >> John Snow <js...@redhat.com> writes:
> >>
> >> > On Wed, Nov 22, 2023 at 7:59 AM Markus Armbruster <arm...@redhat.com>
> >> > wrote:
> >> >>
> >> >> John Snow <js...@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 <js...@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
