On Thu, Sep 30, 2021 at 5:45 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.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"? > > "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 <js...@redhat.com> > > --- > > 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
