Re: [PATCH v2 07/13] qapi/docs: add "Intro" section parsing

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> On Mon, May 4, 2026 at 7:57 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Add parsing for explicit Intro section syntax.
>> >
>> > A side effect of this patch is that we will always create an empty Intro
>> > section, similar to how we used to have an empty Plain section. The
>> > tests are adjusted accordingly, rendered document output does not change
>> > at all.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  docs/devel/qapi-code-gen.rst            | 17 +++++----
>> >  scripts/qapi/parser.py                  | 47 ++++++++++++++++++-------
>> >  tests/qapi-schema/doc-good.out          | 18 ++++++++++
>> >  tests/qapi-schema/doc-missing-colon.err |  2 +-
>> >  4 files changed, 64 insertions(+), 20 deletions(-)
>> >
>> > diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
>> > index 3a632b4a648..a8175934d52 100644
>> > --- a/docs/devel/qapi-code-gen.rst
>> > +++ b/docs/devel/qapi-code-gen.rst
>> > @@ -862,7 +862,7 @@ documentation comment.
>> >  If the documentation comment starts like ::
>> >
>> >      ##
>> > -    # @SYMBOL:
>> > +    # @SYMBOL: [...]
>> >
>> >  it documents the definition of SYMBOL, else it's free-form
>> >  documentation.
>> > @@ -984,11 +984,11 @@ definition it documents.
>> >  When documentation is required (see pragma_ 'doc-required'), every
>> >  definition must have documentation.
>> >
>> > -Definition documentation starts with a line naming the definition,
>> > -followed by an optional overview, a description of each argument (for
>> > -commands and events), member (for structs and unions), branch (for
>> > -alternates), or value (for enums), a description of each feature (if
>> > -any), and finally optional tagged sections.
>> > +Definition documentation starts with a description naming the definition
>> > +with an optional overview, a description of each argument (for commands
>> > +and events), member (for structs and unions), branch (for alternates),
>> > +or value (for enums), a description of each feature (if any), and
>> > +finally optional tagged sections.
>> >
>> >  Descriptions start with '\@name:'.  The description text must be
>> >  indented like this::
>> > @@ -996,6 +996,11 @@ indented like this::
>> >   # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed
>> >   #     do eiusmod tempor incididunt ut labore et dolore magna aliqua.
>> >
>> > +Definition descriptions are special: the optional introductory overview
>> > +describing the definition will not be inlined when referenced by other
>> > +definitions (such as when using 'base' to include members from another
>> > +definition), while other descriptions and tagged sections will be.
>> > +
>>
>> Isn't this premature?  We're not inlining anything just yet.
>
> It is, but I felt compelled to justify it holistically instead of
> playing "dumb" with the documentation and pretending it didn't exist.
> Feel free to suggest better phrasing/rationale here...

I think we have two reasons for clearly delineated "intro":

1. The existing one: we need to know where to insert synthesized stuff.
   Unclear when there is only a "plain" section.  We abuse TODO to mark
   the spot then.  Your "qapi: enforce section ordering" series makes
   this worse.

   We synthesize missing member documentation, references to base and
   branch types, and omitted "Returns".  docs/devel/qapi-code-gen.rst
   mostly fails to document this, but that's not this patch's fault.

2. The future one: we need to know what not to inline.  This is actually
   exactly the text up to the point where we would insert synthesized
   stuff if we had any.  So any solution for 1. will also solve 2.

I think 1. is sufficient rationale for this patch.

In the commit message, you can mention both if you like.  Just be clear
that 2. is about the future.

If you want rationale in qapi-code-gen.rst: best to stick to 1 there to
avoid confusion.

>> >  .. FIXME The parser accepts these things in almost any order.
>> >
>> >  .. FIXME union branches should be described, too.
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 6612f471bb8..c23fd26aaa7 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -24,6 +24,7 @@
>> >      Match,
>> >      Optional,
>> >      Set,
>> > +    Tuple,
>> >      Union,
>> >  )
>> >
>> > @@ -524,6 +525,30 @@ def get_doc_paragraph(self, doc: 'QAPIDoc') -> Optional[str]:
>> >                  return line
>> >              doc.append_line(line)
>> >
>> > +    def _get_doc_intro(
>> > +        self,
>> > +        line: str,
>> > +        info: QAPISourceInfo
>> > +    ) -> Tuple['QAPIDoc', Optional[str]]:
>> > +        match = self._match_at_name_colon(line)
>> > +        if not match:
>> > +            raise QAPIParseError(self, "@name must end with ':'")
>> > +
>> > +        # Invalid names are not checked here, but the name
>> > +        # provided *must* match the following definition,
>> > +        # which *is* validated in expr.py.
>> > +        symbol = match.group(1)
>> > +        if not symbol:
>> > +            raise QAPIParseError(self, "name required after '@'")
>> > +        doc = QAPIDoc(info, symbol)
>> > +
>>
>> Drop the blank line.  For what it's worth, you do it already in PATCH
>> 13.
>>
>> > +        doc.ensure_untagged_section(info, QAPIDoc.Kind.INTRO)
>>
>> Hmm.
>>
>> .ensure_untagged_section() extends the current section if we have one
>> and it is of kind .INTRO, else creates a new one of kind .INTRO.
>>
>> But there is no current section here!  Why not simply create one?
>
> Leftover. I originally wanted to create this conditionally, but ran
> into troubles doing so and gave up and made it unconditional, but
> still using the conditional constructor.
>
>>
>> > +        text = line[match.end():]
>> > +        if text:
>> > +            doc.append_line(text)
>> > +
>> > +        return doc, self.get_doc_indented(doc)
>> > +
>> >      def get_doc(self) -> 'QAPIDoc':
>> >          if self.val != '##':
>> >              raise QAPIParseError(
>> > @@ -532,18 +557,9 @@ def get_doc(self) -> 'QAPIDoc':
>> >          self.accept(False)
>> >          line = self.get_doc_line()
>> >          if line is not None and line.startswith('@'):
>> > +
>>
>> Drop the blank line, please.
>>
>> >              # Definition documentation
>> > -            if not line.endswith(':'):
>> > -                raise QAPIParseError(self, "line should end with ':'")
>> > -            # Invalid names are not checked here, but the name
>> > -            # provided *must* match the following definition,
>> > -            # which *is* validated in expr.py.
>> > -            symbol = line[1:-1]
>> > -            if not symbol:
>> > -                raise QAPIParseError(self, "name required after '@'")
>> > -            doc = QAPIDoc(info, symbol)
>> > -            self.accept(False)
>> > -            line = self.get_doc_line()
>> > +            doc, line = self._get_doc_intro(line, info)
>> >              no_more_args = False
>> >
>> >              while line is not None:
>> > @@ -751,8 +767,13 @@ def end(self) -> None:
>> >                  raise QAPISemError(
>> >                      section.info, "text required after '%s:'" % section.kind)
>> >
>> > -    def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>> > -        kind = QAPIDoc.Kind.PLAIN
>> > +    def ensure_untagged_section(
>> > +        self,
>> > +        info: QAPISourceInfo,
>> > +        kind: Optional['QAPIDoc.Kind'] = None,
>>
>> Why not = QAPIDoc.Kind.PLAIN here, so ...
>>
>> > +    ) -> None:
>> > +        if kind is None:
>> > +            kind = QAPIDoc.Kind.PLAIN
>>
>> ... you don't need this?
>
> Because at the time this function definition is parsed, that
> definition does not exist yet :)

Are you telling me that kind = QAPIDoc.Kind.Plain works as assignment in
the function body, but not as default in the argument list?

Aisde: I'm quite unhappy about the entanglement between QAPISchemaParser
and QAPIDoc.  The latter should be in its own .py.

>
>>
>> >
>> >          if self.all_sections and self.all_sections[-1].kind == kind:
>> >              # extend current section
>>
>> Hmm.
>>
>> QAPIDoc.ensure_untagged_section() extends the current section if we have
>> one and it's "untagged", else creates a new "untagged" section.
>>
>> In the beginning, there was just one kind of "untagged" section.  Its
>> encoding eventually became "Section.kind is QAPIDoc.Kind.PLAIN".  So
>> what the function does was clear enough.
>>
>> This patch adds a second kind: QAPIDoc.Kind.INTRO.
>> .ensure_untagged_section() now takes a kind argument.  It ensures *that*
>> kind of section.
>>
>> I think the function name becomes misleading.  It can now ensure *any*
>> kind of section, not just "untagged".
>>
>> Rename to ensure_section() and make the @kind argument mandatory?
>
> Sure. I agree "untagged" has kind of long since lost its meaning, and
> I'm kind of mumbly-brained about what these terms mean anyway, so I
> was somewhat hesitant to rename it and wanted to see if I could get
> away with not sweeping that corner of the room.
>
> No such luck. Rats.

No rest for the wicked!  ;)

>> > diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
>> > index 6fcc8175cfe..bc89853765f 100644
>> > --- a/tests/qapi-schema/doc-good.out
>> > +++ b/tests/qapi-schema/doc-good.out
>> > @@ -106,6 +106,8 @@ Examples:
>> >  - *verbatim*
>> >  - {braces}
>> >  doc symbol=Enum
>> > +    section=Intro
>> > +
>> >      arg=one
>> >  The _one_ {and only}, description on the same line
>> >      arg=two
>> > @@ -117,10 +119,14 @@ a member feature
>> >      section=Plain
>> >  @two is undocumented
>> >  doc symbol=Base
>> > +    section=Intro
>> > +
>> >      arg=base1
>> >   description starts on a new line,
>> >   minimally indented
>> >  doc symbol=Variant1
>> > +    section=Intro
>> > +
>> >      section=Plain
>> >  A paragraph
>> >
>> > @@ -134,10 +140,16 @@ a feature
>> >      feature=member-feat
>> >  a member feature
>> >  doc symbol=Variant2
>> > +    section=Intro
>> > +
>> >  doc symbol=Object
>> > +    section=Intro
>> > +
>> >      feature=union-feat1
>> >  a feature
>> >  doc symbol=Alternate
>> > +    section=Intro
>> > +
>> >      arg=i
>> >  description starts on the same line
>> >      remainder indented the same
>> > @@ -151,6 +163,8 @@ doc freeform
>> >  Another subsection
>> >  ==================
>> >  doc symbol=cmd
>> > +    section=Intro
>> > +
>> >      arg=arg1
>> >      description starts on a new line,
>> >      indented
>> > @@ -198,6 +212,8 @@ Note::
>> >      section=Since
>> >  2.10
>> >  doc symbol=cmd-boxed
>> > +    section=Intro
>> > +
>> >      section=Plain
>> >  If you're bored enough to read this, go see a video of boxed cats
>> >      feature=cmd-feat1
>> > @@ -211,5 +227,7 @@ another feature
>> >
>> >     <- ... has no title ...
>> >  doc symbol=EVT_BOXED
>> > +    section=Intro
>> > +
>> >      feature=feat3
>> >  a feature
>> > diff --git a/tests/qapi-schema/doc-missing-colon.err b/tests/qapi-schema/doc-missing-colon.err
>> > index cbcea007153..bd5862b30f3 100644
>> > --- a/tests/qapi-schema/doc-missing-colon.err
>> > +++ b/tests/qapi-schema/doc-missing-colon.err
>> > @@ -1 +1 @@
>> > -doc-missing-colon.json:4:1: line should end with ':'
>> > +doc-missing-colon.json:4:1: @name must end with ':'
>>
>> Let's not change this error message in this patch.  For what it's worth,
>> PATCH 13 changes it right back.
>
> That one is an RFC! This patch was my original proposal and 13 is
> "Here's the what-if you asked for."
>
> I will definitely merge the PATCH 13 changes to their proper positions
> if we conclude that's the direction we are going in, but that was not
> clear to me when I sent this series. I wanted to highlight to you that
> making the newline "optional" was actually proactive code changes and
> not that the reverse was true; i.e. we already forbid the @name: line
> from having anything else present on it.
>
> So before getting to your review of later patches, it still isn't
> clear to me if you want patch 13 or not, so I can't squash things for
> consistency yet as we haven't made a choice.

Understood.

You offer two styles for definition descriptions.  Let me show them both
together with a member description:

    ##
    # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed
    #     do eiusmod tempor incididunt ut labore et dolore magna aliqua.
    #
    # @member1: Ut enim ad minim veniam, quis nostrud exercitation ullamco
    #     laboris nisi ut aliquip ex ea commodo consequat.

or

    ##
    # @name:
    #     Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed
    #     do eiusmod tempor incididunt ut labore et dolore magna aliqua.
    #
    # @member1: Ut enim ad minim veniam, quis nostrud exercitation ullamco
    #     laboris nisi ut aliquip ex ea commodo consequat.

The latter makes the defined name (here: "name") stand out a bit more.

It also makes the definition description (@name: ...) differ from member
descriptions (@member: ...).  This could be viewed as a feature or as a
pointless complication.

I lean towards feature.  The lack of visual distinction between
definition description and member descriptions bothers me more in the
first style than in the second style.

I think this is something we should discuss in a 1:1.