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