[PATCH 00/23] docs: add basic sphinx-domain rST generator to qapidoc

[PATCH 00/23] docs: add basic sphinx-domain rST generator to qapidoc

[John Snow]

based-on: https://patchew.org/QEMU/20241213011307.2942030-1-jsnow@redhat.com/

Hi!

This series is a very, very barebones implementation for the new QAPI
doc generator. It does not have many features that I presented on at KVM
Forum; the point of this patch set is instead to present a stripped down
basis for ongoing work so we can discuss on-list with full context of
the code available to do so.

The documentation this series generates is *not suitable* for replacing
the current document generator, it has a few glaring omissions - on
purpose - those features have been factored out intentionally so they
can be reviewed with fuller context and more careful review.

What this series does:

- Adds the new "Transmogrifier" rST generator to qapidoc.py, which
  generates an in-memory rST document using qapi-domain directives.
- Adds a test document that showcases this new transmogrifier.

What this series very notably does not do (yet):

- "ifcond" data for anything other than top-level entities is not
  considered or rendered. This means "if" statements for features and
  members are entirely absent.

- The inliner is not present at all. This series renders only
  documentation exactly as it is exists in the source files.

- *branches* are themselves not considered at all; they're skipped
   entirely for now. They will be included alongside the inliner in
   either a subsequent series or a followup to this series.

- Undocumented members and return statements are not autogenerated.

- Pseudofeatures (Things like allow-oob) are not generated as documented
  features.

- Documentation culling: all entities are documented whether or not
  they're relevant to the wire format.

My goal in doing it this way is to save the "fancy" features for later
so we can focus on reviewing and tightening up the core functionality of
the transmogrifier. Once we're on steadier ground, I will re-add the
fanciful features while adjusting the qapi-domain mechanisms. Once
everything looks "roughly right, give or take some minor nits", I will
switch back to the qapi-domain series itself for review before we merge
everything together.

John Snow (23):
  docs/qapidoc: support header-less freeform sections
  qapi/parser: adjust info location for doc body section
  docs/qapidoc: remove example section support
  qapi: expand tags to all doc sections
  qapi/schema: add __repr__ to QAPIDoc.Section
  docs/qapidoc: add transmogrifier stub
  docs/qapidoc: add transmogrifier class stub
  docs/qapidoc: add visit_module() method
  qapi/source: allow multi-line QAPISourceInfo advancing
  docs/qapidoc: add visit_freeform() method
  docs/qapidoc: add preamble() method
  docs/qapidoc: add visit_paragraph() method
  docs/qapidoc: add visit_errors() method
  docs/qapidoc: add format_type() method
  docs/qapidoc: add add_field() and generate_field() helper methods
  docs/qapidoc: add visit_feature() method
  docs/qapidoc: record current documented entity in transmogrifier
  docs/qapidoc: add visit_returns() method
  docs/qapidoc: add visit_member() method
  docs/qapidoc: add visit_sections() method
  docs/qapidoc: add visit_entity()
  docs/qapidoc: implement transmogrify() method
  docs/qapidoc: add transmogrifier test document

 docs/index.rst         |   1 +
 docs/qapi/index.rst    |  53 ++++++
 docs/sphinx/qapidoc.py | 419 ++++++++++++++++++++++++++++++++++++++---
 scripts/qapi/parser.py |  97 +++++++---
 scripts/qapi/source.py |   4 +-
 5 files changed, 524 insertions(+), 50 deletions(-)
 create mode 100644 docs/qapi/index.rst

-- 
2.47.0

[PATCH 01/23] docs/qapidoc: support header-less freeform sections

[John Snow]

The code as written can't handle if a header isn't found, because `node`
will be uninitialized. If we don't have a section title, create a
generic block to insert text into instead.

This patch removes a lingering pylint warning in the QAPIDoc implementation
that prevents getting a clean baseline to use for forthcoming
additions.

I am not attempting to *fully* clean up the existing QAPIDoc
implementation in pylint because I intend to delete it anyway; this
patch merely accomplishes a baseline under a specific pylint
configuration:

PYTHONPATH=../../scripts/ pylint --disable=fixme,too-many-lines,\
    consider-using-f-string,missing-docstring,unused-argument,\
    too-many-arguments,too-many-positional-arguments,\
    too-many-public-methods \
    qapidoc.py

This at least ensures there aren't regressions outside of these general
warnings in the new qapidoc.py code to be committed.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 5f96b46270b..5a4d7388b29 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -421,6 +421,8 @@ def freeform(self, doc):
             node = self._start_new_heading(heading, len(leader))
             if text == '':
                 return
+        else:
+            node = nodes.container()
 
         self._parse_text_into_node(text, node)
         self._cur_doc = None
-- 
2.47.0

Re: [PATCH 01/23] docs/qapidoc: support header-less freeform sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The code as written can't handle if a header isn't found, because `node`
> will be uninitialized.

Yes, we initialize @node only if we have a heading.

Made me wonder what happens when we don't.  So I deleted the = from the
"# = Subsection" line in doc-good.json, and got:

    Exception occurred:
      File "/work/armbru/qemu/docs/sphinx/qapidoc.py", line 425, in freeform
        self._parse_text_into_node(text, node)
                                         ^^^^
    UnboundLocalError: cannot access local variable 'node' where it is not associated with a value

So you're fixing a crash bug, but that's perhaps less than clear from
the commit message.

>                        If we don't have a section title, create a
> generic block to insert text into instead.
>
> This patch removes a lingering pylint warning in the QAPIDoc implementation

Can you show me the warning?  My pylint doesn't...

> that prevents getting a clean baseline to use for forthcoming
> additions.
>
> I am not attempting to *fully* clean up the existing QAPIDoc
> implementation in pylint because I intend to delete it anyway; this
> patch merely accomplishes a baseline under a specific pylint
> configuration:
>
> PYTHONPATH=../../scripts/ pylint --disable=fixme,too-many-lines,\
>     consider-using-f-string,missing-docstring,unused-argument,\
>     too-many-arguments,too-many-positional-arguments,\
>     too-many-public-methods \
>     qapidoc.py

What version of pylint?  Mine chokes on too-many-positional-arguments.

> This at least ensures there aren't regressions outside of these general
> warnings in the new qapidoc.py code to be committed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 5f96b46270b..5a4d7388b29 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -421,6 +421,8 @@ def freeform(self, doc):
>              node = self._start_new_heading(heading, len(leader))
>              if text == '':
>                  return
> +        else:
> +            node = nodes.container()
>  
>          self._parse_text_into_node(text, node)
>          self._cur_doc = None

Plausible enough (and I acked a similar fix previously, commit
2664f3176a8), but I'm a Sphinx ignoramus :)

Re: [PATCH 01/23] docs/qapidoc: support header-less freeform sections

[John Snow]

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

On Mon, Dec 16, 2024 at 8:15 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > The code as written can't handle if a header isn't found, because `node`
> > will be uninitialized.
>
> Yes, we initialize @node only if we have a heading.
>
> Made me wonder what happens when we don't.  So I deleted the = from the
> "# = Subsection" line in doc-good.json, and got:
>
>     Exception occurred:
>       File "/work/armbru/qemu/docs/sphinx/qapidoc.py", line 425, in
> freeform
>         self._parse_text_into_node(text, node)
>                                          ^^^^
>     UnboundLocalError: cannot access local variable 'node' where it is not
> associated with a value
>
> So you're fixing a crash bug, but that's perhaps less than clear from
> the commit message.
>
> >                        If we don't have a section title, create a
> > generic block to insert text into instead.
> >
> > This patch removes a lingering pylint warning in the QAPIDoc
> implementation
>
> Can you show me the warning?  My pylint doesn't...
>
> > that prevents getting a clean baseline to use for forthcoming
> > additions.
> >
> > I am not attempting to *fully* clean up the existing QAPIDoc
> > implementation in pylint because I intend to delete it anyway; this
> > patch merely accomplishes a baseline under a specific pylint
> > configuration:
> >
> > PYTHONPATH=../../scripts/ pylint --disable=fixme,too-many-lines,\
> >     consider-using-f-string,missing-docstring,unused-argument,\
> >     too-many-arguments,too-many-positional-arguments,\
> >     too-many-public-methods \
> >     qapidoc.py
>
> What version of pylint?  Mine chokes on too-many-positional-arguments.
>

3.3.1 here; if yours doesn't have that warning, there's no need to disable
it. just remove that flag from the CLI.

(I promise I do want to get this rigorously checked and automated, I'm
sorry it's taken so long to achieve.)


>
> > This at least ensures there aren't regressions outside of these general
> > warnings in the new qapidoc.py code to be committed.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 5f96b46270b..5a4d7388b29 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -421,6 +421,8 @@ def freeform(self, doc):
> >              node = self._start_new_heading(heading, len(leader))
> >              if text == '':
> >                  return
> > +        else:
> > +            node = nodes.container()
> >
> >          self._parse_text_into_node(text, node)
> >          self._cur_doc = None
>
> Plausible enough (and I acked a similar fix previously, commit
> 2664f3176a8), but I'm a Sphinx ignoramus :)
>
>

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

Re: [PATCH 01/23] docs/qapidoc: support header-less freeform sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Dec 16, 2024 at 8:15 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > The code as written can't handle if a header isn't found, because `node`
>> > will be uninitialized.
>>
>> Yes, we initialize @node only if we have a heading.
>>
>> Made me wonder what happens when we don't.  So I deleted the = from the
>> "# = Subsection" line in doc-good.json, and got:
>>
>>     Exception occurred:
>>       File "/work/armbru/qemu/docs/sphinx/qapidoc.py", line 425, in
>> freeform
>>         self._parse_text_into_node(text, node)
>>                                          ^^^^
>>     UnboundLocalError: cannot access local variable 'node' where it is not
>> associated with a value
>>
>> So you're fixing a crash bug, but that's perhaps less than clear from
>> the commit message.
>>
>> >                        If we don't have a section title, create a
>> > generic block to insert text into instead.
>> >
>> > This patch removes a lingering pylint warning in the QAPIDoc implementation
>>
>> Can you show me the warning?  My pylint doesn't...
>>
>> > that prevents getting a clean baseline to use for forthcoming
>> > additions.
>> >
>> > I am not attempting to *fully* clean up the existing QAPIDoc
>> > implementation in pylint because I intend to delete it anyway; this
>> > patch merely accomplishes a baseline under a specific pylint
>> > configuration:
>> >
>> > PYTHONPATH=../../scripts/ pylint --disable=fixme,too-many-lines,\
>> >     consider-using-f-string,missing-docstring,unused-argument,\
>> >     too-many-arguments,too-many-positional-arguments,\
>> >     too-many-public-methods \
>> >     qapidoc.py
>>
>> What version of pylint?  Mine chokes on too-many-positional-arguments.
>
> 3.3.1 here; if yours doesn't have that warning, there's no need to disable
> it. just remove that flag from the CLI.

I've since upgraded to  3.3.3, which doesn't choke.

> (I promise I do want to get this rigorously checked and automated, I'm
> sorry it's taken so long to achieve.)

[...]

[PATCH 02/23] qapi/parser: adjust info location for doc body section

[John Snow]

Instead of using the info object for the doc block as a whole (which
always points to the very first line of the block), update the info
pointer for each call to ensure_untagged_section when the existing
section is otherwise empty. This way, Sphinx error information will
match precisely to where the text actually starts.

For example, this patch will move the info pointer for the "Hello!"
untagged section ...

> ##       <-- from here ...
> # Hello! <-- ... to here.
> ##

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index adc85b5b394..36cb64a677a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -687,7 +687,11 @@ def end(self) -> None:
     def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
         if self.all_sections and not self.all_sections[-1].tag:
             # extend current section
-            self.all_sections[-1].text += '\n'
+            section = self.all_sections[-1]
+            if not section.text:
+                # Section is empty so far; update info to start *here*.
+                section.info = info
+            section.text += '\n'
             return
         # start new section
         section = self.Section(info)
-- 
2.47.0

[PATCH 03/23] docs/qapidoc: remove example section support

[John Snow]

Since 3c5f6114 we no longer actually generate Example sections, so this
support in qapidoc is now dead code.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 5a4d7388b29..61997fd21af 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -254,10 +254,6 @@ def _nodes_for_features(self, doc):
         section += dlnode
         return [section]
 
-    def _nodes_for_example(self, exampletext):
-        """Return list of doctree nodes for a code example snippet"""
-        return [nodes.literal_block(exampletext, exampletext)]
-
     def _nodes_for_sections(self, doc):
         """Return list of doctree nodes for additional sections"""
         nodelist = []
@@ -275,10 +271,7 @@ def _nodes_for_sections(self, doc):
                 continue
 
             snode = self._make_section(section.tag)
-            if section.tag.startswith('Example'):
-                snode += self._nodes_for_example(dedent(section.text))
-            else:
-                self._parse_text_into_node(dedent(section.text), snode)
+            self._parse_text_into_node(dedent(section.text), snode)
             nodelist.append(snode)
         return nodelist
 
-- 
2.47.0

Re: [PATCH 03/23] docs/qapidoc: remove example section support

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Since 3c5f6114 we no longer actually generate Example sections, so this

Suggest s/generate/support/

> support in qapidoc is now dead code.

Oversight, I guess.

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

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 03/23] docs/qapidoc: remove example section support

[John Snow]

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

On Wed, Dec 18, 2024, 7:27 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Since 3c5f6114 we no longer actually generate Example sections, so this
>
> Suggest s/generate/support/
>

I meant to say: because the parser doesn't support them, it no longer
creates such section objects, so there's no need to handle them.


> > support in qapidoc is now dead code.
>
> Oversight, I guess.
>

O:-)


> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
>

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

Re: [PATCH 03/23] docs/qapidoc: remove example section support

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Dec 18, 2024, 7:27 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Since 3c5f6114 we no longer actually generate Example sections, so this
>>
>> Suggest s/generate/support/
>>
>
> I meant to say: because the parser doesn't support them, it no longer
> creates such section objects, so there's no need to handle them.

"Since commit 3c5f6114d9f (qapi: remove "Example" doc section), Example
sections no longer exist, so ..."?

>> > support in qapidoc is now dead code.
>>
>> Oversight, I guess.
>>
>
> O:-)
>
>
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 04/23] qapi: expand tags to all doc sections

[John Snow]

This patch adds an explicit section tag to all QAPIDoc
sections. Members/Features are now explicitly tagged as such, with the
name now being stored in a dedicated "name" field (which qapidoc.py was
not actually using anyway.)

WIP: Yeah, the difference between "tagged" and "untagged" sections is
now pretty poorly named, and explicitly giving "untagged" sections an
"UNTAGGED" tag is ... well, worse. but mechanically, this accomplishes
what I need for the series.

Please suggest better naming conventions, keeping in mind that I
currently have plans for a future patch that splits the "UNTAGGED" tag
into "INTRO" and "DETAILS" tags. But, we still need a meta-name for the
category of sections that are "formerly known as untagged" but cannot be
called "freeform" because that name is used for the category of
docblocks that are not attached to an entity (but happens to be
comprised entirely of "formerly known as untagged" sections.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py |  7 ++--
 scripts/qapi/parser.py | 88 ++++++++++++++++++++++++++++++++----------
 2 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 61997fd21af..6abdcc884f5 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -35,6 +35,7 @@
 from docutils.statemachine import ViewList
 from qapi.error import QAPIError, QAPISemError
 from qapi.gen import QAPISchemaVisitor
+from qapi.parser import QAPIDoc
 from qapi.schema import QAPISchema
 
 from sphinx import addnodes
@@ -258,11 +259,11 @@ def _nodes_for_sections(self, doc):
         """Return list of doctree nodes for additional sections"""
         nodelist = []
         for section in doc.sections:
-            if section.tag and section.tag == 'TODO':
+            if section.tag == QAPIDoc.Tag.TODO:
                 # Hide TODO: sections
                 continue
 
-            if not section.tag:
+            if section.tag == QAPIDoc.Tag.UNTAGGED:
                 # Sphinx cannot handle sectionless titles;
                 # Instead, just append the results to the prior section.
                 container = nodes.container()
@@ -270,7 +271,7 @@ def _nodes_for_sections(self, doc):
                 nodelist += container.children
                 continue
 
-            snode = self._make_section(section.tag)
+            snode = self._make_section(section.tag.name.title())
             self._parse_text_into_node(dedent(section.text), snode)
             nodelist.append(snode)
         return nodelist
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 36cb64a677a..fd841725527 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -15,6 +15,7 @@
 # See the COPYING file in the top-level directory.
 
 from collections import OrderedDict
+import enum
 import os
 import re
 from typing import (
@@ -575,7 +576,10 @@ def get_doc(self) -> 'QAPIDoc':
                         )
                         raise QAPIParseError(self, emsg)
 
-                    doc.new_tagged_section(self.info, match.group(1))
+                    doc.new_tagged_section(
+                        self.info,
+                        QAPIDoc.Tag.from_string(match.group(1))
+                    )
                     text = line[match.end():]
                     if text:
                         doc.append_line(text)
@@ -635,10 +639,30 @@ class QAPIDoc:
     Free-form documentation blocks consist only of a body section.
     """
 
+    class Tag(enum.Enum):
+        UNTAGGED = 0
+        MEMBER = 1
+        FEATURE = 2
+        RETURNS = 3
+        ERRORS = 4
+        SINCE = 5
+        TODO = 6
+
+        @staticmethod
+        def from_string(tag: str) -> 'QAPIDoc.Tag':
+            return QAPIDoc.Tag[tag.upper()]
+
+        def text_required(self) -> bool:
+            # Only "untagged" sections can be empty
+            return self.value not in (0,)
+
     class Section:
         # pylint: disable=too-few-public-methods
-        def __init__(self, info: QAPISourceInfo,
-                     tag: Optional[str] = None):
+        def __init__(
+            self,
+            info: QAPISourceInfo,
+            tag: 'QAPIDoc.Tag',
+        ):
             # section source info, i.e. where it begins
             self.info = info
             # section tag, if any ('Returns', '@name', ...)
@@ -650,8 +674,14 @@ def append_line(self, line: str) -> None:
             self.text += line + '\n'
 
     class ArgSection(Section):
-        def __init__(self, info: QAPISourceInfo, tag: str):
+        def __init__(
+            self,
+            info: QAPISourceInfo,
+            tag: 'QAPIDoc.Tag',
+            name: str
+        ):
             super().__init__(info, tag)
+            self.name = name
             self.member: Optional['QAPISchemaMember'] = None
 
         def connect(self, member: 'QAPISchemaMember') -> None:
@@ -663,7 +693,9 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
         # definition doc's symbol, None for free-form doc
         self.symbol: Optional[str] = symbol
         # the sections in textual order
-        self.all_sections: List[QAPIDoc.Section] = [QAPIDoc.Section(info)]
+        self.all_sections: List[QAPIDoc.Section] = [
+            QAPIDoc.Section(info, QAPIDoc.Tag.UNTAGGED)
+        ]
         # the body section
         self.body: Optional[QAPIDoc.Section] = self.all_sections[0]
         # dicts mapping parameter/feature names to their description
@@ -680,12 +712,17 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
     def end(self) -> None:
         for section in self.all_sections:
             section.text = section.text.strip('\n')
-            if section.tag is not None and section.text == '':
+            if section.tag.text_required() and section.text == '':
                 raise QAPISemError(
                     section.info, "text required after '%s:'" % section.tag)
 
-    def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
-        if self.all_sections and not self.all_sections[-1].tag:
+    def ensure_untagged_section(
+        self,
+        info: QAPISourceInfo,
+    ) -> None:
+        tag = QAPIDoc.Tag.UNTAGGED
+
+        if self.all_sections and self.all_sections[-1].tag == tag:
             # extend current section
             section = self.all_sections[-1]
             if not section.text:
@@ -693,24 +730,29 @@ def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
                 section.info = info
             section.text += '\n'
             return
+
         # start new section
-        section = self.Section(info)
+        section = self.Section(info, tag)
         self.sections.append(section)
         self.all_sections.append(section)
 
-    def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None:
+    def new_tagged_section(
+        self,
+        info: QAPISourceInfo,
+        tag: 'QAPIDoc.Tag',
+    ) -> None:
         section = self.Section(info, tag)
-        if tag == 'Returns':
+        if tag == QAPIDoc.Tag.RETURNS:
             if self.returns:
                 raise QAPISemError(
                     info, "duplicated '%s' section" % tag)
             self.returns = section
-        elif tag == 'Errors':
+        elif tag == QAPIDoc.Tag.ERRORS:
             if self.errors:
                 raise QAPISemError(
                     info, "duplicated '%s' section" % tag)
             self.errors = section
-        elif tag == 'Since':
+        elif tag == QAPIDoc.Tag.SINCE:
             if self.since:
                 raise QAPISemError(
                     info, "duplicated '%s' section" % tag)
@@ -718,21 +760,26 @@ def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None:
         self.sections.append(section)
         self.all_sections.append(section)
 
-    def _new_description(self, info: QAPISourceInfo, name: str,
-                         desc: Dict[str, ArgSection]) -> None:
+    def _new_description(
+        self,
+        info: QAPISourceInfo,
+        name: str,
+        tag: 'QAPIDoc.Tag',
+        desc: Dict[str, ArgSection]
+    ) -> None:
         if not name:
             raise QAPISemError(info, "invalid parameter name")
         if name in desc:
             raise QAPISemError(info, "'%s' parameter name duplicated" % name)
-        section = self.ArgSection(info, '@' + name)
+        section = self.ArgSection(info, tag, name)
         self.all_sections.append(section)
         desc[name] = section
 
     def new_argument(self, info: QAPISourceInfo, name: str) -> None:
-        self._new_description(info, name, self.args)
+        self._new_description(info, name, QAPIDoc.Tag.MEMBER, self.args)
 
     def new_feature(self, info: QAPISourceInfo, name: str) -> None:
-        self._new_description(info, name, self.features)
+        self._new_description(info, name, QAPIDoc.Tag.FEATURE, self.features)
 
     def append_line(self, line: str) -> None:
         self.all_sections[-1].append_line(line)
@@ -744,8 +791,9 @@ def connect_member(self, member: 'QAPISchemaMember') -> None:
                 raise QAPISemError(member.info,
                                    "%s '%s' lacks documentation"
                                    % (member.role, member.name))
-            self.args[member.name] = QAPIDoc.ArgSection(
-                self.info, '@' + member.name)
+            section = QAPIDoc.ArgSection(
+                self.info, QAPIDoc.Tag.MEMBER, member.name)
+            self.args[member.name] = section
         self.args[member.name].connect(member)
 
     def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
-- 
2.47.0

Re: [PATCH 04/23] qapi: expand tags to all doc sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch adds an explicit section tag to all QAPIDoc
> sections. Members/Features are now explicitly tagged as such, with the
> name now being stored in a dedicated "name" field (which qapidoc.py was
> not actually using anyway.)
>
> WIP: Yeah, the difference between "tagged" and "untagged" sections is
> now pretty poorly named, and explicitly giving "untagged" sections an
> "UNTAGGED" tag is ... well, worse. but mechanically, this accomplishes
> what I need for the series.
>
> Please suggest better naming conventions, keeping in mind that I
> currently have plans for a future patch that splits the "UNTAGGED" tag
> into "INTRO" and "DETAILS" tags. But, we still need a meta-name for the
> category of sections that are "formerly known as untagged" but cannot be
> called "freeform" because that name is used for the category of
> docblocks that are not attached to an entity (but happens to be
> comprised entirely of "formerly known as untagged" sections.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Doesn't pass 'make check' for me.  Diff appended.  It shows the error
messages get worse.


diff --git a/tests/qapi-schema/doc-duplicated-return.err b/tests/qapi-schema/doc-duplicated-return.err
index 503b916b25..c0036fe8aa 100644
--- a/tests/qapi-schema/doc-duplicated-return.err
+++ b/tests/qapi-schema/doc-duplicated-return.err
@@ -1 +1 @@
-doc-duplicated-return.json:8: duplicated 'Returns' section
+doc-duplicated-return.json:8: duplicated 'Tag.RETURNS' section
diff --git a/tests/qapi-schema/doc-duplicated-since.err b/tests/qapi-schema/doc-duplicated-since.err
index a9b60c0c3d..1066040560 100644
--- a/tests/qapi-schema/doc-duplicated-since.err
+++ b/tests/qapi-schema/doc-duplicated-since.err
@@ -1 +1 @@
-doc-duplicated-since.json:8: duplicated 'Since' section
+doc-duplicated-since.json:8: duplicated 'Tag.SINCE' section
diff --git a/tests/qapi-schema/doc-empty-section.err b/tests/qapi-schema/doc-empty-section.err
index 711a0d629c..3eae1b93c9 100644
--- a/tests/qapi-schema/doc-empty-section.err
+++ b/tests/qapi-schema/doc-empty-section.err
@@ -1 +1 @@
-doc-empty-section.json:6: text required after 'Errors:'
+doc-empty-section.json:6: text required after 'Tag.ERRORS:'
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index ec277be91e..87d2f074cf 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -110,7 +110,7 @@ The _one_ {and only}, description on the same line
 Also _one_ {and only}
     feature=enum-member-feat
 a member feature
-    section=None
+    section=Tag.UNTAGGED
 @two is undocumented
 doc symbol=Base
     body=
@@ -168,15 +168,15 @@ description starts on the same line
 a feature
     feature=cmd-feat2
 another feature
-    section=None
+    section=Tag.UNTAGGED
 .. note:: @arg3 is undocumented
-    section=Returns
+    section=Tag.RETURNS
 @Object
-    section=Errors
+    section=Tag.ERRORS
 some
-    section=TODO
+    section=Tag.TODO
 frobnicate
-    section=None
+    section=Tag.UNTAGGED
 .. admonition:: Notes
 
  - Lorem ipsum dolor sit amet
@@ -200,7 +200,7 @@ Examples::
 
 Note::
     Ceci n'est pas une note
-    section=Since
+    section=Tag.SINCE
 2.10
 doc symbol=cmd-boxed
     body=
@@ -209,7 +209,7 @@ If you're bored enough to read this, go see a video of boxed cats
 a feature
     feature=cmd-feat2
 another feature
-    section=None
+    section=Tag.UNTAGGED
 .. qmp-example::
 
    -> "this example"

Re: [PATCH 04/23] qapi: expand tags to all doc sections

[John Snow]

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

On Wed, Dec 18, 2024, 5:58 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This patch adds an explicit section tag to all QAPIDoc
> > sections. Members/Features are now explicitly tagged as such, with the
> > name now being stored in a dedicated "name" field (which qapidoc.py was
> > not actually using anyway.)
> >
> > WIP: Yeah, the difference between "tagged" and "untagged" sections is
> > now pretty poorly named, and explicitly giving "untagged" sections an
> > "UNTAGGED" tag is ... well, worse. but mechanically, this accomplishes
> > what I need for the series.
> >
> > Please suggest better naming conventions, keeping in mind that I
> > currently have plans for a future patch that splits the "UNTAGGED" tag
> > into "INTRO" and "DETAILS" tags. But, we still need a meta-name for the
> > category of sections that are "formerly known as untagged" but cannot be
> > called "freeform" because that name is used for the category of
> > docblocks that are not attached to an entity (but happens to be
> > comprised entirely of "formerly known as untagged" sections.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Doesn't pass 'make check' for me.  Diff appended.  It shows the error
> messages get worse.
>

Whoops! My per-patch tester wasn't running make check, only linters and
build testing. I'll fix this, but please keep reviewing in the meantime.

Apologies for the oversight.


>
> diff --git a/tests/qapi-schema/doc-duplicated-return.err
> b/tests/qapi-schema/doc-duplicated-return.err
> index 503b916b25..c0036fe8aa 100644
> --- a/tests/qapi-schema/doc-duplicated-return.err
> +++ b/tests/qapi-schema/doc-duplicated-return.err
> @@ -1 +1 @@
> -doc-duplicated-return.json:8: duplicated 'Returns' section
> +doc-duplicated-return.json:8: duplicated 'Tag.RETURNS' section
> diff --git a/tests/qapi-schema/doc-duplicated-since.err
> b/tests/qapi-schema/doc-duplicated-since.err
> index a9b60c0c3d..1066040560 100644
> --- a/tests/qapi-schema/doc-duplicated-since.err
> +++ b/tests/qapi-schema/doc-duplicated-since.err
> @@ -1 +1 @@
> -doc-duplicated-since.json:8: duplicated 'Since' section
> +doc-duplicated-since.json:8: duplicated 'Tag.SINCE' section
> diff --git a/tests/qapi-schema/doc-empty-section.err
> b/tests/qapi-schema/doc-empty-section.err
> index 711a0d629c..3eae1b93c9 100644
> --- a/tests/qapi-schema/doc-empty-section.err
> +++ b/tests/qapi-schema/doc-empty-section.err
> @@ -1 +1 @@
> -doc-empty-section.json:6: text required after 'Errors:'
> +doc-empty-section.json:6: text required after 'Tag.ERRORS:'
> diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> index ec277be91e..87d2f074cf 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -110,7 +110,7 @@ The _one_ {and only}, description on the same line
>  Also _one_ {and only}
>      feature=enum-member-feat
>  a member feature
> -    section=None
> +    section=Tag.UNTAGGED
>  @two is undocumented
>  doc symbol=Base
>      body=
> @@ -168,15 +168,15 @@ description starts on the same line
>  a feature
>      feature=cmd-feat2
>  another feature
> -    section=None
> +    section=Tag.UNTAGGED
>  .. note:: @arg3 is undocumented
> -    section=Returns
> +    section=Tag.RETURNS
>  @Object
> -    section=Errors
> +    section=Tag.ERRORS
>  some
> -    section=TODO
> +    section=Tag.TODO
>  frobnicate
> -    section=None
> +    section=Tag.UNTAGGED
>  .. admonition:: Notes
>
>   - Lorem ipsum dolor sit amet
> @@ -200,7 +200,7 @@ Examples::
>
>  Note::
>      Ceci n'est pas une note
> -    section=Since
> +    section=Tag.SINCE
>  2.10
>  doc symbol=cmd-boxed
>      body=
> @@ -209,7 +209,7 @@ If you're bored enough to read this, go see a video of
> boxed cats
>  a feature
>      feature=cmd-feat2
>  another feature
> -    section=None
> +    section=Tag.UNTAGGED
>  .. qmp-example::
>
>     -> "this example"
>
>

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

Re: [PATCH 04/23] qapi: expand tags to all doc sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Dec 18, 2024, 5:58 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This patch adds an explicit section tag to all QAPIDoc
>> > sections. Members/Features are now explicitly tagged as such, with the
>> > name now being stored in a dedicated "name" field (which qapidoc.py was
>> > not actually using anyway.)
>> >
>> > WIP: Yeah, the difference between "tagged" and "untagged" sections is
>> > now pretty poorly named, and explicitly giving "untagged" sections an
>> > "UNTAGGED" tag is ... well, worse. but mechanically, this accomplishes
>> > what I need for the series.
>> >
>> > Please suggest better naming conventions, keeping in mind that I
>> > currently have plans for a future patch that splits the "UNTAGGED" tag
>> > into "INTRO" and "DETAILS" tags. But, we still need a meta-name for the
>> > category of sections that are "formerly known as untagged" but cannot be
>> > called "freeform" because that name is used for the category of
>> > docblocks that are not attached to an entity (but happens to be
>> > comprised entirely of "formerly known as untagged" sections.)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> Doesn't pass 'make check' for me.  Diff appended.  It shows the error
>> messages get worse.
>
> Whoops! My per-patch tester wasn't running make check, only linters and
> build testing. I'll fix this, but please keep reviewing in the meantime.
>
> Apologies for the oversight.

No worries :)

Re: [PATCH 04/23] qapi: expand tags to all doc sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch adds an explicit section tag to all QAPIDoc
> sections. Members/Features are now explicitly tagged as such, with the
> name now being stored in a dedicated "name" field (which qapidoc.py was
> not actually using anyway.)
>
> WIP: Yeah, the difference between "tagged" and "untagged" sections is
> now pretty poorly named, and explicitly giving "untagged" sections an
> "UNTAGGED" tag is ... well, worse. but mechanically, this accomplishes
> what I need for the series.
>
> Please suggest better naming conventions, keeping in mind that I
> currently have plans for a future patch that splits the "UNTAGGED" tag
> into "INTRO" and "DETAILS" tags. But, we still need a meta-name for the
> category of sections that are "formerly known as untagged" but cannot be
> called "freeform" because that name is used for the category of
> docblocks that are not attached to an entity (but happens to be
> comprised entirely of "formerly known as untagged" sections.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>

A free-form doc comment consists of just one untagged section, actually.
I don't remember whether anything relies on "just one".

The term "tagged" is rooted in doc comment syntax.
docs/devel/qapi-code-gen.rst section "Definition 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.

Sadly, this isn't fully accurate anymore.

    Descriptions start with '\@name:'.  The description text must be
    indented [...]

    A tagged section begins with a paragraph that starts with one of the
    following words: "Since:", "Returns:", "Errors:", "TODO:".  It ends with
    the start of a new section.

    The second and subsequent lines of tagged sections must be indented
    [...]

Nothing about untagged sections.  These are sections that aren't
descriptions or tagged.  Example:

    # @Returns: Lorem ipsum dolor sit amet, consectetur adipiscing elit,
    #     sed do eiusmod tempor incididunt ut labore et dolore magna
    #     aliqua.
    #
    # Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris
    # nisi ut aliquip ex ea commodo consequat.
    #
    # Duis aute irure dolor in reprehenderit in voluptate velit esse
    # cillum dolore eu fugiat nulla pariatur.
    ##

Here, the tagged "Returns" section ends after "aliqua."  Why?  Because
"Ut enim" isn't indented.  The untagged section ends after "pariatur."

We parse a definition doc comment as a sequence of sections.

The first one is the overview.

Member / argument descriptions, if any, are next.

Then we may have any number of tagged or untagged sections.  If I
remember correctly, you'd like to banish them.  Let's pretend they can't
exist here.

Then we may have a "Features:" line followed by feature descriptions.

Finally, we may have any number of tagged or untagged sections.

Each of these sections is represented as an instance of type Section,
and the entire definition doc as an instance of type QAPIDoc.

Section has a member @tag of type str.

For tagged sections, it's the tag, i.e "Since", "Returns", ...  Obvious
enough.

For overview and other untagged sections, it's None.  Still obvious.

For descriptions, it's the name of the thing being described.  Less than
obvious.  Note that descriptions are actually instances of ArgSection, a
subtype of Section, which prevents confusion with tagged sections.

QAPIDoc has the overview in member @body, member / argument descriptions
in @args, feature descriptions in @features, and the remaining sections
in @sections.

I'm in favor of cleaning this up some.

I think we can keep the Section name.

Moving the name of the thing being described from @tag to @name is good.
What value to put into @tag then?  Whatever suits you.

Perhaps we should rename @tag to avoid undue ties to tagged sections.
@kind would work for me.

Value None for untagged sections is fine with me.  If a string suits you
better, that's fine, too.  "untagged", "plain", I don't know, propose
something.

@body, @args, and so forth aren't exactly great names.  If they truly
annoy or confuse you, feel free to propose better ones.

Re: [PATCH 04/23] qapi: expand tags to all doc sections

[John Snow]

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

On Fri, Dec 20, 2024, 8:13 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This patch adds an explicit section tag to all QAPIDoc
> > sections. Members/Features are now explicitly tagged as such, with the
> > name now being stored in a dedicated "name" field (which qapidoc.py was
> > not actually using anyway.)
> >
> > WIP: Yeah, the difference between "tagged" and "untagged" sections is
> > now pretty poorly named, and explicitly giving "untagged" sections an
> > "UNTAGGED" tag is ... well, worse. but mechanically, this accomplishes
> > what I need for the series.
> >
> > Please suggest better naming conventions, keeping in mind that I
> > currently have plans for a future patch that splits the "UNTAGGED" tag
> > into "INTRO" and "DETAILS" tags. But, we still need a meta-name for the
> > category of sections that are "formerly known as untagged" but cannot be
> > called "freeform" because that name is used for the category of
> > docblocks that are not attached to an entity (but happens to be
> > comprised entirely of "formerly known as untagged" sections.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> A free-form doc comment consists of just one untagged section, actually.
> I don't remember whether anything relies on "just one".
>

Sure, yes. Sorry, I keep thinking of documentation as containing "any
number of sections" but keep eliding the fact that our parser
implementation currently will never actually create multiple adjacent
"untagged" sections.

I don't even change this anywhere even in my offline WIP, so it's just me
being over-general.

(I don't think it winds up being relevant or mattering to anything in this
series or my larger project beyond some word choices.)


> The term "tagged" is rooted in doc comment syntax.
> docs/devel/qapi-code-gen.rst section "Definition 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.
>
> Sadly, this isn't fully accurate anymore.
>
>     Descriptions start with '\@name:'.  The description text must be
>     indented [...]
>
>     A tagged section begins with a paragraph that starts with one of the
>     following words: "Since:", "Returns:", "Errors:", "TODO:".  It ends
> with
>     the start of a new section.
>
>     The second and subsequent lines of tagged sections must be indented
>     [...]
>
> Nothing about untagged sections.  These are sections that aren't
> descriptions or tagged.  Example:
>
>     # @Returns: Lorem ipsum dolor sit amet, consectetur adipiscing elit,
>     #     sed do eiusmod tempor incididunt ut labore et dolore magna
>     #     aliqua.
>     #
>     # Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris
>     # nisi ut aliquip ex ea commodo consequat.
>     #
>     # Duis aute irure dolor in reprehenderit in voluptate velit esse
>     # cillum dolore eu fugiat nulla pariatur.
>     ##
>
> Here, the tagged "Returns" section ends after "aliqua."  Why?  Because
> "Ut enim" isn't indented.  The untagged section ends after "pariatur."
>
> We parse a definition doc comment as a sequence of sections.
>
> The first one is the overview.
>
> Member / argument descriptions, if any, are next.
>
> Then we may have any number of tagged or untagged sections.  If I
> remember correctly, you'd like to banish them.  Let's pretend they can't
> exist here.
>

I think you're referring to my desire to banish "untagged" sections from
appearing *between* "tagged" sections. Yes, that's still a desire; though I
make no movement on it in this series as sent to list, and this change is
entirely unrelated to that desire.

(It's more related to being able to distinguish features from members, and
later, distinguishing intro/details. This patch still serves a purpose even
without the inliner or the complexities it brings, but serves both needs.)

((Reminder: the reason for this desire is because "tagged sections" are
rendered in html as a two-column list, and free paragraphs appearing
between list entries looks bad in the rendered documentation, because it
means ending the table, starting paragraph(s), then starting a new table.
If free text is meant to be associated with a specific
member/feature/section-group, it should be marked up (in SOME way) so that
the renderer can achieve that grouping visually.

(There will be a standalone patch that implements this restriction and we
can debate this there, I'm only giving you context here.)))


> Then we may have a "Features:" line followed by feature descriptions.
>
> Finally, we may have any number of tagged or untagged sections.
>
> Each of these sections is represented as an instance of type Section,
> and the entire definition doc as an instance of type QAPIDoc.
>
> Section has a member @tag of type str.
>
> For tagged sections, it's the tag, i.e "Since", "Returns", ...  Obvious
> enough.
>
> For overview and other untagged sections, it's None.  Still obvious.
>
> For descriptions, it's the name of the thing being described.  Less than
> obvious.  Note that descriptions are actually instances of ArgSection, a
> subtype of Section, which prevents confusion with tagged sections.
>

Note that this patch changes this as well; it becomes "member" or "feature"
as appropriate and the name is moved into a dedicated name field that
belongs to the ArgSection class.

(Turns out legacy qapidoc doesn't use this stored name at all anyway, it
fetches the name via the linked feature/member instead.)


> QAPIDoc has the overview in member @body, member / argument descriptions
> in @args, feature descriptions in @features, and the remaining sections
> in @sections.
>
> I'm in favor of cleaning this up some.
>
> I think we can keep the Section name.
>
> Moving the name of the thing being described from @tag to @name is good.
> What value to put into @tag then?  Whatever suits you.
>

What suits me is "member" and "feature". :)


> Perhaps we should rename @tag to avoid undue ties to tagged sections.
> @kind would work for me.
>

Sold!


> Value None for untagged sections is fine with me.  If a string suits you
> better, that's fine, too.  "untagged", "plain", I don't know, propose
> something.
>

For static typing reasons, an explicit tag is preferred to distinguish from
it being "optional".

I could cope with any of:

"plain",
"text",
"plaintext",
"paragraphs",
"unstructured",
"free"

... keeping in mind that I do intend to "split" this tag/kind into "intro"
and "details" later. i.e. this is a temporary tag/kind label.

I think I like "text" the most because it says the least. What about you?


> @body, @args, and so forth aren't exactly great names.  If they truly
> annoy or confuse you, feel free to propose better ones.
>

I believe they can be removed entirely once the old qapidoc is sunset,
leaving only .sections[] behind.

This removes the temptation to pick out sections "out of order".

We only need the list of sections in their source order to generate the
appropriate rST.

(Note: the inliner actually does need to filter sections somewhat to do its
inlining magic, but we'll talk about that later. All you need to know right
now is that my WIP does not utilize any field except .sections[], so the
others can in fact be dropped as redundant once we make the switch. This
patch helps enable the paradigm of "everything you need to render a section
is contained within the Section object itself" which lends itself well to
the new transmogrifier, the goal of always processing/rendering in source
order, and facilitating the mechanics of the inliner.)

...

In case I got too rambly, my action items for this patch are:

- fix the test (already done)
- rename tag to kind
- rename "untagged" to "text", possibly changing it again pending your
feedback.

--js

>

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

Re: [PATCH 04/23] qapi: expand tags to all doc sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Dec 20, 2024, 8:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This patch adds an explicit section tag to all QAPIDoc
>> > sections. Members/Features are now explicitly tagged as such, with the
>> > name now being stored in a dedicated "name" field (which qapidoc.py was
>> > not actually using anyway.)
>> >
>> > WIP: Yeah, the difference between "tagged" and "untagged" sections is
>> > now pretty poorly named, and explicitly giving "untagged" sections an
>> > "UNTAGGED" tag is ... well, worse. but mechanically, this accomplishes
>> > what I need for the series.
>> >
>> > Please suggest better naming conventions, keeping in mind that I
>> > currently have plans for a future patch that splits the "UNTAGGED" tag
>> > into "INTRO" and "DETAILS" tags. But, we still need a meta-name for the
>> > category of sections that are "formerly known as untagged" but cannot be
>> > called "freeform" because that name is used for the category of
>> > docblocks that are not attached to an entity (but happens to be
>> > comprised entirely of "formerly known as untagged" sections.)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> A free-form doc comment consists of just one untagged section, actually.
>> I don't remember whether anything relies on "just one".
>>
>
> Sure, yes. Sorry, I keep thinking of documentation as containing "any
> number of sections" but keep eliding the fact that our parser
> implementation currently will never actually create multiple adjacent
> "untagged" sections.

Yes, the parsers grows an untagged section until another section starts.
An untagged section can consist of multiple paragraphs, just like tagged
sections.

> I don't even change this anywhere even in my offline WIP, so it's just me
> being over-general.
>
> (I don't think it winds up being relevant or mattering to anything in this
> series or my larger project beyond some word choices.)
>
>> The term "tagged" is rooted in doc comment syntax.
>> docs/devel/qapi-code-gen.rst section "Definition 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.
>>
>> Sadly, this isn't fully accurate anymore.
>>
>>     Descriptions start with '\@name:'.  The description text must be
>>     indented [...]
>>
>>     A tagged section begins with a paragraph that starts with one of the
>>     following words: "Since:", "Returns:", "Errors:", "TODO:".  It ends
>> with
>>     the start of a new section.
>>
>>     The second and subsequent lines of tagged sections must be indented
>>     [...]
>>
>> Nothing about untagged sections.  These are sections that aren't
>> descriptions or tagged.  Example:
>>
>>     # @Returns: Lorem ipsum dolor sit amet, consectetur adipiscing elit,
>>     #     sed do eiusmod tempor incididunt ut labore et dolore magna
>>     #     aliqua.
>>     #
>>     # Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris
>>     # nisi ut aliquip ex ea commodo consequat.
>>     #
>>     # Duis aute irure dolor in reprehenderit in voluptate velit esse
>>     # cillum dolore eu fugiat nulla pariatur.
>>     ##
>>
>> Here, the tagged "Returns" section ends after "aliqua."  Why?  Because
>> "Ut enim" isn't indented.  The untagged section ends after "pariatur."
>>
>> We parse a definition doc comment as a sequence of sections.
>>
>> The first one is the overview.
>>
>> Member / argument descriptions, if any, are next.
>>
>> Then we may have any number of tagged or untagged sections.  If I
>> remember correctly, you'd like to banish them.  Let's pretend they can't
>> exist here.
>>
>
> I think you're referring to my desire to banish "untagged" sections from
> appearing *between* "tagged" sections.

Editing accident, sorry!  You interpreted it correctly.

>                                        Yes, that's still a desire; though I
> make no movement on it in this series as sent to list, and this change is
> entirely unrelated to that desire.
>
> (It's more related to being able to distinguish features from members, and
> later, distinguishing intro/details. This patch still serves a purpose even
> without the inliner or the complexities it brings, but serves both needs.)
>
> ((Reminder: the reason for this desire is because "tagged sections" are
> rendered in html as a two-column list, and free paragraphs appearing
> between list entries looks bad in the rendered documentation, because it
> means ending the table, starting paragraph(s), then starting a new table.
> If free text is meant to be associated with a specific
> member/feature/section-group, it should be marked up (in SOME way) so that
> the renderer can achieve that grouping visually.
>
> (There will be a standalone patch that implements this restriction and we
> can debate this there, I'm only giving you context here.)))
>
>
>> Then we may have a "Features:" line followed by feature descriptions.
>>
>> Finally, we may have any number of tagged or untagged sections.
>>
>> Each of these sections is represented as an instance of type Section,
>> and the entire definition doc as an instance of type QAPIDoc.
>>
>> Section has a member @tag of type str.
>>
>> For tagged sections, it's the tag, i.e "Since", "Returns", ...  Obvious
>> enough.
>>
>> For overview and other untagged sections, it's None.  Still obvious.
>>
>> For descriptions, it's the name of the thing being described.  Less than
>> obvious.  Note that descriptions are actually instances of ArgSection, a
>> subtype of Section, which prevents confusion with tagged sections.
>>
>
> Note that this patch changes this as well; it becomes "member" or "feature"
> as appropriate and the name is moved into a dedicated name field that
> belongs to the ArgSection class.
>
> (Turns out legacy qapidoc doesn't use this stored name at all anyway, it
> fetches the name via the linked feature/member instead.)
>
>
>> QAPIDoc has the overview in member @body, member / argument descriptions
>> in @args, feature descriptions in @features, and the remaining sections
>> in @sections.
>>
>> I'm in favor of cleaning this up some.
>>
>> I think we can keep the Section name.
>>
>> Moving the name of the thing being described from @tag to @name is good.
>> What value to put into @tag then?  Whatever suits you.
>>
>
> What suits me is "member" and "feature". :)

Okay.

Doesn't entirely clean up the terminology mess.  According to
docs/devel/qapi-code-gen.rst, struct and union types have members,
alternate types have alternatives, enum types have values, commands have
arguments, and events have event-specific data, which is a mouthful, so
we often say arguments.  Using one of them ("member") to refer to the
generalization of them all is suboptimal, but it's no worse than before.

ArgSection is even more general: it's features, too.  Again, no worse
than before.

I'm *not* asking you to clean this up.  I'm just pointing out we could
use fresh naming ideas here.

>> Perhaps we should rename @tag to avoid undue ties to tagged sections.
>> @kind would work for me.
>>
>
> Sold!
>
>
>> Value None for untagged sections is fine with me.  If a string suits you
>> better, that's fine, too.  "untagged", "plain", I don't know, propose
>> something.
>>
>
> For static typing reasons, an explicit tag is preferred to distinguish from
> it being "optional".
>
> I could cope with any of:
>
> "plain",
> "text",
> "plaintext",
> "paragraphs",
> "unstructured",
> "free"
>
> ... keeping in mind that I do intend to "split" this tag/kind into "intro"
> and "details" later. i.e. this is a temporary tag/kind label.
>
> I think I like "text" the most because it says the least. What about you?

Point, but the other kinds of section are text, too.  "plain"?

>> @body, @args, and so forth aren't exactly great names.  If they truly
>> annoy or confuse you, feel free to propose better ones.
>>
>
> I believe they can be removed entirely once the old qapidoc is sunset,
> leaving only .sections[] behind.
>
> This removes the temptation to pick out sections "out of order".

I've long wanted strict in-order processing, to avoid surprising
reordering of input in the output.

> We only need the list of sections in their source order to generate the
> appropriate rST.
>
> (Note: the inliner actually does need to filter sections somewhat to do its
> inlining magic, but we'll talk about that later. All you need to know right
> now is that my WIP does not utilize any field except .sections[], so the
> others can in fact be dropped as redundant once we make the switch. This
> patch helps enable the paradigm of "everything you need to render a section
> is contained within the Section object itself" which lends itself well to
> the new transmogrifier, the goal of always processing/rendering in source
> order, and facilitating the mechanics of the inliner.)
>
> ...
>
> In case I got too rambly, my action items for this patch are:
>
> - fix the test (already done)
> - rename tag to kind
> - rename "untagged" to "text", possibly changing it again pending your
> feedback.

Sounds good!

[PATCH 05/23] qapi/schema: add __repr__ to QAPIDoc.Section

[John Snow]

Makes debugging far more pleasant when you can just print(section) and
get something reasonable to display.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index fd841725527..3c88f9c7d38 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -670,6 +670,9 @@ def __init__(
             # section text without tag
             self.text = ''
 
+        def __repr__(self) -> str:
+            return f"<QAPIDoc.Section tag={self.tag!r} text={self.text!r}>"
+
         def append_line(self, line: str) -> None:
             self.text += line + '\n'
 
-- 
2.47.0

[PATCH 06/23] docs/qapidoc: add transmogrifier stub

[John Snow]

This commit adds a stubbed option to the qapi-doc directive that opts-in
to the new rST generator; the implementation of which will follow in
subsequent commits.

Once all QAPI documents have been converted, this option and the old
qapidoc implementation can be dropped.

Note that moving code outside of the try...except block has no impact
because the code moved outside of that block does not ever raise a
QAPIError.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 6abdcc884f5..230e67bca23 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -452,9 +452,9 @@ def _parse_text_into_node(self, doctext, node):
         rstlist.append("", self._cur_doc.info.fname, self._cur_doc.info.line)
         self._sphinx_directive.do_parse(rstlist, node)
 
-    def get_document_nodes(self):
-        """Return the list of docutils nodes which make up the document"""
-        return self._top_node.children
+    def get_document_node(self):
+        """Return the root docutils node which makes up the document"""
+        return self._top_node
 
 
 # Turn the black formatter on for the rest of the file.
@@ -503,7 +503,10 @@ class QAPIDocDirective(NestedDirective):
 
     required_argument = 1
     optional_arguments = 1
-    option_spec = {"qapifile": directives.unchanged_required}
+    option_spec = {
+        "qapifile": directives.unchanged_required,
+        "transmogrify": directives.flag,
+    }
     has_content = False
 
     def new_serialno(self):
@@ -511,10 +514,24 @@ def new_serialno(self):
         env = self.state.document.settings.env
         return "qapidoc-%d" % env.new_serialno("qapidoc")
 
+    def transmogrify(self, schema) -> nodes.Element:
+        raise NotImplementedError
+
+    def legacy(self, schema) -> nodes.Element:
+        vis = QAPISchemaGenRSTVisitor(self)
+        vis.visit_begin(schema)
+        for doc in schema.docs:
+            if doc.symbol:
+                vis.symbol(doc, schema.lookup_entity(doc.symbol))
+            else:
+                vis.freeform(doc)
+        return vis.get_document_node()
+
     def run(self):
         env = self.state.document.settings.env
         qapifile = env.config.qapidoc_srctree + "/" + self.arguments[0]
         qapidir = os.path.dirname(qapifile)
+        transmogrify = "transmogrify" in self.options
 
         try:
             schema = QAPISchema(qapifile)
@@ -522,20 +539,18 @@ def run(self):
             # First tell Sphinx about all the schema files that the
             # output documentation depends on (including 'qapifile' itself)
             schema.visit(QAPISchemaGenDepVisitor(env, qapidir))
-
-            vis = QAPISchemaGenRSTVisitor(self)
-            vis.visit_begin(schema)
-            for doc in schema.docs:
-                if doc.symbol:
-                    vis.symbol(doc, schema.lookup_entity(doc.symbol))
-                else:
-                    vis.freeform(doc)
-            return vis.get_document_nodes()
         except QAPIError as err:
             # Launder QAPI parse errors into Sphinx extension errors
             # so they are displayed nicely to the user
             raise ExtensionError(str(err)) from err
 
+        if transmogrify:
+            contentnode = self.transmogrify(schema)
+        else:
+            contentnode = self.legacy(schema)
+
+        return contentnode.children
+
 
 class QMPExample(CodeBlock, NestedDirective):
     """
-- 
2.47.0

[PATCH 07/23] docs/qapidoc: add transmogrifier class stub

[John Snow]

Add the beginnings of the Transmogrifier class by adding the rST
conversion helpers that will be used to build the virtual rST document.

This version of the class does not actually "do anything" yet; each
individual feature is added one-at-a-time in the forthcoming commits.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 66 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 230e67bca23..dee97e6f984 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -24,6 +24,7 @@
 https://www.sphinx-doc.org/en/master/development/index.html
 """
 
+from contextlib import contextmanager
 import os
 import re
 import sys
@@ -32,11 +33,12 @@
 
 from docutils import nodes
 from docutils.parsers.rst import Directive, directives
-from docutils.statemachine import ViewList
+from docutils.statemachine import StringList, ViewList
 from qapi.error import QAPIError, QAPISemError
 from qapi.gen import QAPISchemaVisitor
 from qapi.parser import QAPIDoc
 from qapi.schema import QAPISchema
+from qapi.source import QAPISourceInfo
 
 from sphinx import addnodes
 from sphinx.directives.code import CodeBlock
@@ -61,6 +63,68 @@ def dedent(text: str) -> str:
     return lines[0] + textwrap.dedent("".join(lines[1:]))
 
 
+class Transmogrifier:
+    def __init__(self, schema):
+        self._result = StringList()
+        self.indent = 0
+
+    # General-purpose rST generation functions
+
+    def get_indent(self) -> str:
+        return "   " * self.indent
+
+    @contextmanager
+    def indented(self):
+        self.indent += 1
+        try:
+            yield
+        finally:
+            self.indent -= 1
+
+    def add_line_raw(self, line: str, source: str, *lineno: int) -> None:
+        """Append one line of generated reST to the output."""
+
+        # NB: Sphinx uses zero-indexed lines; subtract one.
+        lineno = tuple((n - 1 for n in lineno))
+
+        if line.strip():
+            # not a blank line
+            self._result.append(
+                self.get_indent() + line.rstrip("\n"), source, *lineno
+            )
+        else:
+            self._result.append("", source, *lineno)
+
+    def add_line(self, content: str, info: QAPISourceInfo) -> None:
+        # NB: We *require* an info object; this works out OK because we
+        # don't document built-in objects that don't have
+        # one. Everything else should.
+        assert info
+        self.add_line_raw(content, info.fname, info.line)
+
+    def add_lines(
+        self,
+        content: str,
+        info: QAPISourceInfo,
+    ) -> None:
+        assert info
+        lines = content.splitlines(True)
+        for i, line in enumerate(lines):
+            self.add_line_raw(line, info.fname, info.line + i)
+
+    def ensure_blank_line(self) -> None:
+        # Empty document -- no blank line required.
+        if not self._result:
+            return
+
+        # Last line isn't blank, add one.
+        if self._result[-1].strip():  # pylint: disable=no-member
+            fname, line = self._result.info(-1)
+            # New blank line is credited to one-after the current last line.
+            # +2: correct for zero/one index, then increment by one.
+            self.add_line_raw("", fname, line + 2)
+
+
 # Disable black auto-formatter until re-enabled:
 # fmt: off
 
-- 
2.47.0

[PATCH 08/23] docs/qapidoc: add visit_module() method

[John Snow]

This method annotates the start of a new module, crediting the source
location to the first line of the module file.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index dee97e6f984..7d2f7bfb415 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -26,6 +26,7 @@
 
 from contextlib import contextmanager
 import os
+from pathlib import Path
 import re
 import sys
 import textwrap
@@ -124,6 +125,14 @@ def ensure_blank_line(self) -> None:
             # +2: correct for zero/one index, then increment by one.
             self.add_line_raw("", fname, line + 2)
 
+    # Transmogrification core methods
+
+    def visit_module(self, path: str) -> None:
+        name = Path(path).stem
+        # module directives are credited to the first line of a module file.
+        self.add_line_raw(f".. qapi:module:: {name}", path, 1)
+        self.ensure_blank_line()
+
 
 # Disable black auto-formatter until re-enabled:
 # fmt: off
-- 
2.47.0

[PATCH 09/23] qapi/source: allow multi-line QAPISourceInfo advancing

[John Snow]

This is for the sake of the new rST generator (the "transmogrifier") so
we can advance multiple lines on occasion while keeping the
generated<-->source mappings accurate.

next_line now simply takes an optional n parameter which chooses the
number of lines to advance.


RFC: Here's the exorbitant detail on why I want this:

This is used mainly when converting section syntax in free-form
documentation to more traditional rST section header syntax, which
does not always line up 1:1 for line counts.

For example:

```
 ##
 # = Section     <-- Info is pointing here, "L1"
 #
 # Lorem Ipsum
 ##
```

would be transformed to rST as:

```
=======        <-- L1
Section        <-- L1
=======        <-- L1
               <-- L2
Lorem Ipsum    <-- L3
```

After consuming the single "Section" line from the source, we want to
advance the source pointer to the next non-empty line which requires
jumping by more than one line.

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

diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 7b379fdc925..ffdc3f482ac 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -47,9 +47,9 @@ def set_defn(self, meta: str, name: str) -> None:
         self.defn_meta = meta
         self.defn_name = name
 
-    def next_line(self: T) -> T:
+    def next_line(self: T, n: int = 1) -> T:
         info = copy.copy(self)
-        info.line += 1
+        info.line += n
         return info
 
     def loc(self) -> str:
-- 
2.47.0

Re: [PATCH 09/23] qapi/source: allow multi-line QAPISourceInfo advancing

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is for the sake of the new rST generator (the "transmogrifier") so
> we can advance multiple lines on occasion while keeping the
> generated<-->source mappings accurate.
>
> next_line now simply takes an optional n parameter which chooses the
> number of lines to advance.
>
>
> RFC: Here's the exorbitant detail on why I want this:
>
> This is used mainly when converting section syntax in free-form
> documentation to more traditional rST section header syntax, which
> does not always line up 1:1 for line counts.
>
> For example:
>
> ```
>  ##
>  # = Section     <-- Info is pointing here, "L1"
>  #
>  # Lorem Ipsum
>  ##
> ```
>
> would be transformed to rST as:
>
> ```
> =======        <-- L1
> Section        <-- L1
> =======        <-- L1
>                <-- L2
> Lorem Ipsum    <-- L3
> ```

I can't help to wonder...  Could we simply use rST markup instead?

"Later", "maybe later", or even "please ask me later" would be perfectly
acceptable answers.

> After consuming the single "Section" line from the source, we want to
> advance the source pointer to the next non-empty line which requires
> jumping by more than one line.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/source.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> index 7b379fdc925..ffdc3f482ac 100644
> --- a/scripts/qapi/source.py
> +++ b/scripts/qapi/source.py
> @@ -47,9 +47,9 @@ def set_defn(self, meta: str, name: str) -> None:
>          self.defn_meta = meta
>          self.defn_name = name
>  
> -    def next_line(self: T) -> T:
> +    def next_line(self: T, n: int = 1) -> T:
>          info = copy.copy(self)
> -        info.line += 1
> +        info.line += n
>          return info
>  
>      def loc(self) -> str:

Assuming we need this:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 09/23] qapi/source: allow multi-line QAPISourceInfo advancing

[John Snow]

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

On Fri, Dec 20, 2024 at 8:22 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This is for the sake of the new rST generator (the "transmogrifier") so
> > we can advance multiple lines on occasion while keeping the
> > generated<-->source mappings accurate.
> >
> > next_line now simply takes an optional n parameter which chooses the
> > number of lines to advance.
> >
> >
> > RFC: Here's the exorbitant detail on why I want this:
> >
> > This is used mainly when converting section syntax in free-form
> > documentation to more traditional rST section header syntax, which
> > does not always line up 1:1 for line counts.
> >
> > For example:
> >
> > ```
> >  ##
> >  # = Section     <-- Info is pointing here, "L1"
> >  #
> >  # Lorem Ipsum
> >  ##
> > ```
> >
> > would be transformed to rST as:
> >
> > ```
> > =======        <-- L1
> > Section        <-- L1
> > =======        <-- L1
> >                <-- L2
> > Lorem Ipsum    <-- L3
> > ```
>
> I can't help to wonder...  Could we simply use rST markup instead?
>
> "Later", "maybe later", or even "please ask me later" would be perfectly
> acceptable answers.
>

Yeah, I'd be happy with that, I just didn't want to add more complexity to
the pile so I went for what I felt was "simplest":

- Leave source syntax alone
- Copy and modify the existing freeform doc parser
- Quickly allow for multi-line advancing where it appeared to be important.

Modifying freeform syntax to be purely rST that isn't modified or rewritten
at all has benefits:

- No need to mangle or multiplex source line source information
- Less code
- More straightforward

I'm quite happy to do it later, is there some kind of "ticket" system you'd
tolerate using for tracking nits for cleanup? I *will* forget if we don't
listify and track them, I'm sorry (but wise enough) to admit. I just don't
want to get sidetracked on little side-quests right now. (Quite prone to
this...)


>
> > After consuming the single "Section" line from the source, we want to
> > advance the source pointer to the next non-empty line which requires
> > jumping by more than one line.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/source.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> > index 7b379fdc925..ffdc3f482ac 100644
> > --- a/scripts/qapi/source.py
> > +++ b/scripts/qapi/source.py
> > @@ -47,9 +47,9 @@ def set_defn(self, meta: str, name: str) -> None:
> >          self.defn_meta = meta
> >          self.defn_name = name
> >
> > -    def next_line(self: T) -> T:
> > +    def next_line(self: T, n: int = 1) -> T:
> >          info = copy.copy(self)
> > -        info.line += 1
> > +        info.line += n
> >          return info
> >
> >      def loc(self) -> str:
>
> Assuming we need this:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>


Thanks! We can always drop stuff later if we wind up not needing it, it's
just a means to an end.

--js

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

Re: [PATCH 09/23] qapi/source: allow multi-line QAPISourceInfo advancing

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Dec 20, 2024 at 8:22 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This is for the sake of the new rST generator (the "transmogrifier") so
>> > we can advance multiple lines on occasion while keeping the
>> > generated<-->source mappings accurate.
>> >
>> > next_line now simply takes an optional n parameter which chooses the
>> > number of lines to advance.
>> >
>> >
>> > RFC: Here's the exorbitant detail on why I want this:
>> >
>> > This is used mainly when converting section syntax in free-form
>> > documentation to more traditional rST section header syntax, which
>> > does not always line up 1:1 for line counts.
>> >
>> > For example:
>> >
>> > ```
>> >  ##
>> >  # = Section     <-- Info is pointing here, "L1"
>> >  #
>> >  # Lorem Ipsum
>> >  ##
>> > ```
>> >
>> > would be transformed to rST as:
>> >
>> > ```
>> > =======        <-- L1
>> > Section        <-- L1
>> > =======        <-- L1
>> >                <-- L2
>> > Lorem Ipsum    <-- L3
>> > ```
>>
>> I can't help to wonder...  Could we simply use rST markup instead?
>>
>> "Later", "maybe later", or even "please ask me later" would be perfectly
>> acceptable answers.
>>
>
> Yeah, I'd be happy with that, I just didn't want to add more complexity to
> the pile so I went for what I felt was "simplest":

Avoiding mission creep is good.

> - Leave source syntax alone
> - Copy and modify the existing freeform doc parser
> - Quickly allow for multi-line advancing where it appeared to be important.
>
> Modifying freeform syntax to be purely rST that isn't modified or rewritten
> at all has benefits:
>
> - No need to mangle or multiplex source line source information
> - Less code
> - More straightforward
>
> I'm quite happy to do it later, is there some kind of "ticket" system you'd
> tolerate using for tracking nits for cleanup? I *will* forget if we don't
> listify and track them, I'm sorry (but wise enough) to admit. I just don't
> want to get sidetracked on little side-quests right now. (Quite prone to
> this...)

TODO comment in code this would obviously kill?  Not exactly a ticket
system...

scripts/qapi/TODO?  Still not a ticket system...

Other ideas?

>> > After consuming the single "Section" line from the source, we want to
>> > advance the source pointer to the next non-empty line which requires
>> > jumping by more than one line.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  scripts/qapi/source.py | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
>> > index 7b379fdc925..ffdc3f482ac 100644
>> > --- a/scripts/qapi/source.py
>> > +++ b/scripts/qapi/source.py
>> > @@ -47,9 +47,9 @@ def set_defn(self, meta: str, name: str) -> None:
>> >          self.defn_meta = meta
>> >          self.defn_name = name
>> >
>> > -    def next_line(self: T) -> T:
>> > +    def next_line(self: T, n: int = 1) -> T:
>> >          info = copy.copy(self)
>> > -        info.line += 1
>> > +        info.line += n
>> >          return info
>> >
>> >      def loc(self) -> str:
>>
>> Assuming we need this:
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks! We can always drop stuff later if we wind up not needing it, it's
> just a means to an end.

Yes, and this one isn't exactly a complexity bomb :)

Re: [PATCH 09/23] qapi/source: allow multi-line QAPISourceInfo advancing

[John Snow]

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

On Thu, Jan 9, 2025, 3:00 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Fri, Dec 20, 2024 at 8:22 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > This is for the sake of the new rST generator (the "transmogrifier")
> so
> >> > we can advance multiple lines on occasion while keeping the
> >> > generated<-->source mappings accurate.
> >> >
> >> > next_line now simply takes an optional n parameter which chooses the
> >> > number of lines to advance.
> >> >
> >> >
> >> > RFC: Here's the exorbitant detail on why I want this:
> >> >
> >> > This is used mainly when converting section syntax in free-form
> >> > documentation to more traditional rST section header syntax, which
> >> > does not always line up 1:1 for line counts.
> >> >
> >> > For example:
> >> >
> >> > ```
> >> >  ##
> >> >  # = Section     <-- Info is pointing here, "L1"
> >> >  #
> >> >  # Lorem Ipsum
> >> >  ##
> >> > ```
> >> >
> >> > would be transformed to rST as:
> >> >
> >> > ```
> >> > =======        <-- L1
> >> > Section        <-- L1
> >> > =======        <-- L1
> >> >                <-- L2
> >> > Lorem Ipsum    <-- L3
> >> > ```
> >>
> >> I can't help to wonder...  Could we simply use rST markup instead?
> >>
> >> "Later", "maybe later", or even "please ask me later" would be perfectly
> >> acceptable answers.
> >>
> >
> > Yeah, I'd be happy with that, I just didn't want to add more complexity
> to
> > the pile so I went for what I felt was "simplest":
>
> Avoiding mission creep is good.
>
> > - Leave source syntax alone
> > - Copy and modify the existing freeform doc parser
> > - Quickly allow for multi-line advancing where it appeared to be
> important.
> >
> > Modifying freeform syntax to be purely rST that isn't modified or
> rewritten
> > at all has benefits:
> >
> > - No need to mangle or multiplex source line source information
> > - Less code
> > - More straightforward
> >
> > I'm quite happy to do it later, is there some kind of "ticket" system
> you'd
> > tolerate using for tracking nits for cleanup? I *will* forget if we don't
> > listify and track them, I'm sorry (but wise enough) to admit. I just
> don't
> > want to get sidetracked on little side-quests right now. (Quite prone to
> > this...)
>
> TODO comment in code this would obviously kill?  Not exactly a ticket
> system...
>
> scripts/qapi/TODO?  Still not a ticket system...
>

If a TODO is fine (and you don't mind pinging me in the future), then the
comment I left in the visit_freeform() function (it's in another patch)
explaining that the custom parser can be dropped after we sunset the old
qapidoc is likely sufficient if I just add a "TODO".

Sound good?


> Other ideas?
>
> >> > After consuming the single "Section" line from the source, we want to
> >> > advance the source pointer to the next non-empty line which requires
> >> > jumping by more than one line.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >> > ---
> >> >  scripts/qapi/source.py | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
> >> > index 7b379fdc925..ffdc3f482ac 100644
> >> > --- a/scripts/qapi/source.py
> >> > +++ b/scripts/qapi/source.py
> >> > @@ -47,9 +47,9 @@ def set_defn(self, meta: str, name: str) -> None:
> >> >          self.defn_meta = meta
> >> >          self.defn_name = name
> >> >
> >> > -    def next_line(self: T) -> T:
> >> > +    def next_line(self: T, n: int = 1) -> T:
> >> >          info = copy.copy(self)
> >> > -        info.line += 1
> >> > +        info.line += n
> >> >          return info
> >> >
> >> >      def loc(self) -> str:
> >>
> >> Assuming we need this:
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >
> > Thanks! We can always drop stuff later if we wind up not needing it, it's
> > just a means to an end.
>
> Yes, and this one isn't exactly a complexity bomb :)
>

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

Re: [PATCH 09/23] qapi/source: allow multi-line QAPISourceInfo advancing

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Jan 9, 2025, 3:00 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
[...]
>> > Modifying freeform syntax to be purely rST that isn't modified or rewritten
>> > at all has benefits:
>> >
>> > - No need to mangle or multiplex source line source information
>> > - Less code
>> > - More straightforward
>> >
>> > I'm quite happy to do it later, is there some kind of "ticket" system you'd
>> > tolerate using for tracking nits for cleanup? I *will* forget if we don't
>> > listify and track them, I'm sorry (but wise enough) to admit. I just don't
>> > want to get sidetracked on little side-quests right now. (Quite prone to
>> > this...)
>>
>> TODO comment in code this would obviously kill?  Not exactly a ticket
>> system...
>>
>> scripts/qapi/TODO?  Still not a ticket system...
>>
>
> If a TODO is fine (and you don't mind pinging me in the future), then the
> comment I left in the visit_freeform() function (it's in another patch)
> explaining that the custom parser can be dropped after we sunset the old
> qapidoc is likely sufficient if I just add a "TODO".
>
> Sound good?

Yes.

[...]

[PATCH 10/23] docs/qapidoc: add visit_freeform() method

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 47 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 7d2f7bfb415..6f8f69077b1 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -133,6 +133,53 @@ def visit_module(self, path: str) -> None:
         self.add_line_raw(f".. qapi:module:: {name}", path, 1)
         self.ensure_blank_line()
 
+    def visit_freeform(self, doc) -> None:
+        # Once the old qapidoc transformer is deprecated,
+        # freeform sections can be transformed into pure rST.
+        #
+        # For now, translate our micro-format into rST.
+        # Code adapted from Peter Maydell's freeform().
+
+        assert len(doc.all_sections) == 1, doc.all_sections
+        body = doc.all_sections[0]
+        text = body.text
+        info = doc.info
+
+        if re.match(r"=+ ", text):
+            # Section/subsection heading (if present, will always be the
+            # first line of the block)
+            (heading, _, text) = text.partition("\n")
+            (leader, _, heading) = heading.partition(" ")
+            level = len(leader) + 1  # Implicit +1 for heading in .rST stub
+
+            # https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections
+            markers = {
+                1: "#",
+                2: "*",
+                3: "=",
+                4: "-",
+                5: "^",
+                6: '"',
+            }
+            overline = level <= 2
+            marker = markers[level]
+
+            self.ensure_blank_line()
+            # This credits all 2 or 3 lines to the single source line.
+            if overline:
+                self.add_line(marker * len(heading), info)
+            self.add_line(heading, info)
+            self.add_line(marker * len(heading), info)
+            self.ensure_blank_line()
+
+            # Eat blank line(s) and advance info
+            trimmed = text.lstrip("\n")
+            text = trimmed
+            info = info.next_line(len(text) - len(trimmed) + 1)
+
+        self.add_lines(text, info)
+        self.ensure_blank_line()
+
 
 # Disable black auto-formatter until re-enabled:
 # fmt: off
-- 
2.47.0

Re: [PATCH 10/23] docs/qapidoc: add visit_freeform() method

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py | 47 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 7d2f7bfb415..6f8f69077b1 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -133,6 +133,53 @@ def visit_module(self, path: str) -> None:
>          self.add_line_raw(f".. qapi:module:: {name}", path, 1)
>          self.ensure_blank_line()
>  
> +    def visit_freeform(self, doc) -> None:
> +        # Once the old qapidoc transformer is deprecated,
> +        # freeform sections can be transformed into pure rST.
> +        #
> +        # For now, translate our micro-format into rST.
> +        # Code adapted from Peter Maydell's freeform().
> +
> +        assert len(doc.all_sections) == 1, doc.all_sections
> +        body = doc.all_sections[0]
> +        text = body.text
> +        info = doc.info
> +
> +        if re.match(r"=+ ", text):
> +            # Section/subsection heading (if present, will always be the
> +            # first line of the block)
> +            (heading, _, text) = text.partition("\n")
> +            (leader, _, heading) = heading.partition(" ")
> +            level = len(leader) + 1  # Implicit +1 for heading in .rST stub
> +
> +            # https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections
> +            markers = {
> +                1: "#",
> +                2: "*",
> +                3: "=",
> +                4: "-",
> +                5: "^",
> +                6: '"',
> +            }
> +            overline = level <= 2
> +            marker = markers[level]
> +
> +            self.ensure_blank_line()
> +            # This credits all 2 or 3 lines to the single source line.
> +            if overline:
> +                self.add_line(marker * len(heading), info)
> +            self.add_line(heading, info)
> +            self.add_line(marker * len(heading), info)
> +            self.ensure_blank_line()
> +
> +            # Eat blank line(s) and advance info
> +            trimmed = text.lstrip("\n")
> +            text = trimmed
> +            info = info.next_line(len(text) - len(trimmed) + 1)

We could instead eat newlines one at a time, calling .next_line()
without an argument for each newline eaten.  Less efficient, but won't
matter here.  Possibly easier to understand.

> +
> +        self.add_lines(text, info)
> +        self.ensure_blank_line()
> +
>  
>  # Disable black auto-formatter until re-enabled:
>  # fmt: off

Re: [PATCH 10/23] docs/qapidoc: add visit_freeform() method

[John Snow]

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

On Fri, Dec 20, 2024 at 8:25 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py | 47 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 7d2f7bfb415..6f8f69077b1 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -133,6 +133,53 @@ def visit_module(self, path: str) -> None:
> >          self.add_line_raw(f".. qapi:module:: {name}", path, 1)
> >          self.ensure_blank_line()
> >
> > +    def visit_freeform(self, doc) -> None:
> > +        # Once the old qapidoc transformer is deprecated,
> > +        # freeform sections can be transformed into pure rST.
> > +        #
> > +        # For now, translate our micro-format into rST.
> > +        # Code adapted from Peter Maydell's freeform().
> > +
> > +        assert len(doc.all_sections) == 1, doc.all_sections
> > +        body = doc.all_sections[0]
> > +        text = body.text
> > +        info = doc.info
> > +
> > +        if re.match(r"=+ ", text):
> > +            # Section/subsection heading (if present, will always be the
> > +            # first line of the block)
> > +            (heading, _, text) = text.partition("\n")
> > +            (leader, _, heading) = heading.partition(" ")
> > +            level = len(leader) + 1  # Implicit +1 for heading in .rST
> stub
> > +
> > +            #
> https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections
> > +            markers = {
> > +                1: "#",
> > +                2: "*",
> > +                3: "=",
> > +                4: "-",
> > +                5: "^",
> > +                6: '"',
> > +            }
> > +            overline = level <= 2
> > +            marker = markers[level]
> > +
> > +            self.ensure_blank_line()
> > +            # This credits all 2 or 3 lines to the single source line.
> > +            if overline:
> > +                self.add_line(marker * len(heading), info)
> > +            self.add_line(heading, info)
> > +            self.add_line(marker * len(heading), info)
> > +            self.ensure_blank_line()
> > +
> > +            # Eat blank line(s) and advance info
> > +            trimmed = text.lstrip("\n")
> > +            text = trimmed
> > +            info = info.next_line(len(text) - len(trimmed) + 1)
>
> We could instead eat newlines one at a time, calling .next_line()
> without an argument for each newline eaten.  Less efficient, but won't
> matter here.  Possibly easier to understand.
>

Eh, I'm gonna delete this later anyway. Will send v2 without touching this,
but I can touch it for next time if it still bothers you.

--js

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

[PATCH 11/23] docs/qapidoc: add preamble() method

[John Snow]

This method adds the options/preamble to each definition block. Notably,
:since: and :ifcond: are added, as are any "special features" such as
:deprecated: and :unstable:.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 6f8f69077b1..85c7ce94564 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -38,7 +38,7 @@
 from qapi.error import QAPIError, QAPISemError
 from qapi.gen import QAPISchemaVisitor
 from qapi.parser import QAPIDoc
-from qapi.schema import QAPISchema
+from qapi.schema import QAPISchema, QAPISchemaEntity
 from qapi.source import QAPISourceInfo
 
 from sphinx import addnodes
@@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
             # +2: correct for zero/one index, then increment by one.
             self.add_line_raw("", fname, line + 2)
 
+    # Transmogrification helpers
+
+    def preamble(self, ent: QAPISchemaEntity) -> None:
+        """
+        Generate option lines for qapi entity directives.
+        """
+        if ent.doc and ent.doc.since:
+            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
+            # Generated from the entity's docblock; info location is exact.
+            self.add_line(f":since: {ent.doc.since.text}", ent.doc.since.info)
+
+        if ent.ifcond.is_present():
+            doc = ent.ifcond.docgen()
+            # Generated from entity definition; info location is approximate.
+            self.add_line(f":ifcond: {doc}", ent.info)
+
+        # Hoist special features such as :deprecated: and :unstable:
+        # into the options block for the entity. If, in the future, new
+        # special features are added, qapi-domain will chirp about
+        # unrecognized options and fail.
+        for feat in ent.features:
+            if feat.is_special():
+                # We don't expect special features to have an ifcond property.
+                # (Hello, intrepid developer in the future who changed that!)
+                # ((With luck, you are not me.))
+                assert not feat.ifcond.is_present()
+                # Generated from entity def; info location is approximate.
+                self.add_line(f":{feat.name}:", feat.info)
+
+        self.ensure_blank_line()
+
     # Transmogrification core methods
 
     def visit_module(self, path: str) -> None:
-- 
2.47.0

Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This method adds the options/preamble to each definition block. Notably,
> :since: and :ifcond: are added, as are any "special features" such as
> :deprecated: and :unstable:.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 6f8f69077b1..85c7ce94564 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -38,7 +38,7 @@
>  from qapi.error import QAPIError, QAPISemError
>  from qapi.gen import QAPISchemaVisitor
>  from qapi.parser import QAPIDoc
> -from qapi.schema import QAPISchema
> +from qapi.schema import QAPISchema, QAPISchemaEntity
>  from qapi.source import QAPISourceInfo
>  
>  from sphinx import addnodes
> @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
>              # +2: correct for zero/one index, then increment by one.
>              self.add_line_raw("", fname, line + 2)
>  
> +    # Transmogrification helpers
> +
> +    def preamble(self, ent: QAPISchemaEntity) -> None:
> +        """
> +        Generate option lines for qapi entity directives.
> +        """
> +        if ent.doc and ent.doc.since:
> +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
> +            # Generated from the entity's docblock; info location is exact.
> +            self.add_line(f":since: {ent.doc.since.text}", ent.doc.since.info)
> +
> +        if ent.ifcond.is_present():
> +            doc = ent.ifcond.docgen()
> +            # Generated from entity definition; info location is approximate.
> +            self.add_line(f":ifcond: {doc}", ent.info)
> +
> +        # Hoist special features such as :deprecated: and :unstable:
> +        # into the options block for the entity. If, in the future, new
> +        # special features are added, qapi-domain will chirp about
> +        # unrecognized options and fail.
> +        for feat in ent.features:
> +            if feat.is_special():
> +                # We don't expect special features to have an ifcond property.
> +                # (Hello, intrepid developer in the future who changed that!)
> +                # ((With luck, you are not me.))
> +                assert not feat.ifcond.is_present()

Nope :)

The attempt to add a conditional special feature now fails with

    Sphinx parallel build error:
    AssertionError

If you want to outlaw conditional special features, reject them cleanly
in schema.py, document the restriction in docs/devel/qapi-code-gen.rst,
and explain why in the commit message.  Recommend a separate commit, to
make it stand out in git-log.

> +                # Generated from entity def; info location is approximate.
> +                self.add_line(f":{feat.name}:", feat.info)
> +
> +        self.ensure_blank_line()
> +
>      # Transmogrification core methods
>  
>      def visit_module(self, path: str) -> None:

Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[John Snow]

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

On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This method adds the options/preamble to each definition block. Notably,
> > :since: and :ifcond: are added, as are any "special features" such as
> > :deprecated: and :unstable:.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 6f8f69077b1..85c7ce94564 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -38,7 +38,7 @@
> >  from qapi.error import QAPIError, QAPISemError
> >  from qapi.gen import QAPISchemaVisitor
> >  from qapi.parser import QAPIDoc
> > -from qapi.schema import QAPISchema
> > +from qapi.schema import QAPISchema, QAPISchemaEntity
> >  from qapi.source import QAPISourceInfo
> >
> >  from sphinx import addnodes
> > @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
> >              # +2: correct for zero/one index, then increment by one.
> >              self.add_line_raw("", fname, line + 2)
> >
> > +    # Transmogrification helpers
> > +
> > +    def preamble(self, ent: QAPISchemaEntity) -> None:
> > +        """
> > +        Generate option lines for qapi entity directives.
> > +        """
> > +        if ent.doc and ent.doc.since:
> > +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
> > +            # Generated from the entity's docblock; info location is
> exact.
> > +            self.add_line(f":since: {ent.doc.since.text}",
> ent.doc.since.info)
> > +
> > +        if ent.ifcond.is_present():
> > +            doc = ent.ifcond.docgen()
> > +            # Generated from entity definition; info location is
> approximate.
> > +            self.add_line(f":ifcond: {doc}", ent.info)
> > +
> > +        # Hoist special features such as :deprecated: and :unstable:
> > +        # into the options block for the entity. If, in the future, new
> > +        # special features are added, qapi-domain will chirp about
> > +        # unrecognized options and fail.
> > +        for feat in ent.features:
> > +            if feat.is_special():
> > +                # We don't expect special features to have an ifcond
> property.
> > +                # (Hello, intrepid developer in the future who changed
> that!)
> > +                # ((With luck, you are not me.))
> > +                assert not feat.ifcond.is_present()
>
> Nope :)
>
> The attempt to add a conditional special feature now fails with
>
>     Sphinx parallel build error:
>     AssertionError
>
> If you want to outlaw conditional special features, reject them cleanly
> in schema.py, document the restriction in docs/devel/qapi-code-gen.rst,
> and explain why in the commit message.  Recommend a separate commit, to
> make it stand out in git-log.
>

Do you advocate this? I wasn't sure what it *meant* for a special feature
to be conditional; I couldn't conceive of what it meant to have an ifcond
for "deprecated" or "unstable", for instance. It sounds like it isn't well
defined, but we happen to not expressly forbid it.

I guard against it here because, similarly, I have no idea how to handle
the case where it's true.

I didn't realize we technically allow it, though ... would you like me to
move to expressly forbid it in the parser? (Failing that, I have no idea
how to display this information otherwise, so I'd need you to sketch
something out for me; so my inclination is to forbid it as you suggest.
Future developers can always lift the restriction once they have some
use-case in mind and a plan for how to display that information.)

--js


>
> > +                # Generated from entity def; info location is
> approximate.
> > +                self.add_line(f":{feat.name}:", feat.info)
> > +
> > +        self.ensure_blank_line()
> > +
> >      # Transmogrification core methods
> >
> >      def visit_module(self, path: str) -> None:
>
>

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

Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This method adds the options/preamble to each definition block. Notably,
>> > :since: and :ifcond: are added, as are any "special features" such as
>> > :deprecated: and :unstable:.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
>> >  1 file changed, 32 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> > index 6f8f69077b1..85c7ce94564 100644
>> > --- a/docs/sphinx/qapidoc.py
>> > +++ b/docs/sphinx/qapidoc.py
>> > @@ -38,7 +38,7 @@
>> >  from qapi.error import QAPIError, QAPISemError
>> >  from qapi.gen import QAPISchemaVisitor
>> >  from qapi.parser import QAPIDoc
>> > -from qapi.schema import QAPISchema
>> > +from qapi.schema import QAPISchema, QAPISchemaEntity
>> >  from qapi.source import QAPISourceInfo
>> >
>> >  from sphinx import addnodes
>> > @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
>> >              # +2: correct for zero/one index, then increment by one.
>> >              self.add_line_raw("", fname, line + 2)
>> >
>> > +    # Transmogrification helpers
>> > +
>> > +    def preamble(self, ent: QAPISchemaEntity) -> None:
>> > +        """
>> > +        Generate option lines for qapi entity directives.
>> > +        """
>> > +        if ent.doc and ent.doc.since:
>> > +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
>> > +            # Generated from the entity's docblock; info location is exact.
>> > +            self.add_line(f":since: {ent.doc.since.text}", ent.doc.since.info)
>> > +
>> > +        if ent.ifcond.is_present():
>> > +            doc = ent.ifcond.docgen()
>> > +            # Generated from entity definition; info location is approximate.
>> > +            self.add_line(f":ifcond: {doc}", ent.info)
>> > +
>> > +        # Hoist special features such as :deprecated: and :unstable:
>> > +        # into the options block for the entity. If, in the future, new
>> > +        # special features are added, qapi-domain will chirp about
>> > +        # unrecognized options and fail.
>> > +        for feat in ent.features:
>> > +            if feat.is_special():
>> > +                # We don't expect special features to have an ifcond property.
>> > +                # (Hello, intrepid developer in the future who changed that!)
>> > +                # ((With luck, you are not me.))
>> > +                assert not feat.ifcond.is_present()
>>
>> Nope :)
>>
>> The attempt to add a conditional special feature now fails with
>>
>>     Sphinx parallel build error:
>>     AssertionError
>>
>> If you want to outlaw conditional special features, reject them cleanly
>> in schema.py, document the restriction in docs/devel/qapi-code-gen.rst,
>> and explain why in the commit message.  Recommend a separate commit, to
>> make it stand out in git-log.
>
> Do you advocate this? I wasn't sure what it *meant* for a special feature
> to be conditional; I couldn't conceive of what it meant to have an ifcond
> for "deprecated" or "unstable", for instance. It sounds like it isn't well
> defined, but we happen to not expressly forbid it.

Semantics are clear enough to me.

"Conditional special feature" combines "conditional feature" (which is a
special case of conditional thing) with special feature (which is a
special case of feature).

The QAPI schema language supports compile-time conditionals for certain
things.  Generated code behaves as if the thing didn't exist unless its
condition is true.

QAPI schema features are strings exposed to clients in introspection.

Combine the two: a conditional feature is exposed if and only if its
condition is true.

Existing uses: dynamic-auto-read-only if CONFIG_POSIX, fdset if
CONFIG_BLKIO_VHOST_VDPA_FD.

A special feature is a feature that has special meaning to the
generator, i.e. we generate different code in places when it's present.

Combine: we enable different code for a conditional special feature only
when its condition is true.

No existing uses so far.

Implementation is straightforward, too.

Any code we generate for a conditional thing is guarded by #if
... #endif.

For features, we generate suitable data into qapi-introspect.c.

For special features, we generate a little additional code here and
there; look for .is_special() to find it.

Bug: this additional code lacks #if ... #endif.  Simple oversight,
should be easy enough to fix.

> I guard against it here because, similarly, I have no idea how to handle
> the case where it's true.
>
> I didn't realize we technically allow it, though ... would you like me to
> move to expressly forbid it in the parser? (Failing that, I have no idea
> how to display this information otherwise, so I'd need you to sketch
> something out for me; so my inclination is to forbid it as you suggest.
> Future developers can always lift the restriction once they have some
> use-case in mind and a plan for how to display that information.)

I think we should first make a reasonable effort at figuring out how to
handle conditional special features.  If we fail, we can add the
restriction instead.

How do you handle features in general, and special features in
particular?

How do you handle conditionals in general?

How do you combine feature and conditional?

How could you combine special feature and conditonal?

[...]

Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[John Snow]

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

On Thu, Jan 9, 2025, 5:34 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > This method adds the options/preamble to each definition block.
> Notably,
> >> > :since: and :ifcond: are added, as are any "special features" such as
> >> > :deprecated: and :unstable:.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >> > ---
> >> >  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
> >> >  1 file changed, 32 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> >> > index 6f8f69077b1..85c7ce94564 100644
> >> > --- a/docs/sphinx/qapidoc.py
> >> > +++ b/docs/sphinx/qapidoc.py
> >> > @@ -38,7 +38,7 @@
> >> >  from qapi.error import QAPIError, QAPISemError
> >> >  from qapi.gen import QAPISchemaVisitor
> >> >  from qapi.parser import QAPIDoc
> >> > -from qapi.schema import QAPISchema
> >> > +from qapi.schema import QAPISchema, QAPISchemaEntity
> >> >  from qapi.source import QAPISourceInfo
> >> >
> >> >  from sphinx import addnodes
> >> > @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
> >> >              # +2: correct for zero/one index, then increment by one.
> >> >              self.add_line_raw("", fname, line + 2)
> >> >
> >> > +    # Transmogrification helpers
> >> > +
> >> > +    def preamble(self, ent: QAPISchemaEntity) -> None:
> >> > +        """
> >> > +        Generate option lines for qapi entity directives.
> >> > +        """
> >> > +        if ent.doc and ent.doc.since:
> >> > +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
> >> > +            # Generated from the entity's docblock; info location is
> exact.
> >> > +            self.add_line(f":since: {ent.doc.since.text}",
> ent.doc.since.info)
> >> > +
> >> > +        if ent.ifcond.is_present():
> >> > +            doc = ent.ifcond.docgen()
> >> > +            # Generated from entity definition; info location is
> approximate.
> >> > +            self.add_line(f":ifcond: {doc}", ent.info)
> >> > +
> >> > +        # Hoist special features such as :deprecated: and :unstable:
> >> > +        # into the options block for the entity. If, in the future,
> new
> >> > +        # special features are added, qapi-domain will chirp about
> >> > +        # unrecognized options and fail.
> >> > +        for feat in ent.features:
> >> > +            if feat.is_special():
> >> > +                # We don't expect special features to have an ifcond
> property.
> >> > +                # (Hello, intrepid developer in the future who
> changed that!)
> >> > +                # ((With luck, you are not me.))
> >> > +                assert not feat.ifcond.is_present()
> >>
> >> Nope :)
> >>
> >> The attempt to add a conditional special feature now fails with
> >>
> >>     Sphinx parallel build error:
> >>     AssertionError
> >>
> >> If you want to outlaw conditional special features, reject them cleanly
> >> in schema.py, document the restriction in docs/devel/qapi-code-gen.rst,
> >> and explain why in the commit message.  Recommend a separate commit, to
> >> make it stand out in git-log.
> >
> > Do you advocate this? I wasn't sure what it *meant* for a special feature
> > to be conditional; I couldn't conceive of what it meant to have an ifcond
> > for "deprecated" or "unstable", for instance. It sounds like it isn't
> well
> > defined, but we happen to not expressly forbid it.
>
> Semantics are clear enough to me.
>
> "Conditional special feature" combines "conditional feature" (which is a
> special case of conditional thing) with special feature (which is a
> special case of feature).
>
> The QAPI schema language supports compile-time conditionals for certain
> things.  Generated code behaves as if the thing didn't exist unless its
> condition is true.
>
> QAPI schema features are strings exposed to clients in introspection.
>
> Combine the two: a conditional feature is exposed if and only if its
> condition is true.
>
> Existing uses: dynamic-auto-read-only if CONFIG_POSIX, fdset if
> CONFIG_BLKIO_VHOST_VDPA_FD.
>
> A special feature is a feature that has special meaning to the
> generator, i.e. we generate different code in places when it's present.
>
> Combine: we enable different code for a conditional special feature only
> when its condition is true.
>
> No existing uses so far.
>
> Implementation is straightforward, too.
>

Mechanically, it's well defined... but that isn't my concern.

What I asked you was: "what would it mean for deprecated to be conditional?"

Without understanding that, I have no chance to design a system that
handles that information accordingly for the documentation.


> Any code we generate for a conditional thing is guarded by #if
> ... #endif.
>
> For features, we generate suitable data into qapi-introspect.c.
>
> For special features, we generate a little additional code here and
> there; look for .is_special() to find it.
>
> Bug: this additional code lacks #if ... #endif.  Simple oversight,
> should be easy enough to fix.
>
> > I guard against it here because, similarly, I have no idea how to handle
> > the case where it's true.
> >
> > I didn't realize we technically allow it, though ... would you like me to
> > move to expressly forbid it in the parser? (Failing that, I have no idea
> > how to display this information otherwise, so I'd need you to sketch
> > something out for me; so my inclination is to forbid it as you suggest.
> > Future developers can always lift the restriction once they have some
> > use-case in mind and a plan for how to display that information.)
>
> I think we should first make a reasonable effort at figuring out how to
> handle conditional special features.  If we fail, we can add the
> restriction instead.
>

I don't think I agree; I don't think it's worth spending time defining a
feature we don't use in our minds to then design around a hypothetical
future use.

Easier to admit we don't use it and save defining the semantics for the
future developer who stumbles across a reason for needing it.


> How do you handle features in general, and special features in
> particular?
>

Features: they go in the :feat: field list.
Special features: they go in the :feat: field list, and also receive a
special option flag to the directive to allow for special rendering of the
"signature bar" (the header line for a given entity definition in the
rendered documentation.)

e.g.

@deprecated will generate two things:

1) the definition block will get a :deprecated: option, which enables the
html renderer to (potentially) do special things like color the entry
differently, add a "warning pip"/eyecatch, etc. It's scoped to the whole
definition.

2) A :feat deprecated: line which is added to the features "field list" as
per normal for features.


> How do you handle conditionals in general?
>

In this series, I don't!

...except for top-level conditionals, which are currently passed as-is as
an argument to the :ifcond: directive option. The format of that argument
and what the HTML renderer actually does with it is currently TBD.

(In current prototypes I just print the conditional string in the signature
bar.)


> How do you combine feature and conditional?
>

Notably TBD. Unhandled both in this series as-is *and* in my larger WIP.


> How could you combine special feature and conditonal?
>

Bewilderingly uncertain.

Mechanically, it *could* be handled the same way :ifcond: directive options
are.

e.g.

```
.. qapi:command:: x-beware-of-the-leopard
   :deprecated: {ifcond stuff here}
   :unstable: {ifcond stuff here}

   lorem ipsum dolor sit amet

   :feat deprecated: blah blah (if: ...)
   :feat unstable: blah blah (if: ...)
```

Semantically and in the rendered output, I have absolutely no clue
whatsoever; and doubt I could figure it out without a use case in front of
me to design around ... so I'd prefer to put up a traffic barrier for now.

i.o.w. the design issue arises specifically because special features are
semanticaly special and are "hoisted" to be block-level in the
transmogrified rST doc, and so syntax needs to be designed, implemented and
tested if they are to be conditional.

Yet, we have no such cases to facilitate the design, implementation and
testing.

So, I'd like to prohibit until such time as we have such a case.


> [...]
>
>

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

Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Jan 9, 2025, 5:34 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> John Snow <jsnow@redhat.com> writes:
>> >>
>> >> > This method adds the options/preamble to each definition block. Notably,
>> >> > :since: and :ifcond: are added, as are any "special features" such as
>> >> > :deprecated: and :unstable:.
>> >> >
>> >> > Signed-off-by: John Snow <jsnow@redhat.com>
>> >> > ---
>> >> >  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
>> >> >  1 file changed, 32 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> >> > index 6f8f69077b1..85c7ce94564 100644
>> >> > --- a/docs/sphinx/qapidoc.py
>> >> > +++ b/docs/sphinx/qapidoc.py
>> >> > @@ -38,7 +38,7 @@
>> >> >  from qapi.error import QAPIError, QAPISemError
>> >> >  from qapi.gen import QAPISchemaVisitor
>> >> >  from qapi.parser import QAPIDoc
>> >> > -from qapi.schema import QAPISchema
>> >> > +from qapi.schema import QAPISchema, QAPISchemaEntity
>> >> >  from qapi.source import QAPISourceInfo
>> >> >
>> >> >  from sphinx import addnodes
>> >> > @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
>> >> >              # +2: correct for zero/one index, then increment by one.
>> >> >              self.add_line_raw("", fname, line + 2)
>> >> >
>> >> > +    # Transmogrification helpers
>> >> > +
>> >> > +    def preamble(self, ent: QAPISchemaEntity) -> None:
>> >> > +        """
>> >> > +        Generate option lines for qapi entity directives.
>> >> > +        """
>> >> > +        if ent.doc and ent.doc.since:
>> >> > +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
>> >> > +            # Generated from the entity's docblock; info location is exact.
>> >> > +            self.add_line(f":since: {ent.doc.since.text}", ent.doc.since.info)
>> >> > +
>> >> > +        if ent.ifcond.is_present():
>> >> > +            doc = ent.ifcond.docgen()
>> >> > +            # Generated from entity definition; info location is approximate.
>> >> > +            self.add_line(f":ifcond: {doc}", ent.info)
>> >> > +
>> >> > +        # Hoist special features such as :deprecated: and :unstable:
>> >> > +        # into the options block for the entity. If, in the future, new
>> >> > +        # special features are added, qapi-domain will chirp about
>> >> > +        # unrecognized options and fail.
>> >> > +        for feat in ent.features:
>> >> > +            if feat.is_special():
>> >> > +                # We don't expect special features to have an ifcond property.
>> >> > +                # (Hello, intrepid developer in the future who changed that!)
>> >> > +                # ((With luck, you are not me.))
>> >> > +                assert not feat.ifcond.is_present()
>> >>
>> >> Nope :)
>> >>
>> >> The attempt to add a conditional special feature now fails with
>> >>
>> >>     Sphinx parallel build error:
>> >>     AssertionError
>> >>
>> >> If you want to outlaw conditional special features, reject them cleanly
>> >> in schema.py, document the restriction in docs/devel/qapi-code-gen.rst,
>> >> and explain why in the commit message.  Recommend a separate commit, to
>> >> make it stand out in git-log.
>> >
>> > Do you advocate this? I wasn't sure what it *meant* for a special feature
>> > to be conditional; I couldn't conceive of what it meant to have an ifcond
>> > for "deprecated" or "unstable", for instance. It sounds like it isn't well
>> > defined, but we happen to not expressly forbid it.
>>
>> Semantics are clear enough to me.
>>
>> "Conditional special feature" combines "conditional feature" (which is a
>> special case of conditional thing) with special feature (which is a
>> special case of feature).
>>
>> The QAPI schema language supports compile-time conditionals for certain
>> things.  Generated code behaves as if the thing didn't exist unless its
>> condition is true.
>>
>> QAPI schema features are strings exposed to clients in introspection.
>>
>> Combine the two: a conditional feature is exposed if and only if its
>> condition is true.
>>
>> Existing uses: dynamic-auto-read-only if CONFIG_POSIX, fdset if
>> CONFIG_BLKIO_VHOST_VDPA_FD.
>>
>> A special feature is a feature that has special meaning to the
>> generator, i.e. we generate different code in places when it's present.
>>
>> Combine: we enable different code for a conditional special feature only
>> when its condition is true.
>>
>> No existing uses so far.
>>
>> Implementation is straightforward, too.
>>
>
> Mechanically, it's well defined... but that isn't my concern.
>
> What I asked you was: "what would it mean for deprecated to be conditional?"
>
> Without understanding that, I have no chance to design a system that
> handles that information accordingly for the documentation.

Special feature deprecated means the use of the thing that has it is
deprecated with this particular qemu-system-FOO.

Why "this particular"?  Because it wasn't deprecated in some prior
version of QEMU.  Features (special or not) are always, always about the
specific qemu-system-FOO binary you're talking to via QMP.

Conditionals are compile-time schema configuration.  Either the
condition is true or not at compile time.  If it's true, the thing that
has it is there, else it isn't.

Now combine the two.  If the condition of special feature 'deprecate' is
true, the thing that has it is deprecated with this particular
qemu-system-FOO.  Else it isn't.

>> Any code we generate for a conditional thing is guarded by #if
>> ... #endif.
>>
>> For features, we generate suitable data into qapi-introspect.c.
>>
>> For special features, we generate a little additional code here and
>> there; look for .is_special() to find it.
>>
>> Bug: this additional code lacks #if ... #endif.  Simple oversight,
>> should be easy enough to fix.
>>
>> > I guard against it here because, similarly, I have no idea how to handle
>> > the case where it's true.
>> >
>> > I didn't realize we technically allow it, though ... would you like me to
>> > move to expressly forbid it in the parser? (Failing that, I have no idea
>> > how to display this information otherwise, so I'd need you to sketch
>> > something out for me; so my inclination is to forbid it as you suggest.
>> > Future developers can always lift the restriction once they have some
>> > use-case in mind and a plan for how to display that information.)
>>
>> I think we should first make a reasonable effort at figuring out how to
>> handle conditional special features.  If we fail, we can add the
>> restriction instead.
>
> I don't think I agree; I don't think it's worth spending time defining a
> feature we don't use in our minds to then design around a hypothetical
> future use.
>
> Easier to admit we don't use it and save defining the semantics for the
> future developer who stumbles across a reason for needing it.

Semantics of "apply conditional to feature" don't need defining, because
they are simply the same as "apply conditional to any other thing."
When you have a definition for the general case that works for all the
special cases, then outlawing one special case *increases* complexity.
That's why I'm reluctant.

I figure your actual problem more concrete than "what does it even
mean?", namely how to present conditionals in generated documentation.
In that context, features *may* differ from any other thing!

Aside: the (mostly generated) QMP introspection is about this particular
qemu-system-FOO, like all of QMP, whereas the (mostly generated) QMP
documentation is about any qemu-system-FOO that could be built from this
particular working tree.  May lead to doc generation problems that have
no match elsewhere.

>> How do you handle features in general, and special features in
>> particular?
>>
>
> Features: they go in the :feat: field list.
> Special features: they go in the :feat: field list, and also receive a
> special option flag to the directive to allow for special rendering of the
> "signature bar" (the header line for a given entity definition in the
> rendered documentation.)
>
> e.g.
>
> @deprecated will generate two things:
>
> 1) the definition block will get a :deprecated: option, which enables the
> html renderer to (potentially) do special things like color the entry
> differently, add a "warning pip"/eyecatch, etc. It's scoped to the whole
> definition.
>
> 2) A :feat deprecated: line which is added to the features "field list" as
> per normal for features.
>
>
>> How do you handle conditionals in general?
>>
>
> In this series, I don't!
>
> ...except for top-level conditionals, which are currently passed as-is as
> an argument to the :ifcond: directive option. The format of that argument
> and what the HTML renderer actually does with it is currently TBD.
>
> (In current prototypes I just print the conditional string in the signature
> bar.)
>
>
>> How do you combine feature and conditional?
>>
>
> Notably TBD. Unhandled both in this series as-is *and* in my larger WIP.

How do you combine definition and conditional?  You answered that right
above: largely TBD.

How do you combine enum value / object type member / command argument /
event argument and conditional?  Also answered right above: not yet.

What does that mean for the doc generator code?  Does it simply ignore
conditionals there?

>> How could you combine special feature and conditonal?
>>
>
> Bewilderingly uncertain.
>
> Mechanically, it *could* be handled the same way :ifcond: directive options
> are.
>
> e.g.
>
> ```
> .. qapi:command:: x-beware-of-the-leopard
>    :deprecated: {ifcond stuff here}
>    :unstable: {ifcond stuff here}
>
>    lorem ipsum dolor sit amet
>
>    :feat deprecated: blah blah (if: ...)
>    :feat unstable: blah blah (if: ...)
> ```
>
> Semantically and in the rendered output, I have absolutely no clue
> whatsoever; and doubt I could figure it out without a use case in front of
> me to design around ... so I'd prefer to put up a traffic barrier for now.
>
> i.o.w. the design issue arises specifically because special features are
> semanticaly special and are "hoisted" to be block-level in the
> transmogrified rST doc, and so syntax needs to be designed, implemented and
> tested if they are to be conditional.

You choose to make them special in the documentation.  They haven't been
special there so far.  I'm not telling you not to, I'm just pointing out
this is a problem we elect to have :)

> Yet, we have no such cases to facilitate the design, implementation and
> testing.
>
> So, I'd like to prohibit until such time as we have such a case.

I believe what you need isn't so much an explanation of semantics, it's
use cases.  And that's fair!

Let me offer two.

1. Imagine we're trying to develop something big & complex enough to
   make keeping it out of tree until done impractical.  We feel even an
   in-tree branch would be impractical.  Instead, we commit it to
   master, default off, configure --enable-complex-thing to get it.  Not
   exactly something we do all the time, but not outlandish, either.

   Further imagine that the big & complex thing will involve some new
   QMP commands replacing existing ones.  As usual, we want to mark the
   ones being replaced deprecated, so we can remove them after a grace
   period.

   But simply deprecating them in the schema would be premature!  The
   big & complex thing may fail, and if it does, we rip it out.  If it
   succeeds, we enable it unconditionally.

   We can express what we're doing by making feature deprecated
   conditional on CONFIG_COMPLEX_THING, which is defined by
   --enable-complex-thing.

   That way, the complex thing doesn't affect the QMP interface unless
   we enable it.  Good.  And if we enable it, we do get the deprecation,
   and can test management applications cope with it.

2. Imagine we turned allow-preconfig into a special feature.  Further
   imagine some whole-stack feature requires a certain command to have
   allow-preconfig, and you got tasked with implementing it.  Which you
   duly did, to everybody's satisfaction.  It is now days to the
   release, rc4 has sailed, and some dude comes out of the woodwork to
   report the command crashes in preconfig state.  Weird!  It worked
   *fine* in your meticulous testing.  After some back and forth, you
   discover that the reporter builds with --enable-exotic-crap, which
   you didn't test, because you weren't even aware it exists.  You
   enable it, and now the command crashes for you, too.  Fixing this
   after rc4 is out of the question, too much churn.  You could,
   however, make feature allow-preconfig conditional on not
   CONFIG_EXOTIC_CRAP.

>> [...]

Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[John Snow]

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

On Fri, Jan 10, 2025 at 7:19 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Thu, Jan 9, 2025, 5:34 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >> >
> >> >> John Snow <jsnow@redhat.com> writes:
> >> >>
> >> >> > This method adds the options/preamble to each definition block.
> Notably,
> >> >> > :since: and :ifcond: are added, as are any "special features" such
> as
> >> >> > :deprecated: and :unstable:.
> >> >> >
> >> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >> >> > ---
> >> >> >  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
> >> >> >  1 file changed, 32 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> >> >> > index 6f8f69077b1..85c7ce94564 100644
> >> >> > --- a/docs/sphinx/qapidoc.py
> >> >> > +++ b/docs/sphinx/qapidoc.py
> >> >> > @@ -38,7 +38,7 @@
> >> >> >  from qapi.error import QAPIError, QAPISemError
> >> >> >  from qapi.gen import QAPISchemaVisitor
> >> >> >  from qapi.parser import QAPIDoc
> >> >> > -from qapi.schema import QAPISchema
> >> >> > +from qapi.schema import QAPISchema, QAPISchemaEntity
> >> >> >  from qapi.source import QAPISourceInfo
> >> >> >
> >> >> >  from sphinx import addnodes
> >> >> > @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
> >> >> >              # +2: correct for zero/one index, then increment by
> one.
> >> >> >              self.add_line_raw("", fname, line + 2)
> >> >> >
> >> >> > +    # Transmogrification helpers
> >> >> > +
> >> >> > +    def preamble(self, ent: QAPISchemaEntity) -> None:
> >> >> > +        """
> >> >> > +        Generate option lines for qapi entity directives.
> >> >> > +        """
> >> >> > +        if ent.doc and ent.doc.since:
> >> >> > +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
> >> >> > +            # Generated from the entity's docblock; info location
> is exact.
> >> >> > +            self.add_line(f":since: {ent.doc.since.text}",
> ent.doc.since.info)
> >> >> > +
> >> >> > +        if ent.ifcond.is_present():
> >> >> > +            doc = ent.ifcond.docgen()
> >> >> > +            # Generated from entity definition; info location is
> approximate.
> >> >> > +            self.add_line(f":ifcond: {doc}", ent.info)
> >> >> > +
> >> >> > +        # Hoist special features such as :deprecated: and
> :unstable:
> >> >> > +        # into the options block for the entity. If, in the
> future, new
> >> >> > +        # special features are added, qapi-domain will chirp about
> >> >> > +        # unrecognized options and fail.
> >> >> > +        for feat in ent.features:
> >> >> > +            if feat.is_special():
> >> >> > +                # We don't expect special features to have an
> ifcond property.
> >> >> > +                # (Hello, intrepid developer in the future who
> changed that!)
> >> >> > +                # ((With luck, you are not me.))
> >> >> > +                assert not feat.ifcond.is_present()
> >> >>
> >> >> Nope :)
> >> >>
> >> >> The attempt to add a conditional special feature now fails with
> >> >>
> >> >>     Sphinx parallel build error:
> >> >>     AssertionError
> >> >>
> >> >> If you want to outlaw conditional special features, reject them
> cleanly
> >> >> in schema.py, document the restriction in
> docs/devel/qapi-code-gen.rst,
> >> >> and explain why in the commit message.  Recommend a separate commit,
> to
> >> >> make it stand out in git-log.
> >> >
> >> > Do you advocate this? I wasn't sure what it *meant* for a special
> feature
> >> > to be conditional; I couldn't conceive of what it meant to have an
> ifcond
> >> > for "deprecated" or "unstable", for instance. It sounds like it isn't
> well
> >> > defined, but we happen to not expressly forbid it.
> >>
> >> Semantics are clear enough to me.
> >>
> >> "Conditional special feature" combines "conditional feature" (which is a
> >> special case of conditional thing) with special feature (which is a
> >> special case of feature).
> >>
> >> The QAPI schema language supports compile-time conditionals for certain
> >> things.  Generated code behaves as if the thing didn't exist unless its
> >> condition is true.
> >>
> >> QAPI schema features are strings exposed to clients in introspection.
> >>
> >> Combine the two: a conditional feature is exposed if and only if its
> >> condition is true.
> >>
> >> Existing uses: dynamic-auto-read-only if CONFIG_POSIX, fdset if
> >> CONFIG_BLKIO_VHOST_VDPA_FD.
> >>
> >> A special feature is a feature that has special meaning to the
> >> generator, i.e. we generate different code in places when it's present.
> >>
> >> Combine: we enable different code for a conditional special feature only
> >> when its condition is true.
> >>
> >> No existing uses so far.
> >>
> >> Implementation is straightforward, too.
> >>
> >
> > Mechanically, it's well defined... but that isn't my concern.
> >
> > What I asked you was: "what would it mean for deprecated to be
> conditional?"
> >
> > Without understanding that, I have no chance to design a system that
> > handles that information accordingly for the documentation.
>
> Special feature deprecated means the use of the thing that has it is
> deprecated with this particular qemu-system-FOO.
>
> Why "this particular"?  Because it wasn't deprecated in some prior
> version of QEMU.  Features (special or not) are always, always about the
> specific qemu-system-FOO binary you're talking to via QMP.
>
> Conditionals are compile-time schema configuration.  Either the
> condition is true or not at compile time.  If it's true, the thing that
> has it is there, else it isn't.
>
> Now combine the two.  If the condition of special feature 'deprecate' is
> true, the thing that has it is deprecated with this particular
> qemu-system-FOO.  Else it isn't.
>
> >> Any code we generate for a conditional thing is guarded by #if
> >> ... #endif.
> >>
> >> For features, we generate suitable data into qapi-introspect.c.
> >>
> >> For special features, we generate a little additional code here and
> >> there; look for .is_special() to find it.
> >>
> >> Bug: this additional code lacks #if ... #endif.  Simple oversight,
> >> should be easy enough to fix.
> >>
> >> > I guard against it here because, similarly, I have no idea how to
> handle
> >> > the case where it's true.
> >> >
> >> > I didn't realize we technically allow it, though ... would you like
> me to
> >> > move to expressly forbid it in the parser? (Failing that, I have no
> idea
> >> > how to display this information otherwise, so I'd need you to sketch
> >> > something out for me; so my inclination is to forbid it as you
> suggest.
> >> > Future developers can always lift the restriction once they have some
> >> > use-case in mind and a plan for how to display that information.)
> >>
> >> I think we should first make a reasonable effort at figuring out how to
> >> handle conditional special features.  If we fail, we can add the
> >> restriction instead.
> >
> > I don't think I agree; I don't think it's worth spending time defining a
> > feature we don't use in our minds to then design around a hypothetical
> > future use.
> >
> > Easier to admit we don't use it and save defining the semantics for the
> > future developer who stumbles across a reason for needing it.
>
> Semantics of "apply conditional to feature" don't need defining, because
> they are simply the same as "apply conditional to any other thing."
> When you have a definition for the general case that works for all the
> special cases, then outlawing one special case *increases* complexity.
> That's why I'm reluctant.
>
> I figure your actual problem more concrete than "what does it even
> mean?", namely how to present conditionals in generated documentation.
> In that context, features *may* differ from any other thing!
>
> Aside: the (mostly generated) QMP introspection is about this particular
> qemu-system-FOO, like all of QMP, whereas the (mostly generated) QMP
> documentation is about any qemu-system-FOO that could be built from this
> particular working tree.  May lead to doc generation problems that have
> no match elsewhere.
>
> >> How do you handle features in general, and special features in
> >> particular?
> >>
> >
> > Features: they go in the :feat: field list.
> > Special features: they go in the :feat: field list, and also receive a
> > special option flag to the directive to allow for special rendering of
> the
> > "signature bar" (the header line for a given entity definition in the
> > rendered documentation.)
> >
> > e.g.
> >
> > @deprecated will generate two things:
> >
> > 1) the definition block will get a :deprecated: option, which enables the
> > html renderer to (potentially) do special things like color the entry
> > differently, add a "warning pip"/eyecatch, etc. It's scoped to the whole
> > definition.
> >
> > 2) A :feat deprecated: line which is added to the features "field list"
> as
> > per normal for features.
> >
> >
> >> How do you handle conditionals in general?
> >>
> >
> > In this series, I don't!
> >
> > ...except for top-level conditionals, which are currently passed as-is as
> > an argument to the :ifcond: directive option. The format of that argument
> > and what the HTML renderer actually does with it is currently TBD.
> >
> > (In current prototypes I just print the conditional string in the
> signature
> > bar.)
> >
> >
> >> How do you combine feature and conditional?
> >>
> >
> > Notably TBD. Unhandled both in this series as-is *and* in my larger WIP.
>
> How do you combine definition and conditional?  You answered that right
> above: largely TBD.
>
> How do you combine enum value / object type member / command argument /
> event argument and conditional?  Also answered right above: not yet.
>
> What does that mean for the doc generator code?  Does it simply ignore
> conditionals there?
>
> >> How could you combine special feature and conditonal?
> >>
> >
> > Bewilderingly uncertain.
> >
> > Mechanically, it *could* be handled the same way :ifcond: directive
> options
> > are.
> >
> > e.g.
> >
> > ```
> > .. qapi:command:: x-beware-of-the-leopard
> >    :deprecated: {ifcond stuff here}
> >    :unstable: {ifcond stuff here}
> >
> >    lorem ipsum dolor sit amet
> >
> >    :feat deprecated: blah blah (if: ...)
> >    :feat unstable: blah blah (if: ...)
> > ```
> >
> > Semantically and in the rendered output, I have absolutely no clue
> > whatsoever; and doubt I could figure it out without a use case in front
> of
> > me to design around ... so I'd prefer to put up a traffic barrier for
> now.
> >
> > i.o.w. the design issue arises specifically because special features are
> > semanticaly special and are "hoisted" to be block-level in the
> > transmogrified rST doc, and so syntax needs to be designed, implemented
> and
> > tested if they are to be conditional.
>
> You choose to make them special in the documentation.  They haven't been
> special there so far.  I'm not telling you not to, I'm just pointing out
> this is a problem we elect to have :)
>
> > Yet, we have no such cases to facilitate the design, implementation and
> > testing.
> >
> > So, I'd like to prohibit until such time as we have such a case.
>
> I believe what you need isn't so much an explanation of semantics, it's
> use cases.  And that's fair!
>
> Let me offer two.
>
> 1. Imagine we're trying to develop something big & complex enough to
>    make keeping it out of tree until done impractical.  We feel even an
>    in-tree branch would be impractical.  Instead, we commit it to
>    master, default off, configure --enable-complex-thing to get it.  Not
>    exactly something we do all the time, but not outlandish, either.
>
>    Further imagine that the big & complex thing will involve some new
>    QMP commands replacing existing ones.  As usual, we want to mark the
>    ones being replaced deprecated, so we can remove them after a grace
>    period.
>
>    But simply deprecating them in the schema would be premature!  The
>    big & complex thing may fail, and if it does, we rip it out.  If it
>    succeeds, we enable it unconditionally.
>
>    We can express what we're doing by making feature deprecated
>    conditional on CONFIG_COMPLEX_THING, which is defined by
>    --enable-complex-thing.
>
>    That way, the complex thing doesn't affect the QMP interface unless
>    we enable it.  Good.  And if we enable it, we do get the deprecation,
>    and can test management applications cope with it.
>
> 2. Imagine we turned allow-preconfig into a special feature.  Further
>    imagine some whole-stack feature requires a certain command to have
>    allow-preconfig, and you got tasked with implementing it.  Which you
>    duly did, to everybody's satisfaction.  It is now days to the
>    release, rc4 has sailed, and some dude comes out of the woodwork to
>    report the command crashes in preconfig state.  Weird!  It worked
>    *fine* in your meticulous testing.  After some back and forth, you
>    discover that the reporter builds with --enable-exotic-crap, which
>    you didn't test, because you weren't even aware it exists.  You
>    enable it, and now the command crashes for you, too.  Fixing this
>    after rc4 is out of the question, too much churn.  You could,
>    however, make feature allow-preconfig conditional on not
>    CONFIG_EXOTIC_CRAP.
>
> >> [...]
>
>
Hm, alright. I think I'll just stub out conditionals with a TODO and circle
back to how I'll handle them; I need to do a pass on ifcond handling in
general, so I guess I'll get to it then. I still think it's a little weird,
but you don't, and you're the QAPI guy ;)

--js

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

Re: [PATCH 11/23] docs/qapidoc: add preamble() method

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Jan 10, 2025 at 7:19 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > On Thu, Jan 9, 2025, 5:34 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> John Snow <jsnow@redhat.com> writes:
>> >>
>> >> > On Fri, Dec 20, 2024 at 9:15 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> John Snow <jsnow@redhat.com> writes:
>> >> >>
>> >> >> > This method adds the options/preamble to each definition block. Notably,
>> >> >> > :since: and :ifcond: are added, as are any "special features" such as
>> >> >> > :deprecated: and :unstable:.
>> >> >> >
>> >> >> > Signed-off-by: John Snow <jsnow@redhat.com>
>> >> >> > ---
>> >> >> >  docs/sphinx/qapidoc.py | 33 ++++++++++++++++++++++++++++++++-
>> >> >> >  1 file changed, 32 insertions(+), 1 deletion(-)
>> >> >> >
>> >> >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> >> >> > index 6f8f69077b1..85c7ce94564 100644
>> >> >> > --- a/docs/sphinx/qapidoc.py
>> >> >> > +++ b/docs/sphinx/qapidoc.py
>> >> >> > @@ -38,7 +38,7 @@
>> >> >> >  from qapi.error import QAPIError, QAPISemError
>> >> >> >  from qapi.gen import QAPISchemaVisitor
>> >> >> >  from qapi.parser import QAPIDoc
>> >> >> > -from qapi.schema import QAPISchema
>> >> >> > +from qapi.schema import QAPISchema, QAPISchemaEntity
>> >> >> >  from qapi.source import QAPISourceInfo
>> >> >> >
>> >> >> >  from sphinx import addnodes
>> >> >> > @@ -125,6 +125,37 @@ def ensure_blank_line(self) -> None:
>> >> >> >              # +2: correct for zero/one index, then increment by one.
>> >> >> >              self.add_line_raw("", fname, line + 2)
>> >> >> >
>> >> >> > +    # Transmogrification helpers
>> >> >> > +
>> >> >> > +    def preamble(self, ent: QAPISchemaEntity) -> None:
>> >> >> > +        """
>> >> >> > +        Generate option lines for qapi entity directives.
>> >> >> > +        """
>> >> >> > +        if ent.doc and ent.doc.since:
>> >> >> > +            assert ent.doc.since.tag == QAPIDoc.Tag.SINCE
>> >> >> > +            # Generated from the entity's docblock; info location is exact.
>> >> >> > +            self.add_line(f":since: {ent.doc.since.text}", ent.doc.since.info)
>> >> >> > +
>> >> >> > +        if ent.ifcond.is_present():
>> >> >> > +            doc = ent.ifcond.docgen()
>> >> >> > +            # Generated from entity definition; info location is approximate.
>> >> >> > +            self.add_line(f":ifcond: {doc}", ent.info)
>> >> >> > +
>> >> >> > +        # Hoist special features such as :deprecated: and :unstable:
>> >> >> > +        # into the options block for the entity. If, in the future, new
>> >> >> > +        # special features are added, qapi-domain will chirp about
>> >> >> > +        # unrecognized options and fail.
>> >> >> > +        for feat in ent.features:
>> >> >> > +            if feat.is_special():
>> >> >> > +                # We don't expect special features to have an ifcond property.
>> >> >> > +                # (Hello, intrepid developer in the future who changed that!)
>> >> >> > +                # ((With luck, you are not me.))
>> >> >> > +                assert not feat.ifcond.is_present()
>> >> >>
>> >> >> Nope :)
>> >> >>
>> >> >> The attempt to add a conditional special feature now fails with
>> >> >>
>> >> >>     Sphinx parallel build error:
>> >> >>     AssertionError
>> >> >>
>> >> >> If you want to outlaw conditional special features, reject them cleanly
>> >> >> in schema.py, document the restriction in docs/devel/qapi-code-gen.rst,
>> >> >> and explain why in the commit message.  Recommend a separate commit, to
>> >> >> make it stand out in git-log.
>> >> >
>> >> > Do you advocate this? I wasn't sure what it *meant* for a special feature
>> >> > to be conditional; I couldn't conceive of what it meant to have an ifcond
>> >> > for "deprecated" or "unstable", for instance. It sounds like it isn't well
>> >> > defined, but we happen to not expressly forbid it.

[...]

>> I believe what you need isn't so much an explanation of semantics, it's
>> use cases.  And that's fair!
>>
>> Let me offer two.
>>
>> 1. Imagine we're trying to develop something big & complex enough to
>>    make keeping it out of tree until done impractical.  We feel even an
>>    in-tree branch would be impractical.  Instead, we commit it to
>>    master, default off, configure --enable-complex-thing to get it.  Not
>>    exactly something we do all the time, but not outlandish, either.
>>
>>    Further imagine that the big & complex thing will involve some new
>>    QMP commands replacing existing ones.  As usual, we want to mark the
>>    ones being replaced deprecated, so we can remove them after a grace
>>    period.
>>
>>    But simply deprecating them in the schema would be premature!  The
>>    big & complex thing may fail, and if it does, we rip it out.  If it
>>    succeeds, we enable it unconditionally.
>>
>>    We can express what we're doing by making feature deprecated
>>    conditional on CONFIG_COMPLEX_THING, which is defined by
>>    --enable-complex-thing.
>>
>>    That way, the complex thing doesn't affect the QMP interface unless
>>    we enable it.  Good.  And if we enable it, we do get the deprecation,
>>    and can test management applications cope with it.
>>
>> 2. Imagine we turned allow-preconfig into a special feature.  Further
>>    imagine some whole-stack feature requires a certain command to have
>>    allow-preconfig, and you got tasked with implementing it.  Which you
>>    duly did, to everybody's satisfaction.  It is now days to the
>>    release, rc4 has sailed, and some dude comes out of the woodwork to
>>    report the command crashes in preconfig state.  Weird!  It worked
>>    *fine* in your meticulous testing.  After some back and forth, you
>>    discover that the reporter builds with --enable-exotic-crap, which
>>    you didn't test, because you weren't even aware it exists.  You
>>    enable it, and now the command crashes for you, too.  Fixing this
>>    after rc4 is out of the question, too much churn.  You could,
>>    however, make feature allow-preconfig conditional on not
>>    CONFIG_EXOTIC_CRAP.
>>
>> >> [...]
>>
>>
> Hm, alright. I think I'll just stub out conditionals with a TODO and circle
> back to how I'll handle them; I need to do a pass on ifcond handling in
> general, so I guess I'll get to it then.

Makes sense (although I'd use FIXME rather than TODO).

If resolving them turns out to be hard, we can talk about adding
restrictions to help you over the hump.

>                                          I still think it's a little weird,
> but you don't, and you're the QAPI guy ;)

I admit constructing believable use cases took a bit of thought.
Calling them a little weird can't offend me :)

[PATCH 12/23] docs/qapidoc: add visit_paragraph() method

[John Snow]

This transforms "formerly known as untagged sections" into our pure
intermediate rST format. These sections are already pure rST, so this
method doesn't do a whole lot except ensure appropriate newlines.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 85c7ce94564..40c02dd413f 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -127,6 +127,15 @@ def ensure_blank_line(self) -> None:
 
     # Transmogrification helpers
 
+    def visit_paragraph(self, section: QAPIDoc.Section) -> None:
+        # Squelch empty paragraphs.
+        if not section.text:
+            return
+
+        self.ensure_blank_line()
+        self.add_lines(section.text, section.info)
+        self.ensure_blank_line()
+
     def preamble(self, ent: QAPISchemaEntity) -> None:
         """
         Generate option lines for qapi entity directives.
-- 
2.47.0

[PATCH 13/23] docs/qapidoc: add visit_errors() method

[John Snow]

Notably, this method does not currently address the formatting issues
present with the "errors" section in QAPIDoc and just vomits the text
verbatim into the rST doc, with somewhat inconsistent results.

To be addressed in a future revision.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 40c02dd413f..7ffca5e13f4 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -136,6 +136,12 @@ def visit_paragraph(self, section: QAPIDoc.Section) -> None:
         self.add_lines(section.text, section.info)
         self.ensure_blank_line()
 
+    def visit_errors(self, section: QAPIDoc.Section) -> None:
+        # FIXME: the formatting for errors may be inconsistent and may
+        # or may not require different newline placement to ensure
+        # proper rendering as a nested list.
+        self.add_lines(f":error:\n{section.text}", section.info)
+
     def preamble(self, ent: QAPISchemaEntity) -> None:
         """
         Generate option lines for qapi entity directives.
-- 
2.47.0

[PATCH 14/23] docs/qapidoc: add format_type() method

[John Snow]

This method is responsible for generating a type name for a given member
with the correct annotations for the QAPI domain. Features and enums do
not *have* types, so they return None. Everything else returns the type
name with a "?" suffix if that type is optional, and ensconced in
[brackets] if it's an array type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 7ffca5e13f4..1c83d1b7aac 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -30,7 +30,7 @@
 import re
 import sys
 import textwrap
-from typing import List
+from typing import List, Optional
 
 from docutils import nodes
 from docutils.parsers.rst import Directive, directives
@@ -38,7 +38,14 @@
 from qapi.error import QAPIError, QAPISemError
 from qapi.gen import QAPISchemaVisitor
 from qapi.parser import QAPIDoc
-from qapi.schema import QAPISchema, QAPISchemaEntity
+from qapi.schema import (
+    QAPISchema,
+    QAPISchemaArrayType,
+    QAPISchemaEntity,
+    QAPISchemaEnumMember,
+    QAPISchemaFeature,
+    QAPISchemaObjectTypeMember,
+)
 from qapi.source import QAPISourceInfo
 
 from sphinx import addnodes
@@ -125,6 +132,25 @@ def ensure_blank_line(self) -> None:
             # +2: correct for zero/one index, then increment by one.
             self.add_line_raw("", fname, line + 2)
 
+    def format_type(self, ent) -> Optional[str]:
+        if isinstance(ent, (QAPISchemaEnumMember, QAPISchemaFeature)):
+            return None
+
+        qapi_type = ent
+        optional = False
+        if isinstance(ent, QAPISchemaObjectTypeMember):
+            qapi_type = ent.type
+            optional = ent.optional
+
+        if isinstance(qapi_type, QAPISchemaArrayType):
+            ret = f"[{qapi_type.element_type.doc_type()}]"
+        else:
+            ret = qapi_type.doc_type()
+        if optional:
+            ret += "?"
+
+        return ret
+
     # Transmogrification helpers
 
     def visit_paragraph(self, section: QAPIDoc.Section) -> None:
-- 
2.47.0

[PATCH 15/23] docs/qapidoc: add add_field() and generate_field() helper methods

[John Snow]

These are simple rST generation methods that assist in getting the types
and formatting correct for a field list entry. add_field() is a more
raw, direct call while generate_field() is intended to be used for
generating the correct field from a member object.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 1c83d1b7aac..7efe2d656c0 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -44,6 +44,7 @@
     QAPISchemaEntity,
     QAPISchemaEnumMember,
     QAPISchemaFeature,
+    QAPISchemaMember,
     QAPISchemaObjectTypeMember,
 )
 from qapi.source import QAPISourceInfo
@@ -132,6 +133,20 @@ def ensure_blank_line(self) -> None:
             # +2: correct for zero/one index, then increment by one.
             self.add_line_raw("", fname, line + 2)
 
+    def add_field(
+        self,
+        kind: str,
+        name: str,
+        body: str,
+        info: QAPISourceInfo,
+        typ: Optional[str] = None,
+    ) -> None:
+        if typ:
+            text = f":{kind} {typ} {name}: {body}"
+        else:
+            text = f":{kind} {name}: {body}"
+        self.add_lines(text, info)
+
     def format_type(self, ent) -> Optional[str]:
         if isinstance(ent, (QAPISchemaEnumMember, QAPISchemaFeature)):
             return None
@@ -151,6 +166,16 @@ def format_type(self, ent) -> Optional[str]:
 
         return ret
 
+    def generate_field(
+        self,
+        kind: str,
+        member: QAPISchemaMember,
+        body: str,
+        info: QAPISourceInfo,
+    ) -> None:
+        typ = self.format_type(member)
+        self.add_field(kind, member.name, body, info, typ)
+
     # Transmogrification helpers
 
     def visit_paragraph(self, section: QAPIDoc.Section) -> None:
-- 
2.47.0

[PATCH 16/23] docs/qapidoc: add visit_feature() method

[John Snow]

The format_type() method is adjusted here to return None for features,
because Features don't have documented types. This makes
generate_field() safe to use for features.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 7efe2d656c0..02f434c09ab 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -187,6 +187,15 @@ def visit_paragraph(self, section: QAPIDoc.Section) -> None:
         self.add_lines(section.text, section.info)
         self.ensure_blank_line()
 
+    def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
+        # FIXME - ifcond for features is not handled at all yet!
+        # Proposal: decorate the right-hand column with some graphical
+        # element to indicate conditional availability?
+        assert section.text  # Guaranteed by parser.py
+        assert section.member
+
+        self.generate_field("feat", section.member, section.text, section.info)
+
     def visit_errors(self, section: QAPIDoc.Section) -> None:
         # FIXME: the formatting for errors may be inconsistent and may
         # or may not require different newline placement to ensure
-- 
2.47.0

Re: [PATCH 16/23] docs/qapidoc: add visit_feature() method

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The format_type() method is adjusted here to return None for features,
> because Features don't have documented types. This makes
> generate_field() safe to use for features.

I'm confused: the patch doesn't touch format_type().

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 7efe2d656c0..02f434c09ab 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -187,6 +187,15 @@ def visit_paragraph(self, section: QAPIDoc.Section) -> None:
>          self.add_lines(section.text, section.info)
>          self.ensure_blank_line()
>  
> +    def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
> +        # FIXME - ifcond for features is not handled at all yet!
> +        # Proposal: decorate the right-hand column with some graphical
> +        # element to indicate conditional availability?
> +        assert section.text  # Guaranteed by parser.py
> +        assert section.member
> +
> +        self.generate_field("feat", section.member, section.text, section.info)
> +
>      def visit_errors(self, section: QAPIDoc.Section) -> None:
>          # FIXME: the formatting for errors may be inconsistent and may
>          # or may not require different newline placement to ensure

Re: [PATCH 16/23] docs/qapidoc: add visit_feature() method

[John Snow]

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

On Fri, Dec 20, 2024 at 9:21 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > The format_type() method is adjusted here to return None for features,
> > because Features don't have documented types. This makes
> > generate_field() safe to use for features.
>
> I'm confused: the patch doesn't touch format_type().
>

Patch refactoring booboo, sorry for the confusion.


>
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 7efe2d656c0..02f434c09ab 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -187,6 +187,15 @@ def visit_paragraph(self, section: QAPIDoc.Section)
> -> None:
> >          self.add_lines(section.text, section.info)
> >          self.ensure_blank_line()
> >
> > +    def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
> > +        # FIXME - ifcond for features is not handled at all yet!
> > +        # Proposal: decorate the right-hand column with some graphical
> > +        # element to indicate conditional availability?
> > +        assert section.text  # Guaranteed by parser.py
> > +        assert section.member
> > +
> > +        self.generate_field("feat", section.member, section.text,
> section.info)
> > +
> >      def visit_errors(self, section: QAPIDoc.Section) -> None:
> >          # FIXME: the formatting for errors may be inconsistent and may
> >          # or may not require different newline placement to ensure
>
>

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

[PATCH 17/23] docs/qapidoc: record current documented entity in transmogrifier

[John Snow]

Keep a record of which entity we're working on documenting for the
purposes of being able to change certain generative features
conditionally and create stronger assertions.

If you find yourself asking: "Wait, but where does the current entity
actually get recorded?!", you're right! That part comes with the
visit_entity() implementation, which gets added later.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 02f434c09ab..c731c597daf 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -74,9 +74,15 @@ def dedent(text: str) -> str:
 
 class Transmogrifier:
     def __init__(self, schema):
+        self._curr_ent = None
         self._result = StringList()
         self.indent = 0
 
+    @property
+    def entity(self) -> QAPISchemaEntity:
+        assert self._curr_ent is not None
+        return self._curr_ent
+
     # General-purpose rST generation functions
 
     def get_indent(self) -> str:
-- 
2.47.0

Re: [PATCH 17/23] docs/qapidoc: record current documented entity in transmogrifier

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Keep a record of which entity we're working on documenting for the
> purposes of being able to change certain generative features
> conditionally and create stronger assertions.
>
> If you find yourself asking: "Wait, but where does the current entity
> actually get recorded?!", you're right! That part comes with the
> visit_entity() implementation, which gets added later.

That's fine, except your title promises "record current documented
entity".  Perhaps "Prepare to record entity being transmogrified".

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 02f434c09ab..c731c597daf 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -74,9 +74,15 @@ def dedent(text: str) -> str:
>  
>  class Transmogrifier:
>      def __init__(self, schema):
> +        self._curr_ent = None
>          self._result = StringList()
>          self.indent = 0
>  
> +    @property
> +    def entity(self) -> QAPISchemaEntity:
> +        assert self._curr_ent is not None
> +        return self._curr_ent
> +
>      # General-purpose rST generation functions
>  
>      def get_indent(self) -> str:

Re: [PATCH 17/23] docs/qapidoc: record current documented entity in transmogrifier

[John Snow]

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

On Fri, Dec 20, 2024 at 9:23 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Keep a record of which entity we're working on documenting for the
> > purposes of being able to change certain generative features
> > conditionally and create stronger assertions.
> >
> > If you find yourself asking: "Wait, but where does the current entity
> > actually get recorded?!", you're right! That part comes with the
> > visit_entity() implementation, which gets added later.
>
> That's fine, except your title promises "record current documented
> entity".  Perhaps "Prepare to record entity being transmogrified".
>

Sometimes, I think I exist in a time and space that doesn't actually exist;
a hallucination of multiple timelines that might exist only in my own
consciousness. I'd be worried if I wasn't an ardent solipsist.

--js


>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 02f434c09ab..c731c597daf 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -74,9 +74,15 @@ def dedent(text: str) -> str:
> >
> >  class Transmogrifier:
> >      def __init__(self, schema):
> > +        self._curr_ent = None
> >          self._result = StringList()
> >          self.indent = 0
> >
> > +    @property
> > +    def entity(self) -> QAPISchemaEntity:
> > +        assert self._curr_ent is not None
> > +        return self._curr_ent
> > +
> >      # General-purpose rST generation functions
> >
> >      def get_indent(self) -> str:
>
>

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

Re: [PATCH 17/23] docs/qapidoc: record current documented entity in transmogrifier

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Dec 20, 2024 at 9:23 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Keep a record of which entity we're working on documenting for the
>> > purposes of being able to change certain generative features
>> > conditionally and create stronger assertions.
>> >
>> > If you find yourself asking: "Wait, but where does the current entity
>> > actually get recorded?!", you're right! That part comes with the
>> > visit_entity() implementation, which gets added later.
>>
>> That's fine, except your title promises "record current documented
>> entity".  Perhaps "Prepare to record entity being transmogrified".
>>
>
> Sometimes, I think I exist in a time and space that doesn't actually exist;
> a hallucination of multiple timelines that might exist only in my own
> consciousness. I'd be worried if I wasn't an ardent solipsist.

I'm glad I haven't caused you worry!  Perhaps I should try this
solipsism thing myself some day.

[...]

[PATCH 18/23] docs/qapidoc: add visit_returns() method

[John Snow]

Generates :returns: fields for explicit returns statements. Note that
this does not presently handle undocumented returns, which is handled in
a later commit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index c731c597daf..9d298117fea 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -41,6 +41,7 @@
 from qapi.schema import (
     QAPISchema,
     QAPISchemaArrayType,
+    QAPISchemaCommand,
     QAPISchemaEntity,
     QAPISchemaEnumMember,
     QAPISchemaFeature,
@@ -202,6 +203,19 @@ def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
 
         self.generate_field("feat", section.member, section.text, section.info)
 
+    def visit_returns(self, section: QAPIDoc.Section) -> None:
+        assert isinstance(self.entity, QAPISchemaCommand)
+        rtype = self.entity.ret_type
+        # q_empty can produce None, but we won't be documenting anything
+        # without an explicit return statement in the doc block, and we
+        # should not have any such explicit statements when there is no
+        # return value.
+        assert rtype
+
+        typ = self.format_type(rtype)
+        assert section.text  # We don't expect empty returns sections.
+        self.add_field("returns", typ, section.text, section.info)
+
     def visit_errors(self, section: QAPIDoc.Section) -> None:
         # FIXME: the formatting for errors may be inconsistent and may
         # or may not require different newline placement to ensure
-- 
2.47.0

[PATCH 19/23] docs/qapidoc: add visit_member() method

[John Snow]

This method is used for generating the "members" of a wide variety of
things, including structs, unions, enums, alternates, etc. The field
name it uses to do so is dependent on the type of entity the "member"
belongs to.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 9d298117fea..fe01ee71c26 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -74,6 +74,16 @@ def dedent(text: str) -> str:
 
 
 class Transmogrifier:
+    # Field names used for different entity types:
+    field_types = {
+        "enum": "value",
+        "struct": "memb",
+        "union": "memb",
+        "event": "memb",
+        "command": "arg",
+        "alternate": "choice",
+    }
+
     def __init__(self, schema):
         self._curr_ent = None
         self._result = StringList()
@@ -84,6 +94,10 @@ def entity(self) -> QAPISchemaEntity:
         assert self._curr_ent is not None
         return self._curr_ent
 
+    @property
+    def member_field_type(self) -> str:
+        return self.field_types[self.entity.meta]
+
     # General-purpose rST generation functions
 
     def get_indent(self) -> str:
@@ -194,6 +208,19 @@ def visit_paragraph(self, section: QAPIDoc.Section) -> None:
         self.add_lines(section.text, section.info)
         self.ensure_blank_line()
 
+    def visit_member(self, section: QAPIDoc.ArgSection) -> None:
+        # TODO: ifcond for members
+        # TODO?: features for members (documented at entity-level,
+        # but sometimes defined per-member. Should we add such
+        # information to member descriptions when we can?)
+        assert section.text
+        self.generate_field(
+            self.member_field_type,
+            section.member,
+            section.text,
+            section.info,
+        )
+
     def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
         # FIXME - ifcond for features is not handled at all yet!
         # Proposal: decorate the right-hand column with some graphical
-- 
2.47.0

[PATCH 20/23] docs/qapidoc: add visit_sections() method

[John Snow]

Implement the actual main dispatch method that processes and handles the
list of doc sections for a given QAPI entity.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index fe01ee71c26..eda6aa0897b 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -278,6 +278,29 @@ def preamble(self, ent: QAPISchemaEntity) -> None:
 
         self.ensure_blank_line()
 
+    def visit_sections(self, ent: QAPISchemaEntity) -> None:
+        sections = ent.doc.all_sections if ent.doc else []
+
+        # Add sections *in the order they are documented*:
+        for section in sections:
+            if section.tag == QAPIDoc.Tag.UNTAGGED:
+                self.visit_paragraph(section)
+            elif section.tag == QAPIDoc.Tag.MEMBER:
+                self.visit_member(section)
+            elif section.tag == QAPIDoc.Tag.FEATURE:
+                self.visit_feature(section)
+            elif section.tag in (QAPIDoc.Tag.SINCE, QAPIDoc.Tag.TODO):
+                # Since is handled in preamble, TODO is skipped intentionally.
+                pass
+            elif section.tag == QAPIDoc.Tag.RETURNS:
+                self.visit_returns(section)
+            elif section.tag == QAPIDoc.Tag.ERRORS:
+                self.visit_errors(section)
+            else:
+                assert False
+
+        self.ensure_blank_line()
+
     # Transmogrification core methods
 
     def visit_module(self, path: str) -> None:
-- 
2.47.0

[PATCH 21/23] docs/qapidoc: add visit_entity()

[John Snow]

Finally, the core entry method for a qapi entity.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index eda6aa0897b..9d3fcaa6559 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -356,6 +356,19 @@ def visit_freeform(self, doc) -> None:
         self.add_lines(text, info)
         self.ensure_blank_line()
 
+    def visit_entity(self, ent):
+        assert ent is not None
+
+        try:
+            self._curr_ent = ent
+            # This line gets credited to the start of the /definition/.
+            self.add_line(f".. qapi:{ent.meta}:: {ent.name}", ent.info)
+            with self.indented():
+                self.preamble(ent)
+                self.visit_sections(ent)
+        finally:
+            self._curr_ent = None
+
 
 # Disable black auto-formatter until re-enabled:
 # fmt: off
-- 
2.47.0

[PATCH 22/23] docs/qapidoc: implement transmogrify() method

[John Snow]

This is the true top-level processor for the new transmogrifier;
responsible both for generating the intermediate rST and then running
the nested parse on that generated document to produce the final
docutils tree that is then - very finally - postprocessed by sphinx for
final rendering to HTML &c.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 47 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 9d3fcaa6559..5dc98871b1f 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -2,6 +2,7 @@
 #
 # QEMU qapidoc QAPI file parsing extension
 #
+# Copyright (c) 2024 Red Hat
 # Copyright (c) 2020 Linaro
 #
 # This work is licensed under the terms of the GNU GPLv2 or later.
@@ -53,12 +54,15 @@
 from sphinx import addnodes
 from sphinx.directives.code import CodeBlock
 from sphinx.errors import ExtensionError
+from sphinx.util import logging
 from sphinx.util.docutils import switch_source_input
 from sphinx.util.nodes import nested_parse_with_titles
 
 
 __version__ = "1.0"
 
+logger = logging.getLogger(__name__)
+
 
 def dedent(text: str) -> str:
     # Adjust indentation to make description text parse as paragraph.
@@ -89,6 +93,10 @@ def __init__(self, schema):
         self._result = StringList()
         self.indent = 0
 
+    @property
+    def result(self) -> StringList:
+        return self._result
+
     @property
     def entity(self) -> QAPISchemaEntity:
         assert self._curr_ent is not None
@@ -824,7 +832,43 @@ def new_serialno(self):
         return "qapidoc-%d" % env.new_serialno("qapidoc")
 
     def transmogrify(self, schema) -> nodes.Element:
-        raise NotImplementedError
+        logger.info("Transmogrifying QAPI to rST ...")
+        vis = Transmogrifier(schema)
+        modules = set()
+
+        for doc in schema.docs:
+            module_source = doc.info.fname
+            if module_source not in modules:
+                vis.visit_module(module_source)
+                modules.add(module_source)
+
+            if doc.symbol:
+                ent = schema.lookup_entity(doc.symbol)
+                assert ent
+                vis.visit_entity(ent)
+            else:
+                vis.visit_freeform(doc)
+
+        logger.info("Transmogrification complete.")
+
+        contentnode = nodes.section()
+        content = vis.result
+        titles_allowed = True
+
+        logger.info("Transmogrifier running nested parse ...")
+        with switch_source_input(self.state, content):
+            if titles_allowed:
+                node: nodes.Element = nodes.section()
+                node.document = self.state.document
+                nested_parse_with_titles(self.state, content, contentnode)
+            else:
+                node = nodes.paragraph()
+                node.document = self.state.document
+                self.state.nested_parse(content, 0, contentnode)
+        logger.info("Transmogrifier's nested parse completed.")
+        sys.stdout.flush()
+
+        return contentnode
 
     def legacy(self, schema) -> nodes.Element:
         vis = QAPISchemaGenRSTVisitor(self)
@@ -958,6 +1002,7 @@ def run(self) -> List[nodes.Node]:
 
 def setup(app):
     """Register qapi-doc directive with Sphinx"""
+    app.setup_extension("qapi-domain")
     app.add_config_value("qapidoc_srctree", None, "env")
     app.add_directive("qapi-doc", QAPIDocDirective)
     app.add_directive("qmp-example", QMPExample)
-- 
2.47.0

[PATCH 23/23] docs/qapidoc: add transmogrifier test document

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/index.rst      |  1 +
 docs/qapi/index.rst | 53 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 docs/qapi/index.rst

diff --git a/docs/index.rst b/docs/index.rst
index 0b9ee9901d9..11c18c598a8 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -18,3 +18,4 @@ Welcome to QEMU's documentation!
    interop/index
    specs/index
    devel/index
+   qapi/index
diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
new file mode 100644
index 00000000000..e40dce09119
--- /dev/null
+++ b/docs/qapi/index.rst
@@ -0,0 +1,53 @@
+########################
+QAPI Transmogrifier Test
+########################
+
+This is a test render of the QEMU QMP reference manual using the new
+"transmogrifier" generator in qapidoc.py in conjunction with the
+qapi-domain.py sphinx extension.
+
+Some notable features:
+
+ * Every QAPI definition visible below is available to be
+   cross-referenced from anywhere else in the Sphinx docs; for example
+   ```blockdev-add``` will render to `blockdev-add`.
+
+ * There are type-specific cross-referencing roles available for
+   alternates, commands, events, enums, structs, unions and modules. for
+   example, ``:qapi:cmd:`block-dirty-bitmap-add``` resolves to
+   :qapi:cmd:`block-dirty-bitmap-add`, and only works for commands. The
+   roles available are ``cmd``, ``alt``, ``event``, ``enum``,
+   ``struct``, ``union``, and ``mod``; with two meta-roles available:
+   ``obj`` for absolutely any QAPI definition, and ``type`` for
+   everything except commands, events, and modules.
+
+ * There is a new `qapi-index` page which can be linked to with
+   ```qapi-index```. There, you can browse a list of all QAPI
+   definitions by type or alphabetically.
+
+ * QAPI definitions are also added to the existing `genindex` page.
+
+ * All member/argument/return types are now cross-references to that
+   type's definition. `chardev-add` is a good example.
+
+ * This work-in-progress version does not perform any inlining.
+
+ * This work-in-progress version actually also ignores branches entirely
+   right now!
+
+ * This version currently does not "prune" unnecessary docs.
+
+ * This version does not add undocumented members or return values.
+
+ * This version does not handle ifcond for anything other than top-level
+   entity definitions.
+
+ * This version renders sections in precisely the order they appear in
+   source, even if that winds up looking silly.
+
+
+.. contents::
+   :depth: 2
+
+.. qapi-doc:: qapi/qapi-schema.json
+   :transmogrify:
-- 
2.47.0

Re: [PATCH 00/23] docs: add basic sphinx-domain rST generator to qapidoc

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> based-on: https://patchew.org/QEMU/20241213011307.2942030-1-jsnow@redhat.com/
>
> Hi!
>
> This series is a very, very barebones implementation for the new QAPI
> doc generator. It does not have many features that I presented on at KVM
> Forum; the point of this patch set is instead to present a stripped down
> basis for ongoing work so we can discuss on-list with full context of
> the code available to do so.
>
> The documentation this series generates is *not suitable* for replacing
> the current document generator, it has a few glaring omissions - on
> purpose - those features have been factored out intentionally so they
> can be reviewed with fuller context and more careful review.
>
> What this series does:
>
> - Adds the new "Transmogrifier" rST generator to qapidoc.py, which
>   generates an in-memory rST document using qapi-domain directives.
> - Adds a test document that showcases this new transmogrifier.

Note to other reviewers: transmogrifier output is
docs/manual/qapi/index.html.

> What this series very notably does not do (yet):
>
> - "ifcond" data for anything other than top-level entities is not
>   considered or rendered. This means "if" statements for features and
>   members are entirely absent.
>
> - The inliner is not present at all. This series renders only
>   documentation exactly as it is exists in the source files.

This item is not even a regression.

> - *branches* are themselves not considered at all; they're skipped
>    entirely for now. They will be included alongside the inliner in
>    either a subsequent series or a followup to this series.
>
> - Undocumented members and return statements are not autogenerated.

The current doc generator auto-generates missing member documentation
("Not documented").  It doesn't auto-generate missing returns
documentation.  I explored auto-generating them, but shelved my work to
not interfere with yours.

> - Pseudofeatures (Things like allow-oob) are not generated as documented
>   features.

What exactly are "pseudofeatures"?

> - Documentation culling: all entities are documented whether or not
>   they're relevant to the wire format.

Also not a regression.

> My goal in doing it this way is to save the "fancy" features for later
> so we can focus on reviewing and tightening up the core functionality of
> the transmogrifier. Once we're on steadier ground, I will re-add the
> fanciful features while adjusting the qapi-domain mechanisms. Once
> everything looks "roughly right, give or take some minor nits", I will
> switch back to the qapi-domain series itself for review before we merge
> everything together.

Makes sense to me.

[...]

>  docs/index.rst         |   1 +
>  docs/qapi/index.rst    |  53 ++++++
>  docs/sphinx/qapidoc.py | 419 ++++++++++++++++++++++++++++++++++++++---
>  scripts/qapi/parser.py |  97 +++++++---
>  scripts/qapi/source.py |   4 +-
>  5 files changed, 524 insertions(+), 50 deletions(-)
>  create mode 100644 docs/qapi/index.rst

The changes to the QAPI generator core (scripts/qapi/) are small, and
spread over just four patches:

    qapi/source: allow multi-line QAPISourceInfo advancing
    qapi/schema: add __repr__ to QAPIDoc.Section
    qapi: expand tags to all doc sections
    qapi/parser: adjust info location for doc body section

Re: [PATCH 00/23] docs: add basic sphinx-domain rST generator to qapidoc

[John Snow]

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

On Thu, Dec 19, 2024 at 7:31 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > based-on:
> https://patchew.org/QEMU/20241213011307.2942030-1-jsnow@redhat.com/
> >
> > Hi!
> >
> > This series is a very, very barebones implementation for the new QAPI
> > doc generator. It does not have many features that I presented on at KVM
> > Forum; the point of this patch set is instead to present a stripped down
> > basis for ongoing work so we can discuss on-list with full context of
> > the code available to do so.
> >
> > The documentation this series generates is *not suitable* for replacing
> > the current document generator, it has a few glaring omissions - on
> > purpose - those features have been factored out intentionally so they
> > can be reviewed with fuller context and more careful review.
> >
> > What this series does:
> >
> > - Adds the new "Transmogrifier" rST generator to qapidoc.py, which
> >   generates an in-memory rST document using qapi-domain directives.
> > - Adds a test document that showcases this new transmogrifier.
>
> Note to other reviewers: transmogrifier output is
> docs/manual/qapi/index.html.
>
> > What this series very notably does not do (yet):
> >
> > - "ifcond" data for anything other than top-level entities is not
> >   considered or rendered. This means "if" statements for features and
> >   members are entirely absent.
> >
> > - The inliner is not present at all. This series renders only
> >   documentation exactly as it is exists in the source files.
>
> This item is not even a regression.
>

No; but the version of this series as sent also does not add "The members
of ..." stubs, which would be a regression. I didn't necessarily intend for
this to be merged as-is; more of a "part one, with additional tricky
elements that require more careful thought isolated into separate patches
for later".

where "later" means "in v2" or "as a follow-up series as we stage things in
a development branch before final submission for inclusion to
origin/master" or whatever the actual mechanism is. I don't have a strong
vision there, really; I just wanted to nail down the basics out in the open
even if that was just between you (Markus) and I and we have a gentleman's
agreement that it looks tentatively OK.


>
> > - *branches* are themselves not considered at all; they're skipped
> >    entirely for now. They will be included alongside the inliner in
> >    either a subsequent series or a followup to this series.
> >
> > - Undocumented members and return statements are not autogenerated.
>
> The current doc generator auto-generates missing member documentation
> ("Not documented").  It doesn't auto-generate missing returns
> documentation.  I explored auto-generating them, but shelved my work to
> not interfere with yours.
>
> > - Pseudofeatures (Things like allow-oob) are not generated as documented
> >   features.
>
> What exactly are "pseudofeatures"?
>

What I've named things like allow-oob that aren't features, but ought to be
documented. We may well decide to promote them to real-deal special
features, or maybe not. My work-in-progress branch currently just adds
"dummy" features to document them. We can discuss this later alongside the
patch that implements this.


>
> > - Documentation culling: all entities are documented whether or not
> >   they're relevant to the wire format.
>
> Also not a regression.
>
> > My goal in doing it this way is to save the "fancy" features for later
> > so we can focus on reviewing and tightening up the core functionality of
> > the transmogrifier. Once we're on steadier ground, I will re-add the
> > fanciful features while adjusting the qapi-domain mechanisms. Once
> > everything looks "roughly right, give or take some minor nits", I will
> > switch back to the qapi-domain series itself for review before we merge
> > everything together.
>
> Makes sense to me.
>
> [...]
>
> >  docs/index.rst         |   1 +
> >  docs/qapi/index.rst    |  53 ++++++
> >  docs/sphinx/qapidoc.py | 419 ++++++++++++++++++++++++++++++++++++++---
> >  scripts/qapi/parser.py |  97 +++++++---
> >  scripts/qapi/source.py |   4 +-
> >  5 files changed, 524 insertions(+), 50 deletions(-)
> >  create mode 100644 docs/qapi/index.rst
>
> The changes to the QAPI generator core (scripts/qapi/) are small, and
> spread over just four patches:
>
>     qapi/source: allow multi-line QAPISourceInfo advancing
>     qapi/schema: add __repr__ to QAPIDoc.Section
>     qapi: expand tags to all doc sections
>     qapi/parser: adjust info location for doc body section
>
>

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

Re: [PATCH 00/23] docs: add basic sphinx-domain rST generator to qapidoc

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, Dec 19, 2024 at 7:31 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > based-on:
>> https://patchew.org/QEMU/20241213011307.2942030-1-jsnow@redhat.com/
>> >
>> > Hi!
>> >
>> > This series is a very, very barebones implementation for the new QAPI
>> > doc generator. It does not have many features that I presented on at KVM
>> > Forum; the point of this patch set is instead to present a stripped down
>> > basis for ongoing work so we can discuss on-list with full context of
>> > the code available to do so.
>> >
>> > The documentation this series generates is *not suitable* for replacing
>> > the current document generator, it has a few glaring omissions - on
>> > purpose - those features have been factored out intentionally so they
>> > can be reviewed with fuller context and more careful review.
>> >
>> > What this series does:
>> >
>> > - Adds the new "Transmogrifier" rST generator to qapidoc.py, which
>> >   generates an in-memory rST document using qapi-domain directives.
>> > - Adds a test document that showcases this new transmogrifier.
>>
>> Note to other reviewers: transmogrifier output is
>> docs/manual/qapi/index.html.
>>
>> > What this series very notably does not do (yet):
>> >
>> > - "ifcond" data for anything other than top-level entities is not
>> >   considered or rendered. This means "if" statements for features and
>> >   members are entirely absent.
>> >
>> > - The inliner is not present at all. This series renders only
>> >   documentation exactly as it is exists in the source files.
>>
>> This item is not even a regression.
>>
>
> No; but the version of this series as sent also does not add "The members
> of ..." stubs, which would be a regression.

Right.

>                                             I didn't necessarily intend for
> this to be merged as-is; more of a "part one, with additional tricky
> elements that require more careful thought isolated into separate patches
> for later".
>
> where "later" means "in v2" or "as a follow-up series as we stage things in
> a development branch before final submission for inclusion to
> origin/master" or whatever the actual mechanism is. I don't have a strong
> vision there, really; I just wanted to nail down the basics out in the open
> even if that was just between you (Markus) and I and we have a gentleman's
> agreement that it looks tentatively OK.

Got it.

>> > - *branches* are themselves not considered at all; they're skipped
>> >    entirely for now. They will be included alongside the inliner in
>> >    either a subsequent series or a followup to this series.
>> >
>> > - Undocumented members and return statements are not autogenerated.
>>
>> The current doc generator auto-generates missing member documentation
>> ("Not documented").  It doesn't auto-generate missing returns
>> documentation.  I explored auto-generating them, but shelved my work to
>> not interfere with yours.
>>
>> > - Pseudofeatures (Things like allow-oob) are not generated as documented
>> >   features.
>>
>> What exactly are "pseudofeatures"?
>>
>
> What I've named things like allow-oob that aren't features, but ought to be
> documented. We may well decide to promote them to real-deal special
> features, or maybe not. My work-in-progress branch currently just adds
> "dummy" features to document them. We can discuss this later alongside the
> patch that implements this.

I agree this is a digression, so feel free to ignore the remainder of my
reply for now.

We have two kinds of flags in the QAPI schema language: features and ad
hoc flags.  The ad hoc flags are 'boxed' (commands and events), 'gen',
'success-response', 'allow-oob', 'allow-preconfig', 'coroutine'
(commands only).

The flags sort into three buckets:

1. Code generation directives that do not affect the external interface,
and thus should not be visible in introspection: 'boxed', 'gen',
'coroutine'.

2. Flags that are visible at the external interface, but don't affect
code generation beyond making them visible in introspection: the
non-special features.

3. Code generation directives that affect the external interface, and
thus are (or should be) visible in introspection.

3a. The special features: are visible.

3b. 'allow-oob': is visible, but differently, because it predates
special features.

3c. 'allow-preconfig': not visible.

3d. 'success-response': not visible, because we use it for QGA only,
which doesn't provide introspection.

Bucket 3 could use cleanup.

[...]

Re: [PATCH 00/23] docs: add basic sphinx-domain rST generator to qapidoc

[John Snow]

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

On Thu, Jan 9, 2025, 6:48 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Thu, Dec 19, 2024 at 7:31 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > based-on:
> >> https://patchew.org/QEMU/20241213011307.2942030-1-jsnow@redhat.com/
> >> >
> >> > Hi!
> >> >
> >> > This series is a very, very barebones implementation for the new QAPI
> >> > doc generator. It does not have many features that I presented on at
> KVM
> >> > Forum; the point of this patch set is instead to present a stripped
> down
> >> > basis for ongoing work so we can discuss on-list with full context of
> >> > the code available to do so.
> >> >
> >> > The documentation this series generates is *not suitable* for
> replacing
> >> > the current document generator, it has a few glaring omissions - on
> >> > purpose - those features have been factored out intentionally so they
> >> > can be reviewed with fuller context and more careful review.
> >> >
> >> > What this series does:
> >> >
> >> > - Adds the new "Transmogrifier" rST generator to qapidoc.py, which
> >> >   generates an in-memory rST document using qapi-domain directives.
> >> > - Adds a test document that showcases this new transmogrifier.
> >>
> >> Note to other reviewers: transmogrifier output is
> >> docs/manual/qapi/index.html.
> >>
> >> > What this series very notably does not do (yet):
> >> >
> >> > - "ifcond" data for anything other than top-level entities is not
> >> >   considered or rendered. This means "if" statements for features and
> >> >   members are entirely absent.
> >> >
> >> > - The inliner is not present at all. This series renders only
> >> >   documentation exactly as it is exists in the source files.
> >>
> >> This item is not even a regression.
> >>
> >
> > No; but the version of this series as sent also does not add "The members
> > of ..." stubs, which would be a regression.
>
> Right.
>
> >                                             I didn't necessarily intend
> for
> > this to be merged as-is; more of a "part one, with additional tricky
> > elements that require more careful thought isolated into separate patches
> > for later".
> >
> > where "later" means "in v2" or "as a follow-up series as we stage things
> in
> > a development branch before final submission for inclusion to
> > origin/master" or whatever the actual mechanism is. I don't have a strong
> > vision there, really; I just wanted to nail down the basics out in the
> open
> > even if that was just between you (Markus) and I and we have a
> gentleman's
> > agreement that it looks tentatively OK.
>
> Got it.
>
> >> > - *branches* are themselves not considered at all; they're skipped
> >> >    entirely for now. They will be included alongside the inliner in
> >> >    either a subsequent series or a followup to this series.
> >> >
> >> > - Undocumented members and return statements are not autogenerated.
> >>
> >> The current doc generator auto-generates missing member documentation
> >> ("Not documented").  It doesn't auto-generate missing returns
> >> documentation.  I explored auto-generating them, but shelved my work to
> >> not interfere with yours.
> >>
> >> > - Pseudofeatures (Things like allow-oob) are not generated as
> documented
> >> >   features.
> >>
> >> What exactly are "pseudofeatures"?
> >>
> >
> > What I've named things like allow-oob that aren't features, but ought to
> be
> > documented. We may well decide to promote them to real-deal special
> > features, or maybe not. My work-in-progress branch currently just adds
> > "dummy" features to document them. We can discuss this later alongside
> the
> > patch that implements this.
>
> I agree this is a digression, so feel free to ignore the remainder of my
> reply for now.
>
> We have two kinds of flags in the QAPI schema language: features and ad
> hoc flags.  The ad hoc flags are 'boxed' (commands and events), 'gen',
> 'success-response', 'allow-oob', 'allow-preconfig', 'coroutine'
> (commands only).
>
> The flags sort into three buckets:
>
> 1. Code generation directives that do not affect the external interface,
> and thus should not be visible in introspection: 'boxed', 'gen',
> 'coroutine'.
>
> 2. Flags that are visible at the external interface, but don't affect
> code generation beyond making them visible in introspection: the
> non-special features.
>
> 3. Code generation directives that affect the external interface, and
> thus are (or should be) visible in introspection.
>
> 3a. The special features: are visible.
>
> 3b. 'allow-oob': is visible, but differently, because it predates
> special features.
>
> 3c. 'allow-preconfig': not visible.
>
> 3d. 'success-response': not visible, because we use it for QGA only,
> which doesn't provide introspection.
>
> Bucket 3 could use cleanup.
>

Yep. We can get into it on the pseudofeatures patch. Your analysis here
matches my understanding.


> [...]
>
>

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