Re: [PATCH 04/14] qapi/parser: enforce doc block section ordering

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On Tue, Sep 29, 2026, 8:36 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Enforce the relative ordering of "Intro", "Members", "Features",
>> > "Errors" and "Returns" sections.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  docs/devel/qapi-code-gen.rst | 19 +++++++++++------
>> >  scripts/qapi/parser.py       | 41 ++++++++++++++++++++++++++++++------
>> >  2 files changed, 48 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
>> > index 3ac1c2dae18..29e9c5a3da6 100644
>> > --- a/docs/devel/qapi-code-gen.rst
>> > +++ b/docs/devel/qapi-code-gen.rst
>> > @@ -985,12 +985,19 @@ When documentation is required (see pragma_
>> 'doc-required'), every
>> >  definition must have documentation.
>> >
>> >  Definition documentation starts with a description naming the
>> > -definition with an optional indented overview (the "intro"),
>> > -:ref:`descriptions <doc-descrip>` of each argument (for commands and
>> > -events), member (for structs and unions), branch (for alternates), or
>> > -value (for enums), :ref:`feature descriptions <doc-feat>` of each
>> > -feature (if any), and finally optional :ref:`tagged sections
>> > -<doc-tagged>`.
>> > +definition with an optional indented overview (the "intro").
>> > +
>> > +The introduction is followed by :ref:`descriptions <doc-descrip>` of
>> > +each argument (for commands and events), member (for structs and
>> > +unions), branch (for alternates), or value (for enums).
>> > +
>> > +:ref:`Tagged sections <doc-tagged>` follow in order: Returns (for
>> > +commands), Errors (for commands), and lastly :ref:`feature
>> > +descriptions <doc-feat>` if applicable.
>>
>> *Some* of the tagged sections do.
>>
>> Does this document still need the concept "tagged section"?
>>
>
> I doubt it does, beyond an explanation of common syntax. Happy to hear
> suggestions...

I'll keep it in mind when I review PATCH 02.

> We could also stand to change some of the method names like "ensure
> untagged section" could become "ensure details", and the like.

Fair!

> tagged vs untagged has become increasingly non-meaningful.

True.

>> > +
>> > +Definition documentation concludes with rST-formatted detail
>> > +paragraphs (including examples, notes, &c), and finally an optional
>> > +"Since" tagged section at the very end.
>> >
>> >  .. _doc-descrip:
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index fe284a3f866..94a60a23482 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -545,6 +545,28 @@ def get_doc(self) -> 'QAPIDoc':
>> >              doc.all_sections.append(QAPIDoc.Section(info,
>> QAPIDoc.Kind.INTRO))
>> >              line = self.get_doc_indented(doc)
>> >              no_more_args = False
>> > +            last_ordered_section = QAPIDoc.Kind.INTRO
>> > +
>> > +            def _tag_check(this: Union['QAPIDoc.Kind', str]) -> None:
>> > +                nonlocal last_ordered_section
>> > +                if isinstance(this, str):
>> > +                    this = QAPIDoc.Kind.from_string(this)
>>
>> Simpler, I think: require a .Kind argument, and do the conversion from
>> string in the one caller that needs it.
>>
>
> You're right.
>
>
>> > +
>> > +                if this in (QAPIDoc.Kind.TODO, QAPIDoc.Kind.SINCE,
>> > +                            QAPIDoc.Kind.DETAILS):
>> > +                    return
>> > +
>> > +                if this.value < last_ordered_section.value:
>> > +                    raise QAPIParseError(
>> > +                        self,
>> > +                        f"'{this}' section cannot appear after "
>> > +                        f"'{last_ordered_section}' section, please
>> re-order "
>> > +                        "the sections and adjust phrasing as necessary
>> to "
>> > +                        "ensure consistent documentation flow between
>> the "
>> > +                        "source code and the rendered HTML manual"
>> > +                    )
>>
>> That's a loooong error message.  It actually consists of the error
>> proper and advice on how to correct it.  Please use
>>
>>     "<error>\n<advice>"
>>
>> like we do in expr.py's check_keys(), parser.py's
>> ._validate_doc_line_format(), and schema.py's ._def_definition().  Yes,
>> the embedded newline is a bit of a hack.
>>
>
> ACK.
>
>
>> > +
>> > +                last_ordered_section = this
>> >
>> >              while line is not None:
>> >                  # Blank lines
>> > @@ -558,6 +580,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >                      if doc.has_features:
>> >                          raise QAPIParseError(
>> >                              self, "duplicated 'Features:' line")
>> > +                    _tag_check(QAPIDoc.Kind.FEATURE)
>> >                      self.accept(False)
>> >                      line = self.get_doc_line()
>> >                      while line == '':
>> > @@ -581,6 +604,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >                              self,
>> >                              "description of '@%s:' follows a section"
>> >                              % match.group(1))
>> > +                    _tag_check(QAPIDoc.Kind.MEMBER)
>> >                      while (line is not None
>> >                             and (match :=
>> self._match_at_name_colon(line))):
>> >                          doc.new_argument(self.info, match.group(1))
>> > @@ -621,6 +645,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >                          )
>> >                          raise QAPIParseError(self, emsg)
>> >
>> > +                    _tag_check(match.group(1))
>>
>> This is the caller that needs it.
>>
>
> Yep.
>
>
>> >                      doc.new_tagged_section(
>> >                          self.info,
>> >                          QAPIDoc.Kind.from_string(match.group(1))
>> > @@ -632,6 +657,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >                      no_more_args = True
>> >                  else:
>> >                      # plain paragraph(s)
>> > +                    _tag_check(QAPIDoc.Kind.DETAILS)
>> >                      doc.ensure_untagged_section(self.info)
>> >                      doc.append_line(line)
>> >                      line = self.get_doc_paragraph(doc)
>> > @@ -674,14 +700,17 @@ class QAPIDoc:
>> >      """
>> >
>> >      class Kind(enum.Enum):
>> > -        DETAILS = 0
>> > +        # The order here is the order in which sections must appear in
>> > +        # source code; with the exception of 'TODO' which may appear
>> > +        # anywhere but is treated as a comment.
>> > +        INTRO = 0
>> >          MEMBER = 1
>> > -        FEATURE = 2
>> > -        RETURNS = 3
>> > -        ERRORS = 4
>> > -        SINCE = 5
>> > +        RETURNS = 2
>> > +        ERRORS = 3
>> > +        FEATURE = 4
>> > +        DETAILS = 5
>> >          TODO = 6
>> > -        INTRO = 7
>> > +        SINCE = 7
>> >
>> >          @staticmethod
>> >          def from_string(kind: str) -> 'QAPIDoc.Kind':
>>
>> Missing: negative test for the new error.
>>
>
> Yep. If you are overall pleased with this change enough (pending nits),
> I'll add the tests now.

I am!