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