From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 80620C4707B for ; Wed, 10 Jan 2024 19:34:27 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rNeKP-0001NI-92; Wed, 10 Jan 2024 14:33:29 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rNeKN-0001Mu-Oc for qemu-devel@nongnu.org; Wed, 10 Jan 2024 14:33:27 -0500 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rNeKK-0007r5-4H for qemu-devel@nongnu.org; Wed, 10 Jan 2024 14:33:26 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704915202; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=NjSQ5KLN0K0E082PvDvuAqjXvGu4//QU9m0R4zAiTs4=; b=LR9FRcCW+6yAgiS3eleMuWUqT+2FiX2/ZdKR3UEI4idp3IzmKpDToASDHBiPABwhsLXGIP hQ4gEW/8OqdB/wLzWWPIX9FKfFN5ofo48PM5T+QwstmLA9FOYkTJCZwAKT2tpftvyTxHIK J3ieXbYj5B9Phv5pgX5l1pHRQKQiO80= Received: from mail-pg1-f198.google.com (mail-pg1-f198.google.com [209.85.215.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-13-sWjs8zPaO9uouDy2dRtRxw-1; Wed, 10 Jan 2024 14:33:18 -0500 X-MC-Unique: sWjs8zPaO9uouDy2dRtRxw-1 Received: by mail-pg1-f198.google.com with SMTP id 41be03b00d2f7-5cf1c5f68dcso782600a12.1 for ; Wed, 10 Jan 2024 11:33:18 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704915197; x=1705519997; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=NjSQ5KLN0K0E082PvDvuAqjXvGu4//QU9m0R4zAiTs4=; b=op6eK+Dv/8qyVwL0nXK4pkOa53A2BqmKUezr0+fcgsPkJnwM7Qv13W0rSFKvAv9Xfb xw+I3l0Te+CXrsvv0VsPnyJdDfffSoCVpCqLyGn5c72PwrzkmGrbySsbZbDstT8XneE9 PiQCt683oKbBJIyeCjWgyAwcoPDAXSSwkf7EweCW9a1Gc7dPnZULBCbGYD7s5YsMVUzi nAoekvL2qT+aFNT3Eak1UU1u1H5OviKyWjlEUzMYvK3J64gUXwCtyo/wWThdJ2+c9zwI AYVfN+q6cjQMs5zEV7Vpw6Cg0E/QhzooGuK6dKq3OtxFd3yfCfYgLOPMaXxWzRDbWCnl dOoQ== X-Gm-Message-State: AOJu0Yzju3u1be/Pg5gyD08/juwdmVZqRWi6dgZWu10DFB/ftR8H/ciG /HP3ns6y2iPIwwTkw7P6r010jQCcYylrur4Tm1J8nCLsxKLOgdJR866cVyRVU96uMFHoGKHSity UKIncbmQpq0QHdkt1+ERuvSTL9zzieEUsAgifDQs= X-Received: by 2002:a17:90a:df8b:b0:28b:b4d5:c3e9 with SMTP id p11-20020a17090adf8b00b0028bb4d5c3e9mr12537pjv.11.1704915197363; Wed, 10 Jan 2024 11:33:17 -0800 (PST) X-Google-Smtp-Source: AGHT+IEtpTUCM9fdwD3q38Ln8aCVd1keesAYP596lNuGqZ3P1AQNb8cWpD1Bl6cbHnQOV+4AytyZGduKrpkHWCdzD78= X-Received: by 2002:a17:90a:df8b:b0:28b:b4d5:c3e9 with SMTP id p11-20020a17090adf8b00b0028bb4d5c3e9mr12527pjv.11.1704915197015; Wed, 10 Jan 2024 11:33:17 -0800 (PST) MIME-Version: 1.0 References: <20231116014350.653792-1-jsnow@redhat.com> <20231116014350.653792-12-jsnow@redhat.com> <874jhedjv0.fsf@pond.sub.org> <87wmu84o6a.fsf@pond.sub.org> In-Reply-To: <87wmu84o6a.fsf@pond.sub.org> From: John Snow Date: Wed, 10 Jan 2024 14:33:05 -0500 Message-ID: Subject: Re: [PATCH 11/19] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type To: Markus Armbruster Cc: qemu-devel , Peter Maydell , Michael Roth Content-Type: multipart/alternative; boundary="000000000000b0d004060e9c8039" Received-SPF: pass client-ip=170.10.129.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -38 X-Spam_score: -3.9 X-Spam_bar: --- X-Spam_report: (-3.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-1.774, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --000000000000b0d004060e9c8039 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Nov 23, 2023, 8:03 AM Markus Armbruster wrote: > John Snow writes: > > > On Wed, Nov 22, 2023 at 7:59=E2=80=AFAM Markus Armbruster > wrote: > >> > >> John Snow 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 > >> > --- > >> > 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 =3D self.info.defn_meta > >> > + else: > >> > + what =3D 'built-in array' > >> > + > >> > self._element_type =3D 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.inf= o > > 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 =3D 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 chec= k > ent.check(self) > File "/work/armbru/qemu/scripts/qapi/schema.py", line 373, in check > self.element_type =3D schema.resolve_type( > ^^^^^^^^^^^^^^^^^^^^ > File "/work/armbru/qemu/scripts/qapi/schema.py", line 973, in > resolve_type > raise QAPISemError( > qapi.error.QAPISemError: > > 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 > sys.exit(main.main()) > ^^^^^^^^^^^ > File "/work/armbru/qemu/scripts/qapi/main.py", line 101, in main > print(err, file=3Dsys.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=3DNone to > what=3D'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 =3D 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 =3D 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=3DNone. > 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 =3D 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 > --000000000000b0d004060e9c8039 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Thu, Nov 23, 2023, 8:03 AM Markus Armbruster <armbru@redhat.com> wrote:
John Snow <jsnow@redhat.com> write= s:

> On Wed, Nov 22, 2023 at 7:59=E2=80=AFAM Markus Armbruster <armbru@r= edhat.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 sa= me.)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >=C2=A0 scripts/qapi/schema.py | 11 +++++++++--
>> >=C2=A0 1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py<= br> >> > index 462acb2bb61..164d86c4064 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -384,9 +384,16 @@ def need_has_if_optional(self):
>> >
>> >=C2=A0 =C2=A0 =C2=A0 def check(self, schema):
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super().check(schema)
>> > +
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if self.info:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert self.info.d= efn_meta=C2=A0 # guaranteed to be set by expr.py
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 what =3D self.info= .defn_meta
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 else:
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 what =3D 'buil= t-in array'
>> > +
>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._element_type =3D sche= ma.resolve_type(
>> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._element_type= _name, self.info,
>> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.info an= d self.info.defn_meta)
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._element_type= _name, self.info, what
>> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 )
0>> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 assert not isinstance(self= .element_type, QAPISchemaArrayType)
>> >
>> >=C2=A0 =C2=A0 =C2=A0 def set_module(self, schema):
>>
>> What problem are you solving here?
>>
>
> 1. "self.info and self.info.defn_meta" is the wrong ty= pe 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...=C2=A0 it seems to be Union[QAPISourceInfo, None, str].=
Type inference too weak.

=
I think my expectations match yours: "x and y&= quot; should return either x or y, so the resulting type would naively be U= nion[X | Y], which would indeed be Union[QAPISourceInfo | None | str], but:=

If QAPISourceInfo is *f= alse-y*, but not None, it'd be possible for the expression to yield a Q= APISourceInfo. mypy does not understand that QAPISourceInfo can never be fa= lse-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.=C2=A0 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 th= e 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?"=C2=A0 Short answer: never.<= br>

R= ight, okay. I just couldn't guarantee it statically. I knew this patch = was a little bananas, sorry for tossing you the stinkbomb.


Long answer.=C2=A0 We're dealing with a *specific* QAPISemError here, n= amely
.resolve_type()'s "uses unknown type".=C2=A0 If this happens = for a built-in
array, it's a programming error.

Let's commit such an error to see what happens: stick

=C2=A0 =C2=A0 =C2=A0 =C2=A0 self._make_array_type('xxx', None)

Dies like this:

=C2=A0 =C2=A0 Traceback (most recent call last):
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/main.py"= ;, line 94, in main
=C2=A0 =C2=A0 =C2=A0 =C2=A0 generate(args.schema,
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/main.py"= ;, line 50, in generate
=C2=A0 =C2=A0 =C2=A0 =C2=A0 schema =3D QAPISchema(schema_file)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^^^^^^^^^^^^^= ^^^^^^^^^^
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/schema.py&qu= ot;, line 938, in __init__
=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.check()
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/schema.py&qu= ot;, line 1225, in check
=C2=A0 =C2=A0 =C2=A0 =C2=A0 ent.check(self)
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/schema.py&qu= ot;, line 373, in check
=C2=A0 =C2=A0 =C2=A0 =C2=A0 self.element_type =3D schema.resolve_type(
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 ^^^^^^^^^^^^^^^^^^^^
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/schema.py&qu= ot;, line 973, in resolve_type
=C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISemError(
=C2=A0 =C2=A0 qapi.error.QAPISemError: <exception str() failed>

=C2=A0 =C2=A0 During handling of the above exception, another exception occ= urred:

=C2=A0 =C2=A0 Traceback (most recent call last):
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi-gen.py"= , line 19, in <module>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 sys.exit(main.main())
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^^^^^^^^^^^ =C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/main.py"= ;, line 101, in main
=C2=A0 =C2=A0 =C2=A0 =C2=A0 print(err, file=3Dsys.stderr)
=C2=A0 =C2=A0 =C2=A0 File "/work/armbru/qemu/scripts/qapi/error.py&quo= t;, line 41, in __str__
=C2=A0 =C2=A0 =C2=A0 =C2=A0 assert self.info is not None
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0^^^^^^^^^^^^^^^^^^^^= ^
=C2=A0 =C2=A0 AssertionError

Same before and after your patch.=C2=A0 The patch's change of what=3DNo= ne to
what=3D'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:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0super().check(schema)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self._element_type =3D schema.resolve_typ= e(
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self._element_type_name, = self.info,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.info and self.info.de= fn_meta)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.info.defn_meta if self.= info else None)

Yep.

=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0assert not isinstance(self.element_type, = QAPISchemaArrayType)

=C2=A0 =C2=A0 =C2=A0def set_module(self, schema: QAPISchema) -> None: @@ -1193,7 +1193,7 @@ def resolve_type(
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0self,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0name: str,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0info: Optional[QAPISourceInfo],
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 what: Union[str, Callable[[Optional[QAPISource= Info]], str]],
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 what: Union[None, str, Callable[[Optional[QAPI= SourceInfo]], str]],
=C2=A0 =C2=A0 =C2=A0) -> QAPISchemaType:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0typ =3D self.lookup_type(name)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if not typ:

The first hunk works around mypy's type inference weakness.=C2=A0 It re= writes

=C2=A0 =C2=A0 A and B

as

=C2=A0 =C2=A0 B if A else A

and then partially evaluates to

=C2=A0 =C2=A0 B if A else None

exploiting the fact that falsy A can only be None.=C2=A0 It replaces this patch.

Sounds good to me!


The second hunk corrects .resolve_type()'s typing to accept what=3DNone= .
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. Ex= cept that I'm a little reluctant to allow what to be None if this is th= e *only* caller known to possibly do it, and only in a circumstance that we= assert elsewhere that it should never happen.=C2=A0

Can we do:

what =3D self.info.defn_meta if=C2=A0self.info=C2=A0else None
assert what [is= not None]=C2=A0 # Depending on taste

instead?

N= o 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 c= an, IMO, but I'm not gonna die on the hill for a patch like this - just= sharing my tendencies for discussion.)

--js
--000000000000b0d004060e9c8039--