| Message ID | 20231116014350.653792-12-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
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?
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
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):
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(-)