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

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> It simplifies the typing to say that _section is always a
> QAPIDoc.Section().

If you say so....

> 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?

>
> 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.

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?

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__().

> @@ -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.