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

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

[John Snow]

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

Hiya! This series is based on a rebased version of the above
series. Apply the above patches to origin/master and then apply this
series and you should be good to go. Or just snitch the patches from my
GitLab branch:

https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2

(... ignore the branch name. I ran into some problems with stacked git
corrupting my branches ...)

If you're just tuning in, this series adds a new sphinx documentation
generator for QAPI as presented on at KVM Forum; the big win here is
cross-references and indices for QMP commands and events. It depends on
the qapi-domain plugin for sphinx which is posted in the pre-requisite
series; it's not polished for review and should be considered
POC-quality. I felt it was easier to review this series backwards,
because the design of the rST generator informs the design of the domain
plugin. (Which makes sense: the rST is generated first, and then it's
parsed. Our review follows the flow of data through the generator.)

Overview:

Patches 1-24: Mostly the same as in v2; implements the very basics of
    the new qapidoc generator. "type" was changed to "kind" for the doc
    section metadata, and "untagged" changed to "plain". Some small
    phrasing tweaks here and there.

Patches 25-26: Add auto-generated stub docs for undocumented members.

Patches 27-29: Restrict the source QAPIDoc syntax slightly and
    differentiate "plain" sections as either intro or details. Necessary
    for the inliner.

Patch 30: Add the "inliner", the component that squishes "inherited"
    arguments/members into a single reference for commands/events.

Patches 31-32: Add auto-generated documentation for commands that return
    a value that is not documented.

Patches 33-35: Document the "out-of-band" property on QMP commands.

Patches 36-38: Add branch support to the inliner. Ish. See below.

Patches 39-40: Cull unused definitions from the generated QMP docs; cull
    anything that has been inlined and no longer needs to be documented
    separately.

Patches 41-42: Add intermediate representation rST document writing in
    DEBUG mode

Things notably still not perfect:

  (ignoring aesthetics; we care only about the rST generator itself in
  this series.)

- ifcond for anything other than root level entires is still ignored
- branch inliner ignores all sections except members (ifcond, details,
  features)
- intro/details separation enforces no plain paragraphs to appear in the
  "middle" of the documentation section; new markup may be desired if we
  want to add annotations to categories/regions instead of to specific
  members/features.

If you want to give this a whirl yourself, build QEMU with documentation
support enabled and look at docs/manual/qapi/index.html for a sample
generation of the QMP manual using the new system. You probably need
sphinx >= 4.0 for the time being to do so.

John Snow (42):
  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: prepare to record entity being transmogrified
  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: disambiguate cross-references
  docs/qapidoc: add transmogrifier test document
  docs/qapidoc: generate entries for undocumented members
  qapi/parser: add undocumented stub members to all_sections
  qapi: differentiate "intro" and "detail" sections
  qapi/parser: prohibit untagged sections between tagged sections
  qapi: Add "Details:" disambiguation marker
  docs/qapidoc: add minimalistic inliner
  docs/qapidoc: autogenerate undocumented return docs
  docs/qapidoc: Add generated returns documentation to inliner
  docs/qmp: add target to Out-of-band execution section
  docs/qapidoc: document the "out-of-band" pseudofeature
  docs/qapidoc: generate out-of-band pseudofeature sections
  qapi/parser: add "meta" kind to QAPIDoc.Kind
  qapi/schema: add __iter__ method to QAPISchemaVariants
  docs/qapi: add branch support to inliner
  qapi/schema: add doc_visible property to QAPISchemaDefinition
  docs/qapidoc: cull (most) un-named entities from docs
  qapi: resolve filenames in info structures
  docs/qapidoc: add intermediate output debugger

 docs/devel/codebase.rst         |   6 +-
 docs/glossary.rst               |  10 +-
 docs/index.rst                  |   1 +
 docs/interop/qmp-spec.rst       |   2 +
 docs/qapi/index.rst             |  53 +++
 docs/sphinx/qapidoc.py          | 716 ++++++++++++++++++++++++++++++--
 qapi/machine.json               |   2 +
 qapi/migration.json             |   4 +
 qapi/net.json                   |   4 +-
 qapi/qom.json                   |   8 +-
 qapi/yank.json                  |   2 +
 scripts/qapi/introspect.py      |   4 +-
 scripts/qapi/parser.py          | 178 ++++++--
 scripts/qapi/schema.py          |  48 ++-
 scripts/qapi/source.py          |   4 +-
 scripts/qapi/types.py           |   4 +-
 scripts/qapi/visit.py           |   4 +-
 tests/qapi-schema/doc-good.json |   4 +-
 tests/qapi-schema/doc-good.out  |  12 +-
 tests/qapi-schema/doc-good.txt  |   8 +-
 tests/qapi-schema/test-qapi.py  |   4 +-
 21 files changed, 975 insertions(+), 103 deletions(-)
 create mode 100644 docs/qapi/index.rst

-- 
2.48.1

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

[John Snow]

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

(This patch also 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

(under at least pylint 3.3.1; more robust tamping down of the
environment needed to consistently perform checks will happen later -
hopefully soon, sorry for the inconvenience.)

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

[PATCH 02/42] 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.48.1

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

[John Snow]

Since commit 3c5f6114 (qapi: remove "Example" doc section), Example
sections no longer exist, 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.48.1

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

[John Snow]

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

The qapi-schema tests are updated to account for the new section names;
mostly "TODO" becomes "Todo" and `None` becomes "Plain".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py         |   7 ++-
 scripts/qapi/parser.py         | 109 ++++++++++++++++++++++++---------
 tests/qapi-schema/doc-good.out |  10 +--
 tests/qapi-schema/test-qapi.py |   2 +-
 4 files changed, 90 insertions(+), 38 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 61997fd21af..d622398f1da 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.kind == QAPIDoc.Kind.TODO:
                 # Hide TODO: sections
                 continue
 
-            if not section.tag:
+            if section.kind == QAPIDoc.Kind.PLAIN:
                 # 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.kind.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..c3004aa70c6 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.Kind.from_string(match.group(1))
+                    )
                     text = line[match.end():]
                     if text:
                         doc.append_line(text)
@@ -586,7 +590,7 @@ def get_doc(self) -> 'QAPIDoc':
                         self,
                         "unexpected '=' markup in definition documentation")
                 else:
-                    # tag-less paragraph
+                    # plain paragraph(s)
                     doc.ensure_untagged_section(self.info)
                     doc.append_line(line)
                     line = self.get_doc_paragraph(doc)
@@ -635,14 +639,37 @@ class QAPIDoc:
     Free-form documentation blocks consist only of a body section.
     """
 
+    class Kind(enum.Enum):
+        PLAIN = 0
+        MEMBER = 1
+        FEATURE = 2
+        RETURNS = 3
+        ERRORS = 4
+        SINCE = 5
+        TODO = 6
+
+        @staticmethod
+        def from_string(kind: str) -> 'QAPIDoc.Kind':
+            return QAPIDoc.Kind[kind.upper()]
+
+        def text_required(self) -> bool:
+            # Only "plain" sections can be empty
+            return self.value not in (0,)
+
+        def __str__(self) -> str:
+            return self.name.title()
+
     class Section:
         # pylint: disable=too-few-public-methods
-        def __init__(self, info: QAPISourceInfo,
-                     tag: Optional[str] = None):
+        def __init__(
+            self,
+            info: QAPISourceInfo,
+            kind: 'QAPIDoc.Kind',
+        ):
             # section source info, i.e. where it begins
             self.info = info
-            # section tag, if any ('Returns', '@name', ...)
-            self.tag = tag
+            # section kind
+            self.kind = kind
             # section text without tag
             self.text = ''
 
@@ -650,8 +677,14 @@ def append_line(self, line: str) -> None:
             self.text += line + '\n'
 
     class ArgSection(Section):
-        def __init__(self, info: QAPISourceInfo, tag: str):
-            super().__init__(info, tag)
+        def __init__(
+            self,
+            info: QAPISourceInfo,
+            kind: 'QAPIDoc.Kind',
+            name: str
+        ):
+            super().__init__(info, kind)
+            self.name = name
             self.member: Optional['QAPISchemaMember'] = None
 
         def connect(self, member: 'QAPISchemaMember') -> None:
@@ -663,7 +696,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.Kind.PLAIN)
+        ]
         # the body section
         self.body: Optional[QAPIDoc.Section] = self.all_sections[0]
         # dicts mapping parameter/feature names to their description
@@ -680,12 +715,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.kind.text_required() and section.text == '':
                 raise QAPISemError(
-                    section.info, "text required after '%s:'" % section.tag)
+                    section.info, "text required after '%s:'" % section.kind)
 
-    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:
+        kind = QAPIDoc.Kind.PLAIN
+
+        if self.all_sections and self.all_sections[-1].kind == kind:
             # extend current section
             section = self.all_sections[-1]
             if not section.text:
@@ -693,46 +733,56 @@ 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, kind)
         self.sections.append(section)
         self.all_sections.append(section)
 
-    def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None:
-        section = self.Section(info, tag)
-        if tag == 'Returns':
+    def new_tagged_section(
+        self,
+        info: QAPISourceInfo,
+        kind: 'QAPIDoc.Kind',
+    ) -> None:
+        section = self.Section(info, kind)
+        if kind == QAPIDoc.Kind.RETURNS:
             if self.returns:
                 raise QAPISemError(
-                    info, "duplicated '%s' section" % tag)
+                    info, "duplicated '%s' section" % kind)
             self.returns = section
-        elif tag == 'Errors':
+        elif kind == QAPIDoc.Kind.ERRORS:
             if self.errors:
                 raise QAPISemError(
-                    info, "duplicated '%s' section" % tag)
+                    info, "duplicated '%s' section" % kind)
             self.errors = section
-        elif tag == 'Since':
+        elif kind == QAPIDoc.Kind.SINCE:
             if self.since:
                 raise QAPISemError(
-                    info, "duplicated '%s' section" % tag)
+                    info, "duplicated '%s' section" % kind)
             self.since = section
         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,
+        kind: 'QAPIDoc.Kind',
+        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, kind, 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.Kind.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.Kind.FEATURE, self.features)
 
     def append_line(self, line: str) -> None:
         self.all_sections[-1].append_line(line)
@@ -744,8 +794,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.Kind.MEMBER, member.name)
+            self.args[member.name] = section
         self.args[member.name].connect(member)
 
     def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index ec277be91e9..2d33a305ee7 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=Plain
 @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=Plain
 .. note:: @arg3 is undocumented
     section=Returns
 @Object
     section=Errors
 some
-    section=TODO
+    section=Todo
 frobnicate
-    section=None
+    section=Plain
 .. admonition:: Notes
 
  - Lorem ipsum dolor sit amet
@@ -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=Plain
 .. qmp-example::
 
    -> "this example"
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 7e3f9f4aa1f..bca924309be 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -131,7 +131,7 @@ def test_frontend(fname):
         for feat, section in doc.features.items():
             print('    feature=%s\n%s' % (feat, section.text))
         for section in doc.sections:
-            print('    section=%s\n%s' % (section.tag, section.text))
+            print('    section=%s\n%s' % (section.kind, section.text))
 
 
 def open_test_result(dir_name, file_name, update):
-- 
2.48.1

[PATCH 05/42] 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 c3004aa70c6..af5d7bf892c 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -673,6 +673,9 @@ def __init__(
             # section text without tag
             self.text = ''
 
+        def __repr__(self) -> str:
+            return f"<QAPIDoc.Section kind={self.kind!r} text={self.text!r}>"
+
         def append_line(self, line: str) -> None:
             self.text += line + '\n'
 
-- 
2.48.1

[PATCH 06/42] 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 d622398f1da..dc72f3fd3f3 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.48.1

[PATCH 07/42] 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 dc72f3fd3f3..6593c3f28cd 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.48.1

[PATCH 08/42] 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 6593c3f28cd..658eae3e386 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.48.1

[PATCH 09/42] 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>
Reviewed-by: Markus Armbruster <armbru@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.48.1

[PATCH 10/42] 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 658eae3e386..c42cc3705aa 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:
+        # TODO: Once the old qapidoc transformer is deprecated, freeform
+        # sections can be updated to pure rST, and this transformed removed.
+        #
+        # 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.48.1

[PATCH 11/42] 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 | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index c42cc3705aa..97868e5c375 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,36 @@ 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.kind == QAPIDoc.Kind.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 until they are handled in
+        # qapi-domain.
+        for feat in ent.features:
+            if feat.is_special():
+                # FIXME: handle ifcond if present. How to display that
+                # information is TBD.
+                # 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.48.1

[PATCH 12/42] 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 97868e5c375..4b4cd6359e0 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.48.1

[PATCH 13/42] 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 4b4cd6359e0..64d13565c60 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.48.1

[PATCH 14/42] 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 64d13565c60..b1eaac6e215 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.48.1

[PATCH 15/42] 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 b1eaac6e215..6c0e7b6004c 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.48.1

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

[John Snow]

This adds a simple ":feat name: lorem ipsum ..." line to the generated
rST document, so at the moment it's only for "top level" 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 6c0e7b6004c..e9af072c5a6 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.48.1

[PATCH 17/42] docs/qapidoc: prepare to record entity being transmogrified

[John Snow]

Prepare to 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 e9af072c5a6..a4dbfa3b3dd 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.48.1

[PATCH 18/42] 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 a4dbfa3b3dd..cbc62eb5084 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.48.1

[PATCH 19/42] 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 cbc62eb5084..5f00615ae32 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.48.1

[PATCH 20/42] 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 5f00615ae32..73076a7d6ae 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -277,6 +277,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.kind == QAPIDoc.Kind.PLAIN:
+                self.visit_paragraph(section)
+            elif section.kind == QAPIDoc.Kind.MEMBER:
+                self.visit_member(section)
+            elif section.kind == QAPIDoc.Kind.FEATURE:
+                self.visit_feature(section)
+            elif section.kind in (QAPIDoc.Kind.SINCE, QAPIDoc.Kind.TODO):
+                # Since is handled in preamble, TODO is skipped intentionally.
+                pass
+            elif section.kind == QAPIDoc.Kind.RETURNS:
+                self.visit_returns(section)
+            elif section.kind == QAPIDoc.Kind.ERRORS:
+                self.visit_errors(section)
+            else:
+                assert False
+
+        self.ensure_blank_line()
+
     # Transmogrification core methods
 
     def visit_module(self, path: str) -> None:
-- 
2.48.1

[PATCH 21/42] 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 73076a7d6ae..a2fc8d25ff7 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -355,6 +355,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.48.1

[PATCH 22/42] 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 a2fc8d25ff7..5d946a46637 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
@@ -823,7 +831,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)
@@ -957,6 +1001,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.48.1

[PATCH 23/42] docs: disambiguate cross-references

[John Snow]

The next patch will engage the qapidoc transmogrifier, which creates a
lot of cross-reference targets. Some of the existing targets
("migration", "qom", "replay") will become ambiguous as a result. Nail
them down more explicitly to prevent ambiguous cross-reference warnings.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/devel/codebase.rst |  6 +++---
 docs/glossary.rst       | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/docs/devel/codebase.rst b/docs/devel/codebase.rst
index 4039875ee04..1b09953197b 100644
--- a/docs/devel/codebase.rst
+++ b/docs/devel/codebase.rst
@@ -23,7 +23,7 @@ Some of the main QEMU subsystems are:
 - `Devices<device-emulation>` & Board models
 - `Documentation <documentation-root>`
 - `GDB support<GDB usage>`
-- `Migration<migration>`
+- :ref:`Migration<migration>`
 - `Monitor<QEMU monitor>`
 - :ref:`QOM (QEMU Object Model)<qom>`
 - `System mode<System emulation>`
@@ -112,7 +112,7 @@ yet, so sometimes the source code is all you have.
 * `libdecnumber <https://gitlab.com/qemu-project/qemu/-/tree/master/libdecnumber>`_:
   Import of gcc library, used to implement decimal number arithmetic.
 * `migration <https://gitlab.com/qemu-project/qemu/-/tree/master/migration>`__:
-  `Migration framework <migration>`.
+  :ref:`Migration framework <migration>`.
 * `monitor <https://gitlab.com/qemu-project/qemu/-/tree/master/monitor>`_:
   `Monitor <QEMU monitor>` implementation (HMP & QMP).
 * `nbd <https://gitlab.com/qemu-project/qemu/-/tree/master/nbd>`_:
@@ -193,7 +193,7 @@ yet, so sometimes the source code is all you have.
   - `lcitool <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/lcitool>`_:
     Generate dockerfiles for CI containers.
   - `migration <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/migration>`_:
-    Test scripts and data for `Migration framework <migration>`.
+    Test scripts and data for :ref:`Migration framework <migration>`.
   - `multiboot <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/multiboot>`_:
     Test multiboot functionality for x86_64/i386.
   - `qapi-schema <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/qapi-schema>`_:
diff --git a/docs/glossary.rst b/docs/glossary.rst
index 693d9855dd1..4fa044bfb6e 100644
--- a/docs/glossary.rst
+++ b/docs/glossary.rst
@@ -120,7 +120,7 @@ Migration
 ---------
 
 QEMU can save and restore the execution of a virtual machine between different
-host systems. This is provided by the `Migration framework<migration>`.
+host systems. This is provided by the :ref:`Migration framework<migration>`.
 
 NBD
 ---
@@ -212,14 +212,14 @@ machine emulator and virtualizer.
 QOM
 ---
 
-`QEMU Object Model <qom>` is an object oriented API used to define various
-devices and hardware in the QEMU codebase.
+:ref:`QEMU Object Model <qom>` is an object oriented API used to define
+various devices and hardware in the QEMU codebase.
 
 Record/replay
 -------------
 
-`Record/replay <replay>` is a feature of QEMU allowing to have a deterministic
-and reproducible execution of a virtual machine.
+:ref:`Record/replay <replay>` is a feature of QEMU allowing to have a
+deterministic and reproducible execution of a virtual machine.
 
 Rust
 ----
-- 
2.48.1

[PATCH 24/42] 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 5665de85cab..4364f9f1618 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -21,3 +21,4 @@ Welcome to QEMU's documentation!
    specs/index
    devel/index
    glossary
+   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.48.1

[PATCH 25/42] docs/qapidoc: generate entries for undocumented members

[John Snow]

Presently, we never have any empty text entries for members. The next
patch will explicitly generate such sections, so enable support for it
in advance.

The parser will generate placeholder sections to indicate undocumented
members, but it's the qapidoc generator that's responsible for deciding
what to do with that stub section.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 5d946a46637..2d83556629d 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -221,11 +221,10 @@ def visit_member(self, section: QAPIDoc.ArgSection) -> None:
         # 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.text if section.text else "(Not Documented.)",
             section.info,
         )
 
-- 
2.48.1

[PATCH 26/42] qapi/parser: add undocumented stub members to all_sections

[John Snow]

This helps simplify the new doc generator if it doesn't have to check
for undocumented members; especially when dealing recursively with
inherited documentation blocks.

NB: If there is no existing 'member' section, these undocumented stub
members will be inserted directly after the intro paragraph(s). This is
potentially problematic for cases where a doc block contains *only* free
paragraphs, but some of those paragraphs are conceptually better suited
for the "outro" section; i.e.:


This patch will insert member documentation after the example,
effectively solidifying all preceding paragraphs as "intro" type, which
may not be what we want.

Solution for this issue is still TBD; auditing is needed to understand
how prevalent the problem is to help engineer a fix or decide it's not
really a problem that needs solving.

While I'm rambling about RFC stuff in the commit message though, this
patch really did help cull over a hundred lines from the new generator,
so I think I'm predisposed to find a way to keep it.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index af5d7bf892c..a8b30ae1a4b 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -800,6 +800,18 @@ def connect_member(self, member: 'QAPISchemaMember') -> None:
             section = QAPIDoc.ArgSection(
                 self.info, QAPIDoc.Kind.MEMBER, member.name)
             self.args[member.name] = section
+
+            # Insert stub documentation section for missing member docs.
+            # Determine where to insert stub doc - it should go at the
+            # end of the members section(s), if any. Note that index 0
+            # is assumed to be an untagged intro section, even if it is
+            # empty.
+            index = 1
+            if len(self.all_sections) > 1:
+                while self.all_sections[index].kind == QAPIDoc.Kind.MEMBER:
+                    index += 1
+            self.all_sections.insert(index, section)
+
         self.args[member.name].connect(member)
 
     def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
-- 
2.48.1

[PATCH 27/42] qapi: differentiate "intro" and "detail" sections

[John Snow]

This patch begins distinguishing "Plain" sections as being either
"Intro" or "Details" sections for the purpose of knowing when and where
to inline those sections.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py         |  4 ++--
 scripts/qapi/parser.py         | 30 +++++++++++++++++++-----------
 tests/qapi-schema/doc-good.out |  8 ++++----
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 2d83556629d..154ebc1b4cd 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -289,7 +289,7 @@ def visit_sections(self, ent: QAPISchemaEntity) -> None:
 
         # Add sections *in the order they are documented*:
         for section in sections:
-            if section.kind == QAPIDoc.Kind.PLAIN:
+            if section.kind.name in ("INTRO", "DETAIL"):
                 self.visit_paragraph(section)
             elif section.kind == QAPIDoc.Kind.MEMBER:
                 self.visit_member(section)
@@ -578,7 +578,7 @@ def _nodes_for_sections(self, doc):
                 # Hide TODO: sections
                 continue
 
-            if section.kind == QAPIDoc.Kind.PLAIN:
+            if section.kind.name in ("INTRO", "DETAIL"):
                 # Sphinx cannot handle sectionless titles;
                 # Instead, just append the results to the prior section.
                 container = nodes.container()
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index a8b30ae1a4b..b2f77ffdd7a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -499,7 +499,7 @@ def get_doc(self) -> 'QAPIDoc':
             doc = QAPIDoc(info, symbol)
             self.accept(False)
             line = self.get_doc_line()
-            no_more_args = False
+            have_tagged = False
 
             while line is not None:
                 # Blank lines
@@ -528,10 +528,10 @@ def get_doc(self) -> 'QAPIDoc':
                     if not doc.features:
                         raise QAPIParseError(
                             self, 'feature descriptions expected')
-                    no_more_args = True
+                    have_tagged = True
                 elif match := self._match_at_name_colon(line):
                     # description
-                    if no_more_args:
+                    if have_tagged:
                         raise QAPIParseError(
                             self,
                             "description of '@%s:' follows a section"
@@ -543,7 +543,7 @@ def get_doc(self) -> 'QAPIDoc':
                         if text:
                             doc.append_line(text)
                         line = self.get_doc_indented(doc)
-                    no_more_args = True
+                    have_tagged = True
                 elif match := re.match(
                         r'(Returns|Errors|Since|Notes?|Examples?|TODO)'
                         r'(?!::): *',
@@ -584,14 +584,20 @@ def get_doc(self) -> 'QAPIDoc':
                     if text:
                         doc.append_line(text)
                     line = self.get_doc_indented(doc)
-                    no_more_args = True
+                    have_tagged = True
                 elif line.startswith('='):
                     raise QAPIParseError(
                         self,
                         "unexpected '=' markup in definition documentation")
                 else:
                     # plain paragraph(s)
-                    doc.ensure_untagged_section(self.info)
+                    if have_tagged:
+                        no_more_tags = True
+
+                    # Paragraphs before tagged sections are "intro" paragraphs.
+                    # Any appearing after are "detail" paragraphs.
+                    intro = not have_tagged
+                    doc.ensure_untagged_section(self.info, intro)
                     doc.append_line(line)
                     line = self.get_doc_paragraph(doc)
         else:
@@ -640,21 +646,22 @@ class QAPIDoc:
     """
 
     class Kind(enum.Enum):
-        PLAIN = 0
+        INTRO = 0
         MEMBER = 1
         FEATURE = 2
         RETURNS = 3
         ERRORS = 4
         SINCE = 5
         TODO = 6
+        DETAIL = 7
 
         @staticmethod
         def from_string(kind: str) -> 'QAPIDoc.Kind':
             return QAPIDoc.Kind[kind.upper()]
 
         def text_required(self) -> bool:
-            # Only "plain" sections can be empty
-            return self.value not in (0,)
+            # Only Intro/Detail sections can be empty
+            return self.value not in (0, 7)
 
         def __str__(self) -> str:
             return self.name.title()
@@ -700,7 +707,7 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
         self.symbol: Optional[str] = symbol
         # the sections in textual order
         self.all_sections: List[QAPIDoc.Section] = [
-            QAPIDoc.Section(info, QAPIDoc.Kind.PLAIN)
+            QAPIDoc.Section(info, QAPIDoc.Kind.INTRO)
         ]
         # the body section
         self.body: Optional[QAPIDoc.Section] = self.all_sections[0]
@@ -725,8 +732,9 @@ def end(self) -> None:
     def ensure_untagged_section(
         self,
         info: QAPISourceInfo,
+        intro: bool = True,
     ) -> None:
-        kind = QAPIDoc.Kind.PLAIN
+        kind = QAPIDoc.Kind.INTRO if intro else QAPIDoc.Kind.DETAIL
 
         if self.all_sections and self.all_sections[-1].kind == kind:
             # extend current section
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 2d33a305ee7..3eb28503f6b 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=Plain
+    section=Detail
 @two is undocumented
 doc symbol=Base
     body=
@@ -168,7 +168,7 @@ description starts on the same line
 a feature
     feature=cmd-feat2
 another feature
-    section=Plain
+    section=Detail
 .. note:: @arg3 is undocumented
     section=Returns
 @Object
@@ -176,7 +176,7 @@ another feature
 some
     section=Todo
 frobnicate
-    section=Plain
+    section=Detail
 .. admonition:: Notes
 
  - Lorem ipsum dolor sit amet
@@ -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=Plain
+    section=Detail
 .. qmp-example::
 
    -> "this example"
-- 
2.48.1

[PATCH 28/42] qapi/parser: prohibit untagged sections between tagged sections

[John Snow]

This is being done primarily to ensure consistency between the source
documents and the final, rendered HTML output. Because
member/feature/returns sections will always appear in a visually grouped
element in the HTML output, prohibiting free paragraphs between those
sections ensures ordering consistency between source and the final
render.

Additionally, prohibiting such "middle" text paragraphs allows us to
classify all plain text sections as either "intro" or "detail"
sections, because these sections must either appear before structured
elements ("intro") or afterwards ("detail").

This keeps the inlining algorithm simpler with fewer "splice" points
when inlining multiple documentation blocks.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/net.json                   |  4 ++--
 qapi/qom.json                   |  4 ++--
 scripts/qapi/parser.py          | 16 ++++++++++++++++
 tests/qapi-schema/doc-good.json |  4 ++--
 tests/qapi-schema/doc-good.out  |  4 ++--
 tests/qapi-schema/doc-good.txt  |  8 ++++----
 6 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/qapi/net.json b/qapi/net.json
index 2739a2f4233..49bc7de64e9 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -655,13 +655,13 @@
 #     this to zero disables this function.  This member is mutually
 #     exclusive with @reconnect.  (default: 0) (Since: 9.2)
 #
-# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
-#
 # Features:
 #
 # @deprecated: Member @reconnect is deprecated.  Use @reconnect-ms
 #     instead.
 #
+# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
+#
 # Since: 7.2
 ##
 { 'struct': 'NetdevStreamOptions',
diff --git a/qapi/qom.json b/qapi/qom.json
index 28ce24cd8d0..11277d1f84c 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -195,12 +195,12 @@
 #
 # @typename: the type name of an object
 #
+# Returns: a list of ObjectPropertyInfo describing object properties
+#
 # .. note:: Objects can create properties at runtime, for example to
 #    describe links between different devices and/or objects.  These
 #    properties are not included in the output of this command.
 #
-# Returns: a list of ObjectPropertyInfo describing object properties
-#
 # Since: 2.12
 ##
 { 'command': 'qom-list-properties',
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b2f77ffdd7a..c5d2b950a82 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -500,6 +500,20 @@ def get_doc(self) -> 'QAPIDoc':
             self.accept(False)
             line = self.get_doc_line()
             have_tagged = False
+            no_more_tags = False
+
+            def _tag_check(what: str) -> None:
+                if what in ('TODO', 'Since'):
+                    return
+
+                if no_more_tags:
+                    raise QAPIParseError(
+                        self,
+                        f"{what!r} section cannot appear after free "
+                        "paragraphs that follow other tagged sections. "
+                        "Move this section upwards with the preceding "
+                        "tagged sections."
+                    )
 
             while line is not None:
                 # Blank lines
@@ -513,6 +527,7 @@ def get_doc(self) -> 'QAPIDoc':
                     if doc.features:
                         raise QAPIParseError(
                             self, "duplicated 'Features:' line")
+                    _tag_check("Features")
                     self.accept(False)
                     line = self.get_doc_line()
                     while line == '':
@@ -576,6 +591,7 @@ def get_doc(self) -> 'QAPIDoc':
                         )
                         raise QAPIParseError(self, emsg)
 
+                    _tag_check(match.group(1))
                     doc.new_tagged_section(
                         self.info,
                         QAPIDoc.Kind.from_string(match.group(1))
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index f64bf38d854..14b2091b08f 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -157,12 +157,12 @@
 # @cmd-feat1: a feature
 # @cmd-feat2: another feature
 #
-# .. note:: @arg3 is undocumented
-#
 # Returns: @Object
 #
 # Errors: some
 #
+# .. note:: @arg3 is undocumented
+#
 # TODO: frobnicate
 #
 # .. admonition:: Notes
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 3eb28503f6b..77edadb6e40 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -168,12 +168,12 @@ description starts on the same line
 a feature
     feature=cmd-feat2
 another feature
-    section=Detail
-.. note:: @arg3 is undocumented
     section=Returns
 @Object
     section=Errors
 some
+    section=Detail
+.. note:: @arg3 is undocumented
     section=Todo
 frobnicate
     section=Detail
diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
index cb37db606a6..dc40b3f7046 100644
--- a/tests/qapi-schema/doc-good.txt
+++ b/tests/qapi-schema/doc-good.txt
@@ -193,10 +193,6 @@ Features
 "cmd-feat2"
    another feature
 
-Note:
-
-  "arg3" is undocumented
-
 
 Returns
 ~~~~~~~
@@ -209,6 +205,10 @@ Errors
 
 some
 
+Note:
+
+  "arg3" is undocumented
+
 Notes:
 
 * Lorem ipsum dolor sit amet
-- 
2.48.1

[PATCH 29/42] qapi: Add "Details:" disambiguation marker

[John Snow]

This clarifies sections that are mistaken by the parser as "intro"
sections to be "details" sections instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/machine.json      | 2 ++
 qapi/migration.json    | 4 ++++
 qapi/qom.json          | 4 ++++
 qapi/yank.json         | 2 ++
 scripts/qapi/parser.py | 8 ++++++++
 5 files changed, 20 insertions(+)

diff --git a/qapi/machine.json b/qapi/machine.json
index a6b8795b09e..3c1b397f6cc 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1301,6 +1301,8 @@
 # Return the amount of initially allocated and present hotpluggable
 # (if enabled) memory in bytes.
 #
+# Details:
+#
 # .. qmp-example::
 #
 #     -> { "execute": "query-memory-size-summary" }
diff --git a/qapi/migration.json b/qapi/migration.json
index 43babd1df41..9070a91e655 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1920,6 +1920,8 @@
 #
 # Xen uses this command to notify replication to trigger a checkpoint.
 #
+# Details:
+#
 # .. qmp-example::
 #
 #     -> { "execute": "xen-colo-do-checkpoint" }
@@ -1993,6 +1995,8 @@
 #
 # Pause a migration.  Currently it only supports postcopy.
 #
+# Details:
+#
 # .. qmp-example::
 #
 #     -> { "execute": "migrate-pause" }
diff --git a/qapi/qom.json b/qapi/qom.json
index 11277d1f84c..5d285ef9239 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -729,6 +729,8 @@
 #
 # Properties for memory-backend-shm objects.
 #
+# Details:
+#
 # This memory backend supports only shared memory, which is the
 # default.
 #
@@ -744,6 +746,8 @@
 #
 # Properties for memory-backend-epc objects.
 #
+# Details:
+#
 # The @merge boolean option is false by default with epc
 #
 # The @dump boolean option is false by default with epc
diff --git a/qapi/yank.json b/qapi/yank.json
index 30f46c97c98..4d36d21e76a 100644
--- a/qapi/yank.json
+++ b/qapi/yank.json
@@ -104,6 +104,8 @@
 #
 # Returns: list of @YankInstance
 #
+# Details:
+#
 # .. qmp-example::
 #
 #     -> { "execute": "query-yank" }
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index c5d2b950a82..5890a13b5ba 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -544,6 +544,14 @@ def _tag_check(what: str) -> None:
                         raise QAPIParseError(
                             self, 'feature descriptions expected')
                     have_tagged = True
+                elif line == 'Details:':
+                    _tag_check("Details")
+                    self.accept(False)
+                    line = self.get_doc_line()
+                    while line == '':
+                        self.accept(False)
+                        line = self.get_doc_line()
+                    have_tagged = True
                 elif match := self._match_at_name_colon(line):
                     # description
                     if have_tagged:
-- 
2.48.1

[PATCH 30/42] docs/qapidoc: add minimalistic inliner

[John Snow]

Add a minimalistic inliner that only gets the basics - leaving branch
inlining for a future patch.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 154ebc1b4cd..538c26e812d 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -25,13 +25,15 @@
 https://www.sphinx-doc.org/en/master/development/index.html
 """
 
+from collections import defaultdict
 from contextlib import contextmanager
+import enum
 import os
 from pathlib import Path
 import re
 import sys
 import textwrap
-from typing import List, Optional
+from typing import Dict, List, Optional
 
 from docutils import nodes
 from docutils.parsers.rst import Directive, directives
@@ -45,8 +47,10 @@
     QAPISchemaCommand,
     QAPISchemaEntity,
     QAPISchemaEnumMember,
+    QAPISchemaEvent,
     QAPISchemaFeature,
     QAPISchemaMember,
+    QAPISchemaObjectType,
     QAPISchemaObjectTypeMember,
 )
 from qapi.source import QAPISourceInfo
@@ -77,6 +81,172 @@ def dedent(text: str) -> str:
     return lines[0] + textwrap.dedent("".join(lines[1:]))
 
 
+class DocRegion(enum.Enum):
+    # The inliner introduces the concept of doc Regions, which are
+    # groupings of zero or more Sections. These regions are used to know
+    # how to combine two lists of Sections.
+    INTRO = 0
+    MEMBER = 1
+    OTHER = 2
+    FEATURE = 3
+    DETAIL = 4
+
+    @staticmethod
+    def categorize(section: QAPIDoc.Section) -> "Optional[DocRegion]":
+        return MAPPING[section.kind]
+
+
+MAPPING = {
+    QAPIDoc.Kind.INTRO: DocRegion.INTRO,
+    QAPIDoc.Kind.MEMBER: DocRegion.MEMBER,
+    QAPIDoc.Kind.FEATURE: DocRegion.FEATURE,
+    QAPIDoc.Kind.RETURNS: DocRegion.OTHER,
+    QAPIDoc.Kind.ERRORS: DocRegion.OTHER,
+    QAPIDoc.Kind.SINCE: None,
+    QAPIDoc.Kind.TODO: None,
+    QAPIDoc.Kind.DETAIL: DocRegion.DETAIL,
+}
+
+
+class InlinerSections:
+
+    def __init__(self, sections):
+        self._sections: List[QAPIDoc.Section] = list(sections)
+        self.partitions: Dict[DocRegion, List[QAPIDoc.Section]] = defaultdict(
+            list
+        )
+        self._modified = False
+
+        self._partition()
+
+    def _partition(self):
+        for section in self._sections:
+            # suppress empty text sections for the purpose of, later,
+            # being able to determine if a collection of doc sections
+            # "has an intro" or "has a details section" or not, which is
+            # helpful later on for some assertions about the inliner.
+            if section.kind in (QAPIDoc.Kind.INTRO, QAPIDoc.Kind.DETAIL):
+                if not section.text:
+                    continue
+
+            region = DocRegion.categorize(section)
+            if region is None:
+                continue
+
+            self.partitions[region].append(section)
+
+    def absorb(self, other: "InlinerSections"):
+        for category, oval in other.partitions.items():
+            val = self.partitions[category]
+            if category == DocRegion.INTRO:
+                # The intro region is not inlined, it is dropped.
+                continue
+
+            # Everything else is prepended.
+            self.partitions[category] = oval + val
+            if oval:
+                self._modified = True
+
+    def __iter__(self):
+        return iter(self._flatten())
+
+    def _flatten(self):
+        # Serialize back into a normal list.
+
+        # If nothing has changed, we don't need to do anything.
+        if not self._modified:
+            return tuple(self._sections)
+
+        # Otherwise, we need to rebuild the sequence.
+        #
+        # FIXME: This function assumes a stricter ordering of regions
+        # within the source docs (see DocRegion); This order is not
+        # currently enforced in parser.py, so this method may shift some
+        # contents around compared to the source.
+        tmp = []
+        for category in DocRegion:
+            tmp.extend(self.partitions[category])
+        self._sections = tmp
+        self._modified = False
+        return tuple(self._sections)
+
+
+def inline(ent: QAPISchemaEntity) -> List[QAPIDoc.Section]:
+    """
+    Return all of an entity's doc sections with inlined sections stitched in.
+
+    First, a given entity's sections are retrieved in full, in source order.
+
+    Next, if this entity has either an argument type (Commands and
+    Events) or an inherited base (Structs, Unions), this function is
+    called recursively on that type to retrieve its sections. These
+    sections are then stitched into the result.
+
+    Lastly, if this entity has any branches, this function is called
+    recursively on each branch type and those sections are stitched into
+    the result.
+
+    STITCH ORDER:
+
+      - Introduction
+      - [Inlined introduction sections are dropped.]
+      - Recursively inlined Members
+      - Members
+      - Recursively inlined Branch members
+      - Other special sections (Errors, Returns)
+      - [There are no cases where we have these sections to inline.]
+      - Recursively inlined Features
+      - Features
+      - Recursively inlined Details
+      - Details
+
+    Intro paragraphs are never stitched into the documentation section
+    list. Members, Features, and Details paragraphs are stitched in
+    *before* the given entity's equivalent regions. Individual sections
+    otherwise appear in source order as they do in the parent or child.
+
+    Branch members are stitched in *after* the member section.
+
+    In the event that a paragraph cannot be determined to be an intro or
+    details type, a warning is emitted. It will be treated as an intro
+    section and dropped from the output. QEMU usually treats Sphinx
+    warnings as fatal, so this warning is usually fatal.
+    """
+
+    def _sections(ent) -> List[QAPIDoc.Section]:
+        return ent.doc.all_sections if ent.doc else []
+
+    def _get_inline_target(
+        ent: QAPISchemaEntity,
+    ) -> Optional[QAPISchemaEntity]:
+        """Get the entity to inline from for a given entity."""
+        if isinstance(ent, (QAPISchemaCommand, QAPISchemaEvent)):
+            return ent.arg_type
+        if isinstance(ent, QAPISchemaObjectType):
+            return ent.base
+        return None
+
+    # Let's do this thing!
+
+    if ent is None:
+        return []
+
+    # Grab all directly documented sections for the entity in question.
+    sections = InlinerSections(_sections(ent))
+
+    # Get inlined sections: this includes everything from the
+    # inlined entity (and anything it inlines, too).
+    inlined = InlinerSections(inline(_get_inline_target(ent)))
+
+    # Now, stitch the results together!
+    sections.absorb(inlined)
+
+    # FIXME: Branches should be handled about here O:-)
+
+    # Return the combined list of sections.
+    return list(sections)
+
+
 class Transmogrifier:
     # Field names used for different entity types:
     field_types = {
@@ -285,7 +455,7 @@ 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 []
+        sections = inline(ent)
 
         # Add sections *in the order they are documented*:
         for section in sections:
-- 
2.48.1

[PATCH 31/42] docs/qapidoc: autogenerate undocumented return docs

[John Snow]

This patch adds support for handling undocumented returns values
sections -- which aren't actually implemented until the next patch.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 538c26e812d..b9fe2f476cb 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -410,15 +410,21 @@ def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
     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
+
+        if not rtype:
+            # Markus prefers q_empty returning commands just say
+            # nothing.  I'd like a "Returns nothing" in the generated
+            # docs because I like explicit to implicit - Users may not
+            # know that commands have an implicit return type and may
+            # wrongly assume that a missing "Returns" statement means
+            # it's undocumented. Well, that's my $0.02.
+            return
 
         typ = self.format_type(rtype)
-        assert section.text  # We don't expect empty returns sections.
-        self.add_field("returns", typ, section.text, section.info)
+        if section.text:
+            self.add_field("returns", typ, section.text, section.info)
+        else:
+            self.add_lines(f":returns-nodesc: {typ}", section.info)
 
     def visit_errors(self, section: QAPIDoc.Section) -> None:
         # FIXME: the formatting for errors may be inconsistent and may
-- 
2.48.1

[PATCH 32/42] docs/qapidoc: Add generated returns documentation to inliner

[John Snow]

Generate "empty" returns sections for undocumented returns values in the
inliner: the transmogrifier will pick these up and document them
specially.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index b9fe2f476cb..5c65f3a8025 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -243,6 +243,14 @@ def _get_inline_target(
 
     # FIXME: Branches should be handled about here O:-)
 
+    # Generated "returns" statement.
+    if isinstance(ent, QAPISchemaCommand) and not any(
+        s.kind == QAPIDoc.Kind.RETURNS
+        for s in sections.partitions[DocRegion.OTHER]
+    ):
+        sect = QAPIDoc.Section(ent.info, QAPIDoc.Kind.RETURNS)
+        sections.partitions[DocRegion.OTHER].append(sect)
+
     # Return the combined list of sections.
     return list(sections)
 
-- 
2.48.1

[PATCH 33/42] docs/qmp: add target to Out-of-band execution section

[John Snow]

This allows us to cross-reference this section from other docs; the main
motivator here is to refer to this section when documenting the
Out-of-band capability for QMP commands that support it in the rendered
HTML documentation.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/interop/qmp-spec.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/interop/qmp-spec.rst b/docs/interop/qmp-spec.rst
index 563344160e6..ea713a96ac0 100644
--- a/docs/interop/qmp-spec.rst
+++ b/docs/interop/qmp-spec.rst
@@ -119,6 +119,8 @@ Where:
 
 The actual commands are documented in the :doc:`qemu-qmp-ref`.
 
+.. _out-of-band execution:
+
 Out-of-band execution
 ---------------------
 
-- 
2.48.1

[PATCH 34/42] docs/qapidoc: document the "out-of-band" pseudofeature

[John Snow]

Add support to the transmogrifier for documenting the "out-of-band"
"pseudofeature" of QMP commands. This patch relies on the inliner adding
a dummy feature based on the presence of the oob flag for a QMP command,
which happens in the next commit.

A "pseudofeature" as I'm terming it here is not associated with a
particular member, unlike a standard real-deal feature.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 5c65f3a8025..e70a85a6403 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -411,9 +411,15 @@ def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
         # 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)
+        if section.member:
+            # Normal feature
+            self.generate_field(
+                "feat", section.member, section.text, section.info
+            )
+        else:
+            # Pseudo-feature (OOB)
+            self.add_field("feat", section.name, section.text, section.info)
 
     def visit_returns(self, section: QAPIDoc.Section) -> None:
         assert isinstance(self.entity, QAPISchemaCommand)
-- 
2.48.1

[PATCH 35/42] docs/qapidoc: generate out-of-band pseudofeature sections

[John Snow]

For QMP commands with the oob flag set, insert a dummy feature
("pseudofeature") representing this behavior so that it can be
documented.

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 e70a85a6403..81133b9b441 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -243,6 +243,12 @@ def _get_inline_target(
 
     # FIXME: Branches should be handled about here O:-)
 
+    # Pseudo-feature: document the OOB property.
+    if isinstance(ent, QAPISchemaCommand) and ent.allow_oob:
+        feat = QAPIDoc.ArgSection(ent.info, QAPIDoc.Kind.FEATURE, "allow-oob")
+        feat.append_line("This command supports `out-of-band execution`.")
+        sections.partitions[DocRegion.FEATURE].append(feat)
+
     # Generated "returns" statement.
     if isinstance(ent, QAPISchemaCommand) and not any(
         s.kind == QAPIDoc.Kind.RETURNS
-- 
2.48.1

[PATCH 36/42] qapi/parser: add "meta" kind to QAPIDoc.Kind

[John Snow]

This adds a "META" category for sections, for the express purpose of
adding empty branch-start and branch-end sections into a linear section list
for the purpose of rendering collapsible branches in sphinx HTML output.

This is ... a little hacky. Sorry.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 5890a13b5ba..c92bbc908e7 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -678,6 +678,7 @@ class Kind(enum.Enum):
         SINCE = 5
         TODO = 6
         DETAIL = 7
+        META = 8
 
         @staticmethod
         def from_string(kind: str) -> 'QAPIDoc.Kind':
-- 
2.48.1

[PATCH 37/42] qapi/schema: add __iter__ method to QAPISchemaVariants

[John Snow]

This is just semantic sugar that makes it easier to do something like:

for var in variants:
    ...

Instead of the more cumbersome and repetitive:

for var in variants.variants:
    ...

Especially in conjunction with entities that aren't guaranteed to have
variants. Compare:

for var in variants.variants if variants else []:
    ...

against:

for var in variants or []:
    ...

Update callsites to reflect the new usage pattern.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py     | 2 +-
 scripts/qapi/introspect.py | 4 ++--
 scripts/qapi/schema.py     | 8 ++++++--
 scripts/qapi/types.py      | 4 ++--
 scripts/qapi/visit.py      | 4 ++--
 5 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 81133b9b441..755eb0fa0ec 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -695,7 +695,7 @@ def _nodes_for_members(self, doc, what, base=None, branches=None):
                                         None)
 
         if branches:
-            for v in branches.variants:
+            for v in branches:
                 if v.type.name == 'q_empty':
                     continue
                 assert not v.type.is_implicit()
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index ac14b20f308..6ec34e055d3 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -342,7 +342,7 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
         }
         if branches:
             obj['tag'] = branches.tag_member.name
-            obj['variants'] = [self._gen_variant(v) for v in branches.variants]
+            obj['variants'] = [self._gen_variant(v) for v in branches]
         self._gen_tree(name, 'object', obj, ifcond, features)
 
     def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
@@ -353,7 +353,7 @@ def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
             name, 'alternate',
             {'members': [Annotated({'type': self._use_type(m.type)},
                                    m.ifcond)
-                         for m in alternatives.variants]},
+                         for m in alternatives]},
             ifcond, features
         )
 
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index e97c978d38d..4c55f7640b6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -26,6 +26,7 @@
     Any,
     Callable,
     Dict,
+    Iterator,
     List,
     Optional,
     Union,
@@ -669,7 +670,7 @@ def check(self, schema: QAPISchema) -> None:
         # so we have to check for potential name collisions ourselves.
         seen: Dict[str, QAPISchemaMember] = {}
         types_seen: Dict[str, str] = {}
-        for v in self.alternatives.variants:
+        for v in self.alternatives:
             v.check_clash(self.info, seen)
             qtype = v.type.alternate_qtype()
             if not qtype:
@@ -700,7 +701,7 @@ def check(self, schema: QAPISchema) -> None:
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
-        for v in self.alternatives.variants:
+        for v in self.alternatives:
             v.connect_doc(doc)
 
     def c_type(self) -> str:
@@ -726,6 +727,9 @@ def __init__(
         self.tag_member: QAPISchemaObjectTypeMember
         self.variants = variants
 
+    def __iter__(self) -> Iterator[QAPISchemaVariant]:
+        return iter(self.variants)
+
     def set_defined_in(self, name: str) -> None:
         for v in self.variants:
             v.set_defined_in(name)
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 0dd0b00ada3..ad36b55488f 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -166,7 +166,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
     objects_seen.add(name)
 
     ret = ''
-    for var in variants.variants if variants else ():
+    for var in variants or ():
         obj = var.type
         if not isinstance(obj, QAPISchemaObjectType):
             continue
@@ -234,7 +234,7 @@ def gen_variants(variants: QAPISchemaVariants) -> str:
 ''',
                 c_name=c_name(variants.tag_member.name))
 
-    for var in variants.variants:
+    for var in variants:
         if var.type.name == 'q_empty':
             continue
         ret += var.ifcond.gen_if()
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 12f92e429f6..1eca452378c 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -141,7 +141,7 @@ def gen_visit_object_members(name: str,
 ''',
                      c_name=c_name(tag_member.name))
 
-        for var in branches.variants:
+        for var in branches:
             case_str = c_enum_const(tag_member.type.name, var.name,
                                     tag_member.type.prefix)
             ret += var.ifcond.gen_if()
@@ -246,7 +246,7 @@ def gen_visit_alternate(name: str,
 ''',
                 c_name=c_name(name))
 
-    for var in alternatives.variants:
+    for var in alternatives:
         ret += var.ifcond.gen_if()
         ret += mcgen('''
     case %(case)s:
-- 
2.48.1

[PATCH 38/42] docs/qapi: add branch support to inliner

[John Snow]

Well, kind of. Anything beyond simple member definitions aren't
included; including ifcond, details sections, features, etc. Definitely
the most "WIP" part of this entire patch series.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 755eb0fa0ec..86c13520d94 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -44,6 +44,7 @@
 from qapi.schema import (
     QAPISchema,
     QAPISchemaArrayType,
+    QAPISchemaBranches,
     QAPISchemaCommand,
     QAPISchemaEntity,
     QAPISchemaEnumMember,
@@ -68,6 +69,20 @@
 logger = logging.getLogger(__name__)
 
 
+# These classes serve as pseudo-sections that this generator uses to
+# flatten and inline arg sections from multiple entities.
+class BranchStart(QAPIDoc.Section):
+    def __init__(self, key: str, value: str, info: QAPISourceInfo):
+        super().__init__(info, QAPIDoc.Kind.META)
+        self.key = key
+        self.value = value
+
+
+class BranchEnd(QAPIDoc.Section):
+    def __init__(self, info: QAPISourceInfo):
+        super().__init__(info, QAPIDoc.Kind.META)
+
+
 def dedent(text: str) -> str:
     # Adjust indentation to make description text parse as paragraph.
 
@@ -105,6 +120,7 @@ def categorize(section: QAPIDoc.Section) -> "Optional[DocRegion]":
     QAPIDoc.Kind.SINCE: None,
     QAPIDoc.Kind.TODO: None,
     QAPIDoc.Kind.DETAIL: DocRegion.DETAIL,
+    QAPIDoc.Kind.META: DocRegion.MEMBER,
 }
 
 
@@ -226,6 +242,15 @@ def _get_inline_target(
             return ent.base
         return None
 
+    def _variants(ent) -> Optional[QAPISchemaBranches]:
+        if isinstance(ent, QAPISchemaObjectType):
+            return ent.branches
+        return None
+
+    def _memb_filter(sec: QAPIDoc.Section) -> bool:
+        # meta grabs branch start/end markers, too.
+        return sec.kind in (QAPIDoc.Kind.MEMBER, QAPIDoc.Kind.META)
+
     # Let's do this thing!
 
     if ent is None:
@@ -241,7 +266,22 @@ def _get_inline_target(
     # Now, stitch the results together!
     sections.absorb(inlined)
 
-    # FIXME: Branches should be handled about here O:-)
+    # Now, pick up member sections from branches, if any.
+    # FIXME: Anything other than members are unhandled/ignored here...!
+    branch_sections = []
+    if variants := _variants(ent):
+        for variant in variants.variants:
+            branch_sections.append(
+                BranchStart(
+                    variants.tag_member.name, variant.name, variants.info
+                )
+            )
+            var_sections = inline(variant.type)
+            branch_sections.extend(filter(_memb_filter, var_sections))
+            branch_sections.append(BranchEnd(variants.info))
+
+    # Inject branches *after* the member section.
+    sections.partitions[DocRegion.MEMBER].extend(branch_sections)
 
     # Pseudo-feature: document the OOB property.
     if isinstance(ent, QAPISchemaCommand) and ent.allow_oob:
@@ -485,6 +525,21 @@ def visit_sections(self, ent: QAPISchemaEntity) -> None:
 
         # Add sections *in the order they are documented*:
         for section in sections:
+            if isinstance(section, BranchStart):
+                self.ensure_blank_line()
+                self.add_line(
+                    f".. qapi:branch:: {section.key} {section.value}",
+                    section.info,
+                )
+                self.ensure_blank_line()
+                self.indent += 1
+                continue
+
+            if isinstance(section, BranchEnd):
+                self.ensure_blank_line()
+                self.indent -= 1
+                continue
+
             if section.kind.name in ("INTRO", "DETAIL"):
                 self.visit_paragraph(section)
             elif section.kind == QAPIDoc.Kind.MEMBER:
-- 
2.48.1

[PATCH 39/42] qapi/schema: add doc_visible property to QAPISchemaDefinition

[John Snow]

This patch adds a boolean flag to mark definitions as "visible" in the
end-user HTML docs. The intent is that definitions that are not marked
as visible will not be rendered.

What gets marked visible?

The short version: all commands and events are inherently visible. All
other definitions are visible if and only if they are the target of a
cross-reference in the generated output.

The long version:

- All commands:
  - All arg_type members, excluding arg_type itself
    (because it's inlined)
  - All ret_type members, *including* ret_type itself
    (because we cross-reference the return type)

- All events:
  - All arg_type members, excluding arg_type itself
    (because it's inlined)

- For any object marked visible by the above;
  - All members (which includes inherited members)
  - All branch objects, excluding the branch object itself
    (because the branches are inlined)

- For any array marked visible by the above;
  - The array itself, and
  - The contained element_type

- For any alternate marked visible by the above;
  - The alternate itself, and
  - All of its branch types.
    (because we don't currently inline anything for alternates.)

All other definitions are not doc_visible; those exclusions are:

- Any definition not recursively referenced by any command or event;
  i.e. definitions used internally by QEMU but not actually used by the
  QMP protocol.

- Any definition used only as an arg_type for events or
  commands; because the doc generator inlines arguments, there is no
  need to generate documentation for the arguments/members by
  themselves.

- Any definition used only as a base_type for objects. The new
  doc generator will inline inheritance, so there is no need to generate
  standalone documentation for factored/common objects.

- Any definition used only as a branch type for a union. The new doc
  generator also inlines branch members as it does for local and
  inherited members, so there's no need to document each branch as a
  standalone entity.

Note that if a type is used both in an "inlined" context and a
"referenced" context, documentation *will* be generated for that
type; e.g. a struct type used both as a base_type *and* as a member
argument.

This does not necessarily match the data revealed by the runtime
introspection feature: in the generated documentation case, we want
anything that we are cross-referencing in generated documentation to be
available to target with cross-reference syntax.

Some built-in types may be marked visible with this approach, but if
they do not have a documentation block, they'll be skipped by the
generator anyway. This includes array types and built-in primitives
which do not get their own documentation objects.

i.e., for documentation to be generated for a given QAPISchemaDefinition
in the new generator, it must have non-empty documentation AND be
doc_visible. This differs from the current qapidoc generator which only
requires non-empty documentation. This means some items marked
doc_visible *still* will not be rendered because they lack documentation
to render.

This information is not yet used by the current generator, which
continues to render documentation exactly as it has. This information
will be used by the new qapidoc (the "transmogrifier") in the next
commit.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 4c55f7640b6..32c9a8f4cd2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -131,6 +131,7 @@ def __init__(
         self.doc = doc
         self._ifcond = ifcond or QAPISchemaIfCond()
         self.features = features or []
+        self.doc_visible = False
 
     def __repr__(self) -> str:
         return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
@@ -146,6 +147,10 @@ def check(self, schema: QAPISchema) -> None:
         for f in self.features:
             f.check_clash(self.info, seen)
 
+    def mark_visible(self, mark_self: bool = True) -> None:
+        if mark_self:
+            self.doc_visible = True
+
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
@@ -483,6 +488,10 @@ def check(self, schema: QAPISchema) -> None:
             self.info.defn_meta if self.info else None)
         assert not isinstance(self.element_type, QAPISchemaArrayType)
 
+    def mark_visible(self, mark_self: bool = True) -> None:
+        super().mark_visible(mark_self)
+        self.element_type.mark_visible()
+
     def set_module(self, schema: QAPISchema) -> None:
         self._set_module(schema, self.element_type.info)
 
@@ -607,6 +616,17 @@ def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         for m in self.local_members:
             m.connect_doc(doc)
 
+    def mark_visible(self, mark_self: bool = True) -> None:
+        # Mark this object and its members as visible in the user-facing docs.
+        if self.doc_visible:
+            return
+
+        super().mark_visible(mark_self)
+        for m in self.members:
+            m.type.mark_visible()
+        for var in self.branches or []:
+            var.type.mark_visible(False)
+
     def is_implicit(self) -> bool:
         # See QAPISchema._make_implicit_object_type(), as well as
         # _def_predefineds()
@@ -698,6 +718,11 @@ def check(self, schema: QAPISchema) -> None:
                         % (v.describe(self.info), types_seen[qt]))
                 types_seen[qt] = v.name
 
+    def mark_visible(self, mark_self: bool = True) -> None:
+        super().mark_visible(mark_self)
+        for var in self.alternatives:
+            var.type.mark_visible()
+
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
@@ -1056,6 +1081,13 @@ def check(self, schema: QAPISchema) -> None:
                         "command's 'returns' cannot take %s"
                         % self.ret_type.describe())
 
+    def mark_visible(self, mark_self: bool = True) -> None:
+        super().mark_visible(mark_self)
+        if self.arg_type:
+            self.arg_type.mark_visible(False)
+        if self.ret_type:
+            self.ret_type.mark_visible()
+
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
@@ -1112,6 +1144,11 @@ def check(self, schema: QAPISchema) -> None:
                     self.info,
                     "conditional event arguments require 'boxed': true")
 
+    def mark_visible(self, mark_self: bool = True) -> None:
+        super().mark_visible(mark_self)
+        if self.arg_type:
+            self.arg_type.mark_visible(False)
+
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
@@ -1488,6 +1525,9 @@ def check(self) -> None:
             ent.set_module(self)
         for doc in self.docs:
             doc.check()
+        for ent in self._entity_list:
+            if isinstance(ent, (QAPISchemaCommand, QAPISchemaEvent)):
+                ent.mark_visible()
 
     def visit(self, visitor: QAPISchemaVisitor) -> None:
         visitor.visit_begin(self)
-- 
2.48.1

[PATCH 40/42] docs/qapidoc: cull (most) un-named entities from docs

[John Snow]

This patch excludes any items not marked visible from the transmogrifier
output. The legacy qapidoc mechanism continues to ignore this flag, so
the existing docs are not effected.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 86c13520d94..e7da5b2225f 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -616,6 +616,13 @@ def visit_freeform(self, doc) -> None:
     def visit_entity(self, ent):
         assert ent is not None
 
+        # Some entities need not be rendered; they are not exposed via
+        # introspection and are only relevant for purposes of
+        # inlining/inheritance. They don't need their own entries and
+        # don't need to be in the index.
+        if not ent.doc_visible:
+            return
+
         try:
             self._curr_ent = ent
             # This line gets credited to the start of the /definition/.
-- 
2.48.1

[PATCH 41/42] qapi: resolve filenames in info structures

[John Snow]

Resolve symbolic filenames (i.e. build/../tests/qapi-schema)
to fully specified absolute paths in QAPI info structures.

Normalizing filenames in this way makes trimming common path prefixes
for test output more consistent. It's also used for the intermediate
output representation for the new transmogrifier.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py         | 3 ++-
 tests/qapi-schema/test-qapi.py | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index c92bbc908e7..76fac70c477 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -17,6 +17,7 @@
 from collections import OrderedDict
 import enum
 import os
+from pathlib import Path
 import re
 from typing import (
     TYPE_CHECKING,
@@ -100,7 +101,7 @@ def __init__(self,
         self.src = ''
 
         # Lexer state (see `accept` for details):
-        self.info = QAPISourceInfo(self._fname, incl_info)
+        self.info = QAPISourceInfo(str(Path(self._fname).resolve()), incl_info)
         self.tok: Union[None, str] = None
         self.pos = 0
         self.cursor = 0
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index bca924309be..27324d53850 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -15,6 +15,7 @@
 import argparse
 import difflib
 import os
+from pathlib import Path
 import sys
 from io import StringIO
 
@@ -216,6 +217,7 @@ def main(argv):
         (dir_name, base_name) = os.path.split(t)
         dir_name = dir_name or args.dir
         test_name = os.path.splitext(base_name)[0]
+        dir_name = str(Path(dir_name).resolve())
         status |= test_and_diff(test_name, dir_name, args.update)
 
     sys.exit(status)
-- 
2.48.1

[PATCH 42/42] docs/qapidoc: add intermediate output debugger

[John Snow]

Add debugging output for the qapidoc transmogrifier - setting DEBUG=1
will produce .ir files (one for each qapidoc directive) that write the
generated rst file to disk to allow for easy debugging and verification
of the generated document.

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

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index e7da5b2225f..7b895c3668e 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -36,7 +36,7 @@
 from typing import Dict, List, Optional
 
 from docutils import nodes
-from docutils.parsers.rst import Directive, directives
+from docutils.parsers.rst import directives
 from docutils.statemachine import StringList, ViewList
 from qapi.error import QAPIError, QAPISemError
 from qapi.gen import QAPISchemaVisitor
@@ -60,7 +60,7 @@
 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.docutils import SphinxDirective, switch_source_input
 from sphinx.util.nodes import nested_parse_with_titles
 
 
@@ -1053,7 +1053,7 @@ def visit_module(self, name):
         super().visit_module(name)
 
 
-class NestedDirective(Directive):
+class NestedDirective(SphinxDirective):
     def run(self):
         raise NotImplementedError
 
@@ -1122,10 +1122,43 @@ def transmogrify(self, schema) -> nodes.Element:
                 node.document = self.state.document
                 self.state.nested_parse(content, 0, contentnode)
         logger.info("Transmogrifier's nested parse completed.")
+
+        if self.env.app.verbosity >= 2 or os.environ.get("DEBUG"):
+            argname = "_".join(Path(self.arguments[0]).parts)
+            name = Path(argname).stem + ".ir"
+            self.write_intermediate(content, name)
+
         sys.stdout.flush()
-
         return contentnode
 
+    def write_intermediate(self, content, filename):
+        logger.info(
+            "writing intermediate rST for '%s' to '%s'",
+            self.arguments[0],
+            filename,
+        )
+
+        srctree = Path(self.env.app.config.qapidoc_srctree).resolve()
+        outlines = []
+        lcol_width = 0
+
+        for i, line in enumerate(content):
+            src, lineno = content.info(i)
+            src = Path(src).resolve()
+            src = src.relative_to(srctree)
+
+            lcol = f"{src}:{lineno:04d}"
+            lcol_width = max(lcol_width, len(lcol))
+            outlines.append((lcol, line))
+
+        with open(filename, "w", encoding="UTF-8") as outfile:
+            for lcol, rcol in outlines:
+                outfile.write(lcol.rjust(lcol_width))
+                outfile.write(" |")
+                if rcol:
+                    outfile.write(f" {rcol}")
+                outfile.write("\n")
+
     def legacy(self, schema) -> nodes.Element:
         vis = QAPISchemaGenRSTVisitor(self)
         vis.visit_begin(schema)
-- 
2.48.1

Re: [PATCH 01/42] 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 and will crash,
> because `node` will be uninitialized. If we don't have a section title,
> create a generic block to insert text into instead.

Suggest to show input that makes it crash.  Something like

  The code as written crashes when a free-form documentation block
  doesn't start with a heading or subheading, like so

      ##
      # Just text, no heading.
      ##

  The code then uses `node` uninitialized.

  To fix, create a generic block to insert the doc text into.

> (This patch also 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
>
> (under at least pylint 3.3.1; more robust tamping down of the
> environment needed to consistently perform checks will happen later -
> hopefully soon, sorry for the inconvenience.)
>
> 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>

Fixes: 43e0d14ee09a (docs/sphinx: fix extra stuff in TOC after freeform QMP sections)

Re: [PATCH 27/42] qapi: differentiate "intro" and "detail" sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch begins distinguishing "Plain" sections as being either
> "Intro" or "Details" sections for the purpose of knowing when and where
> to inline those sections.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

[...]

> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index a8b30ae1a4b..b2f77ffdd7a 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py

[...]

> @@ -584,14 +584,20 @@ def get_doc(self) -> 'QAPIDoc':
>                      if text:
>                          doc.append_line(text)
>                      line = self.get_doc_indented(doc)
> -                    no_more_args = True
> +                    have_tagged = True
>                  elif line.startswith('='):
>                      raise QAPIParseError(
>                          self,
>                          "unexpected '=' markup in definition documentation")
>                  else:
>                      # plain paragraph(s)
> -                    doc.ensure_untagged_section(self.info)
> +                    if have_tagged:
> +                        no_more_tags = True

@no_more_tags is not used in this patch.  Does the conditional
assignment belong to the next patch?

> +
> +                    # Paragraphs before tagged sections are "intro" paragraphs.
> +                    # Any appearing after are "detail" paragraphs.
> +                    intro = not have_tagged
> +                    doc.ensure_untagged_section(self.info, intro)
>                      doc.append_line(line)
>                      line = self.get_doc_paragraph(doc)
>          else:

[...]

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

[John Snow]

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

On Tue, Feb 11, 2025, 9:51 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 and will crash,
> > because `node` will be uninitialized. If we don't have a section title,
> > create a generic block to insert text into instead.
>
> Suggest to show input that makes it crash.  Something like
>
>   The code as written crashes when a free-form documentation block
>   doesn't start with a heading or subheading, like so
>
>       ##
>       # Just text, no heading.
>       ##
>
>   The code then uses `node` uninitialized.
>
>   To fix, create a generic block to insert the doc text into.
>


Okeydokey. Simple enough, thanks.


> > (This patch also 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
> >
> > (under at least pylint 3.3.1; more robust tamping down of the
> > environment needed to consistently perform checks will happen later -
> > hopefully soon, sorry for the inconvenience.)
> >
> > 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>
>
> Fixes: 43e0d14ee09a (docs/sphinx: fix extra stuff in TOC after freeform
> QMP sections)
>

Ah, yep. You got it. Thanks for reviewing this mammoth project.

>

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

Re: [PATCH 28/42] qapi/parser: prohibit untagged sections between tagged sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is being done primarily to ensure consistency between the source
> documents and the final, rendered HTML output. Because
> member/feature/returns sections will always appear in a visually grouped
> element in the HTML output, prohibiting free paragraphs between those
> sections ensures ordering consistency between source and the final
> render.
>
> Additionally, prohibiting such "middle" text paragraphs allows us to
> classify all plain text sections as either "intro" or "detail"
> sections, because these sections must either appear before structured
> elements ("intro") or afterwards ("detail").
>
> This keeps the inlining algorithm simpler with fewer "splice" points
> when inlining multiple documentation blocks.

Mention the two "middle" paragraphs you have to eliminate in this patch?

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/net.json                   |  4 ++--
>  qapi/qom.json                   |  4 ++--
>  scripts/qapi/parser.py          | 16 ++++++++++++++++
>  tests/qapi-schema/doc-good.json |  4 ++--
>  tests/qapi-schema/doc-good.out  |  4 ++--
>  tests/qapi-schema/doc-good.txt  |  8 ++++----
>  6 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 2739a2f4233..49bc7de64e9 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -655,13 +655,13 @@
>  #     this to zero disables this function.  This member is mutually
>  #     exclusive with @reconnect.  (default: 0) (Since: 9.2)
>  #
> -# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
> -#
>  # Features:
>  #
>  # @deprecated: Member @reconnect is deprecated.  Use @reconnect-ms
>  #     instead.
>  #
> +# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
> +#
>  # Since: 7.2
>  ##
>  { 'struct': 'NetdevStreamOptions',

The text moved applies to member @addr.  You're moving it even farther
away from @addr.  Move it into @addr instead?  Could be done as a
separate cleanup patch to keep this one as simple as possible; matter of
taste.

The same text is in NetdevDgramOptions below, where it applies to both
@remote and @local.  It just happens to follow @remote and @local
immediately, because there are no other members and no features.  Hmm.

Ideally, we'd have a way to put such notes next to the stuff they apply
to without having to rely on happy accidents like "no features".
Alternatively, have a way to link stuff and note.  Footnotes?  Food for
thought, not demand.

> diff --git a/qapi/qom.json b/qapi/qom.json
> index 28ce24cd8d0..11277d1f84c 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -195,12 +195,12 @@
>  #
>  # @typename: the type name of an object
>  #
> +# Returns: a list of ObjectPropertyInfo describing object properties
> +#
>  # .. note:: Objects can create properties at runtime, for example to
>  #    describe links between different devices and/or objects.  These
>  #    properties are not included in the output of this command.
>  #
> -# Returns: a list of ObjectPropertyInfo describing object properties
> -#
>  # Since: 2.12
>  ##

This move is fine.  Placing notes at the end is more common already.

>  { 'command': 'qom-list-properties',
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b2f77ffdd7a..c5d2b950a82 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -500,6 +500,20 @@ def get_doc(self) -> 'QAPIDoc':
>              self.accept(False)
>              line = self.get_doc_line()
>              have_tagged = False
> +            no_more_tags = False
> +
> +            def _tag_check(what: str) -> None:
> +                if what in ('TODO', 'Since'):
> +                    return
> +
> +                if no_more_tags:
> +                    raise QAPIParseError(
> +                        self,
> +                        f"{what!r} section cannot appear after free "
> +                        "paragraphs that follow other tagged sections. "
> +                        "Move this section upwards with the preceding "
> +                        "tagged sections."
> +                    )

Why !r conversion?

Error messages should be a single, short phrase, no punctuation at the
end.  Sometimes a helpful hint is desirable.  Here's one in expr.py:

        raise QAPISemError(
            info,
            "%s has unknown key%s %s\nValid keys are %s."
            % (source, 's' if len(unknown) > 1 else '',
               pprint(unknown), pprint(allowed)))

Needs a negative test case.

Aside: we should probably convert most string interpolation to f-strings
en masse at some point.

>  
>              while line is not None:
>                  # Blank lines
> @@ -513,6 +527,7 @@ def get_doc(self) -> 'QAPIDoc':
>                      if doc.features:
>                          raise QAPIParseError(
>                              self, "duplicated 'Features:' line")
> +                    _tag_check("Features")
>                      self.accept(False)
>                      line = self.get_doc_line()
>                      while line == '':
> @@ -576,6 +591,7 @@ def get_doc(self) -> 'QAPIDoc':
>                          )
>                          raise QAPIParseError(self, emsg)
>  
> +                    _tag_check(match.group(1))
>                      doc.new_tagged_section(
>                          self.info,
>                          QAPIDoc.Kind.from_string(match.group(1))
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index f64bf38d854..14b2091b08f 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -157,12 +157,12 @@
>  # @cmd-feat1: a feature
>  # @cmd-feat2: another feature
>  #
> -# .. note:: @arg3 is undocumented
> -#
>  # Returns: @Object
>  #
>  # Errors: some
>  #
> +# .. note:: @arg3 is undocumented
> +#

This used to be right next to @arg1 and arg2 (commit 80d1f2e4a5d) until
commit 79598c8a634 added features in between.  This patch adds more
stuff there.  Right next is clearly the best spot, but this is just a
test, so it doesn't really matter.  Related: NetdevDgramOptions' note
discussed above.

>  # TODO: frobnicate
>  #
>  # .. admonition:: Notes

[...]

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This clarifies sections that are mistaken by the parser as "intro"
> sections to be "details" sections instead.

Impact on output?  See notes inline.

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/machine.json      | 2 ++
>  qapi/migration.json    | 4 ++++
>  qapi/qom.json          | 4 ++++
>  qapi/yank.json         | 2 ++
>  scripts/qapi/parser.py | 8 ++++++++
>  5 files changed, 20 insertions(+)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index a6b8795b09e..3c1b397f6cc 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1301,6 +1301,8 @@
>  # Return the amount of initially allocated and present hotpluggable
>  # (if enabled) memory in bytes.
>  #
> +# Details:
> +#
>  # .. qmp-example::
>  #
>  #     -> { "execute": "query-memory-size-summary" }

Output unchanged in my testing.  Same for the other hunks unless
otherwise noted.

> diff --git a/qapi/migration.json b/qapi/migration.json
> index 43babd1df41..9070a91e655 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1920,6 +1920,8 @@
>  #
>  # Xen uses this command to notify replication to trigger a checkpoint.
>  #
> +# Details:
> +#
>  # .. qmp-example::
>  #
>  #     -> { "execute": "xen-colo-do-checkpoint" }
> @@ -1993,6 +1995,8 @@
>  #
>  # Pause a migration.  Currently it only supports postcopy.
>  #
> +# Details:
> +#
>  # .. qmp-example::
>  #
>  #     -> { "execute": "migrate-pause" }
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 11277d1f84c..5d285ef9239 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -729,6 +729,8 @@
>  #
>  # Properties for memory-backend-shm objects.
>  #
> +# Details:
> +#
>  # This memory backend supports only shared memory, which is the
>  # default.
>  #

The paragraphs moves from above to below the auto-generated member
documentation, like this:

    @@ -25908,13 +25908,13 @@ If

     Properties for memory-backend-shm objects.

    -This memory backend supports only shared memory, which is the default.
    -

     Members
     ~~~~~~~

     The members of "MemoryBackendProperties"
    +This memory backend supports only shared memory, which is the default.
    +

     Since
     ~~~~~

This is sphinx-build -b text.  I don't understand why there is no blank
line between "The members of ... " and the moved paragraph.

> @@ -744,6 +746,8 @@
>  #
>  # Properties for memory-backend-epc objects.
>  #
> +# Details:
> +#
>  # The @merge boolean option is false by default with epc
>  #
>  # The @dump boolean option is false by default with epc

Likewise.

> diff --git a/qapi/yank.json b/qapi/yank.json
> index 30f46c97c98..4d36d21e76a 100644
> --- a/qapi/yank.json
> +++ b/qapi/yank.json
> @@ -104,6 +104,8 @@
>  #
>  # Returns: list of @YankInstance
>  #
> +# Details:
> +#
>  # .. qmp-example::
>  #
>  #     -> { "execute": "query-yank" }
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index c5d2b950a82..5890a13b5ba 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -544,6 +544,14 @@ def _tag_check(what: str) -> None:
>                          raise QAPIParseError(
>                              self, 'feature descriptions expected')
>                      have_tagged = True
> +                elif line == 'Details:':
> +                    _tag_check("Details")
> +                    self.accept(False)
> +                    line = self.get_doc_line()
> +                    while line == '':
> +                        self.accept(False)
> +                        line = self.get_doc_line()
> +                    have_tagged = True
>                  elif match := self._match_at_name_colon(line):
>                      # description
>                      if have_tagged:

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

[Markus Armbruster]

I started to eyeball old and new generated output side by side.

New table of contents shows one level, old two.  No objection; the
navigation thingie on the left is more useful anyway.

The new generator elides unreferenced types.  Generally good, but two
observations:

* QapiErrorClass is unreferenced, but its members are mentioned in
  Errors sections.  QapiErrorClass serves as better than nothing error
  code documentation, but it's gone in the new doc.  So this is a minor
  regression.  We can figure out what to do about it later.

* Section "QMP errors" is empty in the new doc, because its entire
  contents is elided.  I guess we should elide the section as well, but
  it's fine to leave that for later.

Old doc shows a definition's since information like any other section.
New doc has it in the heading box.  Looks prettier and uses much less
space.  Not sure the heading box is the best place, but it'll do for
now, we can always move it around later.

The new doc's headings use "Struct" or "Union" where the old one uses
just "Object".  Let's keep "Object", please.

In the new doc, some member references are no longer rendered as such,
e.g. @on-source-error and @on-target-error in BackupCommon's note.
Another small regression.

Union branches are busted in the new generator's output.  I know they
used to work, so I'm not worried about it.

The new doc shows the return type, the old doc doesn't.  Showing it is
definitely an improvement, but we need to adjust the doc text to avoid
silliness like "Returns: SnapshotInfo – SnapshotInfo".

The new doc shows Arguments / Members, Returns, and Errors in two-column
format.  Looks nice.  But for some reason, the two columns don't align
horizontally for Errors like they do for the others.  Certainly not a
blocker of anything, but we should try to fix it at some point.

The new doc doesn't show non-definition conditionals, as mentioned in
the cover letter.  It shows definition conditionals twice.  Once should
suffice.

There's probably more, but this is it for now.

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This clarifies sections that are mistaken by the parser as "intro"
> sections to be "details" sections instead.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Is this missing announce-self in net.json?

diff --git a/qapi/net.json b/qapi/net.json
index 49bc7de64e..44ed72dbe9 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -948,7 +948,7 @@
 # switches.  This can be useful when network bonds fail-over the
 # active slave.
 #
-# TODO: This line is a hack to separate the example from the body
+# Details:
 #
 # .. qmp-example::
 #

Re: [PATCH 28/42] qapi/parser: prohibit untagged sections between tagged sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is being done primarily to ensure consistency between the source
> documents and the final, rendered HTML output. Because
> member/feature/returns sections will always appear in a visually grouped
> element in the HTML output, prohibiting free paragraphs between those
> sections ensures ordering consistency between source and the final
> render.
>
> Additionally, prohibiting such "middle" text paragraphs allows us to
> classify all plain text sections as either "intro" or "detail"
> sections, because these sections must either appear before structured
> elements ("intro") or afterwards ("detail").
>
> This keeps the inlining algorithm simpler with fewer "splice" points
> when inlining multiple documentation blocks.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

[...]

> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b2f77ffdd7a..c5d2b950a82 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -500,6 +500,20 @@ def get_doc(self) -> 'QAPIDoc':
>              self.accept(False)
>              line = self.get_doc_line()
>              have_tagged = False
> +            no_more_tags = False
> +
> +            def _tag_check(what: str) -> None:
> +                if what in ('TODO', 'Since'):
> +                    return
> +
> +                if no_more_tags:
> +                    raise QAPIParseError(
> +                        self,
> +                        f"{what!r} section cannot appear after free "
> +                        "paragraphs that follow other tagged sections. "
> +                        "Move this section upwards with the preceding "
> +                        "tagged sections."
> +                    )

Negative test case(s), please.

>  
>              while line is not None:
>                  # Blank lines
> @@ -513,6 +527,7 @@ def get_doc(self) -> 'QAPIDoc':
>                      if doc.features:
>                          raise QAPIParseError(
>                              self, "duplicated 'Features:' line")
> +                    _tag_check("Features")
>                      self.accept(False)
>                      line = self.get_doc_line()
>                      while line == '':
> @@ -576,6 +591,7 @@ def get_doc(self) -> 'QAPIDoc':
>                          )
>                          raise QAPIParseError(self, emsg)
>  
> +                    _tag_check(match.group(1))
>                      doc.new_tagged_section(
>                          self.info,
>                          QAPIDoc.Kind.from_string(match.group(1))

[...]

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This clarifies sections that are mistaken by the parser as "intro"
> sections to be "details" sections instead.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qapi/machine.json      | 2 ++
>  qapi/migration.json    | 4 ++++
>  qapi/qom.json          | 4 ++++
>  qapi/yank.json         | 2 ++
>  scripts/qapi/parser.py | 8 ++++++++
>  5 files changed, 20 insertions(+)

Missing updates for the new syntax

* Documentation: docs/devel/qapi-code-gen.rst

* Positive test case(s): tests/qapi-schema/doc-good.json

* Maybe a negative test case for _tag_check() failure

[...]

> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index c5d2b950a82..5890a13b5ba 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -544,6 +544,14 @@ def _tag_check(what: str) -> None:
>                          raise QAPIParseError(
>                              self, 'feature descriptions expected')
>                      have_tagged = True
> +                elif line == 'Details:':
> +                    _tag_check("Details")

This one.

> +                    self.accept(False)
> +                    line = self.get_doc_line()
> +                    while line == '':
> +                        self.accept(False)
> +                        line = self.get_doc_line()
> +                    have_tagged = True
>                  elif match := self._match_at_name_colon(line):
>                      # description
>                      if have_tagged:

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This clarifies sections that are mistaken by the parser as "intro"
> sections to be "details" sections instead.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

This is rather terse.

Why does the boundary between "intro" (previously "body") and "details"
matter?  As far as I understand, it matters for inlining.

What is inlining?

The old doc generator emits "The members of T" into the argument
description in the following cases:

* When a command's arguments are given as a type T, the doc comment has
  no argument descriptions, and the generated argument description
  becomes "The members of T".

* When an object type has a base type T, "The members of T" is appended
  to the doc comment's (possibly empty) argument descriptions.

* For union types, "The members of T when TAG is VALUE" is appended to
  the doc comment's argument descriptions for every tag VALUE and
  associated type T.

We want a description of the members of T right there instead.  To get
it right there, we need to inline from T's documentation.

What exactly do we need to inline?  Turns out we don't want "intro", we
do want the argument descriptions and other stuff we can ignore here.

"intro" ends before the argument descriptions, features, or a tagged
section, whatever comes first.  Most of the time, this works fine.  But
there are a few troublesome cases.  Here's one:

    ##
    # @MemoryBackendShmProperties:
    #
    # Properties for memory-backend-shm objects.
    #
    # This memory backend supports only shared memory, which is the
    # default.
    #
    # Since: 9.1
    ##
    { 'struct': 'MemoryBackendShmProperties',
      'base': 'MemoryBackendProperties',
      'data': { },
      'if': 'CONFIG_POSIX' }

Everything up to "Since:" is "intro".  Consequently, the old doc
generator emits "The members of MemoryBackendProperties" right there:

    "MemoryBackendShmProperties" (Object)
    -------------------------------------

    Properties for memory-backend-shm objects.

    This memory backend supports only shared memory, which is the default.


    Members
    ~~~~~~~

    The members of "MemoryBackendProperties"

    Since
    ~~~~~

    9.1


    If
    ~~

    "CONFIG_POSIX"

That's also where the new one inlines.  Okay so far.

This gets in turn inlined into ObjectOptions for branch
memory-backend-shm.  Since we don't inline "intro", we don't inline
"This memory backend supports only shared memory, which is the default."
That's a problem.

This patch moves the boundary between "intro" and the remainder up that
paragraph, so we don't lose that line.  It accomplishes that by giving
us syntax to manually mark the end of "intro"

However, your solution is manual: it gives us the means[*] to mark the
boundary with "Details:" to avoid loss of text.  What if we don't
notice?  Should we tweak the syntax to force us to be explicit?  How
many doc comments would that affect?


[*] Actually, we have means even before this patch, they're just ugly.
See the TODO comment added in commit 14b48aaab92 (qapi: convert
"Example" sections without titles).

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

[John Snow]

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

"The text handler you add looks just like the existing latex handler. Does
LaTeX output lack "little headings", too?"

Yes, almost certainly. Can you let me know which output formats we actually
"care about"? I'll have to test them all. In the meantime, I upgraded my
patch so that the text translator properly handles branches with headings
that delineate the different branches so that the text output is fully
reasonable. I will need to do the same for any format we care about.

I've re-pushed as of "about 30 minutes before I wrote this email" --
https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2

This branch includes the text generator fixes (which technically belong
with the predecessor series we skipped, but I'll refactor that later.)
it also includes fixes to the branch inliner, generated return statements,
and generated out-of-band feature sections.

(Long story short: inserting new sections in certain spots was broken
because of cache. Oops. We can discuss more why I wrote that part of the
code like I did in review for the patch that introduced that problem. It's
the "basic inliner" patch.)

Below, I'm going to try a new communication approach where I explicitly say
if I have added something to my tasklist or not so that it's clear to you
what I believe is actionable (and what I am agreeing to change) and what I
believe needs stronger input from you before I do anything. Apologies if it
seems a little robotic, just trying new things O:-)

On that note: not added to tasklist: do we need the LaTeX handler? Do we
need any others? Please confirm O:-)


On Fri, Feb 14, 2025 at 7:05 AM Markus Armbruster <armbru@redhat.com> wrote:

> I started to eyeball old and new generated output side by side.
>
> New table of contents shows one level, old two.  No objection; the
> navigation thingie on the left is more useful anyway.
>

Unintentional, but if you like it, it's fine by me. Nothing added to my
tasklist.


>
> The new generator elides unreferenced types.  Generally good, but two
> observations:
>
> * QapiErrorClass is unreferenced, but its members are mentioned in
>   Errors sections.  QapiErrorClass serves as better than nothing error
>   code documentation, but it's gone in the new doc.  So this is a minor
>   regression.  We can figure out what to do about it later.
>

Right. I debated making the members references to that class, but recalled
that you disliked this class and figured you'd not like such a change, so I
just left it alone. I do not have cross-references for individual members
of objects at all yet anyway, so this is definitely more work regardless.

We could always create a pragma of some sort (or just hardcode a list) of
items that must be documented regardless of if they're referenced or not.
Please let me know your preference and I will add a "ticket" on my personal
tasklist for this project to handle that at /some point/. Nothing added to
my tasklist just yet.


>
> * Section "QMP errors" is empty in the new doc, because its entire
>   contents is elided.  I guess we should elide the section as well, but
>   it's fine to leave that for later.
>

Adding to tasklist to elide empty modules, but "for later".


>
> Old doc shows a definition's since information like any other section.
> New doc has it in the heading box.  Looks prettier and uses much less
> space.  Not sure the heading box is the best place, but it'll do for
> now, we can always move it around later.
>

Agree, it's a strict improvement - there may be further improvements, but
that is always true anyway. When we tackle "autogenerated since
information" we can tackle the since display issues more meticulously. Or
maybe we'll need do sooner because of conflicting info in branches or
whatever else. I dunno, I'll burn that bridge when I get to it. Nothing
added to tasklist.


>
> The new doc's headings use "Struct" or "Union" where the old one uses
> just "Object".  Let's keep "Object", please.
>

I was afraid you'd ask for this. OK, I think it's an easy change. Can I
keep the index page segmented by object type still, though?

I do find knowing the *type* of object to be helpful as a developer, though
I understand that from the point of view of a QMP user, they're all just
objects, so your request makes sense.

Replace JSON object type headers with "Object" instead of QAPI data types
added to tasklist.


>
> In the new doc, some member references are no longer rendered as such,
> e.g. @on-source-error and @on-target-error in BackupCommon's note.
> Another small regression.
>

Ah, I actually knew this one. I didn't implement special formatting for
these yet. I do not have cross-references for individual members, so
there's nothing to transform these *into* yet. If you'd like special
rendering for them (fixed width, no link?) that's easy to accomplish. I am
not yet sure where I will do that conversion.

Reminder/Note that in my KVM Forum branch, I did actually replace all
@references that pointed to something actually cross-referenceable with an
actual sphinx cross-reference, leaving only @member references behind.

Nothing added to tasklist just yet.


>
> Union branches are busted in the new generator's output.  I know they
> used to work, so I'm not worried about it.
>

Fixed in new push, sorry! An embarrassing mistake that took me aeons to
spot.


>
> The new doc shows the return type, the old doc doesn't.  Showing it is
> definitely an improvement, but we need to adjust the doc text to avoid
> silliness like "Returns: SnapshotInfo – SnapshotInfo".
>

My KVM Forum branch actually corrected the QAPI documentation to remove
pointless returns. I didn't include that with this series yet, it was long
enough. This issue will be addressed solely through source documentation
edits, of which I believe I already have a comprehensive patch for.

Added to my tasklist: "Submit source documentation patches to remove
pointless return documentation"

It was my intent to submit those patches *afterwards*, but we can always do
it before if you'd like. Let me know. (I don't know offhand how easy they
are to extricate from my KVM Forum branch. I reserve the right to change my
mind on being flexible depending on the answer there :p)


>
> The new doc shows Arguments / Members, Returns, and Errors in two-column
> format.  Looks nice.  But for some reason, the two columns don't align
> horizontally for Errors like they do for the others.  Certainly not a
> blocker of anything, but we should try to fix it at some point.
>

Known issue. The reason is because we do not mandate a source documentation
format for errors - by convention, it is a list. There is (or was?) one
occurrence where it wasn't a list and I wrote a patch to change that. I
don't recall right now if we merged that or not. The misalignment is a
result of nesting a list inside of a list.

If we *mandate* the source format, I gain the ability to "peel off the
nesting", which will fix the alignment.

Added to tasklist: "Address vertical misalignment in Errors formatting"

Not added: how? need more input from you, please.


>
> The new doc doesn't show non-definition conditionals, as mentioned in
> the cover letter.  It shows definition conditionals twice.  Once should
> suffice.
>

Known/intentional issue. I couldn't decide where I wanted it, so I put it
in both places. If you have a strong opinion right now, please let me know
what it is and I'll take care of it, it's easy - but it's code in the
predecessor series and nothing to do with qapidoc, so please put it out of
mind for now.

If you don't have strong feelings, or you feel that the answer may depend
on how we solve other glaring issues (non-definition conditionals), let's
wait a little bit before making a decision.

Added to tasklist: "Remove the duplication of definition conditionals";
left unspecified is how or in what direction :)


>
> There's probably more, but this is it for now.
>
>

Tasklist:

 For the qapi-domain (prequel!) series:
  - Remove the duplication of definition conditionals

For this (qapidoc) series:
  - Display all JSON object types as "Object" and not as their QAPI data
type.

For later:
  - Elide empty modules
  - Submit source documentation patches to remove pointless return
documentation
  - Address vertical misalignment in Errors formatting

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

Re: [PATCH 27/42] qapi: differentiate "intro" and "detail" sections

[John Snow]

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

On Tue, Feb 11, 2025 at 10:00 AM Markus Armbruster <armbru@redhat.com>
wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This patch begins distinguishing "Plain" sections as being either
> > "Intro" or "Details" sections for the purpose of knowing when and where
> > to inline those sections.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> [...]
>
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index a8b30ae1a4b..b2f77ffdd7a 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
>
> [...]
>
> > @@ -584,14 +584,20 @@ def get_doc(self) -> 'QAPIDoc':
> >                      if text:
> >                          doc.append_line(text)
> >                      line = self.get_doc_indented(doc)
> > -                    no_more_args = True
> > +                    have_tagged = True
> >                  elif line.startswith('='):
> >                      raise QAPIParseError(
> >                          self,
> >                          "unexpected '=' markup in definition
> documentation")
> >                  else:
> >                      # plain paragraph(s)
> > -                    doc.ensure_untagged_section(self.info)
> > +                    if have_tagged:
> > +                        no_more_tags = True
>
> @no_more_tags is not used in this patch.  Does the conditional
> assignment belong to the next patch?
>

Yep, oopsie. Refactor blunder.

--js

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

Re: [PATCH 28/42] qapi/parser: prohibit untagged sections between tagged sections

[John Snow]

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

On Wed, Feb 12, 2025 at 4:06 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This is being done primarily to ensure consistency between the source
> > documents and the final, rendered HTML output. Because
> > member/feature/returns sections will always appear in a visually grouped
> > element in the HTML output, prohibiting free paragraphs between those
> > sections ensures ordering consistency between source and the final
> > render.
> >
> > Additionally, prohibiting such "middle" text paragraphs allows us to
> > classify all plain text sections as either "intro" or "detail"
> > sections, because these sections must either appear before structured
> > elements ("intro") or afterwards ("detail").
> >
> > This keeps the inlining algorithm simpler with fewer "splice" points
> > when inlining multiple documentation blocks.
>
> Mention the two "middle" paragraphs you have to eliminate in this patch?
>

OK; I will mention that this patch adjusts the source documentation but I
won't go into detail on which. You can read the patch to find out easily
enough.


>
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  qapi/net.json                   |  4 ++--
> >  qapi/qom.json                   |  4 ++--
> >  scripts/qapi/parser.py          | 16 ++++++++++++++++
> >  tests/qapi-schema/doc-good.json |  4 ++--
> >  tests/qapi-schema/doc-good.out  |  4 ++--
> >  tests/qapi-schema/doc-good.txt  |  8 ++++----
> >  6 files changed, 28 insertions(+), 12 deletions(-)
> >
> > diff --git a/qapi/net.json b/qapi/net.json
> > index 2739a2f4233..49bc7de64e9 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -655,13 +655,13 @@
> >  #     this to zero disables this function.  This member is mutually
> >  #     exclusive with @reconnect.  (default: 0) (Since: 9.2)
> >  #
> > -# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
> > -#
> >  # Features:
> >  #
> >  # @deprecated: Member @reconnect is deprecated.  Use @reconnect-ms
> >  #     instead.
> >  #
> > +# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
> > +#
> >  # Since: 7.2
> >  ##
> >  { 'struct': 'NetdevStreamOptions',
>
> The text moved applies to member @addr.  You're moving it even farther
> away from @addr.  Move it into @addr instead?  Could be done as a
> separate cleanup patch to keep this one as simple as possible; matter of
> taste.
>

Mmm, I was doing a mechanical hacksaw job here, admittedly. I can do a more
tactful adjustment. I think it should be in this patch in order to preserve
the context of precisely *why* it was juggled around, because I admit in
this one case it is a slight downgrade.

Moving it into @addr.


>
> The same text is in NetdevDgramOptions below, where it applies to both
> @remote and @local.  It just happens to follow @remote and @local
> immediately, because there are no other members and no features.  Hmm.
>
> Ideally, we'd have a way to put such notes next to the stuff they apply
> to without having to rely on happy accidents like "no features".
> Alternatively, have a way to link stuff and note.  Footnotes?  Food for
> thought, not demand.
>

Yes, we discussed this at KVM Forum and I was dreaming of some complicated
solution like "section-details: " or something that allows us to add
amendments to documentation regions that aren't associated with any one
particular member or feature, but can be inserted visually at that point.

I know it's a capability you'd like to preserve, but I think we only use it
once, so I'd be happy to push this off until a bit later and just suffer
the indignity of slightly suboptimal documentation in one spot until then
in exchange for the massive upgrade everywhere else.

What would help a good deal is if you could brainstorm some source syntax
that you think would be pleasant for the purpose, and then for my end I can
worry about how to munge the docutils tree and HTML renderer to make it
happen in some pleasing way.

For now... "Figure out how to add notes or footnotes to the members section
as a whole" added to the "for later" part of my tasklist?


>
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 28ce24cd8d0..11277d1f84c 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -195,12 +195,12 @@
> >  #
> >  # @typename: the type name of an object
> >  #
> > +# Returns: a list of ObjectPropertyInfo describing object properties
> > +#
> >  # .. note:: Objects can create properties at runtime, for example to
> >  #    describe links between different devices and/or objects.  These
> >  #    properties are not included in the output of this command.
> >  #
> > -# Returns: a list of ObjectPropertyInfo describing object properties
> > -#
> >  # Since: 2.12
> >  ##
>
> This move is fine.  Placing notes at the end is more common already.
>
> >  { 'command': 'qom-list-properties',
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index b2f77ffdd7a..c5d2b950a82 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -500,6 +500,20 @@ def get_doc(self) -> 'QAPIDoc':
> >              self.accept(False)
> >              line = self.get_doc_line()
> >              have_tagged = False
> > +            no_more_tags = False
> > +
> > +            def _tag_check(what: str) -> None:
> > +                if what in ('TODO', 'Since'):
> > +                    return
> > +
> > +                if no_more_tags:
> > +                    raise QAPIParseError(
> > +                        self,
> > +                        f"{what!r} section cannot appear after free "
> > +                        "paragraphs that follow other tagged sections. "
> > +                        "Move this section upwards with the preceding "
> > +                        "tagged sections."
> > +                    )
>
> Why !r conversion?
>

It just adds quotes so it'll print like: "Returns" section cannot appear
after ...
I thought it looked nicer codewise than: f'"{what}" section cannot appear
after ...'


> Error messages should be a single, short phrase, no punctuation at the
> end.  Sometimes a helpful hint is desirable.  Here's one in expr.py:
>

>         raise QAPISemError(
>             info,
>             "%s has unknown key%s %s\nValid keys are %s."
>             % (source, 's' if len(unknown) > 1 else '',
>                pprint(unknown), pprint(allowed)))
>

Cold day in hell when I successfully remember to omit the punctuation. Will
rephrase and fix.


>
> Needs a negative test case.
>

Yes, I didn't add any new tests because I was being lazy and wanted to see
which things you'd toss out before I had to write them. No reason to do all
that work in advance of review. Please ask for tests wherever you feel they
are required and I'll add them. In this case, I knew you had some qualms
about this patch, so I was hesitant ...


>
> Aside: we should probably convert most string interpolation to f-strings
> en masse at some point.
>

Yeah, just putting it off because the review for that sounds like a hassle
;)
I can do a mechanical conversion to get you started and then let you
finesse it if you want?
We can share the pain.

If you really wanna have a flag day about it, we can just run the black
formatter on everything and get it all over with at once...

later stuff, to be clear.


>
> >
> >              while line is not None:
> >                  # Blank lines
> > @@ -513,6 +527,7 @@ def get_doc(self) -> 'QAPIDoc':
> >                      if doc.features:
> >                          raise QAPIParseError(
> >                              self, "duplicated 'Features:' line")
> > +                    _tag_check("Features")
> >                      self.accept(False)
> >                      line = self.get_doc_line()
> >                      while line == '':
> > @@ -576,6 +591,7 @@ def get_doc(self) -> 'QAPIDoc':
> >                          )
> >                          raise QAPIParseError(self, emsg)
> >
> > +                    _tag_check(match.group(1))
> >                      doc.new_tagged_section(
> >                          self.info,
> >                          QAPIDoc.Kind.from_string(match.group(1))
> > diff --git a/tests/qapi-schema/doc-good.json
> b/tests/qapi-schema/doc-good.json
> > index f64bf38d854..14b2091b08f 100644
> > --- a/tests/qapi-schema/doc-good.json
> > +++ b/tests/qapi-schema/doc-good.json
> > @@ -157,12 +157,12 @@
> >  # @cmd-feat1: a feature
> >  # @cmd-feat2: another feature
> >  #
> > -# .. note:: @arg3 is undocumented
> > -#
> >  # Returns: @Object
> >  #
> >  # Errors: some
> >  #
> > +# .. note:: @arg3 is undocumented
> > +#
>
> This used to be right next to @arg1 and arg2 (commit 80d1f2e4a5d) until
> commit 79598c8a634 added features in between.  This patch adds more
> stuff there.  Right next is clearly the best spot, but this is just a
> test, so it doesn't really matter.  Related: NetdevDgramOptions' note
> discussed above.
>

So long as it doesn't disturb the point of what is being tested. It's not
always directly clear when adjusting the good doc what precisely is being
tested.


>
> >  # TODO: frobnicate
> >  #
> >  # .. admonition:: Notes
>
> [...]
>
>

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

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[John Snow]

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

On Wed, Feb 12, 2025 at 4:37 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This clarifies sections that are mistaken by the parser as "intro"
> > sections to be "details" sections instead.
>
> Impact on output?  See notes inline.
>

It's very possible that there is none; in cases where the text is not
inlined, it won't make any visual difference. The occurrences in this patch
were identified with a warning from the generator that I didn't actually
submit as part of this patch series.

I was obeying an unseen master.


>
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  qapi/machine.json      | 2 ++
> >  qapi/migration.json    | 4 ++++
> >  qapi/qom.json          | 4 ++++
> >  qapi/yank.json         | 2 ++
> >  scripts/qapi/parser.py | 8 ++++++++
> >  5 files changed, 20 insertions(+)
> >
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index a6b8795b09e..3c1b397f6cc 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1301,6 +1301,8 @@
> >  # Return the amount of initially allocated and present hotpluggable
> >  # (if enabled) memory in bytes.
> >  #
> > +# Details:
> > +#
> >  # .. qmp-example::
> >  #
> >  #     -> { "execute": "query-memory-size-summary" }
>
> Output unchanged in my testing.  Same for the other hunks unless
> otherwise noted.
>
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 43babd1df41..9070a91e655 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1920,6 +1920,8 @@
> >  #
> >  # Xen uses this command to notify replication to trigger a checkpoint.
> >  #
> > +# Details:
> > +#
> >  # .. qmp-example::
> >  #
> >  #     -> { "execute": "xen-colo-do-checkpoint" }
> > @@ -1993,6 +1995,8 @@
> >  #
> >  # Pause a migration.  Currently it only supports postcopy.
> >  #
> > +# Details:
> > +#
> >  # .. qmp-example::
> >  #
> >  #     -> { "execute": "migrate-pause" }
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 11277d1f84c..5d285ef9239 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -729,6 +729,8 @@
> >  #
> >  # Properties for memory-backend-shm objects.
> >  #
> > +# Details:
> > +#
> >  # This memory backend supports only shared memory, which is the
> >  # default.
> >  #
>
> The paragraphs moves from above to below the auto-generated member
> documentation, like this:
>
>     @@ -25908,13 +25908,13 @@ If
>
>      Properties for memory-backend-shm objects.
>
>     -This memory backend supports only shared memory, which is the default.
>     -
>
>      Members
>      ~~~~~~~
>
>      The members of "MemoryBackendProperties"
>     +This memory backend supports only shared memory, which is the default.
>     +
>
>      Since
>      ~~~~~
>
> This is sphinx-build -b text.  I don't understand why there is no blank
> line between "The members of ... " and the moved paragraph.
>

... Me either! I'll investigate.


>
> > @@ -744,6 +746,8 @@
> >  #
> >  # Properties for memory-backend-epc objects.
> >  #
> > +# Details:
> > +#
> >  # The @merge boolean option is false by default with epc
> >  #
> >  # The @dump boolean option is false by default with epc
>
> Likewise.
>
> > diff --git a/qapi/yank.json b/qapi/yank.json
> > index 30f46c97c98..4d36d21e76a 100644
> > --- a/qapi/yank.json
> > +++ b/qapi/yank.json
> > @@ -104,6 +104,8 @@
> >  #
> >  # Returns: list of @YankInstance
> >  #
> > +# Details:
> > +#
> >  # .. qmp-example::
> >  #
> >  #     -> { "execute": "query-yank" }
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index c5d2b950a82..5890a13b5ba 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -544,6 +544,14 @@ def _tag_check(what: str) -> None:
> >                          raise QAPIParseError(
> >                              self, 'feature descriptions expected')
> >                      have_tagged = True
> > +                elif line == 'Details:':
> > +                    _tag_check("Details")
> > +                    self.accept(False)
> > +                    line = self.get_doc_line()
> > +                    while line == '':
> > +                        self.accept(False)
> > +                        line = self.get_doc_line()
> > +                    have_tagged = True
> >                  elif match := self._match_at_name_colon(line):
> >                      # description
> >                      if have_tagged:
>
>

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

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[John Snow]

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

On Mon, Feb 17, 2025 at 5:51 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This clarifies sections that are mistaken by the parser as "intro"
> > sections to be "details" sections instead.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Is this missing announce-self in net.json?
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 49bc7de64e..44ed72dbe9 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -948,7 +948,7 @@
>  # switches.  This can be useful when network bonds fail-over the
>  # active slave.
>  #
> -# TODO: This line is a hack to separate the example from the body
> +# Details:
>  #
>  # .. qmp-example::
>  #
>

Yes, overlooked. The "hack" still works, so I missed it in my tests. Will
remedy, pending your other emails that I'm about to read in a second ...

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

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[John Snow]

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

On Mon, Feb 17, 2025 at 6:55 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This clarifies sections that are mistaken by the parser as "intro"
> > sections to be "details" sections instead.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  qapi/machine.json      | 2 ++
> >  qapi/migration.json    | 4 ++++
> >  qapi/qom.json          | 4 ++++
> >  qapi/yank.json         | 2 ++
> >  scripts/qapi/parser.py | 8 ++++++++
> >  5 files changed, 20 insertions(+)
>
> Missing updates for the new syntax
>
> * Documentation: docs/devel/qapi-code-gen.rst
>

> * Positive test case(s): tests/qapi-schema/doc-good.json
>
> * Maybe a negative test case for _tag_check() failure
>
>
Understood; I wasn't entirely sure if this concept would fly, so I saved
the polish and you got an RFC quality patch. Forgive me, please! If you
think this approach is fine, I will certainly do all the things you
outlined above.


> [...]
>
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index c5d2b950a82..5890a13b5ba 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -544,6 +544,14 @@ def _tag_check(what: str) -> None:
> >                          raise QAPIParseError(
> >                              self, 'feature descriptions expected')
> >                      have_tagged = True
> > +                elif line == 'Details:':
> > +                    _tag_check("Details")
>
> This one.
>

ACK


>
> > +                    self.accept(False)
> > +                    line = self.get_doc_line()
> > +                    while line == '':
> > +                        self.accept(False)
> > +                        line = self.get_doc_line()
> > +                    have_tagged = True
> >                  elif match := self._match_at_name_colon(line):
> >                      # description
> >                      if have_tagged:
>
>

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

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[John Snow]

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

On Mon, Feb 17, 2025 at 7:13 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > This clarifies sections that are mistaken by the parser as "intro"
> > sections to be "details" sections instead.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> This is rather terse.
>

Mea culpa. I can write more at length if we agree on the general approach.
For now, you got an RFC as this was the subject of a considerable amount of
controversy between us in the past ... so I am doing baby steps.

"Commit message needs to be hit with the unterseification beam" added to
tasklist. :)


>
> Why does the boundary between "intro" (previously "body") and "details"
> matter?  As far as I understand, it matters for inlining.
>

> What is inlining?
>

> The old doc generator emits "The members of T" into the argument
> description in the following cases:
>
> * When a command's arguments are given as a type T, the doc comment has
>   no argument descriptions, and the generated argument description
>   becomes "The members of T".
>
> * When an object type has a base type T, "The members of T" is appended
>   to the doc comment's (possibly empty) argument descriptions.
>
> * For union types, "The members of T when TAG is VALUE" is appended to
>   the doc comment's argument descriptions for every tag VALUE and
>   associated type T.
>
> We want a description of the members of T right there instead.  To get
> it right there, we need to inline from T's documentation.
>
> What exactly do we need to inline?  Turns out we don't want "intro", we
> do want the argument descriptions and other stuff we can ignore here.
>
> "intro" ends before the argument descriptions, features, or a tagged
> section, whatever comes first.  Most of the time, this works fine.  But
> there are a few troublesome cases.  Here's one:
>
>     ##
>     # @MemoryBackendShmProperties:
>     #
>     # Properties for memory-backend-shm objects.
>     #
>     # This memory backend supports only shared memory, which is the
>     # default.
>     #
>     # Since: 9.1
>     ##
>     { 'struct': 'MemoryBackendShmProperties',
>       'base': 'MemoryBackendProperties',
>       'data': { },
>       'if': 'CONFIG_POSIX' }
>
> Everything up to "Since:" is "intro".  Consequently, the old doc
> generator emits "The members of MemoryBackendProperties" right there:
>
>     "MemoryBackendShmProperties" (Object)
>     -------------------------------------
>
>     Properties for memory-backend-shm objects.
>
>     This memory backend supports only shared memory, which is the default.
>
>
>     Members
>     ~~~~~~~
>
>     The members of "MemoryBackendProperties"
>
>     Since
>     ~~~~~
>
>     9.1
>
>
>     If
>     ~~
>
>     "CONFIG_POSIX"
>
> That's also where the new one inlines.  Okay so far.
>
> This gets in turn inlined into ObjectOptions for branch
> memory-backend-shm.  Since we don't inline "intro", we don't inline
> "This memory backend supports only shared memory, which is the default."
> That's a problem.
>

Yes, this is all correct so far.


>
> This patch moves the boundary between "intro" and the remainder up that
> paragraph, so we don't lose that line.  It accomplishes that by giving
> us syntax to manually mark the end of "intro"
>
> However, your solution is manual: it gives us the means[*] to mark the
> boundary with "Details:" to avoid loss of text.  What if we don't
> notice?  Should we tweak the syntax to force us to be explicit?  How
> many doc comments would that affect?
>

I'm leaving that question to you. The calculus I made was that there were
fewer SLOC changes to explicitly denote the "Details:" sections only in the
handful of cases where it was (potentially) relevant than to mandate its
use unconditionally. If you have an idea that is enforceable at runtime and
has fewer SLOC changes, suggest away!

Unseen in this patch is a warning I added to the /inliner/ that identified
potentially "ambiguous" delineation spots and issued a warning (error); the
exact code that did this is possibly a little hokey but it was what I used
to identify the spots addressed by this patch.

Point being: it's possible to enforce, but I enforced it in qapidoc.py in
the inliner instead of directly in the parser. We could discuss moving the
check to the parser if you'd like. The check itself is somewhat "dumb":

- If a doc block has only one *paragraph* (knowingly/intentionally not
using the term section here) of text, it's assumed to be the intro.
- If a doc block has any number of tagged sections, all text above (if any)
is assumed to be the "intro" and all text below (if any) is assumed to be
"details".

It's only in this case that it whines:

- A doc block has *multiple paragraphs* of text at the start of the block,
but has no other sections and so if there is semantically a "details"
section or not is unclear to the parser and inliner.

The check as I wrote it is unintelligent in that it does not bother to
check if the doc block it is checking is ever one that *could* be inlined;
i.e. it will complain about being unable to delineate for commands -- even
though it wouldn't really matter in that case. It's a potential improvement
to the algorithm to ignore cases where that "ambiguity" is not actually
important.

But, it's possible to mechanically enforce and nudge documentation writers
to add the delineation marker where the parser is uncertain.


>
> [*] Actually, we have means even before this patch, they're just ugly.
> See the TODO comment added in commit 14b48aaab92 (qapi: convert
> "Example" sections without titles)


That's right. This is merely a formalization of that hack: I add a
"section" that is intentionally empty and serves only as a marker to the
parser to begin recording a new section.

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

Re: [PATCH 28/42] qapi/parser: prohibit untagged sections between tagged sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Feb 12, 2025 at 4:06 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This is being done primarily to ensure consistency between the source
>> > documents and the final, rendered HTML output. Because
>> > member/feature/returns sections will always appear in a visually grouped
>> > element in the HTML output, prohibiting free paragraphs between those
>> > sections ensures ordering consistency between source and the final
>> > render.
>> >
>> > Additionally, prohibiting such "middle" text paragraphs allows us to
>> > classify all plain text sections as either "intro" or "detail"
>> > sections, because these sections must either appear before structured
>> > elements ("intro") or afterwards ("detail").
>> >
>> > This keeps the inlining algorithm simpler with fewer "splice" points
>> > when inlining multiple documentation blocks.
>>
>> Mention the two "middle" paragraphs you have to eliminate in this patch?
>>
>
> OK; I will mention that this patch adjusts the source documentation but I
> won't go into detail on which. You can read the patch to find out easily
> enough.
>
>
>>
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  qapi/net.json                   |  4 ++--
>> >  qapi/qom.json                   |  4 ++--
>> >  scripts/qapi/parser.py          | 16 ++++++++++++++++
>> >  tests/qapi-schema/doc-good.json |  4 ++--
>> >  tests/qapi-schema/doc-good.out  |  4 ++--
>> >  tests/qapi-schema/doc-good.txt  |  8 ++++----
>> >  6 files changed, 28 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json
>> > index 2739a2f4233..49bc7de64e9 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -655,13 +655,13 @@
>> >  #     this to zero disables this function.  This member is mutually
>> >  #     exclusive with @reconnect.  (default: 0) (Since: 9.2)
>> >  #
>> > -# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
>> > -#
>> >  # Features:
>> >  #
>> >  # @deprecated: Member @reconnect is deprecated.  Use @reconnect-ms
>> >  #     instead.
>> >  #
>> > +# Only SocketAddress types 'unix', 'inet' and 'fd' are supported.
>> > +#
>> >  # Since: 7.2
>> >  ##
>> >  { 'struct': 'NetdevStreamOptions',
>>
>> The text moved applies to member @addr.  You're moving it even farther
>> away from @addr.  Move it into @addr instead?  Could be done as a
>> separate cleanup patch to keep this one as simple as possible; matter of
>> taste.
>>
>
> Mmm, I was doing a mechanical hacksaw job here, admittedly. I can do a more
> tactful adjustment. I think it should be in this patch in order to preserve
> the context of precisely *why* it was juggled around, because I admit in
> this one case it is a slight downgrade.
>
> Moving it into @addr.
>
>
>>
>> The same text is in NetdevDgramOptions below, where it applies to both
>> @remote and @local.  It just happens to follow @remote and @local
>> immediately, because there are no other members and no features.  Hmm.
>>
>> Ideally, we'd have a way to put such notes next to the stuff they apply
>> to without having to rely on happy accidents like "no features".
>> Alternatively, have a way to link stuff and note.  Footnotes?  Food for
>> thought, not demand.
>>
>
> Yes, we discussed this at KVM Forum and I was dreaming of some complicated
> solution like "section-details: " or something that allows us to add
> amendments to documentation regions that aren't associated with any one
> particular member or feature, but can be inserted visually at that point.
>
> I know it's a capability you'd like to preserve, but I think we only use it
> once, so I'd be happy to push this off until a bit later and just suffer
> the indignity of slightly suboptimal documentation in one spot until then
> in exchange for the massive upgrade everywhere else.

If minor degradations are the price of major improvement, we pay.

> What would help a good deal is if you could brainstorm some source syntax
> that you think would be pleasant for the purpose, and then for my end I can
> worry about how to munge the docutils tree and HTML renderer to make it
> happen in some pleasing way.

Can we make do with just ReST?  Footnotes?  Best if we can control their
placement somehow.

> For now... "Figure out how to add notes or footnotes to the members section
> as a whole" added to the "for later" part of my tasklist?

Yes, please, with the understanding that this is *not* a blocker for
getting the new doc generator accepted.

>> > diff --git a/qapi/qom.json b/qapi/qom.json
>> > index 28ce24cd8d0..11277d1f84c 100644
>> > --- a/qapi/qom.json
>> > +++ b/qapi/qom.json
>> > @@ -195,12 +195,12 @@
>> >  #
>> >  # @typename: the type name of an object
>> >  #
>> > +# Returns: a list of ObjectPropertyInfo describing object properties
>> > +#
>> >  # .. note:: Objects can create properties at runtime, for example to
>> >  #    describe links between different devices and/or objects.  These
>> >  #    properties are not included in the output of this command.
>> >  #
>> > -# Returns: a list of ObjectPropertyInfo describing object properties
>> > -#
>> >  # Since: 2.12
>> >  ##
>>
>> This move is fine.  Placing notes at the end is more common already.
>>
>> >  { 'command': 'qom-list-properties',
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index b2f77ffdd7a..c5d2b950a82 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -500,6 +500,20 @@ def get_doc(self) -> 'QAPIDoc':
>> >              self.accept(False)
>> >              line = self.get_doc_line()
>> >              have_tagged = False
>> > +            no_more_tags = False
>> > +
>> > +            def _tag_check(what: str) -> None:
>> > +                if what in ('TODO', 'Since'):
>> > +                    return
>> > +
>> > +                if no_more_tags:
>> > +                    raise QAPIParseError(
>> > +                        self,
>> > +                        f"{what!r} section cannot appear after free "
>> > +                        "paragraphs that follow other tagged sections. "
>> > +                        "Move this section upwards with the preceding "
>> > +                        "tagged sections."
>> > +                    )
>>
>> Why !r conversion?
>>
>
> It just adds quotes so it'll print like: "Returns" section cannot appear
> after ...
> I thought it looked nicer codewise than: f'"{what}" section cannot appear
> after ...'

Prior art:

                raise QAPISemError(
                    info, "duplicated '%s' section" % kind)

                raise QAPISemError(
                    self.returns.info,
                    "'Returns' section, but command doesn't return anything")

                raise QAPISemError(
                    self.returns.info,
                    "'Returns' section is only valid for commands")

                raise QAPISemError(
                    self.errors.info,
                    "'Errors' section is only valid for commands")

Let's stick to this style.

>> Error messages should be a single, short phrase, no punctuation at the
>> end.  Sometimes a helpful hint is desirable.  Here's one in expr.py:
>>
>
>>         raise QAPISemError(
>>             info,
>>             "%s has unknown key%s %s\nValid keys are %s."
>>             % (source, 's' if len(unknown) > 1 else '',
>>                pprint(unknown), pprint(allowed)))
>>
>
> Cold day in hell when I successfully remember to omit the punctuation. Will
> rephrase and fix.

:)

>> Needs a negative test case.
>>
>
> Yes, I didn't add any new tests because I was being lazy and wanted to see
> which things you'd toss out before I had to write them. No reason to do all
> that work in advance of review. Please ask for tests wherever you feel they
> are required and I'll add them. In this case, I knew you had some qualms
> about this patch, so I was hesitant ...

Good strategy.  I'd appreciate notes on known gaps in the commit message
and/or the cover letter, though.  Don't go out of your way to find gaps,
just make a reasonable effort to write down the ones you already know.

>> Aside: we should probably convert most string interpolation to f-strings
>> en masse at some point.
>>
>
> Yeah, just putting it off because the review for that sounds like a hassle
> ;)
> I can do a mechanical conversion to get you started and then let you
> finesse it if you want?
> We can share the pain.
>
> If you really wanna have a flag day about it, we can just run the black
> formatter on everything and get it all over with at once...
>
> later stuff, to be clear.

Yes, all of it.

>> >
>> >              while line is not None:
>> >                  # Blank lines
>> > @@ -513,6 +527,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >                      if doc.features:
>> >                          raise QAPIParseError(
>> >                              self, "duplicated 'Features:' line")
>> > +                    _tag_check("Features")
>> >                      self.accept(False)
>> >                      line = self.get_doc_line()
>> >                      while line == '':
>> > @@ -576,6 +591,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >                          )
>> >                          raise QAPIParseError(self, emsg)
>> >
>> > +                    _tag_check(match.group(1))
>> >                      doc.new_tagged_section(
>> >                          self.info,
>> >                          QAPIDoc.Kind.from_string(match.group(1))
>> > diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
>> > index f64bf38d854..14b2091b08f 100644
>> > --- a/tests/qapi-schema/doc-good.json
>> > +++ b/tests/qapi-schema/doc-good.json
>> > @@ -157,12 +157,12 @@
>> >  # @cmd-feat1: a feature
>> >  # @cmd-feat2: another feature
>> >  #
>> > -# .. note:: @arg3 is undocumented
>> > -#
>> >  # Returns: @Object
>> >  #
>> >  # Errors: some
>> >  #
>> > +# .. note:: @arg3 is undocumented
>> > +#
>>
>> This used to be right next to @arg1 and arg2 (commit 80d1f2e4a5d) until
>> commit 79598c8a634 added features in between.  This patch adds more
>> stuff there.  Right next is clearly the best spot, but this is just a
>> test, so it doesn't really matter.  Related: NetdevDgramOptions' note
>> discussed above.
>>
>
> So long as it doesn't disturb the point of what is being tested. It's not
> always directly clear when adjusting the good doc what precisely is being
> tested.

True.

>> >  # TODO: frobnicate
>> >  #
>> >  # .. admonition:: Notes
>>
>> [...]
>>
>>

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Feb 17, 2025 at 6:55 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This clarifies sections that are mistaken by the parser as "intro"
>> > sections to be "details" sections instead.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  qapi/machine.json      | 2 ++
>> >  qapi/migration.json    | 4 ++++
>> >  qapi/qom.json          | 4 ++++
>> >  qapi/yank.json         | 2 ++
>> >  scripts/qapi/parser.py | 8 ++++++++
>> >  5 files changed, 20 insertions(+)
>>
>> Missing updates for the new syntax
>>
>> * Documentation: docs/devel/qapi-code-gen.rst
>>
>
>> * Positive test case(s): tests/qapi-schema/doc-good.json
>>
>> * Maybe a negative test case for _tag_check() failure
>>
>>
> Understood; I wasn't entirely sure if this concept would fly, so I saved
> the polish and you got an RFC quality patch. Forgive me, please! If you

As I wrote in review of PATCH 28, this is good strategy.

> think this approach is fine, I will certainly do all the things you
> outlined above.
>
>
>> [...]
>>
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index c5d2b950a82..5890a13b5ba 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -544,6 +544,14 @@ def _tag_check(what: str) -> None:
>> >                          raise QAPIParseError(
>> >                              self, 'feature descriptions expected')
>> >                      have_tagged = True
>> > +                elif line == 'Details:':
>> > +                    _tag_check("Details")
>>
>> This one.
>>
>
> ACK
>
>
>>
>> > +                    self.accept(False)
>> > +                    line = self.get_doc_line()
>> > +                    while line == '':
>> > +                        self.accept(False)
>> > +                        line = self.get_doc_line()
>> > +                    have_tagged = True
>> >                  elif match := self._match_at_name_colon(line):
>> >                      # description
>> >                      if have_tagged:
>>
>>

Re: [PATCH 29/42] qapi: Add "Details:" disambiguation marker

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Feb 17, 2025 at 7:13 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This clarifies sections that are mistaken by the parser as "intro"
>> > sections to be "details" sections instead.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> This is rather terse.
>>
>
> Mea culpa. I can write more at length if we agree on the general approach.
> For now, you got an RFC as this was the subject of a considerable amount of
> controversy between us in the past ... so I am doing baby steps.
>
> "Commit message needs to be hit with the unterseification beam" added to
> tasklist. :)
>
>
>>
>> Why does the boundary between "intro" (previously "body") and "details"
>> matter?  As far as I understand, it matters for inlining.
>>
>
>> What is inlining?
>>
>
>> The old doc generator emits "The members of T" into the argument
>> description in the following cases:
>>
>> * When a command's arguments are given as a type T, the doc comment has
>>   no argument descriptions, and the generated argument description
>>   becomes "The members of T".
>>
>> * When an object type has a base type T, "The members of T" is appended
>>   to the doc comment's (possibly empty) argument descriptions.
>>
>> * For union types, "The members of T when TAG is VALUE" is appended to
>>   the doc comment's argument descriptions for every tag VALUE and
>>   associated type T.
>>
>> We want a description of the members of T right there instead.  To get
>> it right there, we need to inline from T's documentation.
>>
>> What exactly do we need to inline?  Turns out we don't want "intro", we
>> do want the argument descriptions and other stuff we can ignore here.
>>
>> "intro" ends before the argument descriptions, features, or a tagged
>> section, whatever comes first.  Most of the time, this works fine.  But
>> there are a few troublesome cases.  Here's one:
>>
>>     ##
>>     # @MemoryBackendShmProperties:
>>     #
>>     # Properties for memory-backend-shm objects.
>>     #
>>     # This memory backend supports only shared memory, which is the
>>     # default.
>>     #
>>     # Since: 9.1
>>     ##
>>     { 'struct': 'MemoryBackendShmProperties',
>>       'base': 'MemoryBackendProperties',
>>       'data': { },
>>       'if': 'CONFIG_POSIX' }
>>
>> Everything up to "Since:" is "intro".  Consequently, the old doc
>> generator emits "The members of MemoryBackendProperties" right there:
>>
>>     "MemoryBackendShmProperties" (Object)
>>     -------------------------------------
>>
>>     Properties for memory-backend-shm objects.
>>
>>     This memory backend supports only shared memory, which is the default.
>>
>>
>>     Members
>>     ~~~~~~~
>>
>>     The members of "MemoryBackendProperties"
>>
>>     Since
>>     ~~~~~
>>
>>     9.1
>>
>>
>>     If
>>     ~~
>>
>>     "CONFIG_POSIX"
>>
>> That's also where the new one inlines.  Okay so far.
>>
>> This gets in turn inlined into ObjectOptions for branch
>> memory-backend-shm.  Since we don't inline "intro", we don't inline
>> "This memory backend supports only shared memory, which is the default."
>> That's a problem.
>>
>
> Yes, this is all correct so far.
>
>
>>
>> This patch moves the boundary between "intro" and the remainder up that
>> paragraph, so we don't lose that line.  It accomplishes that by giving
>> us syntax to manually mark the end of "intro"
>>
>> However, your solution is manual: it gives us the means[*] to mark the
>> boundary with "Details:" to avoid loss of text.  What if we don't
>> notice?  Should we tweak the syntax to force us to be explicit?  How
>> many doc comments would that affect?
>>
>
> I'm leaving that question to you. The calculus I made was that there were
> fewer SLOC changes to explicitly denote the "Details:" sections only in the
> handful of cases where it was (potentially) relevant than to mandate its
> use unconditionally.

How did you determine where it is (potentially) relevant?  Oh, wait ...

>                      If you have an idea that is enforceable at runtime and
> has fewer SLOC changes, suggest away!
>
> Unseen in this patch is a warning I added to the /inliner/ that identified
> potentially "ambiguous" delineation spots and issued a warning (error); the
> exact code that did this is possibly a little hokey but it was what I used
> to identify the spots addressed by this patch.

... that's how.

> Point being: it's possible to enforce, but I enforced it in qapidoc.py in
> the inliner instead of directly in the parser. We could discuss moving the
> check to the parser if you'd like. The check itself is somewhat "dumb":
>
> - If a doc block has only one *paragraph* (knowingly/intentionally not
> using the term section here) of text, it's assumed to be the intro.

You mean if the "body" has just one paragraph, right?  The "body" is the
first section, always untagged, possibly empty.  It's contains the text
between the line naming the definition and the first tagged section.

The tagged sections are member / argument descriptions, feature
descriptions, 'Returns', 'Errors', 'Since', and 'TODO'.

> - If a doc block has any number of tagged sections, all text above (if any)
> is assumed to be the "intro" and all text below (if any) is assumed to be
> "details".

Uh, this can't be quite right.

Consider:

    ##
    # @query-memory-size-summary:
    #
    # Return the amount of initially allocated and present hotpluggable
    # (if enabled) memory in bytes.
    #
    # .. qmp-example::
    #
    #     -> { "execute": "query-memory-size-summary" }
    #     <- { "return": { "base-memory": 4294967296, "plugged-memory": 0 } }
    #
--> # Since: 2.11
    ##

There is a tagged section.  According to your explanation, the text
above, i.e. everything between @query-memory-size-summary: and Since: is
assumed to be "intro".

According to your patch, which adds "Details:" in the middle, we do not
assume this.  Contradiction.

> It's only in this case that it whines:
>
> - A doc block has *multiple paragraphs* of text at the start of the block,
> but has no other sections and so if there is semantically a "details"
> section or not is unclear to the parser and inliner.

Let's take a step back.  docs/devel/qapi-code-gen.rst:

    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.

Bug: should be "finally optional tagged or untagged sections".

Your generator wants all but 'Since' and 'TODO' together, so it can
render them in a single two-column table.

This description table separates "intro" (above) and "details" (below).
Fair?

Fine and dandy separation unless the description table is *empty*.

Then the "body" (first section, always untagged) extends to the first
'Since', 'TODO', or the end of the doc comment.

Heuristic: when this first untagged section is a single paragraph, we
quietly assume it's "intro".  If it's more than one, we ask the
programmer to mark the end of "intro" explicitly.

Let's see how this works out in practice.  I stick

        if self.symbol and not (self.args or self.features or self.returns or self.errors):
            if self.body.text.find('\n\n') == -1:
                print(f"{self.info}: single para")
            else:
                print(f"{self.info}: ambiguous")

into QAPIDoc.check().  The outer conditional is true for definition
documentation (doc.symbol) where the table is empty (not ...).  The
inner conditional is a crude check for paragraphs.

This reports 47 "single para" and 8 "ambiguous" in the main QAPI schema
in master.

Your patch hits 5 of 8 ambiguous ones, and throws in a 6th that doesn't
seem to need it:

    ##
    # @query-yank:
    #
    # Query yank instances.  See @YankInstance for more information.
    #
    # Returns: list of @YankInstance
    #
    # .. qmp-example::
    #
    #     -> { "execute": "query-yank" }
    #     <- { "return": [
    #              { "type": "block-node",
    #                "node-name": "nbd0" }
    #          ] }
    #
    # Since: 6.0
    ##

It misses in run-state.json:

    ##
    # @SUSPEND_DISK:
    #
    # Emitted when guest enters a hardware suspension state with data
    # saved on disk, for example, S4 state, which is sometimes called
    # hibernate state
    #
    # .. note:: QEMU shuts down (similar to event @SHUTDOWN) when entering
    #    this state.
    #
    # Since: 1.2
    #
    # .. qmp-example::
    #
    #     <- { "event": "SUSPEND_DISK",
    #          "timestamp": { "seconds": 1344456160, "microseconds": 309119 } }
    ##

and in migration.json:

    ##
    # @migrate_cancel:
    #
    # Cancel the current executing migration process.
    #
    # .. note:: This command succeeds even if there is no migration
    #    process running.
    #
    # Since: 0.14
    #
    # .. qmp-example::
    #
    #     -> { "execute": "migrate_cancel" }
    #     <- { "return": {} }
    ##

and in machine.json

    ##
    # @HV_BALLOON_STATUS_REPORT:
    #
    # Emitted when the hv-balloon driver receives a "STATUS" message from
    # the guest.
    #
    # .. note:: This event is rate-limited.
    #
    # Since: 8.2
    #
    # .. qmp-example::
    #
    #     <- { "event": "HV_BALLOON_STATUS_REPORT",
    #          "data": { "committed": 816640000, "available": 3333054464 },
    #          "timestamp": { "seconds": 1600295492, "microseconds": 661044 } }
    ##

> The check as I wrote it is unintelligent in that it does not bother to
> check if the doc block it is checking is ever one that *could* be inlined;
> i.e. it will complain about being unable to delineate for commands -- even
> though it wouldn't really matter in that case. It's a potential improvement
> to the algorithm to ignore cases where that "ambiguity" is not actually
> important.

The ambiguity affects both doc blocks the inliner inlines from and doc
blocks the inliner inlines into.

When inlining from, the inliner omits "intro", and therefore needs to
know where "intro" ends.

When inlining into, the inliner needs to know where to insert the
inlined material.  When the answer is "right after intro", it needs to
know where "intro" ends.

Getting the former wrong loses information.  Getting the latter wrong
may look funny, which is a lot less serious, but still useful to avoid.

> But, it's possible to mechanically enforce and nudge documentation writers
> to add the delineation marker where the parser is uncertain.
>
>> [*] Actually, we have means even before this patch, they're just ugly.
>> See the TODO comment added in commit 14b48aaab92 (qapi: convert
>> "Example" sections without titles)
>
>
> That's right. This is merely a formalization of that hack: I add a
> "section" that is intentionally empty and serves only as a marker to the
> parser to begin recording a new section.

Yes.


Let's take a step back again.

Recall the problem's cause is "empty description table".  Can we enforce
non-empty?

Here's the table's syntactic structure:

    member / argument descriptions *
    ( "Features:" line
       feature descriptions ("features") + ) ?
    "Returns" section ?
    "Errors" section ?

This is slightly more strict than what we actually accept now, but
that's detail.

Consider:

    "Members:" / "Arguments:" line
    member / argument descriptions *
    ( "Features:" line
       feature descriptions ("features") + ) ?
    "Returns" section ?
    "Errors" section ?

With this, the table always starts with a "Members" / "Arguments" line,
and thus cannot be empty.

Drawback: we'd have to add this line to every single definition comment.
The main QAPI schema has almost 1000.  Tolerable?

We could require it only when there are no member / argument
descriptions.  55 instances.

We could require it only when there are none, and our "one paragraph"
heuristic for finding the end of "intro" fails.  8 instances.

You might ask what the difference to your "Details:" proposal is.  There
are two.

1. The keyword(s).  Matter of taste, best discussed last.

2. As coded, your patch accepts "Details:" almost[*] anywhere.
   "Members:" / "Arguments" would be accepted only where member / argument
   descriptions can go, i.e. not after feature descriptions etc.  Consider:

    ##
    # @Enum:
    #
    # @one: The _one_ {and only}, description on the same line
    #
    # Features:
    # @enum-feat: Also _one_ {and only}
    # @enum-member-feat: a member feature
    #
    # Details:
    #
    # @two is undocumented
    ##

   This is accepted, and the "Details:" line gets swallowed.

   I figure tightening the position makes accidents slightly less
   likely.

Here's another way to force non-empty:

    ( "Members: none" / "Arguments: none" line
    | member / argument descriptions * )
    ( "Features:" line
       feature descriptions ("features") + ) ?
    "Returns" section ?
    "Errors" section ?

This is similar to "require it only when there are no member / argument
descriptions" above, except we also accept it only then.  55 instances.

Syntax ideas better than "Members: none" are welcome.

Thoughts?


[*] Not after untagged sections following tagged ones.

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> "The text handler you add looks just like the existing latex handler. Does
> LaTeX output lack "little headings", too?"
>
> Yes, almost certainly. Can you let me know which output formats we actually
> "care about"? I'll have to test them all.

As far as I can tell, our build system runs sphinx-build -b html and -b
man.

I run it with -b text manually all the time to hunt for and review
changes in output.  I'd prefer to keep it working if practical.

For what it's worth, there is a bit of LaTeX configuration in
docs/conf.py.

>                                           In the meantime, I upgraded my
> patch so that the text translator properly handles branches with headings
> that delineate the different branches so that the text output is fully
> reasonable. I will need to do the same for any format we care about.
>
> I've re-pushed as of "about 30 minutes before I wrote this email" --
> https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2
>
> This branch includes the text generator fixes (which technically belong
> with the predecessor series we skipped, but I'll refactor that later.)
> it also includes fixes to the branch inliner, generated return statements,
> and generated out-of-band feature sections.

I'll fetch it, thanks!

> (Long story short: inserting new sections in certain spots was broken
> because of cache. Oops. We can discuss more why I wrote that part of the
> code like I did in review for the patch that introduced that problem. It's
> the "basic inliner" patch.)
>
> Below, I'm going to try a new communication approach where I explicitly say
> if I have added something to my tasklist or not so that it's clear to you
> what I believe is actionable (and what I am agreeing to change) and what I
> believe needs stronger input from you before I do anything. Apologies if it
> seems a little robotic, just trying new things O:-)
>
> On that note: not added to tasklist: do we need the LaTeX handler? Do we
> need any others? Please confirm O:-)

See above.

> On Fri, Feb 14, 2025 at 7:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> I started to eyeball old and new generated output side by side.
>>
>> New table of contents shows one level, old two.  No objection; the
>> navigation thingie on the left is more useful anyway.
>>
>
> Unintentional, but if you like it, it's fine by me. Nothing added to my
> tasklist.

Mention in a commit message.

>> The new generator elides unreferenced types.  Generally good, but two
>> observations:
>>
>> * QapiErrorClass is unreferenced, but its members are mentioned in
>>   Errors sections.  QapiErrorClass serves as better than nothing error
>>   code documentation, but it's gone in the new doc.  So this is a minor
>>   regression.  We can figure out what to do about it later.
>>
>
> Right. I debated making the members references to that class, but recalled
> that you disliked this class and figured you'd not like such a change, so I
> just left it alone. I do not have cross-references for individual members
> of objects at all yet anyway, so this is definitely more work regardless.
>
> We could always create a pragma of some sort (or just hardcode a list) of
> items that must be documented regardless of if they're referenced or not.
> Please let me know your preference and I will add a "ticket" on my personal
> tasklist for this project to handle that at /some point/. Nothing added to
> my tasklist just yet.

Suggest to add something like "compensate for the loss of QapiErrorClass
documentation in the QEMU QMP Reference Manual".

>> * Section "QMP errors" is empty in the new doc, because its entire
>>   contents is elided.  I guess we should elide the section as well, but
>>   it's fine to leave that for later.
>>
>
> Adding to tasklist to elide empty modules, but "for later".

ACK

>> Old doc shows a definition's since information like any other section.
>> New doc has it in the heading box.  Looks prettier and uses much less
>> space.  Not sure the heading box is the best place, but it'll do for
>> now, we can always move it around later.
>>
>
> Agree, it's a strict improvement - there may be further improvements, but
> that is always true anyway. When we tackle "autogenerated since
> information" we can tackle the since display issues more meticulously. Or
> maybe we'll need do sooner because of conflicting info in branches or
> whatever else. I dunno, I'll burn that bridge when I get to it. Nothing
> added to tasklist.

ACK

>> The new doc's headings use "Struct" or "Union" where the old one uses
>> just "Object".  Let's keep "Object", please.
>>
>
> I was afraid you'd ask for this. OK, I think it's an easy change. Can I
> keep the index page segmented by object type still, though?
>
> I do find knowing the *type* of object to be helpful as a developer,

Can you explain why and how struct vs. union matters to you as a
developer?

>                                                                      though
> I understand that from the point of view of a QMP user, they're all just
> objects, so your request makes sense.

I'd prefer a single index.

> Replace JSON object type headers with "Object" instead of QAPI data types
> added to tasklist.

ACK

>> In the new doc, some member references are no longer rendered as such,
>> e.g. @on-source-error and @on-target-error in BackupCommon's note.
>> Another small regression.
>>
>
> Ah, I actually knew this one. I didn't implement special formatting for
> these yet. I do not have cross-references for individual members, so
> there's nothing to transform these *into* yet. If you'd like special
> rendering for them (fixed width, no link?) that's easy to accomplish. I am
> not yet sure where I will do that conversion.

Suggest the render them the same as before.

Have a look at BackupCommon's "Note" box in the old docs: the member
names appear to use a fixed-width font.

Peeking at old qapidoc.py...  it seems to rewrite @foo to ``foo``.

> Reminder/Note that in my KVM Forum branch, I did actually replace all
> @references that pointed to something actually cross-referenceable with an
> actual sphinx cross-reference, leaving only @member references behind.
>
> Nothing added to tasklist just yet.
>
>
>>
>> Union branches are busted in the new generator's output.  I know they
>> used to work, so I'm not worried about it.
>>
>
> Fixed in new push, sorry! An embarrassing mistake that took me aeons to
> spot.
>
>
>>
>> The new doc shows the return type, the old doc doesn't.  Showing it is
>> definitely an improvement, but we need to adjust the doc text to avoid
>> silliness like "Returns: SnapshotInfo – SnapshotInfo".
>>
>
> My KVM Forum branch actually corrected the QAPI documentation to remove
> pointless returns. I didn't include that with this series yet, it was long
> enough. This issue will be addressed solely through source documentation
> edits, of which I believe I already have a comprehensive patch for.
>
> Added to my tasklist: "Submit source documentation patches to remove
> pointless return documentation"

ACK

> It was my intent to submit those patches *afterwards*, but we can always do
> it before if you'd like. Let me know. (I don't know offhand how easy they
> are to extricate from my KVM Forum branch. I reserve the right to change my
> mind on being flexible depending on the answer there :p)

No need to decide or extricate right now.  Tasklist is good enough for
me.

>> The new doc shows Arguments / Members, Returns, and Errors in two-column
>> format.  Looks nice.  But for some reason, the two columns don't align
>> horizontally for Errors like they do for the others.  Certainly not a
>> blocker of anything, but we should try to fix it at some point.
>>
>
> Known issue. The reason is because we do not mandate a source documentation
> format for errors - by convention, it is a list. There is (or was?) one
> occurrence where it wasn't a list and I wrote a patch to change that. I
> don't recall right now if we merged that or not. The misalignment is a
> result of nesting a list inside of a list.

Commit b32a6b62a82 (qapi: nail down convention that Errors sections are
lists)

> If we *mandate* the source format, I gain the ability to "peel off the
> nesting", which will fix the alignment.

"Mandate" means changing "should be formatted as an rST list" into "must
be", plus enforcement.  Works for me.

> Added to tasklist: "Address vertical misalignment in Errors formatting"

ACK, low priority.

> Not added: how? need more input from you, please.
>
>
>>
>> The new doc doesn't show non-definition conditionals, as mentioned in
>> the cover letter.  It shows definition conditionals twice.  Once should
>> suffice.
>>
>
> Known/intentional issue. I couldn't decide where I wanted it, so I put it
> in both places. If you have a strong opinion right now, please let me know
> what it is and I'll take care of it, it's easy - but it's code in the
> predecessor series and nothing to do with qapidoc, so please put it out of
> mind for now.
>
> If you don't have strong feelings, or you feel that the answer may depend
> on how we solve other glaring issues (non-definition conditionals), let's
> wait a little bit before making a decision.
>
> Added to tasklist: "Remove the duplication of definition conditionals";
> left unspecified is how or in what direction :)

ACK

I'll try to make up my mind :)

>> There's probably more, but this is it for now.
>>
>>
>
> Tasklist:
>
>  For the qapi-domain (prequel!) series:
>   - Remove the duplication of definition conditionals
>
> For this (qapidoc) series:
>   - Display all JSON object types as "Object" and not as their QAPI data
> type.
>
> For later:
>   - Elide empty modules
>   - Submit source documentation patches to remove pointless return
> documentation
>   - Address vertical misalignment in Errors formatting

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

[John Snow]

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

On Wed, Feb 19, 2025 at 8:22 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > "The text handler you add looks just like the existing latex handler.
> Does
> > LaTeX output lack "little headings", too?"
> >
> > Yes, almost certainly. Can you let me know which output formats we
> actually
> > "care about"? I'll have to test them all.
>
> As far as I can tell, our build system runs sphinx-build -b html and -b
> man.
>
> I run it with -b text manually all the time to hunt for and review
> changes in output.  I'd prefer to keep it working if practical.
>
> For what it's worth, there is a bit of LaTeX configuration in
> docs/conf.py.
>
> >                                           In the meantime, I upgraded my
> > patch so that the text translator properly handles branches with headings
> > that delineate the different branches so that the text output is fully
> > reasonable. I will need to do the same for any format we care about.
> >
> > I've re-pushed as of "about 30 minutes before I wrote this email" --
> > https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2
> >
> > This branch includes the text generator fixes (which technically belong
> > with the predecessor series we skipped, but I'll refactor that later.)
> > it also includes fixes to the branch inliner, generated return
> statements,
> > and generated out-of-band feature sections.
>
> I'll fetch it, thanks!
>
> > (Long story short: inserting new sections in certain spots was broken
> > because of cache. Oops. We can discuss more why I wrote that part of the
> > code like I did in review for the patch that introduced that problem.
> It's
> > the "basic inliner" patch.)
> >
> > Below, I'm going to try a new communication approach where I explicitly
> say
> > if I have added something to my tasklist or not so that it's clear to you
> > what I believe is actionable (and what I am agreeing to change) and what
> I
> > believe needs stronger input from you before I do anything. Apologies if
> it
> > seems a little robotic, just trying new things O:-)
> >
> > On that note: not added to tasklist: do we need the LaTeX handler? Do we
> > need any others? Please confirm O:-)
>
> See above.
>

I've got html and text working, text wasn't hard. I will give it a good
college try on the LaTeX and man formats. Might be easy. The issue here is
the custom node I introduced for the collapsible details sections which has
no default handler in the generators. I'll have to learn more about that
part of the API, I haven't interfaced with it much yet.


>
> > On Fri, Feb 14, 2025 at 7:05 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> I started to eyeball old and new generated output side by side.
> >>
> >> New table of contents shows one level, old two.  No objection; the
> >> navigation thingie on the left is more useful anyway.
> >>
> >
> > Unintentional, but if you like it, it's fine by me. Nothing added to my
> > tasklist.
>
> Mention in a commit message.
>

Sure. I... just need to figure out which commit to mention it in. Added to
my list, anyway.


>
> >> The new generator elides unreferenced types.  Generally good, but two
> >> observations:
> >>
> >> * QapiErrorClass is unreferenced, but its members are mentioned in
> >>   Errors sections.  QapiErrorClass serves as better than nothing error
> >>   code documentation, but it's gone in the new doc.  So this is a minor
> >>   regression.  We can figure out what to do about it later.
> >>
> >
> > Right. I debated making the members references to that class, but
> recalled
> > that you disliked this class and figured you'd not like such a change,
> so I
> > just left it alone. I do not have cross-references for individual members
> > of objects at all yet anyway, so this is definitely more work regardless.
> >
> > We could always create a pragma of some sort (or just hardcode a list) of
> > items that must be documented regardless of if they're referenced or not.
> > Please let me know your preference and I will add a "ticket" on my
> personal
> > tasklist for this project to handle that at /some point/. Nothing added
> to
> > my tasklist just yet.
>
> Suggest to add something like "compensate for the loss of QapiErrorClass
> documentation in the QEMU QMP Reference Manual".
>

Got it. Possibly a "for later" task but not much later. It can always come
after this first series, but before we "turn on" the new generator, if that
makes sense. Just so we reach a quiescent point and flush the staggeringly
large queue.

I guess what I mean is: "Let's make sure what I've got here so far is good
first, and then I'll start adding stuff."


>
> >> * Section "QMP errors" is empty in the new doc, because its entire
> >>   contents is elided.  I guess we should elide the section as well, but
> >>   it's fine to leave that for later.
> >>
> >
> > Adding to tasklist to elide empty modules, but "for later".
>
> ACK
>
> >> Old doc shows a definition's since information like any other section.
> >> New doc has it in the heading box.  Looks prettier and uses much less
> >> space.  Not sure the heading box is the best place, but it'll do for
> >> now, we can always move it around later.
> >>
> >
> > Agree, it's a strict improvement - there may be further improvements, but
> > that is always true anyway. When we tackle "autogenerated since
> > information" we can tackle the since display issues more meticulously. Or
> > maybe we'll need do sooner because of conflicting info in branches or
> > whatever else. I dunno, I'll burn that bridge when I get to it. Nothing
> > added to tasklist.
>
> ACK
>
> >> The new doc's headings use "Struct" or "Union" where the old one uses
> >> just "Object".  Let's keep "Object", please.
> >>
> >
> > I was afraid you'd ask for this. OK, I think it's an easy change. Can I
> > keep the index page segmented by object type still, though?
> >
> > I do find knowing the *type* of object to be helpful as a developer,
>
> Can you explain why and how struct vs. union matters to you as a
> developer?
>

I suppose it's just internal details that I like to know, but tend to find
the HTML reference easier to work with than grepping through the qapi
files. I'm gonna change it for you anyway because I agree it's not
consistent with the philosophy of "end user QMP reference". Just feels like
a tiny shame somehow.


>
> >
> though
> > I understand that from the point of view of a QMP user, they're all just
> > objects, so your request makes sense.
>
> I'd prefer a single index.
>

So ... structs, unions, alternates all condensed down to "Object", is that
right? We get to keep command/enum/event separate, I assume.


>
> > Replace JSON object type headers with "Object" instead of QAPI data types
> > added to tasklist.
>
> ACK
>
> >> In the new doc, some member references are no longer rendered as such,
> >> e.g. @on-source-error and @on-target-error in BackupCommon's note.
> >> Another small regression.
> >>
> >
> > Ah, I actually knew this one. I didn't implement special formatting for
> > these yet. I do not have cross-references for individual members, so
> > there's nothing to transform these *into* yet. If you'd like special
> > rendering for them (fixed width, no link?) that's easy to accomplish. I
> am
> > not yet sure where I will do that conversion.
>
> Suggest the render them the same as before.
>
> Have a look at BackupCommon's "Note" box in the old docs: the member
> names appear to use a fixed-width font.
>
> Peeking at old qapidoc.py...  it seems to rewrite @foo to ``foo``.
>

OK.


>
> > Reminder/Note that in my KVM Forum branch, I did actually replace all
> > @references that pointed to something actually cross-referenceable with
> an
> > actual sphinx cross-reference, leaving only @member references behind.
> >
> > Nothing added to tasklist just yet.
> >
> >
> >>
> >> Union branches are busted in the new generator's output.  I know they
> >> used to work, so I'm not worried about it.
> >>
> >
> > Fixed in new push, sorry! An embarrassing mistake that took me aeons to
> > spot.
> >
> >
> >>
> >> The new doc shows the return type, the old doc doesn't.  Showing it is
> >> definitely an improvement, but we need to adjust the doc text to avoid
> >> silliness like "Returns: SnapshotInfo – SnapshotInfo".
> >>
> >
> > My KVM Forum branch actually corrected the QAPI documentation to remove
> > pointless returns. I didn't include that with this series yet, it was
> long
> > enough. This issue will be addressed solely through source documentation
> > edits, of which I believe I already have a comprehensive patch for.
> >
> > Added to my tasklist: "Submit source documentation patches to remove
> > pointless return documentation"
>
> ACK
>
> > It was my intent to submit those patches *afterwards*, but we can always
> do
> > it before if you'd like. Let me know. (I don't know offhand how easy they
> > are to extricate from my KVM Forum branch. I reserve the right to change
> my
> > mind on being flexible depending on the answer there :p)
>
> No need to decide or extricate right now.  Tasklist is good enough for
> me.
>
> >> The new doc shows Arguments / Members, Returns, and Errors in two-column
> >> format.  Looks nice.  But for some reason, the two columns don't align
> >> horizontally for Errors like they do for the others.  Certainly not a
> >> blocker of anything, but we should try to fix it at some point.
> >>
> >
> > Known issue. The reason is because we do not mandate a source
> documentation
> > format for errors - by convention, it is a list. There is (or was?) one
> > occurrence where it wasn't a list and I wrote a patch to change that. I
> > don't recall right now if we merged that or not. The misalignment is a
> > result of nesting a list inside of a list.
>
> Commit b32a6b62a82 (qapi: nail down convention that Errors sections are
> lists)
>
> > If we *mandate* the source format, I gain the ability to "peel off the
> > nesting", which will fix the alignment.
>
> "Mandate" means changing "should be formatted as an rST list" into "must
> be", plus enforcement.  Works for me.
>
> > Added to tasklist: "Address vertical misalignment in Errors formatting"
>
> ACK, low priority.
>
> > Not added: how? need more input from you, please.
> >
> >
> >>
> >> The new doc doesn't show non-definition conditionals, as mentioned in
> >> the cover letter.  It shows definition conditionals twice.  Once should
> >> suffice.
> >>
> >
> > Known/intentional issue. I couldn't decide where I wanted it, so I put it
> > in both places. If you have a strong opinion right now, please let me
> know
> > what it is and I'll take care of it, it's easy - but it's code in the
> > predecessor series and nothing to do with qapidoc, so please put it out
> of
> > mind for now.
> >
> > If you don't have strong feelings, or you feel that the answer may depend
> > on how we solve other glaring issues (non-definition conditionals), let's
> > wait a little bit before making a decision.
> >
> > Added to tasklist: "Remove the duplication of definition conditionals";
> > left unspecified is how or in what direction :)
>
> ACK
>
> I'll try to make up my mind :)
>

I should also point out, this is an issue in the domain and not the
generator; the generated rst document doesn't have this duplication. So
it's kind of a no-op while we look and consider this specific series, but
it's still on my list when we go to look at the predecessor series.


>
> >> There's probably more, but this is it for now.
> >>
> >>
> >
> > Tasklist:
> >
> >  For the qapi-domain (prequel!) series:
> >   - Remove the duplication of definition conditionals
> >
> > For this (qapidoc) series:
> >   - Display all JSON object types as "Object" and not as their QAPI data
> > type.
> >
> > For later:
> >   - Elide empty modules
> >   - Submit source documentation patches to remove pointless return
> > documentation
> >   - Address vertical misalignment in Errors formatting
>
>

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Feb 19, 2025 at 8:22 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > "The text handler you add looks just like the existing latex handler. Does
>> > LaTeX output lack "little headings", too?"
>> >
>> > Yes, almost certainly. Can you let me know which output formats we actually
>> > "care about"? I'll have to test them all.
>>
>> As far as I can tell, our build system runs sphinx-build -b html and -b
>> man.
>>
>> I run it with -b text manually all the time to hunt for and review
>> changes in output.  I'd prefer to keep it working if practical.
>>
>> For what it's worth, there is a bit of LaTeX configuration in
>> docs/conf.py.
>>
>> >                                           In the meantime, I upgraded my
>> > patch so that the text translator properly handles branches with headings
>> > that delineate the different branches so that the text output is fully
>> > reasonable. I will need to do the same for any format we care about.
>> >
>> > I've re-pushed as of "about 30 minutes before I wrote this email" --
>> > https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2
>> >
>> > This branch includes the text generator fixes (which technically belong
>> > with the predecessor series we skipped, but I'll refactor that later.)
>> > it also includes fixes to the branch inliner, generated return statements,
>> > and generated out-of-band feature sections.
>>
>> I'll fetch it, thanks!
>>
>> > (Long story short: inserting new sections in certain spots was broken
>> > because of cache. Oops. We can discuss more why I wrote that part of the
>> > code like I did in review for the patch that introduced that problem. It's
>> > the "basic inliner" patch.)
>> >
>> > Below, I'm going to try a new communication approach where I explicitly say
>> > if I have added something to my tasklist or not so that it's clear to you
>> > what I believe is actionable (and what I am agreeing to change) and what I
>> > believe needs stronger input from you before I do anything. Apologies if it
>> > seems a little robotic, just trying new things O:-)
>> >
>> > On that note: not added to tasklist: do we need the LaTeX handler? Do we
>> > need any others? Please confirm O:-)
>>
>> See above.
>>
>
> I've got html and text working, text wasn't hard. I will give it a good
> college try on the LaTeX and man formats. Might be easy. The issue here is
> the custom node I introduced for the collapsible details sections which has
> no default handler in the generators. I'll have to learn more about that
> part of the API, I haven't interfaced with it much yet.

Understand.

Have you considered cutting the series in half before the inliner?
First part emits "The members of ..." like the old doc generator.
Second part replaces that with inlined material.

We could totally release with just the first half!  Inlining is great,
but even without it, your work looks so much better and is so much more
usable.

>> > On Fri, Feb 14, 2025 at 7:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> I started to eyeball old and new generated output side by side.
>> >>
>> >> New table of contents shows one level, old two.  No objection; the
>> >> navigation thingie on the left is more useful anyway.
>> >>
>> >
>> > Unintentional, but if you like it, it's fine by me. Nothing added to my
>> > tasklist.
>>
>> Mention in a commit message.
>>
>
> Sure. I... just need to figure out which commit to mention it in. Added to
> my list, anyway.
>
>
>>
>> >> The new generator elides unreferenced types.  Generally good, but two
>> >> observations:
>> >>
>> >> * QapiErrorClass is unreferenced, but its members are mentioned in
>> >>   Errors sections.  QapiErrorClass serves as better than nothing error
>> >>   code documentation, but it's gone in the new doc.  So this is a minor
>> >>   regression.  We can figure out what to do about it later.
>> >>
>> >
>> > Right. I debated making the members references to that class, but recalled
>> > that you disliked this class and figured you'd not like such a change, so I
>> > just left it alone. I do not have cross-references for individual members
>> > of objects at all yet anyway, so this is definitely more work regardless.
>> >
>> > We could always create a pragma of some sort (or just hardcode a list) of
>> > items that must be documented regardless of if they're referenced or not.
>> > Please let me know your preference and I will add a "ticket" on my personal
>> > tasklist for this project to handle that at /some point/. Nothing added to
>> > my tasklist just yet.
>>
>> Suggest to add something like "compensate for the loss of QapiErrorClass
>> documentation in the QEMU QMP Reference Manual".
>>
>
> Got it. Possibly a "for later" task but not much later. It can always come
> after this first series, but before we "turn on" the new generator, if that
> makes sense. Just so we reach a quiescent point and flush the staggeringly
> large queue.

I think we could even do it after "turn on".  Yes, it's a small
regression, but I believe the improvements are big enough to outweigh
small regressions like this one.

> I guess what I mean is: "Let's make sure what I've got here so far is good
> first, and then I'll start adding stuff."

[...]

>> >> The new doc's headings use "Struct" or "Union" where the old one uses
>> >> just "Object".  Let's keep "Object", please.
>> >>
>> >
>> > I was afraid you'd ask for this. OK, I think it's an easy change. Can I
>> > keep the index page segmented by object type still, though?
>> >
>> > I do find knowing the *type* of object to be helpful as a developer,
>>
>> Can you explain why and how struct vs. union matters to you as a
>> developer?
>>
>
> I suppose it's just internal details that I like to know, but tend to find
> the HTML reference easier to work with than grepping through the qapi
> files. I'm gonna change it for you anyway because I agree it's not
> consistent with the philosophy of "end user QMP reference". Just feels like
> a tiny shame somehow.
>
>
>>
>> > though
>> > I understand that from the point of view of a QMP user, they're all just
>> > objects, so your request makes sense.
>>
>> I'd prefer a single index.
>>
>
> So ... structs, unions, alternates all condensed down to "Object", is that
> right? We get to keep command/enum/event separate, I assume.

No, only structs and unions are "Object", alternates are "Alternate".

For me, the separation between struct and union is an unfortunate
remnant of somewhat winding development history.

A union is has common members, one of them is the tag, and for each tag
value, it may have variant members.

A struct is a degenerate union: no variants.

This is as old as the hills: Pascal records are just like this.

QMP introspection doesn't show structs and unions, just objects, which
may or may not have variants.

The schema language syntax, however, is still rooted (stuck?) in a past
when unions could not have common members other than the tag.

[...]

>> >> The new doc doesn't show non-definition conditionals, as mentioned in
>> >> the cover letter.  It shows definition conditionals twice.  Once should
>> >> suffice.
>> >>
>> >
>> > Known/intentional issue. I couldn't decide where I wanted it, so I put it
>> > in both places. If you have a strong opinion right now, please let me know
>> > what it is and I'll take care of it, it's easy - but it's code in the
>> > predecessor series and nothing to do with qapidoc, so please put it out of
>> > mind for now.
>> >
>> > If you don't have strong feelings, or you feel that the answer may depend
>> > on how we solve other glaring issues (non-definition conditionals), let's
>> > wait a little bit before making a decision.
>> >
>> > Added to tasklist: "Remove the duplication of definition conditionals";
>> > left unspecified is how or in what direction :)
>>
>> ACK
>>
>> I'll try to make up my mind :)
>>
>
> I should also point out, this is an issue in the domain and not the
> generator; the generated rst document doesn't have this duplication. So
> it's kind of a no-op while we look and consider this specific series, but
> it's still on my list when we go to look at the predecessor series.

Understood.

[...]

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

[John Snow]

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

On Fri, Feb 21, 2025 at 1:42 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Wed, Feb 19, 2025 at 8:22 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > "The text handler you add looks just like the existing latex handler.
> Does
> >> > LaTeX output lack "little headings", too?"
> >> >
> >> > Yes, almost certainly. Can you let me know which output formats we
> actually
> >> > "care about"? I'll have to test them all.
> >>
> >> As far as I can tell, our build system runs sphinx-build -b html and -b
> >> man.
> >>
> >> I run it with -b text manually all the time to hunt for and review
> >> changes in output.  I'd prefer to keep it working if practical.
> >>
> >> For what it's worth, there is a bit of LaTeX configuration in
> >> docs/conf.py.
> >>
> >> >                                           In the meantime, I upgraded
> my
> >> > patch so that the text translator properly handles branches with
> headings
> >> > that delineate the different branches so that the text output is fully
> >> > reasonable. I will need to do the same for any format we care about.
> >> >
> >> > I've re-pushed as of "about 30 minutes before I wrote this email" --
> >> > https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2
> >> >
> >> > This branch includes the text generator fixes (which technically
> belong
> >> > with the predecessor series we skipped, but I'll refactor that later.)
> >> > it also includes fixes to the branch inliner, generated return
> statements,
> >> > and generated out-of-band feature sections.
> >>
> >> I'll fetch it, thanks!
> >>
> >> > (Long story short: inserting new sections in certain spots was broken
> >> > because of cache. Oops. We can discuss more why I wrote that part of
> the
> >> > code like I did in review for the patch that introduced that problem.
> It's
> >> > the "basic inliner" patch.)
> >> >
> >> > Below, I'm going to try a new communication approach where I
> explicitly say
> >> > if I have added something to my tasklist or not so that it's clear to
> you
> >> > what I believe is actionable (and what I am agreeing to change) and
> what I
> >> > believe needs stronger input from you before I do anything. Apologies
> if it
> >> > seems a little robotic, just trying new things O:-)
> >> >
> >> > On that note: not added to tasklist: do we need the LaTeX handler? Do
> we
> >> > need any others? Please confirm O:-)
> >>
> >> See above.
> >>
> >
> > I've got html and text working, text wasn't hard. I will give it a good
> > college try on the LaTeX and man formats. Might be easy. The issue here
> is
> > the custom node I introduced for the collapsible details sections which
> has
> > no default handler in the generators. I'll have to learn more about that
> > part of the API, I haven't interfaced with it much yet.
>
> Understand.
>
> Have you considered cutting the series in half before the inliner?
> First part emits "The members of ..." like the old doc generator.
> Second part replaces that with inlined material.
>
> We could totally release with just the first half!  Inlining is great,
> but even without it, your work looks so much better and is so much more
> usable.
>

I may indeed just do that... though we still need to solve "where to put
the ifcond data?" question. The documentation culling also must be held
back in this case too, which I am fine with.

Let me fork my work (again) and see how complicated an inlinerless version
would be... maybe that's a great way to flush the queue. maybe.


>
> >> > On Fri, Feb 14, 2025 at 7:05 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >> >
> >> >> I started to eyeball old and new generated output side by side.
> >> >>
> >> >> New table of contents shows one level, old two.  No objection; the
> >> >> navigation thingie on the left is more useful anyway.
> >> >>
> >> >
> >> > Unintentional, but if you like it, it's fine by me. Nothing added to
> my
> >> > tasklist.
> >>
> >> Mention in a commit message.
> >>
> >
> > Sure. I... just need to figure out which commit to mention it in. Added
> to
> > my list, anyway.
>

It turns out this happens in the "example" doc patch, it's just a setting
in index.rst. I didn't even intend to commit that patch anyway. So this is
a nothing-burger.


> >
> >
> >>
> >> >> The new generator elides unreferenced types.  Generally good, but two
> >> >> observations:
> >> >>
> >> >> * QapiErrorClass is unreferenced, but its members are mentioned in
> >> >>   Errors sections.  QapiErrorClass serves as better than nothing
> error
> >> >>   code documentation, but it's gone in the new doc.  So this is a
> minor
> >> >>   regression.  We can figure out what to do about it later.
> >> >>
> >> >
> >> > Right. I debated making the members references to that class, but
> recalled
> >> > that you disliked this class and figured you'd not like such a
> change, so I
> >> > just left it alone. I do not have cross-references for individual
> members
> >> > of objects at all yet anyway, so this is definitely more work
> regardless.
> >> >
> >> > We could always create a pragma of some sort (or just hardcode a
> list) of
> >> > items that must be documented regardless of if they're referenced or
> not.
> >> > Please let me know your preference and I will add a "ticket" on my
> personal
> >> > tasklist for this project to handle that at /some point/. Nothing
> added to
> >> > my tasklist just yet.
> >>
> >> Suggest to add something like "compensate for the loss of QapiErrorClass
> >> documentation in the QEMU QMP Reference Manual".
> >>
> >
> > Got it. Possibly a "for later" task but not much later. It can always
> come
> > after this first series, but before we "turn on" the new generator, if
> that
> > makes sense. Just so we reach a quiescent point and flush the
> staggeringly
> > large queue.
>
> I think we could even do it after "turn on".  Yes, it's a small
> regression, but I believe the improvements are big enough to outweigh
> small regressions like this one.
>

OK.


>
> > I guess what I mean is: "Let's make sure what I've got here so far is
> good
> > first, and then I'll start adding stuff."
>
> [...]
>
> >> >> The new doc's headings use "Struct" or "Union" where the old one uses
> >> >> just "Object".  Let's keep "Object", please.
> >> >>
> >> >
> >> > I was afraid you'd ask for this. OK, I think it's an easy change. Can
> I
> >> > keep the index page segmented by object type still, though?
> >> >
> >> > I do find knowing the *type* of object to be helpful as a developer,
> >>
> >> Can you explain why and how struct vs. union matters to you as a
> >> developer?
> >>
> >
> > I suppose it's just internal details that I like to know, but tend to
> find
> > the HTML reference easier to work with than grepping through the qapi
> > files. I'm gonna change it for you anyway because I agree it's not
> > consistent with the philosophy of "end user QMP reference". Just feels
> like
> > a tiny shame somehow.
> >
> >
> >>
> >> > though
> >> > I understand that from the point of view of a QMP user, they're all
> just
> >> > objects, so your request makes sense.
> >>
> >> I'd prefer a single index.
> >>
> >
> > So ... structs, unions, alternates all condensed down to "Object", is
> that
> > right? We get to keep command/enum/event separate, I assume.
>
> No, only structs and unions are "Object", alternates are "Alternate".
>
> For me, the separation between struct and union is an unfortunate
> remnant of somewhat winding development history.
>
> A union is has common members, one of them is the tag, and for each tag
> value, it may have variant members.
>
> A struct is a degenerate union: no variants.
>
> This is as old as the hills: Pascal records are just like this.
>
> QMP introspection doesn't show structs and unions, just objects, which
> may or may not have variants.
>
> The schema language syntax, however, is still rooted (stuck?) in a past
> when unions could not have common members other than the tag.
>

OK, I can combine these two easily then. I see why you feel it isn't worth
knowing the difference for developers either under this view.


>
> [...]
>
> >> >> The new doc doesn't show non-definition conditionals, as mentioned in
> >> >> the cover letter.  It shows definition conditionals twice.  Once
> should
> >> >> suffice.
> >> >>
> >> >
> >> > Known/intentional issue. I couldn't decide where I wanted it, so I
> put it
> >> > in both places. If you have a strong opinion right now, please let me
> know
> >> > what it is and I'll take care of it, it's easy - but it's code in the
> >> > predecessor series and nothing to do with qapidoc, so please put it
> out of
> >> > mind for now.
> >> >
> >> > If you don't have strong feelings, or you feel that the answer may
> depend
> >> > on how we solve other glaring issues (non-definition conditionals),
> let's
> >> > wait a little bit before making a decision.
> >> >
> >> > Added to tasklist: "Remove the duplication of definition
> conditionals";
> >> > left unspecified is how or in what direction :)
> >>
> >> ACK
> >>
> >> I'll try to make up my mind :)
> >>
> >
> > I should also point out, this is an issue in the domain and not the
> > generator; the generated rst document doesn't have this duplication. So
> > it's kind of a no-op while we look and consider this specific series, but
> > it's still on my list when we go to look at the predecessor series.
>
> Understood.
>
> [...]
>
>

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