Here's the weird bit. QAPIDoc generally expects -- virtually everywhere
-- that it will always have a current section. The sole exception to
this is in the case that end_comment() is called, which leaves us with
*no* section. However, in this case, we also don't expect to actually
ever mutate the comment contents ever again.

NullSection is just a Null-object that allows us to maintain the
invariant that we *always* have a current section, enforced by static
typing -- allowing us to type that field as QAPIDoc.Section instead of
the more ambiguous Optional[QAPIDoc.Section].

end_section is renamed to switch_section and now accepts as an argument
the new section to activate, clarifying that no callers ever just
unilaterally end a section; they only do so when starting a new section.

Signed-off-by: John Snow <jsnow@redhat.com>

---

For my money: Optional types can be a nuisance because an unfamiliar
reader may wonder in what circumstances the field may be unset. This
makes the condition quite a bit more explicit and statically provable.

Doing it in this way (and not by creating a dummy section) will also
continue to reject (rather noisily) any erroneous attempts to append
additional lines after end_comment() has been called.

Also, this section isn't indexed into .sections[] and isn't really
visible in any way to external users of the class, so it seems like a
harmless and low-cost way to formalize the "life cycle" of a QAPIDoc
parser.

Clean and clear as I can make it, in as few lines as I could muster.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 1fdc5bc7056..123fc2f099c 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0):
         def connect(self, member):
             self.member = member
 
+    class NullSection(Section):
+        """
+        Empty section that signifies the end of a doc block.
+        """
+        def append(self, line):
+            assert False, "BUG: Text appended after end_comment() called."
+
     def __init__(self, parser, info):
         # self._parser is used to report errors with QAPIParseError.  The
         # resulting error position depends on the state of the parser.
@@ -525,7 +532,7 @@ def append(self, line):
         self._append_line(line)
 
     def end_comment(self):
-        self._end_section()
+        self._switch_section(QAPIDoc.NullSection(self._parser))
 
     @staticmethod
     def _is_section_tag(name):
@@ -702,9 +709,9 @@ def _start_symbol_section(self, symbols_dict, name, indent):
             raise QAPIParseError(self._parser,
                                  "'%s' parameter name duplicated" % name)
         assert not self.sections
-        self._end_section()
-        self._section = QAPIDoc.ArgSection(self._parser, name, indent)
-        symbols_dict[name] = self._section
+        new_section = QAPIDoc.ArgSection(self._parser, name, indent)
+        self._switch_section(new_section)
+        symbols_dict[name] = new_section
 
     def _start_args_section(self, name, indent):
         self._start_symbol_section(self.args, name, indent)
@@ -716,13 +723,11 @@ def _start_section(self, name=None, indent=0):
         if name in ('Returns', 'Since') and self.has_section(name):
             raise QAPIParseError(self._parser,
                                  "duplicated '%s' section" % name)
-        self._end_section()
-        self._section = QAPIDoc.Section(self._parser, name, indent)
-        self.sections.append(self._section)
-
-    def _end_section(self):
-        assert self._section is not None
+        new_section = QAPIDoc.Section(self._parser, name, indent)
+        self._switch_section(new_section)
+        self.sections.append(new_section)
 
+    def _switch_section(self, new_section):
         text = self._section.text = self._section.text.strip()
 
         # Only the 'body' section is allowed to have an empty body.
@@ -735,7 +740,7 @@ def _end_section(self):
                 self._parser,
                 "empty doc section '%s'" % self._section.name)
 
-        self._section = None
+        self._section = new_section
 
     def _append_freeform(self, line):
         match = re.match(r'(@\S+:)', line)
-- 
2.31.1

John Snow <jsnow@redhat.com> writes:

> Here's the weird bit. QAPIDoc generally expects -- virtually everywhere
> -- that it will always have a current section. The sole exception to
> this is in the case that end_comment() is called, which leaves us with
> *no* section. However, in this case, we also don't expect to actually
> ever mutate the comment contents ever again.
>
> NullSection is just a Null-object that allows us to maintain the
> invariant that we *always* have a current section, enforced by static
> typing -- allowing us to type that field as QAPIDoc.Section instead of
> the more ambiguous Optional[QAPIDoc.Section].
>
> end_section is renamed to switch_section and now accepts as an argument
> the new section to activate, clarifying that no callers ever just
> unilaterally end a section; they only do so when starting a new section.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> For my money: Optional types can be a nuisance because an unfamiliar
> reader may wonder in what circumstances the field may be unset. This
> makes the condition quite a bit more explicit and statically provable.
>
> Doing it in this way (and not by creating a dummy section) will also
> continue to reject (rather noisily) any erroneous attempts to append
> additional lines after end_comment() has been called.
>
> Also, this section isn't indexed into .sections[] and isn't really
> visible in any way to external users of the class, so it seems like a
> harmless and low-cost way to formalize the "life cycle" of a QAPIDoc
> parser.
>
> Clean and clear as I can make it, in as few lines as I could muster.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1fdc5bc7056..123fc2f099c 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0):
>          def connect(self, member):
>              self.member = member
>  
> +    class NullSection(Section):
> +        """
> +        Empty section that signifies the end of a doc block.

What about "Dummy section for use at the end of a doc block"?

> +        """
> +        def append(self, line):
> +            assert False, "BUG: Text appended after end_comment() called."

How can a failing assertion *not* be a bug?

> +
>      def __init__(self, parser, info):
>          # self._parser is used to report errors with QAPIParseError.  The
>          # resulting error position depends on the state of the parser.
> @@ -525,7 +532,7 @@ def append(self, line):
>          self._append_line(line)
>  
>      def end_comment(self):
> -        self._end_section()
> +        self._switch_section(QAPIDoc.NullSection(self._parser))
>  
>      @staticmethod
>      def _is_section_tag(name):
> @@ -702,9 +709,9 @@ def _start_symbol_section(self, symbols_dict, name, indent):
>              raise QAPIParseError(self._parser,
>                                   "'%s' parameter name duplicated" % name)
>          assert not self.sections
> -        self._end_section()
> -        self._section = QAPIDoc.ArgSection(self._parser, name, indent)
> -        symbols_dict[name] = self._section
> +        new_section = QAPIDoc.ArgSection(self._parser, name, indent)
> +        self._switch_section(new_section)
> +        symbols_dict[name] = new_section
>  
>      def _start_args_section(self, name, indent):
>          self._start_symbol_section(self.args, name, indent)
> @@ -716,13 +723,11 @@ def _start_section(self, name=None, indent=0):
>          if name in ('Returns', 'Since') and self.has_section(name):
>              raise QAPIParseError(self._parser,
>                                   "duplicated '%s' section" % name)
> -        self._end_section()
> -        self._section = QAPIDoc.Section(self._parser, name, indent)
> -        self.sections.append(self._section)
> -
> -    def _end_section(self):
> -        assert self._section is not None
> +        new_section = QAPIDoc.Section(self._parser, name, indent)
> +        self._switch_section(new_section)
> +        self.sections.append(new_section)
>  
> +    def _switch_section(self, new_section):
>          text = self._section.text = self._section.text.strip()
>  
>          # Only the 'body' section is allowed to have an empty body.
> @@ -735,7 +740,7 @@ def _end_section(self):
>                  self._parser,
>                  "empty doc section '%s'" % self._section.name)
>  
> -        self._section = None
> +        self._section = new_section
>  
>      def _append_freeform(self, line):
>          match = re.match(r'(@\S+:)', line)

Feels clearer, thanks!

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

> John Snow <jsnow@redhat.com> writes:
>
> > Here's the weird bit. QAPIDoc generally expects -- virtually everywhere
> > -- that it will always have a current section. The sole exception to
> > this is in the case that end_comment() is called, which leaves us with
> > *no* section. However, in this case, we also don't expect to actually
> > ever mutate the comment contents ever again.
> >
> > NullSection is just a Null-object that allows us to maintain the
> > invariant that we *always* have a current section, enforced by static
> > typing -- allowing us to type that field as QAPIDoc.Section instead of
> > the more ambiguous Optional[QAPIDoc.Section].
> >
> > end_section is renamed to switch_section and now accepts as an argument
> > the new section to activate, clarifying that no callers ever just
> > unilaterally end a section; they only do so when starting a new section.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > For my money: Optional types can be a nuisance because an unfamiliar
> > reader may wonder in what circumstances the field may be unset. This
> > makes the condition quite a bit more explicit and statically provable.
> >
> > Doing it in this way (and not by creating a dummy section) will also
>

("And not by creating a [mutable] dummy section" ... but this wasn't
destined for the git log anyway.)


> > continue to reject (rather noisily) any erroneous attempts to append
> > additional lines after end_comment() has been called.
> >
> > Also, this section isn't indexed into .sections[] and isn't really
> > visible in any way to external users of the class, so it seems like a
> > harmless and low-cost way to formalize the "life cycle" of a QAPIDoc
> > parser.
> >
> > Clean and clear as I can make it, in as few lines as I could muster.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/parser.py | 27 ++++++++++++++++-----------
> >  1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 1fdc5bc7056..123fc2f099c 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0):
> >          def connect(self, member):
> >              self.member = member
> >
> > +    class NullSection(Section):
> > +        """
> > +        Empty section that signifies the end of a doc block.
>
> What about "Dummy section for use at the end of a doc block"?
>
>
Sure. "Immutable dummy section for use at the end of a doc block."


> > +        """
> > +        def append(self, line):
> > +            assert False, "BUG: Text appended after end_comment()
> called."
>
> How can a failing assertion *not* be a bug?
>
>
Haha. I'll drop the prefix. (I'll update my branch with these tiny edits
and you can decide what you'd like to do with that.)


> > +
> >      def __init__(self, parser, info):
> >          # self._parser is used to report errors with QAPIParseError.
> The
> >          # resulting error position depends on the state of the parser.
> > @@ -525,7 +532,7 @@ def append(self, line):
> >          self._append_line(line)
> >
> >      def end_comment(self):
> > -        self._end_section()
> > +        self._switch_section(QAPIDoc.NullSection(self._parser))
> >
> >      @staticmethod
> >      def _is_section_tag(name):
> > @@ -702,9 +709,9 @@ def _start_symbol_section(self, symbols_dict, name,
> indent):
> >              raise QAPIParseError(self._parser,
> >                                   "'%s' parameter name duplicated" %
> name)
> >          assert not self.sections
> > -        self._end_section()
> > -        self._section = QAPIDoc.ArgSection(self._parser, name, indent)
> > -        symbols_dict[name] = self._section
> > +        new_section = QAPIDoc.ArgSection(self._parser, name, indent)
> > +        self._switch_section(new_section)
> > +        symbols_dict[name] = new_section
> >
> >      def _start_args_section(self, name, indent):
> >          self._start_symbol_section(self.args, name, indent)
> > @@ -716,13 +723,11 @@ def _start_section(self, name=None, indent=0):
> >          if name in ('Returns', 'Since') and self.has_section(name):
> >              raise QAPIParseError(self._parser,
> >                                   "duplicated '%s' section" % name)
> > -        self._end_section()
> > -        self._section = QAPIDoc.Section(self._parser, name, indent)
> > -        self.sections.append(self._section)
> > -
> > -    def _end_section(self):
> > -        assert self._section is not None
> > +        new_section = QAPIDoc.Section(self._parser, name, indent)
> > +        self._switch_section(new_section)
> > +        self.sections.append(new_section)
> >
> > +    def _switch_section(self, new_section):
> >          text = self._section.text = self._section.text.strip()
> >
> >          # Only the 'body' section is allowed to have an empty body.
> > @@ -735,7 +740,7 @@ def _end_section(self):
> >                  self._parser,
> >                  "empty doc section '%s'" % self._section.name)
> >
> > -        self._section = None
> > +        self._section = new_section
> >
> >      def _append_freeform(self, line):
> >          match = re.match(r'(@\S+:)', line)
>
> Feels clearer, thanks!
>
>
Relieved you think so O:-)

--js