Re: [PATCH v3 09/13] qapi/parser: add import cycle workaround

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5099 bytes --]

On Thu, Sep 30, 2021 at 5:45 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@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 <jsnow@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

[-- Attachment #2: Type: text/html, Size: 7567 bytes --]