[PATCH 22/27] docs/qapi-domain: add warnings for malformed field lists

[John Snow]

Normally, Sphinx will silently fall back to its standard field list
processing if it doesn't match one of your defined fields. A lot of the
time, that's not what we want - we want to be warned if we goof
something up.

For instance, the canonical argument field list form is:

:arg type name: descr

This form is captured by Sphinx and transformed so that the field label
will become "Arguments:". It's possible to omit the type name and descr
and still have it be processed correctly. However, if you omit the type
name, Sphinx no longer recognizes it:

:arg: this is not recognized.

This will turn into an arbitrary field list entry whose label is "Arg:",
and it otherwise silently fails. You may also see failures for doing
things like using :values: instead of :value:, or :errors: instead of
:error:, and so on. It's also case sensitive, and easy to trip up.

Add a validator that guarantees all field list entries that are the
direct child of an ObjectDescription use only recognized forms of field
lists, and emit a warning (treated as error by default in most build
configurations) whenever we detect one that is goofed up.

However, there's still benefit to allowing arbitrary fields -- they are
after all not a Sphinx invention, but perfectly normal docutils
syntax. Create an allow list for known spellings we don't mind letting
through, but warn against anything else.

-

RFC: Yes, this patch breaks the build! I thought it'd be helpful for you
to see the error checking in action. The next commit drops the erroneous
fields.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/conf.py               | 12 ++++++++
 docs/qapi/index.rst        | 27 +++++++++++++++++
 docs/sphinx/qapi-domain.py | 59 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

diff --git a/docs/conf.py b/docs/conf.py
index b15665d956d..7a7d7005780 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -148,6 +148,18 @@
 with open(os.path.join(qemu_docdir, 'defs.rst.inc')) as f:
     rst_epilog += f.read()
 
+
+# Normally, the QAPI domain is picky about what field lists you use to
+# describe a QAPI entity. If you'd like to use arbitrary additional
+# fields in source documentation, add them here.
+qapi_allowed_fields = {
+    "example",
+    "note",
+    "see also",
+    "TODO",
+}
+
+
 # -- Options for HTML output ----------------------------------------------
 
 # The theme to use for HTML and HTML Help pages.  See the documentation for
diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 5bb1c37a5ed..ef58dfc4bcd 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -133,6 +133,33 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
    :memb: this is malformed and unrecognized.
    :choice type name: This is unrecognized.
    :errors FooError: Also malformed.
+   :example: This isn't a "semantic" field, but it's been added to the
+      allowed field names list. you can use whatever field names you'd
+      like; but to prevent accidental typos, there is an allow list of
+      "arbitrary" section names.
+
+      You can nestle code-blocks in here, too, by using the ``::``
+      syntax::
+
+         -> { [ "bidirectional QMP example" ] }
+         <- { [ "hello world!"] }
+
+      Or use explicit ``.. code-block:: QMP`` syntax, but it must start
+      on its own line with a blank line both before and after the
+      directive to render correctly:
+
+      .. code-block:: QMP
+
+         -> "Hello friend!"
+
+      Note that the QMP highlighter is merely garden-variety JSON, but
+      with the addition of ``->``, ``<-`` and ``...`` symbols to help
+      denote bidirectionality and elided segments. Eduardo Habkost and I
+      wrote this lexer many moons ago to support the
+      :doc:`/interop/bitmaps` documentation.
+   :see also: This is also not a "semantic" field. The only limit is
+      your imagination and what you can convince others to let you check
+      into conf.py.
 
    Field lists can appear anywhere in the directive block, but any field
    list entries in the same list block that are recognized as special
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index e44db10488f..bf8bb933345 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -334,10 +334,63 @@ def _merge_adjoining_field_lists(self, contentnode: 
addnodes.desc_content) -> No
         for child in delete_queue:
             contentnode.remove(child)
 
+    def _validate_field(self, field: nodes.field) -> None:
+        field_name = field.children[0]
+        assert isinstance(field_name, nodes.field_name)
+        allowed_fields = set(self.env.app.config.qapi_allowed_fields)
+
+        field_label = field_name.astext()
+        if re.match(r"\[\S+ = \S+\]", field_label) or field_label in 
allowed_fields:
+            # okie-dokey. branch entry or known good allowed name.
+            return
+
+        try:
+            # split into field type and argument
+            field_type, fieldarg = field_label.split(None, 1)
+        except ValueError:
+            # No arguments provided
+            field_type = field_label
+            fieldarg = ""
+
+        typemap = self.get_field_type_map()
+        if field_type in typemap:
+            # This is a semantic field, yet-to-be-processed. Catch
+            # correct names, but incorrect arguments (which WILL fail to
+            # process correctly).
+            typedesc = typemap[field_type][0]
+            if typedesc.has_arg != bool(fieldarg):
+                msg = f"semantic field list type {field_type!r} "
+                if typedesc.has_arg:
+                    msg += "requires an argument."
+                else:
+                    msg += "takes no arguments."
+                logger.warning(msg, location=field)
+        else:
+            # This is unrecognized entirely.
+            valid = ", ".join(sorted(set(typemap) | allowed_fields))
+            msg = (
+                f"Unrecognized field list name {field_label!r}.\n"
+                f"Valid fields for qapi:{self.objtype} are: {valid}\n"
+                "\n"
+                "If this usage is intentional, please add it to "
+                "'qapi_allowed_fields' in docs/conf.py."
+            )
+            logger.warning(msg, location=field)
+
     def transform_content(self, contentnode: addnodes.desc_content) -> None:
         self._add_infopips(contentnode)
         self._merge_adjoining_field_lists(contentnode)
 
+        # Validate field lists. Note that this hook runs after any
+        # branch directives have processed and transformed their field
+        # lists, but before the main object directive has had a chance
+        # to do so.
+        for child in contentnode:
+            if isinstance(child, nodes.field_list):
+                for field in child.children:
+                    assert isinstance(field, nodes.field)
+                    self._validate_field(field)
+
     def _toc_entry_name(self, sig_node: desc_signature) -> str:
         # This controls the name in the TOC and on the sidebar.
 
@@ -872,6 +925,12 @@ def resolve_any_xref(
 
 def setup(app: Sphinx) -> Dict[str, Any]:
     app.setup_extension("sphinx.directives")
+    app.add_config_value(
+        "qapi_allowed_fields",
+        set(),
+        "env",  # Setting impacts parsing phase
+        types=set,
+    )
     app.add_domain(QAPIDomain)
 
     return {
-- 
2.44.0