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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A35A0C433EF for ; Thu, 30 Sep 2021 17:13:46 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 004CF61440 for ; Thu, 30 Sep 2021 17:13:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 004CF61440 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:55322 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mVzcv-0007Mt-5K for qemu-devel@archiver.kernel.org; Thu, 30 Sep 2021 13:13:45 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34520) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mVzbi-0006E5-0v for qemu-devel@nongnu.org; Thu, 30 Sep 2021 13:12:30 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:60158) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mVzbf-0002qI-Ap for qemu-devel@nongnu.org; Thu, 30 Sep 2021 13:12:29 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1633021946; 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=2ZRN643k1EdMGEMviLuvYFmhHTNdeR10nx4MZt90OLc=; b=empvG/cNSQziLoWfWVz5lZ0suhsGyj/UguoG6f7LW9V6dDrC5g1vAfK0k/g46whrBUIsYL 6SJGSDaA26IOVj3gAQtRh0CTIJiKg9FcECyJluV9tF8xpomqMCU6Nr1XkA3xXbCdnmz28u 4tBavpuAGjF2gDlmwGukVyKyziqsH/c= Received: from mail-ua1-f71.google.com (mail-ua1-f71.google.com [209.85.222.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-137-v5hq7nEHMbWXaHW_rPY7ig-1; Thu, 30 Sep 2021 13:12:07 -0400 X-MC-Unique: v5hq7nEHMbWXaHW_rPY7ig-1 Received: by mail-ua1-f71.google.com with SMTP id i9-20020ab029c90000b02902aa59690c5aso3389586uaq.3 for ; Thu, 30 Sep 2021 10:12:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2ZRN643k1EdMGEMviLuvYFmhHTNdeR10nx4MZt90OLc=; b=E0KyqVnxnJqKsUJqbz4S9iisXA8+p1b7gCndX2QoGVL9qcGA0qglG2TzzB4vXKD1qp clkKcGdZ27qqcpuswBlF43OC9t2l6GqDBZX+mN8SL7tJZpksbl9CoUqyHO4BUK2B/5jv o0ZNCOgfIk1K2HS12LVuPaAqD3Cd92paqvD+FGYoEH46o23NoVRvHuUOaQZt3q6lXGei joHQnp+91U17uGHkKvnFBIf9YvLAxCK/5HcTGsbk4XL1pPVR5J3sSjcl96+rBwW2pYqQ OqpDRwXVupllupsiyt+z299LMguyCZMLnRWOEAPRK6WEAqZLL87bNEa/VWkoLM6eHCH8 HDjg== X-Gm-Message-State: AOAM530TFMKhFn/q/lfRF6UZuMgfZ9FvEm8VcVG2lL56HhyQTH5qL46z Wgi6ayFlyb6cXf0FOJg9eacGj1IKx0m/3JzaEp0pnYRx7DcNw/jzdACznZSHYrAy50mvsCovMXi LqeZ52T8+64zHzAuWpQEFBkvFkH7t0q4= X-Received: by 2002:ab0:558d:: with SMTP id v13mr6706490uaa.50.1633021927047; Thu, 30 Sep 2021 10:12:07 -0700 (PDT) X-Google-Smtp-Source: ABdhPJykoLwJdxgqmNfM92HFkymVp9dGzoYPQkTt+SsEh4kwcRvXdduH/4AipbqOV/5+AOdqBGiv2uta6QSO/zW1R3Y= X-Received: by 2002:ab0:558d:: with SMTP id v13mr6706472uaa.50.1633021926854; Thu, 30 Sep 2021 10:12:06 -0700 (PDT) MIME-Version: 1.0 References: <20210929194428.1038496-1-jsnow@redhat.com> <20210929194428.1038496-10-jsnow@redhat.com> <87o88aqtw4.fsf@dusky.pond.sub.org> In-Reply-To: <87o88aqtw4.fsf@dusky.pond.sub.org> From: John Snow Date: Thu, 30 Sep 2021 13:11:55 -0400 Message-ID: Subject: Re: [PATCH v3 09/13] qapi/parser: add import cycle workaround To: Markus Armbruster Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=jsnow@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="000000000000dc61c005cd398b82" Received-SPF: pass client-ip=216.205.24.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, 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_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Michael Roth , Cleber Rosa , Eric Blake , qemu-devel , Eduardo Habkost Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000dc61c005cd398b82 Content-Type: text/plain; charset="UTF-8" On Thu, Sep 30, 2021 at 5:45 AM Markus Armbruster wrote: > John Snow writes: > > > There is a cycle that exists in the QAPI generator: [schema -> expr -> > > "There is" or "there will be once we add strong type hints"? > > "There exists in my mind-palace a cycle where, ..." (Will adjust the commit message.) > > parser -> schema]. It exists because the QAPIDoc class needs the names > > of types defined by the schema module, but the schema module needs to > > import both expr.py/parser.py to do its actual parsing. > > > > Ultimately, the layering violation is that parser.py should not have any > > knowledge of specifics of the Schema. QAPIDoc performs double-duty here > > both as a parser *and* as a finalized object that is part of the schema. > > > > I see three paths here: > > > > (1) Just use the TYPE_CHECKING trick to eliminate the cycle which is only > > present during static analysis. > > > > (2) Don't bother to annotate connect_member() et al, give them 'object' > > or 'Any'. I don't particularly like this, because it diminishes the > > usefulness of type hints for documentation purposes. Still, it's an > > extremely quick fix. > > > > (3) Reimplement doc <--> definition correlation directly in schema.py, > > integrating doc fields directly into QAPISchemaMember and relieving > > the QAPIDoc class of the responsibility. Users of the information > > would instead visit the members first and retrieve their > > documentation instead of the inverse operation -- visiting the > > documentation and retrieving their members. > > > > I prefer (3), but (1) is the easiest way to have my cake (strong type > > hints) and eat it too (Not have import cycles). Do (1) for now, but plan > > for (3). See also: > > > https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles > > > > Signed-off-by: John Snow > > --- > > scripts/qapi/parser.py | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 123fc2f099c..30b1d98df0b 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -18,6 +18,7 @@ > > import os > > import re > > from typing import ( > > + TYPE_CHECKING, > > Dict, > > List, > > Optional, > > @@ -30,6 +31,12 @@ > > from .source import QAPISourceInfo > > > > > > +if TYPE_CHECKING: > > + # pylint: disable=cyclic-import > > + # TODO: Remove cycle. [schema -> expr -> parser -> schema] > > + from .schema import QAPISchemaFeature, QAPISchemaMember > > + > > + > > # Return value alias for get_expr(). > > _ExprValue = Union[List[object], Dict[str, object], str, bool] > > > > @@ -473,9 +480,9 @@ def append(self, line): > > class ArgSection(Section): > > def __init__(self, parser, name, indent=0): > > super().__init__(parser, name, indent) > > - self.member = None > > + self.member: Optional['QAPISchemaMember'] = None > > > > - def connect(self, member): > > + def connect(self, member: 'QAPISchemaMember') -> None: > > self.member = member > > > > class NullSection(Section): > > @@ -750,14 +757,14 @@ def _append_freeform(self, line): > > % match.group(1)) > > self._section.append(line) > > > > - def connect_member(self, member): > > + def connect_member(self, member: 'QAPISchemaMember') -> None: > > if member.name not in self.args: > > # Undocumented TODO outlaw > > self.args[member.name] = QAPIDoc.ArgSection(self._parser, > > member.name) > > self.args[member.name].connect(member) > > > > - def connect_feature(self, feature): > > + def connect_feature(self, feature: 'QAPISchemaFeature') -> None: > > if feature.name not in self.features: > > raise QAPISemError(feature.info, > > "feature '%s' lacks documentation" > > This adds just the type hints that cause the cycle. I like that, > because it illustrates the cycle. Would be nice if the commit message > mentioned this, perhaps > > I prefer (3), but (1) is the easiest way to have my cake (strong type > hints) and eat it too (Not have import cycles). Do (1) for now, but plan > for (3). Also add the type hints that cause the cycle right away to > illustrate. See also: > > https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles > > Slightly nicer, I think, would be swapping this and the next patch. > Then that one's commit message needs to say something like "except for a > few problematic ones, which the next commit will add". Worthwhile? Up > to you. > > Doing it the other way around means you can't squash the mypy patch into the bulk-type-hints patch, but I think the git log usefulness is not better or worse either way around. (Reviewer usefulness is maybe a ship that has sailed, by now?) --js --000000000000dc61c005cd398b82 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Sep 30, 2021 at 5:45 AM Marku= s Armbruster <armbru@redhat.com= > wrote:
John= Snow <jsnow@redha= t.com> writes:

> There is a cycle that exists in the QAPI generator: [schema -> expr= ->

"There is" or "there will be once we add strong type hints&q= uot;?


"There exists in my mind-palace a= cycle where, ..."

(Will adjust the commit me= ssage.)
=C2=A0
> parser -> schema]. It exists because the QAPIDoc class needs the na= mes
> of types defined by the schema module, but the schema module needs to<= br> > import both expr.py/parser.py to do its actual parsing.
>
> Ultimately, the layering violation is that parser.py should not have a= ny
> knowledge of specifics of the Schema. QAPIDoc performs double-duty her= e
> both as a parser *and* as a finalized object that is part of the schem= a.
>
> I see three paths here:
>
> (1) Just use the TYPE_CHECKING trick to eliminate the cycle which is o= nly
>=C2=A0 =C2=A0 =C2=A0present during static analysis.
>
> (2) Don't bother to annotate connect_member() et al, give them = 9;object'
>=C2=A0 =C2=A0 =C2=A0or 'Any'. I don't particularly like thi= s, because it diminishes the
>=C2=A0 =C2=A0 =C2=A0usefulness of type hints for documentation purposes= . Still, it's an
>=C2=A0 =C2=A0 =C2=A0extremely quick fix.
>
> (3) Reimplement doc <--> definition correlation directly in sche= ma.py,
>=C2=A0 =C2=A0 =C2=A0integrating doc fields directly into QAPISchemaMemb= er and relieving
>=C2=A0 =C2=A0 =C2=A0the QAPIDoc class of the responsibility. Users of t= he information
>=C2=A0 =C2=A0 =C2=A0would instead visit the members first and retrieve = their
>=C2=A0 =C2=A0 =C2=A0documentation instead of the inverse operation -- v= isiting the
>=C2=A0 =C2=A0 =C2=A0documentation and retrieving their members.
>
> I prefer (3), but (1) is the easiest way to have my cake (strong type<= br> > hints) and eat it too (Not have import cycles). Do (1) for now, but pl= an
> for (3). See also:
> https://mypy.readthedo= cs.io/en/latest/runtime_troubles.html#import-cycles
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>=C2=A0 scripts/qapi/parser.py | 15 +++++++++++----
>=C2=A0 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 123fc2f099c..30b1d98df0b 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -18,6 +18,7 @@
>=C2=A0 import os
>=C2=A0 import re
>=C2=A0 from typing import (
> +=C2=A0 =C2=A0 TYPE_CHECKING,
>=C2=A0 =C2=A0 =C2=A0 Dict,
>=C2=A0 =C2=A0 =C2=A0 List,
>=C2=A0 =C2=A0 =C2=A0 Optional,
> @@ -30,6 +31,12 @@
>=C2=A0 from .source import QAPISourceInfo
>=C2=A0
>=C2=A0
> +if TYPE_CHECKING:
> +=C2=A0 =C2=A0 # pylint: disable=3Dcyclic-import
> +=C2=A0 =C2=A0 # TODO: Remove cycle. [schema -> expr -> parser -= > schema]
> +=C2=A0 =C2=A0 from .schema import QAPISchemaFeature, QAPISchemaMember=
> +
> +
>=C2=A0 # Return value alias for get_expr().
>=C2=A0 _ExprValue =3D Union[List[object], Dict[str, object], str, bool]=
>=C2=A0
> @@ -473,9 +480,9 @@ def append(self, line):
>=C2=A0 =C2=A0 =C2=A0 class ArgSection(Section):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 def __init__(self, parser, name, ind= ent=3D0):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 super().__init__(parse= r, name, indent)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.member =3D None
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.member: Optional['= QAPISchemaMember'] =3D None
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 def connect(self, member):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 def connect(self, member: 'QAPISchema= Member') -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.member =3D member=
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 class NullSection(Section):
> @@ -750,14 +757,14 @@ def _append_freeform(self, line):
>=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 =C2=A0% match.group(1)) >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self._section.append(line)
>=C2=A0
> -=C2=A0 =C2=A0 def connect_member(self, member):
> +=C2=A0 =C2=A0 def connect_member(self, member: 'QAPISchemaMember&= #39;) -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if member.name not in self.args:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # Undocumented TODO ou= tlaw
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.args[member.name] = =3D QAPIDoc.ArgSection(self._parser,
>=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 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 member.name)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.args[member.name].connect(member)<= br> >=C2=A0
> -=C2=A0 =C2=A0 def connect_feature(self, feature):
> +=C2=A0 =C2=A0 def connect_feature(self, feature: 'QAPISchemaFeatu= re') -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if feature.name not in self.features:<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 raise QAPISemError(feature.i= nfo,
>=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"feature '%s' = lacks documentation"

This adds just the type hints that cause the cycle.=C2=A0 I like that,
because it illustrates the cycle.=C2=A0 Would be nice if the commit message=
mentioned this, perhaps

=C2=A0 I prefer (3), but (1) is the easiest way to have my cake (strong typ= e
=C2=A0 hints) and eat it too (Not have import cycles). Do (1) for now, but = plan
=C2=A0 for (3). Also add the type hints that cause the cycle right away to<= br> =C2=A0 illustrate. See also:
=C2=A0 https://mypy.readthe= docs.io/en/latest/runtime_troubles.html#import-cycles

Slightly nicer, I think, would be swapping this and the next patch.
Then that one's commit message needs to say something like "except= for a
few problematic ones, which the next commit will add".=C2=A0 Worthwhil= e?=C2=A0 Up
to you.


Doing it the other way around means you can't squash the my= py patch into the bulk-type-hints patch, but I think the git log usefulness= is not better or worse either way around. (Reviewer usefulness is maybe a = ship that has sailed, by now?)

--js
--000000000000dc61c005cd398b82--