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
+ )
assert not isinstance(self.element_type, QAPISchemaArrayType)
def set_module(self, schema):
--
2.41.0John 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 > + ) > assert not isinstance(self.element_type, QAPISchemaArrayType) > > def set_module(self, schema): What problem are you solving here?
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 > > + ) > > 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 2. self.info.defn_meta is *also* not guaranteed by static types 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.
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.
> 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.
> 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.
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)
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.
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?
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
>
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.
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
© 2016 - 2024 Red Hat, Inc.