Re: [PATCH v2 2/6] qapi/parser: Allow empty QAPIDoc Sections

[John Snow <jsnow@redhat.com>]

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

On Tue, Sep 7, 2021 at 4:28 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > It simplifies the typing to say that _section is always a
> > QAPIDoc.Section().
>
> If you say so....
>
>
I mean, I thought so at the time. I have an aversion to making optional
types and then littering isinstance() checks in places. I like keeping as
much of the typing statically provable as I can without adding runtime
checks. I'm re-evaluating this patch now, and I'll see if there's anything
I can do, but it's hard to predict differences in matters of taste and
style.

One thing an Optional[T] class instance variable can do that might be
helpful is to remind users that they need to check to see if it's present
or not. If I see a path to eliminate those checks, though, I will generally
always take it - eliminates the need to check everywhere else and puts all
the information you need into the static typing. As a habit, I prefer that
when feasible.

> To accommodate this change, we must allow for this object to evaluate to
> > False for functions like _end_section which behave differently based on
> > whether or not a Section has been started.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > Probably a better fix is to restructure the code to prevent empty
> > sections from being "ended", but that seems like a bigger whale than
> > what I'm after at the immediate moment.
>
> Do we have a TODO comment for that?
>
>
Nope. I'll either add one or just fix the root issue, because I want to go
back to writing the cross-referenceable QAPI symbol plugin for sphinx now.
At the time I thought I'd get to fixing some of the issues raised by pt5 a
lot sooner, but.


> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/parser.py | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index b6a5e661215..3ddde318376 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -456,6 +456,9 @@ def __init__(self, parser, name=None, indent=0):
> >              # the expected indent level of the text of this section
> >              self._indent = indent
> >
> > +        def __bool__(self) -> bool:
> > +            return bool(self.name or self.text)
> > +
> >          def append(self, line):
> >              # Strip leading spaces corresponding to the expected indent
> level
> >              # Blank lines are always OK.
>
> Overriding __bool__() is the minimally invasive compensation for the
> next hunk's replacement of None by a QAPIDoc.Section
>
> However, I'm wary of overriding __bool__().  It creates a difference
> between "if object:" and "if object is not None:".  Gives me a queasy
> feeling, as shortening the latter to the former is pretty much
> automatic.
>
>
That's just Python, though. If collections are empty, they're falsey. If
QAPIDoc is a collection of documentation lines and we have no lines,
shouldn't that also be falsey?

Nah, I get the apprehension. It's not a strictly linear collection and I
didn't check *every* last field, just name and text. It's kind of a hack
because I'm trying to paper over some deeper(?) problem(??), but it felt
safe because the static typing means we're not likely to confuse the two
cases -- we don't need to distinguish "Absent" from "empty". Or at least,
after this patch we don't need to, anymore. (We'll always have a section,
so an empty section versus no section makes no difference.)

A boring .is_empty() would avoid that, but we'd have to adjust "if S" to
> "if S.is_empty()" wherever we changed S from Optional[Section] to
> Section.  Which S could be affected?
>
>
Feels deeply against the grain of how Python is written to go out of your
way to create an .is_empty() function.


> The following variables get assigned Section or ArgSection:
>
>     QAPIDoc.body
>     QAPIDoc._section
>     QAPIDoc.args[NAME]
>     QAPIDoc.features[NAME]
>
> .body, .args[NAME] and .features[NAME] are never None I believe.
>
> ._section is also assigned None, in ._end_section().  It remains None
> until the next ._start*_section().
>
> The only use of .section that doesn't dot into it is in ._end_section().
> That's the only spot to adjust.
>
> Confirm by testing: in all of "make check", Section.__bool__() is only
> ever called from QAPIDoc._end_section().  Checked by sticking
> traceback.print_stack() into .__bool__().
>
>
If that's the only caller, isn't it then perfectly safe to just use the
patch as written?


> > @@ -722,7 +725,7 @@ def _end_section(self):
> >                  raise QAPIParseError(
> >                      self._parser,
> >                      "empty doc section '%s'" % self._section.name)
> > -            self._section = None
> > +            self._section = QAPIDoc.Section(self._parser)
> >
> >      def _append_freeform(self, line):
> >          match = re.match(r'(@\S+:)', line)
>
> Replacing None by QAPIDoc.Section doesn't just simplify the typing!  It
> also creates a bogus "additional section" (in the sense of QAPIDoc's
> class comment) after any section.  Works, because the .start*_section()
> all call .end_section() first, .end_section() does nothing for empty
> sections, and the bogus sections remain empty, unless we somehow screw
> up the code to add contents to them.  Such screwups are now possible,
> whereas before they would crash.
>
> This correctness argument isn't obvious, and therefore should be made in
> the commit message.
>
>
I'll try to suss out the real problem, because I am not sure how to justify
the hypothetical cases where we might add content to a bogus section,
because I do not fully understand the circumstances in which the bogus
sections are generated. I seem to recall we already *have* them for some
reason, and it's caused by some whitespace issues (in some cases), but
writing tests or proving it doesn't cause a hypothetical future breakage is
just not something I know how to write a commit message for.

So, time to detour into QAPIDoc parsing.

--js

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