Re: [PATCH 08/20] qapi/parser: differentiate intro and outro paragraphs

[John Snow]

On Thu, May 16, 2024 at 11:06 AM John Snow  wrote:

>
>
> On Thu, May 16, 2024, 5:34 AM Markus Armbruster  wrote:
>
>> John Snow  writes:
>>
>> > Add a semantic tag to paragraphs that appear *before* tagged
>> > sections/members/features and those that appear after. This will control
>> > how they are inlined when doc sections are merged and flattened.
>>
>> This future use is not obvious to me now.  I guess the effective way to
>> help me see it is actual patches, which will come in due time.
>>
>
> Head recursion and tail recursion, respectively :)
>
> * intro
> * inherited intro
> * members [ancestor-descendent]
> * features [ancestor-descendent]
> * inherited outro
> * outro
>
> Child gets the first and final words. Inherited stuff goes in the sandwich
> fillings.
>
> It feels like a simple rule that's easy to internalize. As a bonus, you
> can explain it by analogy to Americans as a burger, which is the only
> metaphor we understand.
>
>
>> > Signed-off-by: John Snow 
>> > ---
>> >  scripts/qapi/parser.py | 22 +-
>> >  1 file changed, 17 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index cf4cbca1c1f..b1794f71e12 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -503,6 +503,10 @@ def get_doc(self) -> 'QAPIDoc':
>> >  self.accept(False)
>> >  line = self.get_doc_line()
>> >  no_more_args = False
>> > +# Paragraphs before members/features/tagged are "intro"
>> paragraphs.
>> > +# Any appearing subsequently are "outro" paragraphs.
>> > +# This is only semantic metadata for the doc generator.
>>
>> Not sure about the last sentence.  Isn't it true for almost everything
>> around here?
>>
>
> I guess I was trying to say "There's no real difference between the two
> mechanically, it's purely based on where it appears in the doc block, which
> offers only a heuristic for its semantic value- introductory statements or
> additional detail."
>
> In my mind: the other "kind" values have some more mechanical difference
> to them, but intro/outro don't.
>
>
>> Also, long line.
>>
>> > +intro = True
>> >
>> >  while line is not None:
>> >  # Blank lines
>> > @@ -532,6 +536,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >  raise QAPIParseError(
>> >  self, 'feature descriptions expected')
>> >  no_more_args = True
>> > +intro = False
>>
>> After feature descriptions.
>>
>> >  elif match := self._match_at_name_colon(line):
>> >  # description
>> >  if no_more_args:
>> > @@ -547,6 +552,7 @@ def get_doc(self) -> 'QAPIDoc':
>> >  doc.append_line(text)
>> >  line = self.get_doc_indented(doc)
>> >  no_more_args = True
>> > +intro = False
>>
>> Or after member descriptions.
>>
>> >  elif match := re.match(
>> >
>> r'(Returns|Errors|Since|Notes?|Examples?|TODO): *',
>> >  line):
>> > @@ -557,13 +563,14 @@ def get_doc(self) -> 'QAPIDoc':
>> >  doc.append_line(text)
>> >  line = self.get_doc_indented(doc)
>> >  no_more_args = True
>> > +intro = False
>>
>> Or after the first tagged section.
>>
>> Okay, it does what it says on the tin.
>>
>> >  elif line.startswith('='):
>> >  raise QAPIParseError(
>> >  self,
>> >  "unexpected '=' markup in definition
>> documentation")
>> >  else:
>> >  # tag-less paragraph
>> > -doc.ensure_untagged_section(self.info)
>> > +doc.ensure_untagged_section(self.info, intro)
>> >  doc.append_line(line)
>> >  line = self.get_doc_paragraph(doc)
>> >  else:
>> > @@ -617,7 +624,7 @@ def __i

Re: [PATCH 06/20] qapi/parser: fix comment parsing immediately following a doc block

[John Snow]

On Thu, May 16, 2024 at 2:01 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > If a comment immediately follows a doc block, the parser doesn't ignore
> > that token appropriately. Fix that.
>
> Reproducer?
>
> >
> > Signed-off-by: John Snow 
> > ---
> >  scripts/qapi/parser.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 41b9319e5cb..161768b8b96 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -587,7 +587,7 @@ def get_doc(self) -> 'QAPIDoc':
> >  line = self.get_doc_line()
> >  first = False
> >
> > -self.accept(False)
> > +self.accept()
> >  doc.end()
> >  return doc
>
> Can't judge the fix without understanding the problem, and the problem
> will be easier to understand for me with a reproducer.
>

audio.json:

```
##
# = Audio
##

##
# @AudiodevPerDirectionOptions:
#
# General audio backend options that are used for both playback and
# recording.
#
```

Modify this excerpt to have a comment after the "= Audio" header, say for
instance if you were to take out that intro paragraph and transform it into
a comment that preceded the AudiodevPerDirectionOptions doc block.

e.g.

```
##
# = Audio
##

# Lorem Ipsum

##
# @AudiodevPerDirectionOptions:
```

the parser breaks because the line I changed that primes the next token is
still set to "not ignore comments", but that breaks the parser and gives a
rather unhelpful message:

../qapi/audio.json:13:1: junk after '##' at start of documentation comment

Encountered when converting developer commentary from documentation
paragraphs to mere QAPI comments.

--js

Re: [PATCH 08/20] qapi/parser: differentiate intro and outro paragraphs

[John Snow]

On Thu, May 16, 2024, 5:34 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > Add a semantic tag to paragraphs that appear *before* tagged
> > sections/members/features and those that appear after. This will control
> > how they are inlined when doc sections are merged and flattened.
>
> This future use is not obvious to me now.  I guess the effective way to
> help me see it is actual patches, which will come in due time.
>

Head recursion and tail recursion, respectively :)

* intro
* inherited intro
* members [ancestor-descendent]
* features [ancestor-descendent]
* inherited outro
* outro

Child gets the first and final words. Inherited stuff goes in the sandwich
fillings.

It feels like a simple rule that's easy to internalize. As a bonus, you can
explain it by analogy to Americans as a burger, which is the only metaphor
we understand.


> > Signed-off-by: John Snow 
> > ---
> >  scripts/qapi/parser.py | 22 +-
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index cf4cbca1c1f..b1794f71e12 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -503,6 +503,10 @@ def get_doc(self) -> 'QAPIDoc':
> >  self.accept(False)
> >  line = self.get_doc_line()
> >  no_more_args = False
> > +# Paragraphs before members/features/tagged are "intro"
> paragraphs.
> > +# Any appearing subsequently are "outro" paragraphs.
> > +# This is only semantic metadata for the doc generator.
>
> Not sure about the last sentence.  Isn't it true for almost everything
> around here?
>

I guess I was trying to say "There's no real difference between the two
mechanically, it's purely based on where it appears in the doc block, which
offers only a heuristic for its semantic value- introductory statements or
additional detail."

In my mind: the other "kind" values have some more mechanical difference to
them, but intro/outro don't.


> Also, long line.
>
> > +intro = True
> >
> >  while line is not None:
> >  # Blank lines
> > @@ -532,6 +536,7 @@ def get_doc(self) -> 'QAPIDoc':
> >  raise QAPIParseError(
> >  self, 'feature descriptions expected')
> >  no_more_args = True
> > +intro = False
>
> After feature descriptions.
>
> >  elif match := self._match_at_name_colon(line):
> >  # description
> >  if no_more_args:
> > @@ -547,6 +552,7 @@ def get_doc(self) -> 'QAPIDoc':
> >  doc.append_line(text)
> >  line = self.get_doc_indented(doc)
> >  no_more_args = True
> > +intro = False
>
> Or after member descriptions.
>
> >  elif match := re.match(
> >  r'(Returns|Errors|Since|Notes?|Examples?|TODO):
> *',
> >  line):
> > @@ -557,13 +563,14 @@ def get_doc(self) -> 'QAPIDoc':
> >  doc.append_line(text)
> >  line = self.get_doc_indented(doc)
> >  no_more_args = True
> > +intro = False
>
> Or after the first tagged section.
>
> Okay, it does what it says on the tin.
>
> >  elif line.startswith('='):
> >  raise QAPIParseError(
> >  self,
> >  "unexpected '=' markup in definition
> documentation")
> >  else:
> >  # tag-less paragraph
> > -doc.ensure_untagged_section(self.info)
> > +doc.ensure_untagged_section(self.info, intro)
> >  doc.append_line(line)
> >  line = self.get_doc_paragraph(doc)
> >  else:
> > @@ -617,7 +624,7 @@ def __init__(
> >  self,
> >  info: QAPISourceInfo,
> >  tag: Optional[str] = None,
> > -kind: str = 'paragraph',
> > +kind: str = 'intro-paragraph',
>
> The question "why is this optional?" crossed my mind when reviewing the
> previous patch.  I left it unasked, because I felt challenging the
> overlap between @kind and @tag was more useful.  However, the new
> default value 'intro-paragraph' feels more arbitrary to me than

Re: [PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section

[John Snow]

On Thu, May 16, 2024, 2:18 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > When iterating all_sections, this is helpful to be able to distinguish
> > "members" from "features"; the only other way to do so is to
> > cross-reference these sections against QAPIDoc.args or QAPIDoc.features,
> > but if the desired end goal for QAPIDoc is to remove everything except
> > all_sections, we need *something* accessible to distinguish them.
> >
> > To keep types simple, add this semantic parameter to the base Section
> > and not just ArgSection; we can use this to filter out paragraphs and
> > tagged sections, too.
> >
> > Signed-off-by: John Snow 
> > ---
> >  scripts/qapi/parser.py | 25 -
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 161768b8b96..cf4cbca1c1f 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -613,21 +613,27 @@ class QAPIDoc:
> >
> >  class Section:
> >  # pylint: disable=too-few-public-methods
> > -def __init__(self, info: QAPISourceInfo,
> > - tag: Optional[str] = None):
> > +def __init__(
> > +self,
> > +info: QAPISourceInfo,
> > +tag: Optional[str] = None,
> > +kind: str = 'paragraph',
> > +):
> >  # section source info, i.e. where it begins
> >  self.info = info
> >  # section tag, if any ('Returns', '@name', ...)
> >  self.tag = tag
> >  # section text without tag
> >  self.text = ''
> > +# section type - {paragraph, feature, member, tagged}
> > +self.kind = kind
>
> Hmm.  .kind is almost redundant with .tag.
>

Almost, yes. But the crucial bit is members/features as you notice. That's
the real necessity here that saves a lot of code when relying on *only*
all_sections.

(If you want to remove the other fields leaving only all_sections behind,
this is strictly necessary.)


> Untagged section:.kind is 'paragraph', .tag is None
>
> Member description:  .kind is 'member', .tag matches @NAME
>
> Feature description: .kind is 'feature', .tag matches @NAME


> Tagged section:  .kind is 'tagged', .tag matches
>   r'Returns|Errors|Since|Notes?|Examples?|TODO'
>
> .kind can directly be derived from .tag except for member and feature
> descriptions.  And you want to tell these two apart in a straightforward
> manner in later patches, as you explain in your commit message.
>
> If .kind is 'member' or 'feature', then self must be an ArgSection.
> Worth a comment?  An assertion?
>

No real need. The classes don't differ much in practice so there's not much
benefit, and asserting it won't help the static typer out anyway because it
can't remember the inference from string->type anyway.

If you wanted to be FANCY, we could use string literal typing on the field
and restrict valid values per-class, but that's so needless not even I'm
tempted by it.


> Some time back, I considered changing .tag for member and feature
> descriptions to suitable strings, like your 'member' and 'feature', and
> move the member / feature name into ArgSection.  I didn't, because the
> benefit wasn't worth the churn at the time.  Perhaps it's worth it now.
> Would it result in simpler code than your solution?
>

Not considerably, I think. Would just be shuffling around which field names
I touch and where/when.

It might actually just add some lines where I have to assert isinstance to
do type narrowing in the generator.


> Terminology nit: the section you call 'paragraph' isn't actually a
> paragraph: it could be several paragraphs.  Best to call it 'untagged',
> as in .ensure_untagged_section().
>

Oh, I hate when you make a good point. I was avoiding the term because I'm
removing Notes and Examples, and we have plans to eliminate Since ... the
tagged sections are almost going away entirely, leaving just TODO, which we
ignore.

Uhm, can I name it paragraphs? :) or open to other suggestions, incl.
untagged if that's still your preference.


> >
> >  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, tag: str, kind: str):
> > +super().__init__(info, tag, kind)
> >  self.member: Optional['QAPISchemaMember'] = None
> >
> >  def connect(self, member: 'QAPISchemaMember') -> None:
>
> [...]
>
>

Re: [PATCH 05/20] qapi/parser: adjust info location for doc body section

[John Snow]

On Thu, May 16, 2024, 1:58 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > Instead of using the info object for the doc block as a whole, 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.
> >
> > Signed-off-by: John Snow 
> > ---
> >  scripts/qapi/parser.py | 9 +++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 8cdd5334ec6..41b9319e5cb 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -662,8 +662,13 @@ 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'
>
> Before, we always append a newline.
>
> > +section = self.all_sections[-1]
> > +# Section is empty so far; update info to start *here*.
> > +if not section.text:
> > +section.info = info
> > +else:
> > +# extend current section
> > +self.all_sections[-1].text += '\n'
>
> Afterwards, we append it only when the section already has some text.
>
> The commit message claims the patch only adjusts section.info.  That's a
> lie :)
>

Well. It wasn't intentional, so it wasn't a lie... it was just wrong :)


> I believe the change makes no difference because .end() strips leading
> and trailing newline.
>
> >  return
> >  # start new section
> >  section = self.Section(info)
>
> You could fix the commit message, but I think backing out the
> no-difference change is easier.  The appended patch works in my testing.
>
> Next one.  Your patch changes the meaning of section.info.  Here's its
> initialization:
>
> class Section:
> # pylint: disable=too-few-public-methods
> def __init__(self, info: QAPISourceInfo,
>  tag: Optional[str] = None):
> ---># section source info, i.e. where it begins
> self.info = info
> # section tag, if any ('Returns', '@name', ...)
> self.tag = tag
> # section text without tag
> self.text = ''
>
> The comment is now wrong.  Calls for a thorough review of .info's uses.
>

Hmm... Did I really change its meaning? I guess it's debatable what "where
it begins" means. Does the tagless section start...

## <-- Here?
# Hello! <-- Or here?
##

I assert the *section* starts wherever the first line of text it contains
starts. Nothing else makes any sense.

There is value in recording where the doc block starts, but that's not a
task for the *section* info.

I don't think I understand your feedback.


> The alternative to changing .info's meaning is to add another member
> with the meaning you need.  Then we have to review .info's uses to find
> out which ones to switch to the new one.


> Left for later.
>
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 8cdd5334ec..abeae1ca77 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -663,7 +663,10 @@ 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.info = info
> +section.text += '\n'
>  return
>  # start new section
>  section = self.Section(info)
>
>

Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module

[John Snow]

On Wed, May 15, 2024, 1:27 PM Markus Armbruster  wrote:

> John Snow  writes:
>
> > On Wed, May 15, 2024 at 5:17 AM Markus Armbruster 
> wrote:
> >
> >> John Snow  writes:
> >>
> >> > In the coming patches, it's helpful to have a linting baseline.
> However,
> >> > there's no need to shuffle around the deck chairs too much, because
> most
> >> > of this code will be removed once the new qapidoc generator (the
> >> > "transmogrifier") is in place.
> >> >
> >> > To ease my pain: just turn off the black auto-formatter for most, but
> >> > not all, of qapidoc.py. This will help ensure that *new* code follows
> a
> >> > coding standard without bothering too much with cleaning up the
> existing
> >> > code.
> >> >
> >> > For manual checking for now, try "black --check qapidoc.py" from the
> >> > docs/sphinx directory. "pip install black" (without root permissions)
> if
> >> > you do not have it installed otherwise.
> >> >
> >> > Signed-off-by: John Snow 
> >> > ---
> >> >  docs/sphinx/qapidoc.py | 16 +---
> >> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> >> > index f270b494f01..1655682d4c7 100644
> >> > --- a/docs/sphinx/qapidoc.py
> >> > +++ b/docs/sphinx/qapidoc.py
> >> > @@ -28,28 +28,30 @@
> >> >  import re
> >> >
> >> >  from docutils import nodes
> >> > +from docutils.parsers.rst import Directive, directives
> >> >  from docutils.statemachine import ViewList
> >> > -from docutils.parsers.rst import directives, Directive
> >> > -from sphinx.errors import ExtensionError
> >> > -from sphinx.util.nodes import nested_parse_with_titles
> >> > -import sphinx
> >> > -from qapi.gen import QAPISchemaVisitor
> >> >  from qapi.error import QAPIError, QAPISemError
> >> > +from qapi.gen import QAPISchemaVisitor
> >> >  from qapi.schema import QAPISchema
> >> >
> >> > +import sphinx
> >> > +from sphinx.errors import ExtensionError
> >> > +from sphinx.util.nodes import nested_parse_with_titles
> >> > +
> >>
> >> Exchanges old pylint gripe
> >>
> >> docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are
> >> not grouped (ungrouped-imports)
> >>
> >> for new gripes
> >>
> >> docs/sphinx/qapidoc.py:37:0: C0411: third party import "import
> sphinx"
> >> should be placed before "from qapi.error import QAPIError, QAPISemError"
> >> (wrong-import-order)
> >> docs/sphinx/qapidoc.py:38:0: C0411: third party import "from
> >> sphinx.errors import ExtensionError" should be placed before "from
> >> qapi.error import QAPIError, QAPISemError" (wrong-import-order)
> >> docs/sphinx/qapidoc.py:39:0: C0411: third party import "from
> >> sphinx.util.nodes import nested_parse_with_titles" should be placed
> before
> >> "from qapi.error import QAPIError, QAPISemError" (wrong-import-order)
> >>
> >> Easy enough to fix.
> >>
> >
> > I believe these errors are caused by the fact that the tools are confused
> > about the "sphinx" namespace - some interpret them as being the local
> > "module", the docs/sphinx/ directory, and others believe them to be the
> > third party external package.
> >
> > I have not been using pylint on docs/sphinx/ files because of the
> > difficulty of managing imports - this environment is generally beyond the
> > reaches of my python borgcube and at present I don't have plans to
> > integrate it.
> >
> > At the moment, I am using black, isort and flake8 for qapidoc.py and
> > they're happy with it. I am not using mypy because I never did the typing
> > boogaloo with qapidoc.py and I won't be bothering - except for any new
> code
> > I write, which *will* bother. By the end of the new transmogrifier,
> > qapidoc.py *will* strictly typecheck.
> >
> > pylint may prove to be an issue with the imports, though. isort also
> seems
> > to misunderstand "sphinx, the stuff in this folder" and "sphinx, the
> stuff
> > in a third party package" and so I'm afraid I don't have any good ability
> > to get pylint

Re: [PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections

[John Snow]

On Wed, May 15, 2024 at 10:18 AM Markus Armbruster 
wrote:

> John Snow  writes:
>
> > On Wed, May 15, 2024, 7:50 AM Markus Armbruster 
> wrote:
> >
> >> John Snow  writes:
> >>
> >> > Prior to this patch, a section like this:
> >> >
> >> > @name: lorem ipsum
> >> >dolor sit amet
> >> >  consectetur adipiscing elit
> >> >
> >> > would be parsed as:
> >> >
> >> > "lorem ipsum\ndolor sit amet\n  consectetur adipiscing elit"
> >> >
> >> > We want to preserve the indentation for even the first body line so
> that
> >> > the entire block can be parsed directly as rST. This patch would now
> >> > parse that segment as:
> >> >
> >> > "lorem ipsum\n   dolor sit amet\n consectetur adipiscing elit"
> >>
> >> I'm afraid it's less than clear *why* we want to parse the entire block
> >> directly as rST.  I have just enough insight into what you've built on
> >> top of this series to hazard a guess.  Bear with me while I try to
> >> explain it.
> >>
> >
> > My own summary: qapidoc expects a paragraph, the new generator expects a
> > block.
> >
> >
> >> We first parse the doc comment with parser.py into an internal
> >> representation.  The structural parts become objects, and the remainder
> >> becomes text attributes of these objects.  Currently, parser.py
> >> carefully adjusts indentation for these text attributes.  Why?  I'll get
> >> to that.
> >>
> >> For your example, parser.py creates an ArgSection object, and sets its
> >> @text member to
> >>
> >> "lorem ipsum\ndolor sit amet\n  consectetur adipiscing elit"
> >>
> >> Printing this string gives us
> >>
> >> lorem ipsum
> >> dolor sit amet
> >>   consectetur adipiscing elit
> >>
> >> qapidoc.py then transforms parser.py's IR into Sphinx IR.  The objects
> >> become (more complicated) Sphinx objects, and their text attributes get
> >> re-parsed as rST text into still more Sphinx objects.
> >>
> >> This re-parse rejects your example with "Unexpected indentation."
> >>
> >
> > Specifically, it'd be an unexpected *unindent*; the indent lacking on the
> > first *two* lines is the problem.
> >
> >
> >> Let me use a slightly different one:
> >>
> >> # @name: lorem ipsum
> >> #dolor sit amet
> >> #consectetur adipiscing elit
> >>
> >> Results in this @text member
> >>
> >> lorem ipsum
> >> dolor sit amet
> >> consectetur adipiscing elit
> >>
> >> which is re-parsed as paragraph, i.e. exactly what we want.
> >>
> >
> > It's what we used to want, anyway.
>
> Yes, I'm describing the current state here.
>
> >> > This understandably breaks qapidoc.py;
> >>
> >> Without indentation adjustment, we'd get
> >>
> >> lorem ipsum
> >>dolor sit amet
> >>consectetur adipiscing elit
> >>
> >> which would be re-parsed as a definition list, I guess.  This isn't what
> >> we want.
> >>
> >> >so a new function is added
> there
> >> > to re-dedent the text.
> >>
> >> Your patch moves the indentation adjustment to another place.  No
> >> functional change.
> >>
> >> You move it so you can branch off your new rendering pipeline before the
> >> indentation adjustment, because ...
> >>
> >> >Once the new generator is merged, this function
> >> > will not be needed any longer and can be dropped.
> >>
> >> ... yours doesn't want it.
> >>
> >> I believe it doesn't want it, because it generates rST (with a QAPI
> >> extension) instead of Sphinx objects.  For your example, something like
> >>
> >> :arg name: lorem ipsum
> >>dolor sit amet
> >>  consectetur adipiscing elit
> >>
> >> For mine:
> >>
> >> :arg name: lorem ipsum
> >>dolor sit amet
> >>consectetur adipiscing elit
> >>
> >> Fair?
> >>
> >
> > Not quite;
> >
> > Old parsing, new generator:
> >
> > :arg type name: lorem ipsum
> &g

Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module

[John Snow]

On Wed, May 15, 2024 at 5:17 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > In the coming patches, it's helpful to have a linting baseline. However,
> > there's no need to shuffle around the deck chairs too much, because most
> > of this code will be removed once the new qapidoc generator (the
> > "transmogrifier") is in place.
> >
> > To ease my pain: just turn off the black auto-formatter for most, but
> > not all, of qapidoc.py. This will help ensure that *new* code follows a
> > coding standard without bothering too much with cleaning up the existing
> > code.
> >
> > For manual checking for now, try "black --check qapidoc.py" from the
> > docs/sphinx directory. "pip install black" (without root permissions) if
> > you do not have it installed otherwise.
> >
> > Signed-off-by: John Snow 
> > ---
> >  docs/sphinx/qapidoc.py | 16 +---
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index f270b494f01..1655682d4c7 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -28,28 +28,30 @@
> >  import re
> >
> >  from docutils import nodes
> > +from docutils.parsers.rst import Directive, directives
> >  from docutils.statemachine import ViewList
> > -from docutils.parsers.rst import directives, Directive
> > -from sphinx.errors import ExtensionError
> > -from sphinx.util.nodes import nested_parse_with_titles
> > -import sphinx
> > -from qapi.gen import QAPISchemaVisitor
> >  from qapi.error import QAPIError, QAPISemError
> > +from qapi.gen import QAPISchemaVisitor
> >  from qapi.schema import QAPISchema
> >
> > +import sphinx
> > +from sphinx.errors import ExtensionError
> > +from sphinx.util.nodes import nested_parse_with_titles
> > +
>
> Exchanges old pylint gripe
>
> docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are
> not grouped (ungrouped-imports)
>
> for new gripes
>
> docs/sphinx/qapidoc.py:37:0: C0411: third party import "import sphinx"
> should be placed before "from qapi.error import QAPIError, QAPISemError"
> (wrong-import-order)
> docs/sphinx/qapidoc.py:38:0: C0411: third party import "from
> sphinx.errors import ExtensionError" should be placed before "from
> qapi.error import QAPIError, QAPISemError" (wrong-import-order)
> docs/sphinx/qapidoc.py:39:0: C0411: third party import "from
> sphinx.util.nodes import nested_parse_with_titles" should be placed before
> "from qapi.error import QAPIError, QAPISemError" (wrong-import-order)
>
> Easy enough to fix.
>

I believe these errors are caused by the fact that the tools are confused
about the "sphinx" namespace - some interpret them as being the local
"module", the docs/sphinx/ directory, and others believe them to be the
third party external package.

I have not been using pylint on docs/sphinx/ files because of the
difficulty of managing imports - this environment is generally beyond the
reaches of my python borgcube and at present I don't have plans to
integrate it.

At the moment, I am using black, isort and flake8 for qapidoc.py and
they're happy with it. I am not using mypy because I never did the typing
boogaloo with qapidoc.py and I won't be bothering - except for any new code
I write, which *will* bother. By the end of the new transmogrifier,
qapidoc.py *will* strictly typecheck.

pylint may prove to be an issue with the imports, though. isort also seems
to misunderstand "sphinx, the stuff in this folder" and "sphinx, the stuff
in a third party package" and so I'm afraid I don't have any good ability
to get pylint to play along, here.

Pleading for "Sorry, this sucks and I can't figure out how to solve it
quickly". Maybe a future project, apologies.


> >
> >  # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
> >  # use switch_source_input. Check borrowed from kerneldoc.py.
> > -Use_SSI = sphinx.__version__[:3] >= '1.7'
> > +Use_SSI = sphinx.__version__[:3] >= "1.7"
> >  if Use_SSI:
> >  from sphinx.util.docutils import switch_source_input
> >  else:
> >  from sphinx.ext.autodoc import AutodocReporter
> >
> >
> > -__version__ = '1.0'
> > +__version__ = "1.0"
> >
> >
> > +# fmt: off
>
> I figure this tells black to keep quiet for the remainder of the file.
> Worth a comment, I think.
>
> >  # Function borrowed from pydash, which is under the MIT license
> >  def intersperse(iterable, separator):
> >  """Yield the members of *iterable* interspersed with *separator*."""
>
> With my comments addressed
> Reviewed-by: Markus Armbruster 
>

^ Dropping this unless you're okay with the weird import orders owing to
the strange import paradigm in the sphinx folder.r

Re: [PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections

[John Snow]

On Wed, May 15, 2024, 7:50 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > Prior to this patch, a section like this:
> >
> > @name: lorem ipsum
> >dolor sit amet
> >  consectetur adipiscing elit
> >
> > would be parsed as:
> >
> > "lorem ipsum\ndolor sit amet\n  consectetur adipiscing elit"
> >
> > We want to preserve the indentation for even the first body line so that
> > the entire block can be parsed directly as rST. This patch would now
> > parse that segment as:
> >
> > "lorem ipsum\n   dolor sit amet\n consectetur adipiscing elit"
>
> I'm afraid it's less than clear *why* we want to parse the entire block
> directly as rST.  I have just enough insight into what you've built on
> top of this series to hazard a guess.  Bear with me while I try to
> explain it.
>

My own summary: qapidoc expects a paragraph, the new generator expects a
block.


> We first parse the doc comment with parser.py into an internal
> representation.  The structural parts become objects, and the remainder
> becomes text attributes of these objects.  Currently, parser.py
> carefully adjusts indentation for these text attributes.  Why?  I'll get
> to that.
>
> For your example, parser.py creates an ArgSection object, and sets its
> @text member to
>
> "lorem ipsum\ndolor sit amet\n  consectetur adipiscing elit"
>
> Printing this string gives us
>
> lorem ipsum
> dolor sit amet
>   consectetur adipiscing elit
>
> qapidoc.py then transforms parser.py's IR into Sphinx IR.  The objects
> become (more complicated) Sphinx objects, and their text attributes get
> re-parsed as rST text into still more Sphinx objects.
>
> This re-parse rejects your example with "Unexpected indentation."
>

Specifically, it'd be an unexpected *unindent*; the indent lacking on the
first *two* lines is the problem.


> Let me use a slightly different one:
>
> # @name: lorem ipsum
> #dolor sit amet
> #consectetur adipiscing elit
>
> Results in this @text member
>
> lorem ipsum
> dolor sit amet
> consectetur adipiscing elit
>
> which is re-parsed as paragraph, i.e. exactly what we want.
>

It's what we used to want, anyway.


> > This understandably breaks qapidoc.py;
>
> Without indentation adjustment, we'd get
>
> lorem ipsum
>dolor sit amet
>consectetur adipiscing elit
>
> which would be re-parsed as a definition list, I guess.  This isn't what
> we want.
>
> >so a new function is added there
> > to re-dedent the text.
>
> Your patch moves the indentation adjustment to another place.  No
> functional change.
>
> You move it so you can branch off your new rendering pipeline before the
> indentation adjustment, because ...
>
> >Once the new generator is merged, this function
> > will not be needed any longer and can be dropped.
>
> ... yours doesn't want it.
>
> I believe it doesn't want it, because it generates rST (with a QAPI
> extension) instead of Sphinx objects.  For your example, something like
>
> :arg name: lorem ipsum
>dolor sit amet
>  consectetur adipiscing elit
>
> For mine:
>
> :arg name: lorem ipsum
>dolor sit amet
>consectetur adipiscing elit
>
> Fair?
>

Not quite;

Old parsing, new generator:

:arg type name: lorem ipsum
dolor sit amet
  consectetur apidiscing elit

This is wrong - continuations of a field list must be indented. Unlike
paragraphs, we want indents to "keep the block".

New parsing, new generator:

:arg type name: lorem ipsum
   dolor sit amet
 consectetur apidiscing elit

indent is preserved, maintaining the block-level element.

I don't have to re-add indents and any nested block elements will be
preserved correctly. i.e. you can use code examples, nested lists, etc. in
argument definitions.

The goal here was "Do not treat this as a paragraph, treat it directly as
rST and do not modify it needlessly."

It's a lot simpler than trying to manage the indent and injecting spaces
manually - and adding a temporary dedent to scheduled-for-demolition code
seemed the nicer place to add the hack.


> The transition from the doc comment to (extended) rST is straightforward
> for these examples.
>
> I hope it'll be as straightforward (and thus predictable) in other
> cases, too.
>
> > (I verified this patch changes absolutely nothing by comparing the
> > md5sums of the QMP ref html pages both before and after the change, so
> > it's certified inert. QAPI test output has been updated to refl

Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module

[John Snow]

On Wed, May 15, 2024, 5:17 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > In the coming patches, it's helpful to have a linting baseline. However,
> > there's no need to shuffle around the deck chairs too much, because most
> > of this code will be removed once the new qapidoc generator (the
> > "transmogrifier") is in place.
> >
> > To ease my pain: just turn off the black auto-formatter for most, but
> > not all, of qapidoc.py. This will help ensure that *new* code follows a
> > coding standard without bothering too much with cleaning up the existing
> > code.
> >
> > For manual checking for now, try "black --check qapidoc.py" from the
> > docs/sphinx directory. "pip install black" (without root permissions) if
> > you do not have it installed otherwise.
> >
> > Signed-off-by: John Snow 
> > ---
> >  docs/sphinx/qapidoc.py | 16 +---
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index f270b494f01..1655682d4c7 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -28,28 +28,30 @@
> >  import re
> >
> >  from docutils import nodes
> > +from docutils.parsers.rst import Directive, directives
> >  from docutils.statemachine import ViewList
> > -from docutils.parsers.rst import directives, Directive
> > -from sphinx.errors import ExtensionError
> > -from sphinx.util.nodes import nested_parse_with_titles
> > -import sphinx
> > -from qapi.gen import QAPISchemaVisitor
> >  from qapi.error import QAPIError, QAPISemError
> > +from qapi.gen import QAPISchemaVisitor
> >  from qapi.schema import QAPISchema
> >
> > +import sphinx
> > +from sphinx.errors import ExtensionError
> > +from sphinx.util.nodes import nested_parse_with_titles
> > +
>
> Exchanges old pylint gripe
>
> docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are
> not grouped (ungrouped-imports)
>
> for new gripes
>
> docs/sphinx/qapidoc.py:37:0: C0411: third party import "import sphinx"
> should be placed before "from qapi.error import QAPIError, QAPISemError"
> (wrong-import-order)
> docs/sphinx/qapidoc.py:38:0: C0411: third party import "from
> sphinx.errors import ExtensionError" should be placed before "from
> qapi.error import QAPIError, QAPISemError" (wrong-import-order)
> docs/sphinx/qapidoc.py:39:0: C0411: third party import "from
> sphinx.util.nodes import nested_parse_with_titles" should be placed before
> "from qapi.error import QAPIError, QAPISemError" (wrong-import-order)
>
> Easy enough to fix.
>

This is a problem where our sphinx directory is colliding with the sphinx
namespace and different versions of the tooling disagree with the
assessment.

I'll try to fix this without renaming our directory, but I'm worried that
might be the most robust solution.


> >
> >  # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
> >  # use switch_source_input. Check borrowed from kerneldoc.py.
> > -Use_SSI = sphinx.__version__[:3] >= '1.7'
> > +Use_SSI = sphinx.__version__[:3] >= "1.7"
> >  if Use_SSI:
> >  from sphinx.util.docutils import switch_source_input
> >  else:
> >  from sphinx.ext.autodoc import AutodocReporter
> >
> >
> > -__version__ = '1.0'
> > +__version__ = "1.0"
> >
> >
> > +# fmt: off
>
> I figure this tells black to keep quiet for the remainder of the file.
> Worth a comment, I think.
>

It does, yes. Want an inline comment here?


> >  # Function borrowed from pydash, which is under the MIT license
> >  def intersperse(iterable, separator):
> >  """Yield the members of *iterable* interspersed with *separator*."""
>
> With my comments addressed
> Reviewed-by: Markus Armbruster 
>
>

[PATCH 08/20] qapi/parser: differentiate intro and outro paragraphs

[John Snow]

Add a semantic tag to paragraphs that appear *before* tagged
sections/members/features and those that appear after. This will control
how they are inlined when doc sections are merged and flattened.

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 22 +-
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index cf4cbca1c1f..b1794f71e12 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -503,6 +503,10 @@ def get_doc(self) -> 'QAPIDoc':
 self.accept(False)
 line = self.get_doc_line()
 no_more_args = False
+# Paragraphs before members/features/tagged are "intro" paragraphs.
+# Any appearing subsequently are "outro" paragraphs.
+# This is only semantic metadata for the doc generator.
+intro = True
 
 while line is not None:
 # Blank lines
@@ -532,6 +536,7 @@ def get_doc(self) -> 'QAPIDoc':
 raise QAPIParseError(
 self, 'feature descriptions expected')
 no_more_args = True
+intro = False
 elif match := self._match_at_name_colon(line):
 # description
 if no_more_args:
@@ -547,6 +552,7 @@ def get_doc(self) -> 'QAPIDoc':
 doc.append_line(text)
 line = self.get_doc_indented(doc)
 no_more_args = True
+intro = False
 elif match := re.match(
 r'(Returns|Errors|Since|Notes?|Examples?|TODO): *',
 line):
@@ -557,13 +563,14 @@ def get_doc(self) -> 'QAPIDoc':
 doc.append_line(text)
 line = self.get_doc_indented(doc)
 no_more_args = True
+intro = False
 elif line.startswith('='):
 raise QAPIParseError(
 self,
 "unexpected '=' markup in definition documentation")
 else:
 # tag-less paragraph
-doc.ensure_untagged_section(self.info)
+doc.ensure_untagged_section(self.info, intro)
 doc.append_line(line)
 line = self.get_doc_paragraph(doc)
 else:
@@ -617,7 +624,7 @@ def __init__(
 self,
 info: QAPISourceInfo,
 tag: Optional[str] = None,
-kind: str = 'paragraph',
+kind: str = 'intro-paragraph',
 ):
 # section source info, i.e. where it begins
 self.info = info
@@ -625,7 +632,7 @@ def __init__(
 self.tag = tag
 # section text without tag
 self.text = ''
-# section type - {paragraph, feature, member, tagged}
+# section type - {-paragraph, feature, member, tagged}
 self.kind = kind
 
 def append_line(self, line: str) -> None:
@@ -666,7 +673,11 @@ def end(self) -> None:
 raise QAPISemError(
 section.info, "text required after '%s:'" % section.tag)
 
-def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
+def ensure_untagged_section(
+self,
+info: QAPISourceInfo,
+intro: bool = True,
+) -> None:
 if self.all_sections and not self.all_sections[-1].tag:
 section = self.all_sections[-1]
 # Section is empty so far; update info to start *here*.
@@ -677,7 +688,8 @@ def ensure_untagged_section(self, info: QAPISourceInfo) -> 
None:
 self.all_sections[-1].text += '\n'
 return
 # start new section
-section = self.Section(info)
+kind = ("intro" if intro else "outro") + "-paragraph"
+section = self.Section(info, kind=kind)
 self.sections.append(section)
 self.all_sections.append(section)
 
-- 
2.44.0

[PATCH 11/20] qapi/schema: add doc_visible property to QAPISchemaDefinition

[John Snow]

The intent here is to mark only certain definitions as visible in the
end-user docs.

All commands and events are inherently visible. Everything else is
visible only if it is a member (or a branch member) of a type that is
visible, or if it is named as a return type for a command.

Notably, this excludes arg_type for commands and events, and any
base_types specified for structures/unions. Those objects may still be
marked visible if they are named as members from a visible type.

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

Some internal and 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.

This information is not yet used by qapidoc, which continues to render
documentation exactly as it has. This information will be used by the
new qapidoc (the "transmogrifier"), to be introduced later. The new
generator verifies that all of the objects that should be rendered *are*
by failing if any cross-references are missing, verifying everything is
in place.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 40 
 1 file changed, 40 insertions(+)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index e15e64ea8cb..6025b4e9354 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:
   

[PATCH 18/20] qapi: ensure all errors sections are uniformly typset

[John Snow]

Transactions have the only instance of an Errors section that isn't a
rST list; turn it into one.

Signed-off-by: John Snow 
---
 qapi/transaction.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/transaction.json b/qapi/transaction.json
index 5749c133d4a..07afc269d54 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -235,7 +235,7 @@
 # additional detail.
 #
 # Errors:
-# Any errors from commands in the transaction
+# - Any errors from commands in the transaction
 #
 # Note: The transaction aborts on the first failure.  Therefore, there
 # will be information on only one failed operation returned in an
-- 
2.44.0

[PATCH 09/20] qapi/parser: add undocumented stub members to all_sections

[John Snow]

This helps simplify the doc generator if it doesn't have to check for
undocumented members.

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b1794f71e12..3cd8e7ee295 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -740,8 +740,24 @@ 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, 'member')
+
+# Insert stub documentation section for missing member docs.
+section = QAPIDoc.ArgSection(
+self.info, f"@{member.name}", "member")
+self.args[member.name] = section
+
+# Determine where to insert stub doc.
+index = 0
+for i, sect in enumerate(self.all_sections):
+# insert after these:
+if sect.kind in ('intro-paragraph', 'member'):
+index = i + 1
+# but before these:
+elif sect.kind in ('tagged', 'feature', 'outro-paragraph'):
+index = i
+break
+self.all_sections.insert(index, section)
+
 self.args[member.name].connect(member)
 
 def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
-- 
2.44.0

[PATCH 05/20] qapi/parser: adjust info location for doc body section

[John Snow]

Instead of using the info object for the doc block as a whole, 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.

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 8cdd5334ec6..41b9319e5cb 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -662,8 +662,13 @@ 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]
+# Section is empty so far; update info to start *here*.
+if not section.text:
+section.info = info
+else:
+# extend current section
+self.all_sections[-1].text += '\n'
 return
 # start new section
 section = self.Section(info)
-- 
2.44.0

[PATCH 19/20] qapi: convert "Note" sections to plain rST

[John Snow]

We do not need a dedicated section for notes. By eliminating a specially
parsed section, these notes can be treated as normal rST paragraphs in
the new QMP reference manual, and can be placed and styled much more
flexibly.

Update the QAPI parser to now prohibit special "Note" sections while
suggesting a new syntax.

The exact formatting to use is a matter of taste, but a good candidate
is simply:

.. note:: lorem ipsum ...

but there are other choices, too. The Sphinx readthedocs theme offers
theming for the following forms (capitalization unimportant); all are
adorned with a (!) symbol in the title bar for rendered HTML docs.

These are rendered in orange:

.. Attention:: ...
.. Caution:: ...
.. WARNING:: ...

These are rendered in red:

.. DANGER:: ...
.. Error:: ...

These are rendered in green:

.. Hint:: ...
.. Important:: ...
.. Tip:: ...

These are rendered in blue:

.. Note:: ...
.. admonition:: custom title

   admonition body text

This patch uses ".. notes::" almost everywhere, with just two "caution"
directives. ".. admonition:: notes" is used in a few places where we had
an ordered list of multiple notes.

Signed-off-by: John Snow 
---
 qapi/block-core.json  | 30 +++
 qapi/block.json   |  2 +-
 qapi/char.json| 12 +--
 qapi/control.json | 15 ++--
 qapi/dump.json|  2 +-
 qapi/introspect.json  |  6 +-
 qapi/machine-target.json  | 26 +++---
 qapi/machine.json | 47 +-
 qapi/migration.json   | 12 +--
 qapi/misc.json| 88 +--
 qapi/net.json |  6 +-
 qapi/pci.json |  7 +-
 qapi/qdev.json| 30 +++
 qapi/qom.json | 19 ++--
 qapi/rocker.json  | 16 ++--
 qapi/run-state.json   | 18 ++--
 qapi/sockets.json | 10 +--
 qapi/stats.json   | 22 ++---
 qapi/transaction.json |  8 +-
 qapi/ui.json  | 29 +++---
 qapi/virtio.json  | 12 +--
 qga/qapi-schema.json  | 48 +-
 scripts/qapi/parser.py|  9 ++
 tests/qapi-schema/doc-empty-section.err   |  2 +-
 tests/qapi-schema/doc-empty-section.json  |  2 +-
 tests/qapi-schema/doc-good.json   |  6 +-
 tests/qapi-schema/doc-good.out| 10 ++-
 tests/qapi-schema/doc-good.txt| 14 ++-
 .../qapi-schema/doc-interleaved-section.json  |  2 +-
 29 files changed, 258 insertions(+), 252 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 64fe5240cc9..530af40404d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1510,7 +1510,7 @@
 # @mode: whether and how QEMU should create a new image, default is
 # 'absolute-paths'.
 #
-# Note: Either @device or @node-name must be set but not both.
+# .. note:: Either @device or @node-name must be set but not both.
 #
 ##
 { 'struct': 'BlockdevSnapshotSync',
@@ -1616,9 +1616,9 @@
 #
 # @unstable: Member @x-perf is experimental.
 #
-# Note: @on-source-error and @on-target-error only affect background
-# I/O.  If an error occurs during a guest write request, the
-# device's rerror/werror actions will be used.
+# .. note:: @on-source-error and @on-target-error only affect background
+#I/O.  If an error occurs during a guest write request, the device's
+#rerror/werror actions will be used.
 #
 # Since: 4.2
 ##
@@ -5534,8 +5534,8 @@
 # after this event and must be repaired (Since 2.2; before, every
 # BLOCK_IMAGE_CORRUPTED event was fatal)
 #
-# Note: If action is "stop", a STOP event will eventually follow the
-# BLOCK_IO_ERROR event.
+# .. note:: If action is "stop", a STOP event will eventually follow the
+#BLOCK_IO_ERROR event.
 #
 # Example:
 #
@@ -5581,8 +5581,8 @@
 # field is a debugging aid for humans, it should not be parsed by
 # applications) (since: 2.2)
 #
-# Note: If action is "stop", a STOP event will eventually follow the
-# BLOCK_IO_ERROR event
+# .. note:: If action is "stop", a STOP event will eventually follow the
+#BLOCK_IO_ERROR event.
 #
 # Since: 0.13
 #
@@ -5720,8 +5720,8 @@
 #
 # @speed: rate limit, bytes per second
 #
-# Note: The "ready to complete" status is always reset by a
-# @BLOCK_JOB_ERROR event
+# .. note:: The "ready to complete" status is always reset by a
+#@BLOCK_JOB_ERROR event.
 #
 # Since: 1.3
 #
@@ -5974,7 +5974,7 @@
 #
 # @sectors-count: failed read operation sector count
 #
-# Note: This ev

[PATCH 16/20] qapi: rewrite StatsFilter comment

[John Snow]

Rewrite the StatsFilter intro paragraph to be more meaningful to
end-users when it is inlined in generated documentation.

Signed-off-by: John Snow 
---
 qapi/stats.json | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/qapi/stats.json b/qapi/stats.json
index 578b52c7ef7..c4a9f3ff70e 100644
--- a/qapi/stats.json
+++ b/qapi/stats.json
@@ -112,10 +112,6 @@
 ##
 # @StatsFilter:
 #
-# The arguments to the query-stats command; specifies a target for
-# which to request statistics and optionally the required subset of
-# information for that target.
-#
 # @target: the kind of objects to query.  Note that each possible
 #  target may enable additional filtering options
 #
@@ -183,8 +179,8 @@
 # Return runtime-collected statistics for objects such as the VM or
 # its vCPUs.
 #
-# The arguments are a StatsFilter and specify the provider and objects
-# to return statistics about.
+# The arguments specify a target for which to request statistics and
+# optionally the required subset of information for that target.
 #
 # Returns: a list of StatsResult, one for each provider and object
 # (e.g., for each vCPU).
-- 
2.44.0

[PATCH 06/20] qapi/parser: fix comment parsing immediately following a doc block

[John Snow]

If a comment immediately follows a doc block, the parser doesn't ignore
that token appropriately. Fix that.

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 41b9319e5cb..161768b8b96 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -587,7 +587,7 @@ def get_doc(self) -> 'QAPIDoc':
 line = self.get_doc_line()
 first = False
 
-self.accept(False)
+self.accept()
 doc.end()
 return doc
 
-- 
2.44.0

[PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section

[John Snow]

When iterating all_sections, this is helpful to be able to distinguish
"members" from "features"; the only other way to do so is to
cross-reference these sections against QAPIDoc.args or QAPIDoc.features,
but if the desired end goal for QAPIDoc is to remove everything except
all_sections, we need *something* accessible to distinguish them.

To keep types simple, add this semantic parameter to the base Section
and not just ArgSection; we can use this to filter out paragraphs and
tagged sections, too.

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 25 -
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 161768b8b96..cf4cbca1c1f 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -613,21 +613,27 @@ class QAPIDoc:
 
 class Section:
 # pylint: disable=too-few-public-methods
-def __init__(self, info: QAPISourceInfo,
- tag: Optional[str] = None):
+def __init__(
+self,
+info: QAPISourceInfo,
+tag: Optional[str] = None,
+kind: str = 'paragraph',
+):
 # section source info, i.e. where it begins
 self.info = info
 # section tag, if any ('Returns', '@name', ...)
 self.tag = tag
 # section text without tag
 self.text = ''
+# section type - {paragraph, feature, member, tagged}
+self.kind = kind
 
 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, tag: str, kind: str):
+super().__init__(info, tag, kind)
 self.member: Optional['QAPISchemaMember'] = None
 
 def connect(self, member: 'QAPISchemaMember') -> None:
@@ -676,7 +682,7 @@ def ensure_untagged_section(self, info: QAPISourceInfo) -> 
None:
 self.all_sections.append(section)
 
 def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None:
-section = self.Section(info, tag)
+section = self.Section(info, tag, "tagged")
 if tag == 'Returns':
 if self.returns:
 raise QAPISemError(
@@ -696,20 +702,21 @@ def new_tagged_section(self, info: QAPISourceInfo, tag: 
str) -> None:
 self.all_sections.append(section)
 
 def _new_description(self, info: QAPISourceInfo, name: str,
+ kind: str,
  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, '@' + name, kind)
 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, 'member', self.args)
 
 def new_feature(self, info: QAPISourceInfo, name: str) -> None:
-self._new_description(info, name, self.features)
+self._new_description(info, name, 'feature', self.features)
 
 def append_line(self, line: str) -> None:
 self.all_sections[-1].append_line(line)
@@ -722,7 +729,7 @@ def connect_member(self, member: 'QAPISchemaMember') -> 
None:
"%s '%s' lacks documentation"
% (member.role, member.name))
 self.args[member.name] = QAPIDoc.ArgSection(
-self.info, '@' + member.name)
+self.info, '@' + member.name, 'member')
 self.args[member.name].connect(member)
 
 def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
-- 
2.44.0

[PATCH 01/20] [DO-NOT-MERGE]: Add some ad-hoc linting helpers.

[John Snow]

These aren't ready for upstream inclusion, because they do not properly
manage version dependencies, execution environment and so on. These are
just the tools I use in my Own Special Environment :tm: for testing and
debugging.

They've been tested only on Fedora 38 for right now, which means:

Python 3.11.4-1.fc38
pylint 2.17.4-2.fc38
mypy 1.4.0-1.fc38
isort 5.12.0-1.fc38
flake8 5.0.3-2.fc38

"Soon" :tm: I'll move the qapi generator code under the python/
directory to take advantage of the more robust linting infrastructure
there, but that's going to happen after this series. Sorry about that!

Signed-off-by: John Snow 
---
 scripts/qapi-lint.sh  | 51 +++
 scripts/qapi/Makefile |  5 +
 2 files changed, 56 insertions(+)
 create mode 100755 scripts/qapi-lint.sh
 create mode 100644 scripts/qapi/Makefile

diff --git a/scripts/qapi-lint.sh b/scripts/qapi-lint.sh
new file mode 100755
index 000..528b31627eb
--- /dev/null
+++ b/scripts/qapi-lint.sh
@@ -0,0 +1,51 @@
+#!/usr/bin/env bash
+set -e
+
+if [[ -f qapi/.flake8 ]]; then
+echo "flake8 --config=qapi/.flake8 qapi/"
+flake8 --config=qapi/.flake8 qapi/
+fi
+if [[ -f qapi/pylintrc ]]; then
+echo "pylint --rcfile=qapi/pylintrc qapi/"
+pylint --rcfile=qapi/pylintrc qapi/
+fi
+if [[ -f qapi/mypy.ini ]]; then
+echo "mypy --config-file=qapi/mypy.ini qapi/"
+mypy --config-file=qapi/mypy.ini qapi/
+fi
+
+if [[ -f qapi/.isort.cfg ]]; then
+pushd qapi
+echo "isort -c ."
+isort -c .
+popd
+fi
+
+if [[ -f ../docs/sphinx/qapi-domain.py ]]; then
+pushd ../docs/sphinx
+
+echo "mypy --strict qapi-domain.py"
+mypy --strict qapi-domain.py
+echo "flake8 qapi-domain.py --max-line-length=99"
+flake8 qapi-domain.py --max-line-length=99
+echo "isort qapi-domain.py"
+isort qapi-domain.py
+echo "black --check qapi-domain.py"
+black --check qapi-domain.py
+
+echo "flake8 qapidoc.py --max-line-length=99"
+flake8 qapidoc.py --max-line-length=99
+echo "isort qapidoc.py"
+isort qapidoc.py
+echo "black --check qapidoc.py"
+black --check qapidoc.py
+
+popd
+fi
+
+pushd ../build
+make -j13
+make check-qapi-schema
+make docs
+make sphinxdocs
+popd
diff --git a/scripts/qapi/Makefile b/scripts/qapi/Makefile
new file mode 100644
index 000..314e8a5505e
--- /dev/null
+++ b/scripts/qapi/Makefile
@@ -0,0 +1,5 @@
+check:
+   isort -c .
+   flake8 .
+   cd .. && pylint --rcfile=qapi/pylintrc qapi
+   cd .. && mypy -p qapi --config-file=qapi/mypy.ini
-- 
2.44.0

[PATCH 04/20] qapi/parser: preserve indentation in QAPIDoc sections

[John Snow]

Prior to this patch, a section like this:

@name: lorem ipsum
   dolor sit amet
 consectetur adipiscing elit

would be parsed as:

"lorem ipsum\ndolor sit amet\n  consectetur adipiscing elit"

We want to preserve the indentation for even the first body line so that
the entire block can be parsed directly as rST. This patch would now
parse that segment as:

"lorem ipsum\n   dolor sit amet\n consectetur adipiscing elit"

This understandably breaks qapidoc.py; so a new function is added there
to re-dedent the text. Once the new generator is merged, this function
will not be needed any longer and can be dropped.

(I verified this patch changes absolutely nothing by comparing the
md5sums of the QMP ref html pages both before and after the change, so
it's certified inert. QAPI test output has been updated to reflect the
new strategy of preserving indents for rST.)

Signed-off-by: John Snow 
---
 docs/sphinx/qapidoc.py | 36 +-
 scripts/qapi/parser.py |  8 ++--
 tests/qapi-schema/doc-good.out | 32 +++---
 3 files changed, 53 insertions(+), 23 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 1655682d4c7..2e3ffcbafb7 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -26,6 +26,7 @@
 
 import os
 import re
+import textwrap
 
 from docutils import nodes
 from docutils.parsers.rst import Directive, directives
@@ -51,6 +52,28 @@
 __version__ = "1.0"
 
 
+def dedent(text: str) -> str:
+# Temporary: In service of the new QAPI domain, the QAPI doc parser
+# now preserves indents in args/members/features text. QAPIDoc does
+# not handle this well, so undo that change here.
+
+# QAPIDoc is being rewritten and will be replaced soon,
+# but this function is here in the interim as transition glue.
+
+lines = text.splitlines(True)
+if len(lines) > 1:
+if re.match(r"\s+", lines[0]):
+# First line is indented; description started on
+# the line after the name. dedent the whole block.
+return textwrap.dedent(text)
+else:
+# Descr started on same line. Dedent line 2+.
+return lines[0] + textwrap.dedent("".join(lines[1:]))
+else:
+# Descr was a single line; dedent entire line.
+return textwrap.dedent(text)
+
+
 # fmt: off
 # Function borrowed from pydash, which is under the MIT license
 def intersperse(iterable, separator):
@@ -169,7 +192,7 @@ def _nodes_for_members(self, doc, what, base=None, 
branches=None):
 term = self._nodes_for_one_member(section.member)
 # TODO drop fallbacks when undocumented members are outlawed
 if section.text:
-defn = section.text
+defn = dedent(section.text)
 else:
 defn = [nodes.Text('Not documented')]
 
@@ -207,7 +230,7 @@ def _nodes_for_enum_values(self, doc):
 termtext.extend(self._nodes_for_ifcond(section.member.ifcond))
 # TODO drop fallbacks when undocumented members are outlawed
 if section.text:
-defn = section.text
+defn = dedent(section.text)
 else:
 defn = [nodes.Text('Not documented')]
 
@@ -242,7 +265,7 @@ def _nodes_for_features(self, doc):
 dlnode = nodes.definition_list()
 for section in doc.features.values():
 dlnode += self._make_dlitem(
-[nodes.literal('', section.member.name)], section.text)
+[nodes.literal('', section.member.name)], dedent(section.text))
 seen_item = True
 
 if not seen_item:
@@ -265,9 +288,12 @@ def _nodes_for_sections(self, doc):
 continue
 snode = self._make_section(section.tag)
 if section.tag and section.tag.startswith('Example'):
-snode += self._nodes_for_example(section.text)
+snode += self._nodes_for_example(dedent(section.text))
 else:
-self._parse_text_into_node(section.text, snode)
+self._parse_text_into_node(
+dedent(section.text) if section.tag else section.text,
+snode,
+)
 nodelist.append(snode)
 return nodelist
 
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 7b13a583ac1..8cdd5334ec6 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -448,7 +448,10 @@ def get_doc_indented(self, doc: 'QAPIDoc') -> 
Optional[str]:
 indent = must_match(r'\s*', line).end()
 if not indent:
 return line
-doc.append_line(line[indent:])
+
+# Preserve the indent, it's needed for rST formatting.
+doc.append_line(line)
+
 prev_line_blank = False
 while True:
 self.accept(False)
@@ -465,7 +468,

[PATCH 02/20] qapi: linter fixups

[John Snow]

Fix minor irritants to pylint/flake8 et al.

(Yes, these need to be guarded by the Python tests. That's a work in
progress, a series that's quite likely to follow once I finish this
Sphinx project. Please pardon the temporary irritation.)

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 8 
 scripts/qapi/schema.py | 6 +++---
 scripts/qapi/visit.py  | 5 +++--
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 86c075a6ad2..ac14b20f308 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -27,8 +27,8 @@
 from .schema import (
 QAPISchema,
 QAPISchemaAlternatives,
-QAPISchemaBranches,
 QAPISchemaArrayType,
+QAPISchemaBranches,
 QAPISchemaBuiltinType,
 QAPISchemaEntity,
 QAPISchemaEnumMember,
@@ -233,9 +233,9 @@ def _use_type(self, typ: QAPISchemaType) -> str:
 typ = type_int
 elif (isinstance(typ, QAPISchemaArrayType) and
   typ.element_type.json_type() == 'int'):
-type_intList = self._schema.lookup_type('intList')
-assert type_intList
-typ = type_intList
+type_intlist = self._schema.lookup_type('intList')
+assert type_intlist
+typ = type_intlist
 # Add type to work queue if new
 if typ not in self._used_types:
 self._used_types.append(typ)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 721c470d2b8..d65c35f6ee6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -730,6 +730,7 @@ def set_defined_in(self, name: str) -> None:
 for v in self.variants:
 v.set_defined_in(name)
 
+# pylint: disable=unused-argument
 def check(
 self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
 ) -> None:
@@ -1166,7 +1167,7 @@ def _def_definition(self, defn: QAPISchemaDefinition) -> 
None:
 defn.info, "%s is already defined" % other_defn.describe())
 self._entity_dict[defn.name] = defn
 
-def lookup_entity(self,name: str) -> Optional[QAPISchemaEntity]:
+def lookup_entity(self, name: str) -> Optional[QAPISchemaEntity]:
 return self._entity_dict.get(name)
 
 def lookup_type(self, name: str) -> Optional[QAPISchemaType]:
@@ -1302,11 +1303,10 @@ def _make_implicit_object_type(
 name = 'q_obj_%s-%s' % (name, role)
 typ = self.lookup_entity(name)
 if typ:
-assert(isinstance(typ, QAPISchemaObjectType))
+assert isinstance(typ, QAPISchemaObjectType)
 # The implicit object type has multiple users.  This can
 # only be a duplicate definition, which will be flagged
 # later.
-pass
 else:
 self._def_definition(QAPISchemaObjectType(
 name, info, None, ifcond, None, None, members, None))
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index e766acaac92..12f92e429f6 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -280,8 +280,9 @@ def gen_visit_alternate(name: str,
 abort();
 default:
 assert(visit_is_input(v));
-error_setg(errp, "Invalid parameter type for '%%s', expected: 
%(name)s",
- name ? name : "null");
+error_setg(errp,
+   "Invalid parameter type for '%%s', expected: %(name)s",
+   name ? name : "null");
 /* Avoid passing invalid *obj to qapi_free_%(c_name)s() */
 g_free(*obj);
 *obj = NULL;
-- 
2.44.0

[PATCH 20/20] qapi: convert "Example" sections to rST

[John Snow]

Eliminate the "Example" sections in QAPI doc blocks, converting them
into QMP example code blocks. This is generally done by converting
"Example:" or "Examples:" lines into ".. code-block:: QMP" lines.

This patch does also allow for the use of the rST syntax "Example::" by
exempting double-colon syntax from the QAPI doc parser, but that form is
not used by this conversion patch. The phrase "Example" here is not
special, it is the double-colon syntax that transforms the following
block into a code-block. By default, *this* form does not apply QMP
highlighting.

This patch has several benefits:

1. Example sections can now be written more arbitrarily, mixing
   explanatory paragraphs and code blocks however desired.

2. Example sections can now use fully arbitrary rST.

3. All code blocks are now lexed and validated as QMP; increasing
   usability of the docs and ensuring validity of example snippets.

4. Each code-block can be captioned independently without bypassing the
   QMP lexer/validator.

For any sections with more than one example, examples are split up into
multiple code-block regions. If annotations are present, those
annotations are converted into code-block captions instead, e.g.

```
Examples:

   1. Lorem Ipsum

   -> { "foo": "bar" }
```

Is rewritten as:

```
.. code-block:: QMP
   :caption: Example: Lorem Ipsum

   -> { "foo": "bar" }
```

This process was only semi-automated:

1. Replace "Examples?:" sections with sed:

sed -i 's|# Example:|# .. code-block:: QMP|' *.json
sed -i 's|# Examples:|# .. code-block:: QMP|' *.json

2. Identify sections that no longer parse successfully by attempting the
   doc build, convert annotations into captions manually.
   (Tedious, oh well.)

3. Add captions where still needed:

sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n#:caption: 
Example\n#\n|g' *.json

Not fully ideal, but hopefully not something that has to be done very
often. (Or ever again.)

Signed-off-by: John Snow 
---
 qapi/acpi.json  |   6 +-
 qapi/block-core.json| 120 --
 qapi/block.json |  60 +++--
 qapi/char.json  |  36 ++--
 qapi/control.json   |  16 ++--
 qapi/dump.json  |  12 ++-
 qapi/machine-target.json|   3 +-
 qapi/machine.json   |  79 ++---
 qapi/migration.json | 145 +++-
 qapi/misc-target.json   |  33 +---
 qapi/misc.json  |  48 +++
 qapi/net.json   |  30 +--
 qapi/pci.json   |   6 +-
 qapi/qapi-schema.json   |   6 +-
 qapi/qdev.json  |  15 +++-
 qapi/qom.json   |  20 +++--
 qapi/replay.json|  12 ++-
 qapi/rocker.json|  12 ++-
 qapi/run-state.json |  45 ++
 qapi/tpm.json   |   9 +-
 qapi/trace.json |   6 +-
 qapi/transaction.json   |   3 +-
 qapi/ui.json|  62 +-
 qapi/virtio.json|  38 +
 qapi/yank.json  |   6 +-
 scripts/qapi/parser.py  |  15 +++-
 tests/qapi-schema/doc-good.json |  12 +--
 tests/qapi-schema/doc-good.out  |  17 ++--
 tests/qapi-schema/doc-good.txt  |  17 +---
 29 files changed, 574 insertions(+), 315 deletions(-)

diff --git a/qapi/acpi.json b/qapi/acpi.json
index aa4dbe57943..3da01f1b7fc 100644
--- a/qapi/acpi.json
+++ b/qapi/acpi.json
@@ -111,7 +111,8 @@
 #
 # Since: 2.1
 #
-# Example:
+# .. code-block:: QMP
+#:caption: Example
 #
 # -> { "execute": "query-acpi-ospm-status" }
 # <- { "return": [ { "device": "d1", "slot": "0", "slot-type": "DIMM", 
"source": 1, "status": 0},
@@ -131,7 +132,8 @@
 #
 # Since: 2.1
 #
-# Example:
+# .. code-block:: QMP
+#:caption: Example
 #
 # <- { "event": "ACPI_DEVICE_OST",
 #  "data": { "info": { "device": "d1", "slot": "0",
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 530af40404d..bb0447207df 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -763,7 +763,8 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. code-block:: QMP
+#:caption: Example
 #
 # -> { "execute": "query-block" }
 # <- {
@@ -1167,7 +1168,8 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. code-block:: QMP
+#:caption: Example
 #
 # -> { "execute": "query-blockstats" }
 # <- {
@@ -1460,7 +1462,8 @@
 #
 # Since: 0.14
 #
-# Example:
+# .. code-block:: QMP
+#:caption: Example
 #
 # -> { "execute": "blo

[PATCH 14/20] qapi: fix non-compliant JSON examples

[John Snow]

If we parse all examples as QMP, we need them to conform to a standard
so that they render correctly. Once the QMP lexer is active for
examples, these will produce warning messages and fail the build.

The QMP lexer still supports elisions, but they must be represented as
the value "...", so two examples have been adjusted to support that
format here.

Signed-off-by: John Snow 
---
 qapi/control.json   | 3 ++-
 qapi/machine.json   | 2 +-
 qapi/migration.json | 2 +-
 qapi/misc.json  | 3 ++-
 qapi/net.json   | 6 +++---
 qapi/rocker.json| 2 +-
 qapi/ui.json| 2 +-
 7 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/qapi/control.json b/qapi/control.json
index 6bdbf077c2e..10c906fa0e7 100644
--- a/qapi/control.json
+++ b/qapi/control.json
@@ -145,7 +145,8 @@
 # },
 # {
 #"name":"system_powerdown"
-# }
+# },
+# ...
 #  ]
 #}
 #
diff --git a/qapi/machine.json b/qapi/machine.json
index bce6e1bbc41..64a77557571 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1057,7 +1057,7 @@
 #"vcpus-count": 1 },
 #  { "props": { "core-id": 0 }, "type": "POWER8-spapr-cpu-core",
 #"vcpus-count": 1, "qom-path": "/machine/unattached/device[0]"}
-#]}'
+#]}
 #
 # For pc machine type started with -smp 1,maxcpus=2:
 #
diff --git a/qapi/migration.json b/qapi/migration.json
index a351fd37143..89047d46c7c 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -2078,7 +2078,7 @@
 # Example:
 #
 # -> {"execute": "calc-dirty-rate", "arguments": {"calc-time": 1,
-# 'sample-pages': 512} }
+# "sample-pages": 512} }
 # <- { "return": {} }
 #
 # Measure dirty rate using dirty bitmap for 500 milliseconds:
diff --git a/qapi/misc.json b/qapi/misc.json
index ec30e5c570a..4b41e15dcd4 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -287,7 +287,8 @@
 #
 # Example:
 #
-# -> { "execute": "get-win32-socket", "arguments": { "info": "abcd123..", 
fdname": "skclient" } }
+# -> { "execute": "get-win32-socket",
+#  "arguments": { "info": "abcd123..", "fdname": "skclient" } }
 # <- { "return": {} }
 ##
 { 'command': 'get-win32-socket', 'data': {'info': 'str', 'fdname': 'str'}, 
'if': 'CONFIG_WIN32' }
diff --git a/qapi/net.json b/qapi/net.json
index 0f5a259475e..c19df435a53 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -1003,9 +1003,9 @@
 #
 # Example:
 #
-# <- { 'event': 'NETDEV_STREAM_DISCONNECTED',
-#  'data': {'netdev-id': 'netdev0'},
-#  'timestamp': {'seconds': 1663330937, 'microseconds': 526695} }
+# <- { "event": "NETDEV_STREAM_DISCONNECTED",
+#  "data": {"netdev-id": "netdev0"},
+#  "timestamp": {"seconds": 1663330937, "microseconds": 526695} }
 ##
 { 'event': 'NETDEV_STREAM_DISCONNECTED',
   'data': { 'netdev-id': 'str' } }
diff --git a/qapi/rocker.json b/qapi/rocker.json
index 5635cf174fd..f5225eb62cc 100644
--- a/qapi/rocker.json
+++ b/qapi/rocker.json
@@ -250,7 +250,7 @@
 #   "action": {"goto-tbl": 10},
 #   "mask": {"in-pport": 4294901760}
 #  },
-#  {...more...},
+#  {...},
 #]}
 ##
 { 'command': 'query-rocker-of-dpa-flows',
diff --git a/qapi/ui.json b/qapi/ui.json
index f610bce118a..c12f5292571 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -361,7 +361,7 @@
 #"channel-id": 0,
 #"tls": false
 # },
-# [ ... more channels follow ... ]
+# ...
 #  ]
 #   }
 #}
-- 
2.44.0

[PATCH 15/20] qapi: remove developer factoring comments from QAPI doc blocks

[John Snow]

This is part of a project to overhaul the QMP reference manual. One goal
of this overhaul is to "inline" inherited argument sections into command
reference sections. A consequence of this design decision is that
inherited doc block sections need to be merged with the inheritor's
sections.

When documentation is written for types whose primary purpose is to be
inherited by other types, we need to know how to merge those paragraphs
into the descendent. Much of the time, these paragraphs aren't actually
useful to end users.

Either remove information that's of little to no use to *either*
audience, and convert what's left to garden-variety comments to prevent
it from showing up in rendered documentation.

Signed-off-by: John Snow 
---
 qapi/audio.json|  5 ++---
 qapi/block-core.json   | 47 ++
 qapi/block-export.json | 10 -
 qapi/char.json |  5 ++---
 qapi/crypto.json   | 33 -
 qapi/machine.json  | 10 -
 qapi/net.json  |  7 ++-
 qapi/qom.json  | 30 +++
 qapi/ui.json   | 14 -
 9 files changed, 59 insertions(+), 102 deletions(-)

diff --git a/qapi/audio.json b/qapi/audio.json
index 519697c0cd8..ee09cd231b6 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -10,11 +10,10 @@
 # = Audio
 ##
 
-##
-# @AudiodevPerDirectionOptions:
-#
 # General audio backend options that are used for both playback and
 # recording.
+##
+# @AudiodevPerDirectionOptions:
 #
 # @mixing-engine: use QEMU's mixing engine to mix all streams inside
 # QEMU and convert audio formats when not supported by the
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 746d1694c25..64fe5240cc9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -267,10 +267,9 @@
   'file': 'ImageInfoSpecificFileWrapper'
   } }
 
-##
-# @BlockNodeInfo:
-#
 # Information about a QEMU image file
+##
+# @BlockNodeInfo:
 #
 # @filename: name of the image file
 #
@@ -1494,8 +1493,6 @@
 ##
 # @BlockdevSnapshotSync:
 #
-# Either @device or @node-name must be set but not both.
-#
 # @device: the name of the device to take a snapshot of.
 #
 # @node-name: graph node name to generate the snapshot from (Since
@@ -1512,6 +1509,9 @@
 #
 # @mode: whether and how QEMU should create a new image, default is
 # 'absolute-paths'.
+#
+# Note: Either @device or @node-name must be set but not both.
+#
 ##
 { 'struct': 'BlockdevSnapshotSync',
   'data': { '*device': 'str', '*node-name': 'str',
@@ -2139,10 +2139,9 @@
   'data': 'DriveMirror',
   'allow-preconfig': true }
 
-##
-# @DriveMirror:
-#
 # A set of parameters describing drive mirror setup.
+##
+# @DriveMirror:
 #
 # @job-id: identifier for the newly-created block job.  If omitted,
 # the device name will be used.  (Since 2.7)
@@ -2553,10 +2552,9 @@
 '*auto-finalize': 'bool', '*auto-dismiss': 'bool' },
   'allow-preconfig': true }
 
-##
-# @BlockIOThrottle:
-#
 # A set of parameters describing block throttling.
+##
+# @BlockIOThrottle:
 #
 # @device: Block device name
 #
@@ -3073,10 +3071,9 @@
 { 'struct': 'BlockJobChangeOptionsMirror',
   'data': { 'copy-mode' : 'MirrorCopyMode' } }
 
-##
-# @BlockJobChangeOptions:
-#
 # Block job options that can be changed after job creation.
+##
+# @BlockJobChangeOptions:
 #
 # @id: The job identifier
 #
@@ -3332,11 +3329,10 @@
   'data': { 'dir': 'str', '*fat-type': 'int', '*floppy': 'bool',
 '*label': 'str', '*rw': 'bool' } }
 
-##
-# @BlockdevOptionsGenericFormat:
-#
 # Driver specific block device options for image format that have no
 # option besides their data source.
+##
+# @BlockdevOptionsGenericFormat:
 #
 # @file: reference to or definition of the data source block device
 #
@@ -3363,11 +3359,10 @@
   'data': { '*key-secret': 'str',
 '*header': 'BlockdevRef'} }
 
-##
-# @BlockdevOptionsGenericCOWFormat:
-#
 # Driver specific block device options for image format that have no
 # option besides their data source and an optional backing file.
+##
+# @BlockdevOptionsGenericCOWFormat:
 #
 # @backing: reference to or definition of the backing file block
 # device, null disables the backing file entirely.  Defaults to
@@ -4385,11 +4380,10 @@
 '*page-cache-size': 'int',
 '*debug': 'int' } }
 
-##
-# @BlockdevOptionsCurlBase:
-#
 # Driver specific block device options shared by all protocols
 # supported by the curl backend.
+##
+# @BlockdevOptionsCurlBase:
 #
 # @url: URL of the image file
 #
@@ -4645,11 +4639,10 @@
   'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap',
 '*on-cbw-error': 'OnCbwError', '*cbw-timeout': 'uint32' } }
 
-##
-# @BlockdevOptions:
-#
 # Options for creating a block device.  Many options are available for
-# all block devices, independent of the block driver:
+# all block devices, independent of the block driver.
+##
+# @BlockdevOptions:
 #
 # @driver: block d

[PATCH 17/20] qapi: rewrite BlockExportOptions doc block

[John Snow]

Rephrase this paragraph so that it can apply to any commands that
inherit from this object.

Signed-off-by: John Snow 
---
 qapi/block-export.json | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index dc328097a94..550763a9f6a 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -342,9 +342,6 @@
 ##
 # @BlockExportOptions:
 #
-# Describes a block export, i.e. how single node should be exported on
-# an external interface.
-#
 # @type: Block export type
 #
 # @id: A unique identifier for the block export (across all export
@@ -396,6 +393,9 @@
 #
 # Creates a new block export.
 #
+# Arguments describe a block export, i.e. how single node should be
+# exported on an external interface.
+#
 # Since: 5.2
 ##
 { 'command': 'block-export-add',
-- 
2.44.0

[PATCH 03/20] docs/qapidoc: delint a tiny portion of the module

[John Snow]

In the coming patches, it's helpful to have a linting baseline. However,
there's no need to shuffle around the deck chairs too much, because most
of this code will be removed once the new qapidoc generator (the
"transmogrifier") is in place.

To ease my pain: just turn off the black auto-formatter for most, but
not all, of qapidoc.py. This will help ensure that *new* code follows a
coding standard without bothering too much with cleaning up the existing
code.

For manual checking for now, try "black --check qapidoc.py" from the
docs/sphinx directory. "pip install black" (without root permissions) if
you do not have it installed otherwise.

Signed-off-by: John Snow 
---
 docs/sphinx/qapidoc.py | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index f270b494f01..1655682d4c7 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -28,28 +28,30 @@
 import re
 
 from docutils import nodes
+from docutils.parsers.rst import Directive, directives
 from docutils.statemachine import ViewList
-from docutils.parsers.rst import directives, Directive
-from sphinx.errors import ExtensionError
-from sphinx.util.nodes import nested_parse_with_titles
-import sphinx
-from qapi.gen import QAPISchemaVisitor
 from qapi.error import QAPIError, QAPISemError
+from qapi.gen import QAPISchemaVisitor
 from qapi.schema import QAPISchema
 
+import sphinx
+from sphinx.errors import ExtensionError
+from sphinx.util.nodes import nested_parse_with_titles
+
 
 # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
 # use switch_source_input. Check borrowed from kerneldoc.py.
-Use_SSI = sphinx.__version__[:3] >= '1.7'
+Use_SSI = sphinx.__version__[:3] >= "1.7"
 if Use_SSI:
 from sphinx.util.docutils import switch_source_input
 else:
 from sphinx.ext.autodoc import AutodocReporter
 
 
-__version__ = '1.0'
+__version__ = "1.0"
 
 
+# fmt: off
 # Function borrowed from pydash, which is under the MIT license
 def intersperse(iterable, separator):
 """Yield the members of *iterable* interspersed with *separator*."""
-- 
2.44.0

[PATCH 00/20] qapi: new sphinx qapi domain pre-requisites

[John Snow]

Howdy - this patch series is the first batch of patches meant to prepare
the QAPI documentation for a new Sphinx module that adds
cross-references, an index, improved inlining, elision of types unseen
on the wire, and other goodies.

This series addresses just existing code and documentation that needs to
be changed and doesn't introduce anything new just yet - except the rST
conversion of Notes and Examples sections, which DOES impact the
existing QAPI documentation generation.

If you're CC'd on this series, it's *probably* because I've adjusted
some QAPI documentation that you're the maintainer of - In most cases,
these changes are purely mechanical (converting QAPI sections into pure
rST) and probably nothing too interesting. In a small handful of cases
(patches 15-17), I've been a bit more invasive and you may want to take
a quick peek.

Overview:

Patches 1-3: linter/typing cleanup
Patches 4-12: QAPI generator fixes/miscellany
Patch 13: qapidoc.py fix (to prepare for rST conversion)
Patches 14-20: QAPI documentation modifications, rST conversion

Sorry,
--js

John Snow (20):
  [DO-NOT-MERGE]: Add some ad-hoc linting helpers.
  qapi: linter fixups
  docs/qapidoc: delint a tiny portion of the module
  qapi/parser: preserve indentation in QAPIDoc sections
  qapi/parser: adjust info location for doc body section
  qapi/parser: fix comment parsing immediately following a doc block
  qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section
  qapi/parser: differentiate intro and outro paragraphs
  qapi/parser: add undocumented stub members to all_sections
  qapi/schema: add __iter__ method to QAPISchemaVariants
  qapi/schema: add doc_visible property to QAPISchemaDefinition
  qapi/source: allow multi-line QAPISourceInfo advancing
  docs/qapidoc: fix nested parsing under untagged sections
  qapi: fix non-compliant JSON examples
  qapi: remove developer factoring comments from QAPI doc blocks
  qapi: rewrite StatsFilter comment
  qapi: rewrite BlockExportOptions doc block
  qapi: ensure all errors sections are uniformly typset
  qapi: convert "Note" sections to plain rST
  qapi: convert "Example" sections to rST

 docs/sphinx/qapidoc.py|  62 --
 qapi/acpi.json|   6 +-
 qapi/audio.json   |   5 +-
 qapi/block-core.json  | 195 ++
 qapi/block-export.json|  16 +-
 qapi/block.json   |  62 +++---
 qapi/char.json|  53 +++--
 qapi/control.json |  32 +--
 qapi/crypto.json  |  33 ++-
 qapi/dump.json|  14 +-
 qapi/introspect.json  |   6 +-
 qapi/machine-target.json  |  29 +--
 qapi/machine.json | 138 +++--
 qapi/migration.json   | 159 +-
 qapi/misc-target.json |  33 ++-
 qapi/misc.json| 139 +++--
 qapi/net.json |  49 +++--
 qapi/pci.json |  11 +-
 qapi/qapi-schema.json |   6 +-
 qapi/qdev.json|  45 ++--
 qapi/qom.json |  69 +++
 qapi/replay.json  |  12 +-
 qapi/rocker.json  |  30 +--
 qapi/run-state.json   |  63 +++---
 qapi/sockets.json |  10 +-
 qapi/stats.json   |  30 ++-
 qapi/tpm.json |   9 +-
 qapi/trace.json   |   6 +-
 qapi/transaction.json |  13 +-
 qapi/ui.json  | 107 +-
 qapi/virtio.json  |  50 ++---
 qapi/yank.json|   6 +-
 qga/qapi-schema.json  |  48 ++---
 scripts/qapi-lint.sh  |  51 +
 scripts/qapi/Makefile |   5 +
 scripts/qapi/introspect.py|  12 +-
 scripts/qapi/parser.py| 104 --
 scripts/qapi/schema.py|  54 -
 scripts/qapi/source.py|   4 +-
 scripts/qapi/types.py |   4 +-
 scripts/qapi/visit.py |   9 +-
 tests/qapi-schema/doc-empty-section.err   |   2 +-
 tests/qapi-schema/doc-empty-section.json  |   2 +-
 tests/qapi-schema/doc-good.json   |  18 +-
 tests/qapi-schema/doc-good.out|  61 +++---
 tests/qapi-schema/doc-good.txt|  31 +--
 .../qapi-schema/doc-interleaved-section.json  |   2 +-
 47 files changed, 1152 insertions(+), 753 deletions(-)
 create mode 100755 

[PATCH 13/20] docs/qapidoc: fix nested parsing under untagged sections

[John Snow]

Sphinx does not like sections without titles, because it wants to
convert every section into a reference. When there is no title, it
struggles to do this and transforms the tree inproperly.

Depending on the rST used, this may result in an assertion error deep in
the docutils HTMLWriter.

When parsing an untagged section (free paragraphs), skip making a hollow
section and instead append the parse results to the prior section.

Many Bothans died to bring us this information.

Signed-off-by: John Snow 
---
 docs/sphinx/qapidoc.py | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 34e95bd168d..cfc0cf169ef 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -286,14 +286,20 @@ def _nodes_for_sections(self, doc):
 if section.tag and section.tag == 'TODO':
 # Hide TODO: sections
 continue
+
+if not section.tag:
+# Sphinx cannot handle sectionless titles;
+# Instead, just append the results to the prior section.
+container = nodes.container()
+self._parse_text_into_node(section.text, container)
+nodelist += container.children
+continue
+
 snode = self._make_section(section.tag)
-if section.tag and section.tag.startswith('Example'):
+if section.tag.startswith('Example'):
 snode += self._nodes_for_example(dedent(section.text))
 else:
-self._parse_text_into_node(
-dedent(section.text) if section.tag else section.text,
-snode,
-)
+self._parse_text_into_node(dedent(section.text), snode)
 nodelist.append(snode)
 return nodelist
 
-- 
2.44.0

[PATCH 10/20] qapi/schema: add __iter__ method to QAPISchemaVariants

[John Snow]

This just 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 
---
 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 2e3ffcbafb7..34e95bd168d 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -204,7 +204,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 d65c35f6ee6..e15e64ea8cb 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,
 ''',
  

[PATCH 12/20] 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.

Signed-off-by: John Snow 
---
 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.44.0

Re: [PATCH 00/27] Add qapi-domain Sphinx extension

[John Snow]

On Tue, Apr 23, 2024, 3:48 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > On Mon, Apr 22, 2024 at 12:38 PM John Snow  wrote:
> >>
> >> On Mon, Apr 22, 2024 at 5:20 AM Markus Armbruster 
> wrote:
> >> >
> >> > John Snow  writes:
> >> >
> >> > > On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster 
> wrote:
> >> > >
> >> > >> John Snow  writes:
> >> > >>
> >> > >> > This series adds a new qapi-domain extension for Sphinx, which
> adds a
> >> > >> > series of custom directives for documenting QAPI definitions.
>
> [...]
>
> >> > >> > Known issues / points of interest:
>
> [...]
>
> >> > >> > - Inlining all members means there is some ambiguity on what to
> do with
> >> > >> >   doc block sections on inlined entities; features and members
> have an
> >> > >> >   obvious home - body, since, and misc sections are not as
> obvious on
> >> > >> >   how to handle. This will feature more prominently in the
> generator
> >> > >> >   series.
> >> > >>
> >> > >> Yes, this is a real problem.
> >> > >>
> >> > >> The member documentation gets written in the context of the type.
> It
> >> > >> may make sense only in that context.  Inlining copies it into a
> >> > >> different context.
> >> > >>
> >> > >> Features may need to combine.  Say a command's arguments are a
> union
> >> > >> type, and several branches of the union both contain deprecated
> members.
> >> > >> These branch types all document feature @deprecated.  Simply
> inlining
> >> > >> their feature documentation results in feature @deprecated
> documented
> >> > >> several times.  Ugh.  Combining them would be better.  But how?
> Do we
> >> > >> need to rethink how we document features?
> >> > >
> >> > > To be honest, I figured I'd plow ahead and then find the
> counter-examples
> >> > > programmatically and decide what to do once I had my pile of edge
> cases.
> >> > >
> >> > > It might involve rewriting docs in some places, but I think the
> usability
> >> > > win is completely worth the hassle.
> >> > >
> >> > > It's possible there might be some rework needed to maximize cogency
> of the
> >> > > generated docs, but I think prioritizing a user-facing reference
> for QMP is
> >> > > the objectively correct end goal and I'm more than happy to work
> backwards
> >> > > from that desired state.
> >> >
> >> > I'm not disputing the idea that documenting the arguments right with
> the
> >> > command is good.  I'm merely pointing out obstacles to pulling that
> off.
> >> >
> >> > Adjusting existing documentation is only half the battle.  The other
> >> > half is making sure documentation stays adjusted.  We may have to come
> >> > up with new documentation rules, and ways to enforce them.
> >>
> >> For the sake of argument, let's say we forbid everything except
> >> arg/features from definitions destined to be used as base/inherited
> >> types. This would be very easy to enforce at the qapidoc level where
> >> the doc inlining is performed by yelping when the base type contains
> >> additional documentation sections.
> >>
> >> Now, in the real world, maybe sometimes those sections are useful and
> >> we don't want to get rid of them, esp. because they may contain useful
> >> documentation that we don't want to duplicate in the source files.
> >>
> >> My plan is to just forbid them at first and enumerate the cases where
> >> they occur, then decide which ones are better off being moved
> >> elsewhere or explicitly tolerated. The generator's tolerance can be
> >> adjusted accordingly and we can formulate a rule for exactly how doc
> >> blocks are combined and merged. I think it won't be a problem to
> >> enforce it programmatically.
> >>
> >> I'll get back to you on how often and precisely where these cases
> >> occur so you can take a look and see how you feel.
> >>
> >
> > For a warmup, let's look at every unique instance of non-empty
> > paragraph text on an object that is used as a base anywhere:
> >

Re: [PATCH 00/27] Add qapi-domain Sphinx extension

[John Snow]

On Mon, Apr 22, 2024 at 12:38 PM John Snow  wrote:
>
> On Mon, Apr 22, 2024 at 5:20 AM Markus Armbruster  wrote:
> >
> > John Snow  writes:
> >
> > > On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster  
> > > wrote:
> > >
> > >> John Snow  writes:
> > >>
> > >> > This series adds a new qapi-domain extension for Sphinx, which adds a
> > >> > series of custom directives for documenting QAPI definitions.
> > >> >
> > >> > GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/1259566476
> > >> >
> > >> > (Link to a demo HTML page at the end of this cover letter, but I want
> > >> > you to read the cover letter first to explain what you're seeing.)
> > >> >
> > >> > This adds a new QAPI Index page, cross-references for QMP commands,
> > >> > events, and data types, and improves the aesthetics of the QAPI/QMP
> > >> > documentation.
> > >>
> > >> Cross-references alone will be a *massive* improvement!  I'm sure
> > >> readers will appreciate better looks and an index, too.
> > >>
> > >> > This series adds only the new ReST syntax, *not* the autogenerator. The
> > >> > ReST syntax used in this series is, in general, not intended for anyone
> > >> > to actually write by hand. This mimics how Sphinx's own autodoc
> > >> > extension generates Python domain directives, which are then re-parsed
> > >> > to produce the final result.
> > >> >
> > >> > I have prototyped such a generator, but it isn't ready for inclusion
> > >> > yet. (Rest assured: error context reporting is preserved down to the
> > >> > line, even in generated ReST. There is no loss in usability for this
> > >> > approach. It will likely either supplant qapidoc.py or heavily alter
> > >> > it.) The generator requires only extremely minor changes to
> > >> > scripts/qapi/parser.py to preserve nested indentation and provide more
> > >> > accurate line information. It is less invasive than you may
> > >> > fear. Relying on a secondary ReST parse phase eliminates much of the
> > >> > complexity of qapidoc.py. Sleep soundly.
> > >>
> > >> I'm a Sphinx noob.  Let me paraphrase you to make sure I understand.
> > >>
> > >> You proprose to generate formatted documentation in two steps:
> > >>
> > >> • First, the QAPI generator generates .rst from the QAPI schema.  The
> > >>   generated .rst makes use of a custom directives.
> > >>
> > >
> > > Yes, but this .rst file is built in-memory and never makes it to disk, 
> > > like
> > > Sphinx's autodoc for Python.
> >
> > Makes sense.
> >
> > > (We can add a debug knob to log it or save it out to disk if needed.)
> >
> > Likely useful at least occasionally.
>
> Yep, python's autodoc has such a knob to use the debugging log for
> this. I just want to point out that avoiding the intermediate file
> on-disk is actually the mechanism by which I can preserve source
> lines, so this is how it's gotta be.
>
> I build an intermediate doc in-memory with source filenames and source
> lines along with the modified doc block content so that ReST errors
> can be tracked back directly to the QAPI json files. If we saved to
> disk and parsed that, it'd obliterate that information.
>
> >
> > >> • Second, Sphinx turns the .rst into formatted documentation.  A Sphinx
> > >>   qapi-domain extension implements the custom directives
> > >
> > > Yes.
> > >
> > >
> > >> This mirrors how Sphinx works for Python docs.  Which is its original
> > >> use case.
> > >>
> > >> Your series demonstrates the second step, with test input you wrote
> > >> manually.
> > >>
> > >> You have code for the first step, but you'd prefer to show it later.
> > >
> > > Right, it's not fully finished, although I have events, commands, and
> > > objects working. Unions, Alternates and Events need work.
> > >
> > >
> > >> Fair?
> > >
> > > Bingo!
> >
> > Thanks!
> >
> > >> > The purpose of sending this series in its current form is largely to
> > >> > solicit feedback on general aesthetics, layout, and features. Sphinx is
> > >> > a wily beast, and feedback at this stage will di

Re: [PATCH 00/27] Add qapi-domain Sphinx extension

[John Snow]

On Mon, Apr 22, 2024 at 5:20 AM Markus Armbruster  wrote:
>
> John Snow  writes:
>
> > On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster  wrote:
> >
> >> John Snow  writes:
> >>
> >> > This series adds a new qapi-domain extension for Sphinx, which adds a
> >> > series of custom directives for documenting QAPI definitions.
> >> >
> >> > GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/1259566476
> >> >
> >> > (Link to a demo HTML page at the end of this cover letter, but I want
> >> > you to read the cover letter first to explain what you're seeing.)
> >> >
> >> > This adds a new QAPI Index page, cross-references for QMP commands,
> >> > events, and data types, and improves the aesthetics of the QAPI/QMP
> >> > documentation.
> >>
> >> Cross-references alone will be a *massive* improvement!  I'm sure
> >> readers will appreciate better looks and an index, too.
> >>
> >> > This series adds only the new ReST syntax, *not* the autogenerator. The
> >> > ReST syntax used in this series is, in general, not intended for anyone
> >> > to actually write by hand. This mimics how Sphinx's own autodoc
> >> > extension generates Python domain directives, which are then re-parsed
> >> > to produce the final result.
> >> >
> >> > I have prototyped such a generator, but it isn't ready for inclusion
> >> > yet. (Rest assured: error context reporting is preserved down to the
> >> > line, even in generated ReST. There is no loss in usability for this
> >> > approach. It will likely either supplant qapidoc.py or heavily alter
> >> > it.) The generator requires only extremely minor changes to
> >> > scripts/qapi/parser.py to preserve nested indentation and provide more
> >> > accurate line information. It is less invasive than you may
> >> > fear. Relying on a secondary ReST parse phase eliminates much of the
> >> > complexity of qapidoc.py. Sleep soundly.
> >>
> >> I'm a Sphinx noob.  Let me paraphrase you to make sure I understand.
> >>
> >> You proprose to generate formatted documentation in two steps:
> >>
> >> • First, the QAPI generator generates .rst from the QAPI schema.  The
> >>   generated .rst makes use of a custom directives.
> >>
> >
> > Yes, but this .rst file is built in-memory and never makes it to disk, like
> > Sphinx's autodoc for Python.
>
> Makes sense.
>
> > (We can add a debug knob to log it or save it out to disk if needed.)
>
> Likely useful at least occasionally.

Yep, python's autodoc has such a knob to use the debugging log for
this. I just want to point out that avoiding the intermediate file
on-disk is actually the mechanism by which I can preserve source
lines, so this is how it's gotta be.

I build an intermediate doc in-memory with source filenames and source
lines along with the modified doc block content so that ReST errors
can be tracked back directly to the QAPI json files. If we saved to
disk and parsed that, it'd obliterate that information.

>
> >> • Second, Sphinx turns the .rst into formatted documentation.  A Sphinx
> >>   qapi-domain extension implements the custom directives
> >
> > Yes.
> >
> >
> >> This mirrors how Sphinx works for Python docs.  Which is its original
> >> use case.
> >>
> >> Your series demonstrates the second step, with test input you wrote
> >> manually.
> >>
> >> You have code for the first step, but you'd prefer to show it later.
> >
> > Right, it's not fully finished, although I have events, commands, and
> > objects working. Unions, Alternates and Events need work.
> >
> >
> >> Fair?
> >
> > Bingo!
>
> Thanks!
>
> >> > The purpose of sending this series in its current form is largely to
> >> > solicit feedback on general aesthetics, layout, and features. Sphinx is
> >> > a wily beast, and feedback at this stage will dictate how and where
> >> > certain features are implemented.
> >>
> >> I'd appreciate help with that.  Opinions?
> >
> >
> >> > A goal for this syntax (and the generator) is to fully in-line all
> >> > command/event/object members, inherited or local, boxed or not, branched
> >> > or not. This should provide a boon to using these docs as a reference,
> >> > because users will not have to grep around the page looking for various
> >> > types, branche

Re: [PATCH 24/27] docs/qapi-domain: add type cross-refs to field lists

[John Snow]

On Fri, Apr 19, 2024 at 12:38 AM John Snow  wrote:
>
> This commit, finally, adds cross-referencing support to various field
> lists; modeled tightly after Sphinx's own Python domain code.
>
> Cross-referencing support is added to type names provided to :arg:,
> :memb:, :returns: and :choice:.
>
> :feat:, :error: and :value:, which do not take type names, do not
> support this syntax.
>
> The general syntax is simple:
>
> :arg TypeName ArgName: Lorem Ipsum ...
>
> The domain will transform TypeName into :qapi:type:`TypeName` in this
> basic case, and also apply the ``literal`` decoration to indicate that
> this is a type cross-reference.
>
> For Optional arguments, the special "?" suffix is used. Because "*" has
> special meaning in ReST that would cause parsing errors, we elect to use
> "?" instead. The special syntax processing in QAPIXrefMixin strips this
> character from the end of any type name argument and will append ",
> Optional" to the rendered output, applying the cross-reference only to
> the actual type name.
>
> The intent here is that the actual syntax in doc-blocks need not change;
> but e.g. qapidoc.py will need to process and transform "@arg foo lorem
> ipsum" into ":arg type? foo: lorem ipsum" based on the schema
> information. Therefore, nobody should ever actually witness this
> intermediate syntax unless they are writing manual documentation or the
> doc transmogrifier breaks.
>
> For array arguments, type names can similarly be surrounded by "[]",
> which are stripped off and then re-appended outside of the
> cross-reference.
>
> Note: The mixin pattern here (borrowed from Sphinx) confuses mypy
> because it cannot tell that it will be mixed into a descendent of
> Field. Doing that instead causes more errors, because many versions of
> Sphinx erroneously did not mark various arguments as Optional, so we're
> a bit hosed either way. Do the simpler thing.
>
> Signed-off-by: John Snow 
> ---
>  docs/qapi/index.rst|  34 
>  docs/sphinx/qapi-domain.py | 110 +++--
>  2 files changed, 138 insertions(+), 6 deletions(-)
>
> diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
> index 8352a27d4a5..6e85ea5280d 100644
> --- a/docs/qapi/index.rst
> +++ b/docs/qapi/index.rst
> @@ -105,6 +105,11 @@ Explicit cross-referencing syntax for QAPI modules is 
> available with
> :arg str bar: Another normal parameter description.
> :arg baz: Missing a type.
> :arg no-descr:
> +   :arg int? oof: Testing optional argument parsing.
> +   :arg [XDbgBlockGraphNode] rab: Testing array argument parsing.
> +   :arg [BitmapSyncMode]? zab: Testing optional array argument parsing,
> +  even though Markus said this should never happen. I believe him,
> +  but I didn't *forbid* the syntax either.
> :arg BitmapSyncMode discrim: How about branches in commands?
>
> .. qapi:branch:: discrim on-success
> @@ -261,3 +266,32 @@ Explicit cross-referencing syntax for QAPI modules is 
> available with
>
>:memb str key-secret: ID of a QCryptoSecret object providing a
>   passphrase for unlocking the encryption
> +
> +.. qapi:command:: x-debug-query-block-graph
> +   :since: 4.0
> +   :unstable:
> +
> +   Get the block graph.
> +
> +   :feat unstable: This command is meant for debugging.
> +   :return XDbgBlockGraph: lorem ipsum ...
> +
> +.. qapi:struct:: XDbgBlockGraph
> +   :since: 4.0
> +
> +   Block Graph - list of nodes and list of edges.
> +
> +   :memb [XDbgBlockGraphNode] nodes:
> +   :memb [XDbgBlockGraphEdge] edges:
> +
> +.. qapi:struct:: XDbgBlockGraphNode
> +   :since: 4.0
> +
> +   :memb uint64 id: Block graph node identifier.  This @id is generated only 
> for
> +  x-debug-query-block-graph and does not relate to any other
> +  identifiers in Qemu.
> +   :memb XDbgBlockGraphNodeType type: Type of graph node.  Can be one of
> +  block-backend, block-job or block-driver-state.
> +   :memb str name: Human readable name of the node.  Corresponds to
> +  node-name for block-driver-state nodes; is not guaranteed to be
> +  unique in the whole graph (with block-jobs and block-backends).
> diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
> index bf8bb933345..074453193ce 100644
> --- a/docs/sphinx/qapi-domain.py
> +++ b/docs/sphinx/qapi-domain.py
> @@ -50,11 +50,12 @@
>
>  if TYPE_CHECKING:
>  from docutils.nodes import Element, Node
> +from docutils.parsers.rst.states import Inliner
>
>  from sphinx.application import Sphinx
>  from sphinx.builders import B

Re: [PATCH 00/27] Add qapi-domain Sphinx extension

[John Snow]

On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > This series adds a new qapi-domain extension for Sphinx, which adds a
> > series of custom directives for documenting QAPI definitions.
> >
> > GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/1259566476
> >
> > (Link to a demo HTML page at the end of this cover letter, but I want
> > you to read the cover letter first to explain what you're seeing.)
> >
> > This adds a new QAPI Index page, cross-references for QMP commands,
> > events, and data types, and improves the aesthetics of the QAPI/QMP
> > documentation.
>
> Cross-references alone will be a *massive* improvement!  I'm sure
> readers will appreciate better looks and an index, too.
>
> > This series adds only the new ReST syntax, *not* the autogenerator. The
> > ReST syntax used in this series is, in general, not intended for anyone
> > to actually write by hand. This mimics how Sphinx's own autodoc
> > extension generates Python domain directives, which are then re-parsed
> > to produce the final result.
> >
> > I have prototyped such a generator, but it isn't ready for inclusion
> > yet. (Rest assured: error context reporting is preserved down to the
> > line, even in generated ReST. There is no loss in usability for this
> > approach. It will likely either supplant qapidoc.py or heavily alter
> > it.) The generator requires only extremely minor changes to
> > scripts/qapi/parser.py to preserve nested indentation and provide more
> > accurate line information. It is less invasive than you may
> > fear. Relying on a secondary ReST parse phase eliminates much of the
> > complexity of qapidoc.py. Sleep soundly.
>
> I'm a Sphinx noob.  Let me paraphrase you to make sure I understand.
>
> You proprose to generate formatted documentation in two steps:
>
> • First, the QAPI generator generates .rst from the QAPI schema.  The
>   generated .rst makes use of a custom directives.
>

Yes, but this .rst file is built in-memory and never makes it to disk, like
Sphinx's autodoc for Python.

(We can add a debug knob to log it or save it out to disk if needed.)


> • Second, Sphinx turns the .rst into formatted documentation.  A Sphinx
>   qapi-domain extension implements the custom directives
>

Yes.


> This mirrors how Sphinx works for Python docs.  Which is its original
> use case.
>
> Your series demonstrates the second step, with test input you wrote
> manually.
>
> You have code for the first step, but you'd prefer to show it later.
>

Right, it's not fully finished, although I have events, commands, and
objects working. Unions, Alternates and Events need work.


> Fair?
>

Bingo!


> > The purpose of sending this series in its current form is largely to
> > solicit feedback on general aesthetics, layout, and features. Sphinx is
> > a wily beast, and feedback at this stage will dictate how and where
> > certain features are implemented.
>
> I'd appreciate help with that.  Opinions?


> > A goal for this syntax (and the generator) is to fully in-line all
> > command/event/object members, inherited or local, boxed or not, branched
> > or not. This should provide a boon to using these docs as a reference,
> > because users will not have to grep around the page looking for various
> > types, branches, or inherited members. Any arguments types will be
> > hyperlinked to their definition, further aiding usability. Commands can
> > be hotlinked from anywhere else in the manual, and will provide a
> > complete reference directly on the first screenful of information.
>
> Let me elaborate a bit here.
>
> A command's arguments can be specified inline, like so:
>
> { 'command': 'job-cancel', 'data': { 'id': 'str' } }
>
> The arguments are then documented right with the command.
>
> But they can also be specified by referencing an object type, like so:
>
> { 'command': 'block-dirty-bitmap-remove',
>   'data': 'BlockDirtyBitmap' }
>
> Reasons for doing it this way:
>
> • Several commands take the same arguments, and you don't want to repeat
>   yourself.
>
> • You want generated C take a single struct argument ('boxed': true).
>
> • The arguments are a union (which requires 'boxed': true).
>
> Drawback: the arguments are then documented elsewhere.  Not nice.
>
> Bug: the generated documentation fails to point there.
>
> You're proposing to inline the argument documentation, so it appears
> right with the command.
>
> An event's data is just like a command's argument.
>
> A command's return value can only specified by referencing a type.  Same
> doc usability 

[PATCH 21/27] docs/qapi-domain: RFC patch - add malformed field list entries

[John Snow]

This patch demonstrates what happens when you mess up a field list
entry. The next patch adds a safeguard against this.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst | 4 
 1 file changed, 4 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index c68e2044890..5bb1c37a5ed 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -129,6 +129,10 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
   argument values. It's very temperamental.
:return SomeTypeName: An esoteric collection of mystical nonsense to
   both confound and delight.
+   :arg: this is malformed.
+   :memb: this is malformed and unrecognized.
+   :choice type name: This is unrecognized.
+   :errors FooError: Also malformed.
 
Field lists can appear anywhere in the directive block, but any field
list entries in the same list block that are recognized as special
-- 
2.44.0

[PATCH 11/27] docs/qapi-domain: add "Errors:" field lists

[John Snow]

``:error type: descr`` can now be used to document error conditions,
naming the type of error object and a description of when the error is
surfaced.

Like the previous Arguments patch, this patch does not apply any special
QAPI syntax highlighting or cross-referencing for the types, but this
can be adjusted in the future if desired.

(At present, I have no commits that add such highlighting. Sphinx also
does not appear to support Grouped fields with optional (or no)
parameters, so the ability to exclude error types is currently not
supported. If you omit the type, Sphinx treats it as a regular field
list and doesn't apply the special Grouping postprocessing to it.)

Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 4 
 docs/sphinx/qapi-domain.py | 6 ++
 2 files changed, 10 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index a570c37abb2..004d02e0437 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -98,6 +98,10 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
:feat unstable: More than unstable, this command doesn't even exist!
:arg no-descr:
:feat hallucination: This command is a figment of your imagination.
+   :error CommandNotFound: When you try to use this command, because it
+  isn't real.
+   :error GenericError: If the system decides it doesn't like the
+  argument values. It's very temperamental.
 
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 c0dc6482204..1f0b168fa2c 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -273,6 +273,12 @@ class QAPICommand(QAPIObject):
 names=("arg",),
 can_collapse=True,
 ),
+GroupedField(
+"error",
+label=_("Errors"),
+names=("error",),
+can_collapse=True,
+),
 ]
 )
 
-- 
2.44.0

[PATCH 20/27] docs/qapi-domain: add :ifcond: directive option

[John Snow]

Add a special :ifcond: option that allows us to annotate the
definition-level conditionals.

RFC: This patch renders IFCOND information in two places, because I'm
undecided about how to style this information. One option is in the
signature bar, and another option is in an eye-catch, like :deprecated:
or :unstable:.

A benefit to having this be a directive option is that we can put it in
the signature bar, the QAPI index, etc. However, if we merely want it in
the content section, a directive would work just as well,
e.g. ".. qapi:ifcond:: CONFIG_LINUX".

(Though, having it be in the same containing box as the unstable/ifcond
boxes might require some extra fiddling/post-processing to
achieve. Generally, the less docutils tree muddling I have to do, the
happier I am.)

The syntax of the argument is currently undefined, but it is possible to
parse it back down into constituent parts to avoid applying literal
formatting to "AND" or "&&" or whichever syntax we formalize. (Or, in
the future, applying cross-reference links to the config values for
additional reading on some of those build options. Not for this series.)

"Vote now on your phones!"

Signed-off-by: Harmonie Snow 
Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  1 +
 docs/sphinx-static/theme_overrides.css | 13 +
 docs/sphinx/qapi-domain.py | 37 --
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 6350791a3ed..c68e2044890 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -97,6 +97,7 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
:since: 13.37
:deprecated:
:unstable:
+   :ifcond: CONFIG_LINUX
 
This is a fake command, it's not real. It can't hurt you.
 
diff --git a/docs/sphinx-static/theme_overrides.css 
b/docs/sphinx-static/theme_overrides.css
index acdf11675db..b239a762a9e 100644
--- a/docs/sphinx-static/theme_overrides.css
+++ b/docs/sphinx-static/theme_overrides.css
@@ -188,3 +188,16 @@ div[class^="highlight"] pre {
 .qapi-deprecated::before {
 content: '⚠️ ';
 }
+
+.qapi-ifcond::before {
+/* gaze ye into the crystal ball to determine feature availability */
+content: ' ';
+}
+
+.qapi-ifcond {
+background-color: #f9f5ff;
+border: solid #dac2ff 6px;
+padding: 8px;
+border-radius: 15px;
+margin: 5px;
+}
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 76157476e02..e44db10488f 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -15,6 +15,7 @@
 NamedTuple,
 Optional,
 Tuple,
+Union,
 cast,
 )
 
@@ -150,6 +151,7 @@ class QAPIObject(ObjectDescription[Signature]):
 "module": directives.unchanged,  # Override contextual module name
 # These are QAPI originals:
 "since": since_validator,
+"ifcond": directives.unchanged,
 "deprecated": directives.flag,
 "unstable": directives.flag,
 }
@@ -176,6 +178,20 @@ def get_signature_suffix(self, sig: str) -> 
List[nodes.Node]:
 """Returns a suffix to put after the object name in the signature."""
 ret: List[nodes.Node] = []
 
+if "ifcond" in self.options:
+ret += [
+addnodes.desc_sig_space(),
+nodes.inline(
+self.options["ifcond"],
+"",
+nodes.Text("["),
+nodes.literal("", "#if"),
+addnodes.desc_sig_space(),
+nodes.literal(self.options["ifcond"], 
self.options["ifcond"]),
+nodes.Text("]"),
+),
+]
+
 if "since" in self.options:
 ret += [
 addnodes.desc_sig_space(),
@@ -257,9 +273,14 @@ def _add_infopips(self, contentnode: 
addnodes.desc_content) -> None:
 infopips = nodes.container()
 infopips.attributes["classes"].append("qapi-infopips")
 
-def _add_pip(source: str, content: str, classname: str) -> None:
+def _add_pip(
+source: str, content: Union[str, List[nodes.Node]], classname: str
+) -> None:
 node = nodes.container(source)
-node.append(nodes.Text(content))
+if isinstance(content, str):
+node.append(nodes.Text(content))
+else:
+node.extend(content)
 node.attributes["classes"].extend(["qapi-infopip", classname])
 infopips.append(node)
 
@@ -277,6 +298,18 @@ def _add_pip(source: str, content: str, classname: str) -> 
None:

[PATCH 13/27] docs/qapi-domain: add qapi:enum directive

[John Snow]

Add the .. qapi:enum:: directive, object, and :qapi:enum:`name`
cross-reference role.

Add the :value name: field list for documenting Enum values.

Of note, also introduce a new "type" role that is intended to be used by
other QAPI object directives to cross-reference arbitrary QAPI type
names, but will exclude commands, events, and modules from
consideration.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 14 ++
 docs/sphinx/qapi-domain.py | 24 
 2 files changed, 38 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 39fe4dd2dae..cf794e6e739 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -113,3 +113,17 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
At the moment, the order of grouped sections is based on the order in
which each group was encountered. This example will render Arguments
first, and then Features; but the order can be any that you choose.
+
+.. qapi:enum:: BitmapSyncMode
+   :since: 4.2
+
+   An enumeration of possible behaviors for the synchronization of a
+   bitmap when used for data copy operations.
+
+   :value on-success: The bitmap is only synced when the operation is
+  successful. This is the behavior always used for
+  ``INCREMENTAL`` backups.
+   :value never: The bitmap is never synchronized with the operation, and
+  is treated solely as a read-only manifest of blocks to copy.
+   :value always: The bitmap is always synchronized with the operation,
+  regardless of whether or not the operation was successful.
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 5d44dba6cd3..6759c39290d 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -289,6 +289,22 @@ class QAPICommand(QAPIObject):
 )
 
 
+class QAPIEnum(QAPIObject):
+"""Description of a QAPI Enum."""
+
+doc_field_types = QAPIObject.doc_field_types.copy()
+doc_field_types.extend(
+[
+GroupedField(
+"value",
+label=_("Values"),
+names=("value",),
+can_collapse=True,
+)
+]
+)
+
+
 class QAPIModule(SphinxDirective):
 """
 Directive to mark description of a new module.
@@ -434,9 +450,14 @@ class QAPIDomain(Domain):
 # This table associates cross-reference object types (key) with an
 # ObjType instance, which defines the valid cross-reference roles
 # for each object type.
+#
+# e.g., the :qapi:type: cross-reference role can refer to enum,
+# struct, union, or alternate objects; but :qapi:obj: can refer to
+# anything. Each object also gets its own targeted cross-reference role.
 object_types: Dict[str, ObjType] = {
 "module": ObjType(_("module"), "mod", "obj"),
 "command": ObjType(_("command"), "cmd", "obj"),
+"enum": ObjType(_("enum"), "enum", "obj", "type"),
 }
 
 # Each of these provides a ReST directive,
@@ -444,6 +465,7 @@ class QAPIDomain(Domain):
 directives = {
 "module": QAPIModule,
 "command": QAPICommand,
+"enum": QAPIEnum,
 }
 
 # These are all cross-reference roles; e.g.
@@ -452,6 +474,8 @@ class QAPIDomain(Domain):
 roles = {
 "mod": QAPIXRefRole(),
 "cmd": QAPIXRefRole(),
+"enum": QAPIXRefRole(),
+"type": QAPIXRefRole(),  # reference any data type (excludes modules, 
commands, events)
 "obj": QAPIXRefRole(),  # reference *any* type of QAPI object.
 }
 
-- 
2.44.0

[PATCH 14/27] docs/qapi-domain: add qapi:alternate directive

[John Snow]

Add the .. qapi:alternate:: directive, object, and qapi:alt:`name`
cross-reference role.

Add the "Choices:" field list for describing alternate choices. Like
other field lists that reference QAPI types, a forthcoming commit will
add cross-referencing support to this field.

RFC: In the future, it would be nice to directly inline Alternates as
part of the type information in the containing object (i.e. directly in
arguments/members) - but that's a task for another series. For now, the
branch "names" are documented just like qapidoc.py does, even though
this information is superfluous for user documentation. Room for future
improvement, but not now.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  7 +++
 docs/sphinx/qapi-domain.py | 19 +++
 2 files changed, 26 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index cf794e6e739..9bfe4d9f454 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -127,3 +127,10 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
   is treated solely as a read-only manifest of blocks to copy.
:value always: The bitmap is always synchronized with the operation,
   regardless of whether or not the operation was successful.
+
+.. qapi:alternate:: BlockDirtyBitmapOrStr
+   :since: 4.1
+
+   :choice str local: name of the bitmap, attached to the same node as
+  target bitmap.
+   :choice BlockDirtyBitmap external: bitmap with specified node
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 6759c39290d..c6eb6324594 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -305,6 +305,22 @@ class QAPIEnum(QAPIObject):
 )
 
 
+class QAPIAlternate(QAPIObject):
+"""Description of a QAPI Alternate."""
+
+doc_field_types = QAPIObject.doc_field_types.copy()
+doc_field_types.extend(
+[
+TypedField(
+"choice",
+label=_("Choices"),
+names=("choice",),
+can_collapse=True,
+),
+]
+)
+
+
 class QAPIModule(SphinxDirective):
 """
 Directive to mark description of a new module.
@@ -458,6 +474,7 @@ class QAPIDomain(Domain):
 "module": ObjType(_("module"), "mod", "obj"),
 "command": ObjType(_("command"), "cmd", "obj"),
 "enum": ObjType(_("enum"), "enum", "obj", "type"),
+"alternate": ObjType(_("alternate"), "alt", "obj", "type"),
 }
 
 # Each of these provides a ReST directive,
@@ -466,6 +483,7 @@ class QAPIDomain(Domain):
 "module": QAPIModule,
 "command": QAPICommand,
 "enum": QAPIEnum,
+"alternate": QAPIAlternate,
 }
 
 # These are all cross-reference roles; e.g.
@@ -475,6 +493,7 @@ class QAPIDomain(Domain):
 "mod": QAPIXRefRole(),
 "cmd": QAPIXRefRole(),
 "enum": QAPIXRefRole(),
+"alt": QAPIXRefRole(),
 "type": QAPIXRefRole(),  # reference any data type (excludes modules, 
commands, events)
 "obj": QAPIXRefRole(),  # reference *any* type of QAPI object.
 }
-- 
2.44.0

[PATCH 08/27] docs/qapi-domain: add :since: directive option

[John Snow]

Add a little special markup for registering "Since:" information. Adding
it as an option instead of generic content lets us hoist the information
into the Signature bar, optionally put it in the index, etc.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  1 +
 docs/sphinx/qapi-domain.py | 27 +--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 5516f762a24..33b9349a3ee 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -54,6 +54,7 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
 
 
 .. qapi:command:: example-command
+   :since: 42.0
 
This directive creates a QAPI command named `example-command` that
appears in both the `genindex` and the `qapi-index`. As of this
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 2c1e60290d9..38a50318d08 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -4,6 +4,7 @@
 
 from __future__ import annotations
 
+import re
 from typing import (
 TYPE_CHECKING,
 Any,
@@ -86,6 +87,18 @@ def process_link(
 return title, target
 
 
+def since_validator(param: str) -> str:
+"""
+Validate the `:since: X.Y` option field.
+"""
+match = re.match(r"[0-9]+\.[0-9]+", param)
+if not match:
+raise ValueError(
+f":since: requires a version number in X.Y format; not {param!r}"
+)
+return param
+
+
 def _nested_parse(directive: SphinxDirective, content_node: Element) -> None:
 """
 This helper preserves error parsing context across sphinx versions.
@@ -127,6 +140,8 @@ class QAPIObject(ObjectDescription[Signature]):
 {
 # Borrowed from the Python domain:
 "module": directives.unchanged,  # Override contextual module name
+# These are QAPI originals:
+"since": since_validator,
 }
 )
 
@@ -138,9 +153,17 @@ def get_signature_prefix(self, sig: str) -> 
List[nodes.Node]:
 addnodes.desc_sig_space(),
 ]
 
-def get_signature_suffix(self, sig: str) -> list[nodes.Node]:
+def get_signature_suffix(self, sig: str) -> List[nodes.Node]:
 """Returns a suffix to put after the object name in the signature."""
-return []
+ret: List[nodes.Node] = []
+
+if "since" in self.options:
+ret += [
+addnodes.desc_sig_space(),
+addnodes.desc_sig_element("", f"(Since: 
{self.options['since']})"),
+]
+
+return ret
 
 def handle_signature(self, sig: str, signode: desc_signature) -> Signature:
 """
-- 
2.44.0

[PATCH 24/27] docs/qapi-domain: add type cross-refs to field lists

[John Snow]

This commit, finally, adds cross-referencing support to various field
lists; modeled tightly after Sphinx's own Python domain code.

Cross-referencing support is added to type names provided to :arg:,
:memb:, :returns: and :choice:.

:feat:, :error: and :value:, which do not take type names, do not
support this syntax.

The general syntax is simple:

:arg TypeName ArgName: Lorem Ipsum ...

The domain will transform TypeName into :qapi:type:`TypeName` in this
basic case, and also apply the ``literal`` decoration to indicate that
this is a type cross-reference.

For Optional arguments, the special "?" suffix is used. Because "*" has
special meaning in ReST that would cause parsing errors, we elect to use
"?" instead. The special syntax processing in QAPIXrefMixin strips this
character from the end of any type name argument and will append ",
Optional" to the rendered output, applying the cross-reference only to
the actual type name.

The intent here is that the actual syntax in doc-blocks need not change;
but e.g. qapidoc.py will need to process and transform "@arg foo lorem
ipsum" into ":arg type? foo: lorem ipsum" based on the schema
information. Therefore, nobody should ever actually witness this
intermediate syntax unless they are writing manual documentation or the
doc transmogrifier breaks.

For array arguments, type names can similarly be surrounded by "[]",
which are stripped off and then re-appended outside of the
cross-reference.

Note: The mixin pattern here (borrowed from Sphinx) confuses mypy
because it cannot tell that it will be mixed into a descendent of
Field. Doing that instead causes more errors, because many versions of
Sphinx erroneously did not mark various arguments as Optional, so we're
a bit hosed either way. Do the simpler thing.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  34 
 docs/sphinx/qapi-domain.py | 110 +++--
 2 files changed, 138 insertions(+), 6 deletions(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 8352a27d4a5..6e85ea5280d 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -105,6 +105,11 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
:arg str bar: Another normal parameter description.
:arg baz: Missing a type.
:arg no-descr:
+   :arg int? oof: Testing optional argument parsing.
+   :arg [XDbgBlockGraphNode] rab: Testing array argument parsing.
+   :arg [BitmapSyncMode]? zab: Testing optional array argument parsing,
+  even though Markus said this should never happen. I believe him,
+  but I didn't *forbid* the syntax either.
:arg BitmapSyncMode discrim: How about branches in commands?
 
.. qapi:branch:: discrim on-success
@@ -261,3 +266,32 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
 
   :memb str key-secret: ID of a QCryptoSecret object providing a
  passphrase for unlocking the encryption
+
+.. qapi:command:: x-debug-query-block-graph
+   :since: 4.0
+   :unstable:
+
+   Get the block graph.
+
+   :feat unstable: This command is meant for debugging.
+   :return XDbgBlockGraph: lorem ipsum ...
+
+.. qapi:struct:: XDbgBlockGraph
+   :since: 4.0
+
+   Block Graph - list of nodes and list of edges.
+
+   :memb [XDbgBlockGraphNode] nodes:
+   :memb [XDbgBlockGraphEdge] edges:
+
+.. qapi:struct:: XDbgBlockGraphNode
+   :since: 4.0
+
+   :memb uint64 id: Block graph node identifier.  This @id is generated only 
for
+  x-debug-query-block-graph and does not relate to any other
+  identifiers in Qemu.
+   :memb XDbgBlockGraphNodeType type: Type of graph node.  Can be one of
+  block-backend, block-job or block-driver-state.
+   :memb str name: Human readable name of the node.  Corresponds to
+  node-name for block-driver-state nodes; is not guaranteed to be
+  unique in the whole graph (with block-jobs and block-backends).
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index bf8bb933345..074453193ce 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -50,11 +50,12 @@
 
 if TYPE_CHECKING:
 from docutils.nodes import Element, Node
+from docutils.parsers.rst.states import Inliner
 
 from sphinx.application import Sphinx
 from sphinx.builders import Builder
 from sphinx.environment import BuildEnvironment
-from sphinx.util.typing import OptionSpec
+from sphinx.util.typing import OptionSpec, TextlikeNode
 
 logger = logging.getLogger(__name__)
 
@@ -68,6 +69,90 @@ class ObjectEntry(NamedTuple):
 aliased: bool
 
 
+class QAPIXrefMixin:
+def make_xref(
+self,
+rolename: str,
+domain: str,
+target: str,
+innernode: type[TextlikeNode] = nodes.literal,
+contnode: Optional[Node] = None,
+env: Optional[BuildEnvironment] = None,
+inliner: Optional[Inliner] = None,
+lo

[PATCH 04/27] docs/qapi-domain: add QAPI index

[John Snow]

Use the QAPI object registry to generate a special index just for QAPI
definitions. The index can show entries both by definition type and
alphabetically.

The index can be linked from anywhere in the QEMU manual by using
`qapi-index`.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  6 +++-
 docs/sphinx/qapi-domain.py | 66 +++---
 2 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 880fd17c709..051dc6b3a37 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -9,7 +9,8 @@ QAPI Domain Test
doesn't create a cross-reference target and it isn't added to the
index.
 
-   Check out the `genindex` for proof that foo-module is not present.
+   Check out the `genindex` or the `qapi-index` for proof that
+   foo-module is not present.
 
 .. qapi:module:: bar-module
:no-typesetting:
@@ -36,3 +37,6 @@ QAPI Domain Test
 
The ``block-core`` module will have two entries in the `genindex`,
under both "block-core" and "QAPI module".
+
+   Modules will also be reported in the `qapi-index`, under the Modules
+   category and in the alphabetical categories that follow.
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index ab80fd5f634..65409786119 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -12,6 +12,7 @@
 Iterable,
 List,
 NamedTuple,
+Optional,
 Tuple,
 cast,
 )
@@ -20,7 +21,12 @@
 from docutils.parsers.rst import directives
 
 from sphinx import addnodes
-from sphinx.domains import Domain, ObjType
+from sphinx.domains import (
+Domain,
+Index,
+IndexEntry,
+ObjType,
+)
 from sphinx.locale import _, __
 from sphinx.util import logging
 from sphinx.util.docutils import SphinxDirective, switch_source_input
@@ -74,9 +80,10 @@ class QAPIModule(SphinxDirective):
 a pass-through for the content body. Named section titles are
 allowed in the content body.
 
-Use this directive to associate subsequent definitions with the
-module they are defined in for purposes of search and QAPI index
-organization.
+Use this directive to create entries for the QAPI module in the
+global index and the qapi index; as well as to associate subsequent
+definitions with the module they are defined in for purposes of
+search and QAPI index organization.
 
 :arg: The name of the module.
 :opt no-index: Don't add cross-reference targets or index entries.
@@ -155,6 +162,52 @@ def run(self) -> List[Node]:
 return ret
 
 
+class QAPIIndex(Index):
+"""
+Index subclass to provide the QAPI definition index.
+"""
+
+name = "index"
+localname = _("QAPI Index")
+shortname = _("QAPI Index")
+
+def generate(
+self,
+docnames: Optional[Iterable[str]] = None,
+) -> Tuple[List[Tuple[str, List[IndexEntry]]], bool]:
+assert isinstance(self.domain, QAPIDomain)
+content: Dict[str, List[IndexEntry]] = {}
+collapse = False
+
+# list of all object (name, ObjectEntry) pairs, sorted by name
+objects = sorted(self.domain.objects.items(), key=lambda x: 
x[0].lower())
+
+for objname, obj in objects:
+if docnames and obj.docname not in docnames:
+continue
+
+# Strip the module name out:
+objname = objname.split(".")[-1]
+
+# Add an alphabetical entry:
+entries = content.setdefault(objname[0].upper(), [])
+entries.append(
+IndexEntry(objname, 0, obj.docname, obj.node_id, obj.objtype, 
"", "")
+)
+
+# Add a categorical entry:
+category = obj.objtype.title() + "s"
+entries = content.setdefault(category, [])
+entries.append(IndexEntry(objname, 0, obj.docname, obj.node_id, 
"", "", ""))
+
+# alphabetically sort categories; type names first, ABC entries last.
+sorted_content = sorted(
+content.items(),
+key=lambda x: (len(x[0]) == 1, x[0]),
+)
+return sorted_content, collapse
+
+
 class QAPIDomain(Domain):
 """QAPI language domain."""
 
@@ -182,7 +235,10 @@ class QAPIDomain(Domain):
 "objects": {},  # fullname -> ObjectEntry
 }
 
-indices = []
+# Index pages to generate; each entry is an Index class.
+indices = [
+QAPIIndex,
+]
 
 @property
 def objects(self) -> Dict[str, ObjectEntry]:
-- 
2.44.0

[PATCH 02/27] docs/qapi-domain: add qapi:module directive

[John Snow]

This adds a qapi:module directive, which just notes the current module
being documented and performs a nested parse of the content block, if
present.

This code is based pretty heavily on Sphinx's PyModule directive, but
with the modindex functionality excised.

This commit also adds the _nested_parse helper, which adds cross-version
compatibility for nested parsing while preserving proper line context
information.

For example:

.. qapi:module:: block-core

   Hello, and welcome to block-core!
   =

   lorem ipsum, dolor sit amet ...

(For RFC purposes, this commit also adds a test document that
demonstrates the functionality-so-far to allow reviewers to easily test
and experiment with each commit. The eventual submission for inclusion
will remove this playground file.)

Signed-off-by: John Snow 
---
 docs/index.rst |   1 +
 docs/qapi/index.rst|  38 +++
 docs/sphinx/qapi-domain.py | 128 -
 3 files changed, 166 insertions(+), 1 deletion(-)
 create mode 100644 docs/qapi/index.rst

diff --git a/docs/index.rst b/docs/index.rst
index 0b9ee9901d9..11c18c598a8 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -18,3 +18,4 @@ Welcome to QEMU's documentation!
interop/index
specs/index
devel/index
+   qapi/index
diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
new file mode 100644
index 000..880fd17c709
--- /dev/null
+++ b/docs/qapi/index.rst
@@ -0,0 +1,38 @@
+
+QAPI Domain Test
+
+
+.. qapi:module:: foo-module
+   :no-index:
+
+   This starts a hypothetical module named ``foo-module``, but it
+   doesn't create a cross-reference target and it isn't added to the
+   index.
+
+   Check out the `genindex` for proof that foo-module is not present.
+
+.. qapi:module:: bar-module
+   :no-typesetting:
+
+   This starts a hypothetical module named ``bar-module``, but the
+   contents of the body here will not be rendered in the
+   output. However, any link targets created here or in nested
+   directives will be preserved and functional.
+
+   Check out the `genindex` for proof that bar-module is present in two
+   places! (under both "bar-module" and "QAPI module".)
+
+.. qapi:module:: block-core
+
+   Block core (VM unrelated)
+   =
+
+   This starts the documentation section for the ``block-core`` module.
+   All documentation objects that follow belong to the block-core module
+   until another ``qapi:module:`` directive is encountered.
+
+   This directive does not create an entry in the sidebar or the TOC
+   *unless* you create a nested section title within the directive.
+
+   The ``block-core`` module will have two entries in the `genindex`,
+   under both "block-core" and "QAPI module".
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 163b9ff21c3..7c5e4407bc1 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -7,21 +7,141 @@
 from typing import (
 TYPE_CHECKING,
 Any,
+ClassVar,
 Dict,
+Iterable,
 List,
 Tuple,
+cast,
 )
 
+from docutils import nodes
+from docutils.parsers.rst import directives
+
+from sphinx import addnodes
 from sphinx.domains import Domain, ObjType
 from sphinx.util import logging
+from sphinx.util.docutils import SphinxDirective, switch_source_input
+from sphinx.util.nodes import make_id, nested_parse_with_titles
 
 
 if TYPE_CHECKING:
+from docutils.nodes import Element, Node
+
 from sphinx.application import Sphinx
+from sphinx.util.typing import OptionSpec
 
 logger = logging.getLogger(__name__)
 
 
+def _nested_parse(directive: SphinxDirective, content_node: Element) -> None:
+"""
+This helper preserves error parsing context across sphinx versions.
+"""
+
+# necessary so that the child nodes get the right source/line set
+content_node.document = directive.state.document
+
+try:
+# Modern sphinx (6.2.0+) supports proper offsetting for
+# nested parse error context management
+nested_parse_with_titles(
+directive.state,
+directive.content,
+content_node,
+content_offset=directive.content_offset,  # type: ignore[call-arg]
+)
+except TypeError:
+# No content_offset argument. Fall back to SSI method.
+with switch_source_input(directive.state, directive.content):
+nested_parse_with_titles(directive.state, directive.content, 
content_node)
+
+
+class QAPIModule(SphinxDirective):
+"""
+Directive to mark description of a new module.
+
+This directive doesn't generate any special formatting, and is just
+a pass-through for the content body. Named section titles are
+allowed in the content body.
+
+Use this directive to associate subsequent definitions with the
+module they are d

[PATCH 25/27] docs/qapi-domain: implement error context reporting fix

[John Snow]

Sphinx 5.3.0 to Sphinx 6.2.0 has a bug where nested content in an
ObjectDescription content block has its error position reported
incorrectly due to an oversight when they added nested section support
to this directive.

(This bug is present in Sphinx's own Python and C domains; test it
yourself by creating a py:func directive and creating a syntax error in
the directive's content block.)

To avoid overriding and re-implementing the entirety of the run()
method, a workaround is employed where we parse the content block
ourselves in before_content(), then null the content block to make
Sphinx's own parsing a no-op. Then, in transform_content (which occurs
after Sphinx's nested parse), we simply swap our own parsed content tree
back in for Sphinx's.

It appears a little tricky, but it's the nicest solution I can find.

Signed-off-by: John Snow 
---
 docs/sphinx/qapi-domain.py | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 074453193ce..7d8911c635f 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -21,7 +21,9 @@
 
 from docutils import nodes
 from docutils.parsers.rst import directives
+from docutils.statemachine import StringList
 
+import sphinx
 from sphinx import addnodes
 from sphinx.addnodes import desc_signature, pending_xref
 from sphinx.directives import ObjectDescription
@@ -470,7 +472,27 @@ def _validate_field(self, field: nodes.field) -> None:
 )
 logger.warning(msg, location=field)
 
+def before_content(self) -> None:
+# Work around a sphinx bug and parse the content ourselves.
+self._temp_content = self.content
+self._temp_offset = self.content_offset
+self._temp_node = None
+
+if sphinx.version_info[:3] >= (5, 3, 0) and sphinx.version_info[:3] < 
(6, 2, 0):
+self._temp_node = addnodes.desc_content()
+self.state.nested_parse(self.content, self.content_offset, 
self._temp_node)
+# Sphinx will try to parse the content block itself,
+# Give it nothingness to parse instead.
+self.content = StringList()
+self.content_offset = 0
+
 def transform_content(self, contentnode: addnodes.desc_content) -> None:
+# Sphinx workaround: Inject our parsed content and restore state.
+if self._temp_node:
+contentnode += self._temp_node.children
+self.content = self._temp_content
+self.content_offset = self._temp_offset
+
 self._add_infopips(contentnode)
 self._merge_adjoining_field_lists(contentnode)
 
-- 
2.44.0

[PATCH 17/27] docs/qapi-domain: add qapi:union and qapi:branch directives

[John Snow]

Adds the .. qapi:union:: directive, object, and :qapi:union:`name`
cross-referencing role.

In order to support discriminated branches of unions, a new qapi:branch
directive is created whose only purpose is to create a dynamically named
field list section based on the name of the branch key and value.

Because the label for these branch sections is dynamically generated,
this patch allows the use of either :memb: or :arg: in branches; they
are simply aliases and do not do anything different.

(This is to allow the directive to be used for either Commands or Unions
as needed; i.e. for commands whose argument type is a discriminated
union.)

For example:

.. qapi:union:: foo-union

   :memb foo-enum key: Discriminator field
   :memb foo-type fieldname: Some base type field that is always present

   .. qapi:branch:: key value1

  :memb type name: lorem ipsum ...

   .. qapi:branch:: key value2

  :memb type2 name2: dolor sit amet ...

In order to support this syntax, the root QAPIObject class needs to
perform some post-processing on field lists to merge adjecent field
lists. because each branch directive will return a separate field list,
and we want to combine them into one unified list for proper rendering.

NOTE: Technically, the branch directive will allow you to put arbitrary
ReST besides field lists inside of it, but it almost certainly won't do
anything you'd consider useful. I don't recommend it, but
programmatically forbidding it is expensive. Recommendation: "Don't!"

... and now ...

RFC: The branches abuse the field list system to create new categories
of field list entries that are dynamically generated. At the moment, I
do not have code to apply appropriate formatting to these headings, but
would like to apply ``literal`` syntax at a minimum to the enum values.

The other idea I have is to create a "blank" members value that doesn't
add a "title" (field label) on the left column and instead adds a
horizontal info bar into the arguments list in the right, but the
problem with this approach is that I won't be able to get rid of the "*
..." list bullet syntax, so it may look odd. Otherwise, a benefit to
this syntax is that it would allow us to write a longer explanation,
e.g. "When ``{key}`` is ``{value}``".

It's possible I could split the argument list into several nested lists,
but I haven't experimented in that direction yet - it's also likely that
Sphinx will want to append a ":" even to blank section titles, so this
may prove tricky to do within the constraints of Sphinx's existing Field
list system. (I believe the HTML formatter is responsible for appending
the colons.)

I'm open to suggestions, but wrestling with Sphinx, html and css is time
consuming (for me, at least), so ultimately, I'm afraid I must say:
***PATCHES WELCOME***.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 37 ++
 docs/sphinx/qapi-domain.py | 98 +-
 2 files changed, 134 insertions(+), 1 deletion(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index b07e6e9e2e3..d37ceac497f 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -103,6 +103,18 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
:arg baz: Missing a type.
:feat unstable: More than unstable, this command doesn't even exist!
:arg no-descr:
+   :arg BitmapSyncMode discrim: How about branches in commands?
+
+   .. qapi:branch:: discrim on-success
+
+  :arg str foobar: This is an argument that belongs to a tagged union 
branch.
+  :arg int? foobaz: This is another argument belonging to the same branch.
+
+   .. qapi:branch:: discrim never
+
+  :arg str barfoo: This is an argument that belongs to a *different* 
tagged union branch.
+  :arg int64 zzxyz: And this is another argument belonging to that same 
branch.
+
:feat hallucination: This command is a figment of your imagination.
:error CommandNotFound: When you try to use this command, because it
   isn't real.
@@ -190,3 +202,28 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
  operations.  0 means unlimited.  If max-chunk is non-zero then it
  should not be less than job cluster size which is calculated as
  maximum of target image cluster size and 64k.  Default 0.
+
+.. qapi:union:: RbdEncryptionOptions
+   :since: 6.1
+
+   :memb RbdImageEncryptionFormat format: Encryption format.
+   :memb RbdEncryptionOptions? parent: Parent image encryption options
+  (for cloned images).  Can be left unspecified if this cloned image
+  is encrypted using the same format and secret as its parent image
+  (i.e. not explicitly formatted) or if its parent image is not
+  encrypted.  (Since 8.0)
+
+   .. qapi:branch:: format luks
+
+  :memb str key-secret: ID of a QCryptoSecret object providing a
+ passphrase for unlocking the encryption
+
+   .. 

[PATCH 18/27] docs/qapi-domain: add :deprecated: directive option

[John Snow]

Although "deprecated" is a feature (and *will* appear in the features
list), add a special :deprecated: option to generate an eye-catch that
makes this information very hard to miss.

(The intent is to modify qapidoc.py to add this option whenever it
detects that the features list attached to a definition contains the
"deprecated" entry.)

-

RFC: Technically, this object-level option is un-needed and could be
replaced with a standard content-level directive that e.g. qapidoc.py
could insert at the beginning of the content block. I've done it here as
an option to demonstrate how it would be possible to do.

It's a matter of taste for "where" we feel like implementing it.

One benefit of doing it this way is that we can create a single
containing box to set CSS style options controlling the flow of multiple
infoboxes. The other way to achieve that would be to create a directive
that allows us to set multiple options instead, e.g.:

.. qapi:infoboxes:: deprecated unstable

or possibly:

.. qapi:infoboxes::
   :deprecated:
   :unstable:

For now, I've left these as top-level QAPI object options. "Hey, it works."

P.S., I outsourced the CSS ;)

Signed-off-by: Harmonie Snow 
Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  4 
 docs/sphinx-static/theme_overrides.css | 25 +
 docs/sphinx/qapi-domain.py | 24 
 3 files changed, 53 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index d37ceac497f..b9a69f6bd17 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -95,6 +95,7 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
 
 .. qapi:command:: fake-command
:since: 13.37
+   :deprecated:
 
This is a fake command, it's not real. It can't hurt you.
 
@@ -116,6 +117,9 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
   :arg int64 zzxyz: And this is another argument belonging to that same 
branch.
 
:feat hallucination: This command is a figment of your imagination.
+   :feat deprecated: Although this command is fake, you should know that
+  it's also deprecated. That's great news! Maybe it will go away and
+  stop haunting you someday.
:error CommandNotFound: When you try to use this command, because it
   isn't real.
:error GenericError: If the system decides it doesn't like the
diff --git a/docs/sphinx-static/theme_overrides.css 
b/docs/sphinx-static/theme_overrides.css
index c70ef951286..97b8c1c60e6 100644
--- a/docs/sphinx-static/theme_overrides.css
+++ b/docs/sphinx-static/theme_overrides.css
@@ -159,3 +159,28 @@ div[class^="highlight"] pre {
 color: inherit;
 }
 }
+
+/* QAPI domain theming */
+
+.qapi-infopips {
+margin-bottom: 1em;
+}
+
+.qapi-infopip {
+display: inline-block;
+padding: 0em 0.5em 0em 0.5em;
+margin: 0.25em;
+}
+
+.qapi-deprecated {
+background-color: #fffef5;
+border: solid #fff176 6px;
+font-weight: bold;
+padding: 8px;
+border-radius: 15px;
+margin: 5px;
+}
+
+.qapi-deprecated::before {
+content: '⚠️ ';
+}
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index f0094300c03..065ad501960 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -150,6 +150,7 @@ class QAPIObject(ObjectDescription[Signature]):
 "module": directives.unchanged,  # Override contextual module name
 # These are QAPI originals:
 "since": since_validator,
+"deprecated": directives.flag,
 }
 )
 
@@ -249,6 +250,28 @@ def add_target_and_index(
 ("single", indextext, node_id, "", None)
 )
 
+def _add_infopips(self, contentnode: addnodes.desc_content) -> None:
+# Add various eye-catches and things that go below the signature
+# bar, but precede the user-defined content.
+infopips = nodes.container()
+infopips.attributes["classes"].append("qapi-infopips")
+
+def _add_pip(source: str, content: str, classname: str) -> None:
+node = nodes.container(source)
+node.append(nodes.Text(content))
+node.attributes["classes"].extend(["qapi-infopip", classname])
+infopips.append(node)
+
+if "deprecated" in self.options:
+_add_pip(
+":deprecated:",
+f"This {self.objtype} is deprecated.",
+"qapi-deprecated",
+)
+
+if infopips.children:
+contentnode.insert(0, infopips)
+
 def _merge_adjoining_field_lists(self, contentnode: addnodes.desc_content) 
-> None:
 # Take any adjacent field lists and glue them together into
 # one list for further processing by Sphinx. T

[PATCH 07/27] docs/qapi-domain: add qapi:command directive

[John Snow]

This commit adds a generic QAPIObject class for use in documenting
various QAPI entities in the Sphinx ecosystem.

It also adds a stubbed version of QAPICommand that utilizes the
QAPIObject class; along with the qapi:command directive, the
:qapi:cmd: cross-reference role, and the "command" object type in the
QAPI object registry.

They don't do anything *particularly* interesting yet, but that will
come in forthcoming commits.

Note: some versions of mypy get a little confused over the difference
between class and instance variables; because sphinx's ObjectDescription
does not declare option_spec as a ClassVar (even though it's obvious
that it is), mypy may produce this error:

qapi-domain.py:125: error: Cannot override instance variable (previously
declared on base class "ObjectDescription") with class variable [misc]

I can't control that; so silence the error with a pragma.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  34 ++
 docs/sphinx/qapi-domain.py | 132 -
 2 files changed, 165 insertions(+), 1 deletion(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index e2223d5f363..5516f762a24 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -51,3 +51,37 @@ the actual output of that directive was suppressed. Here's a 
link to
 Explicit cross-referencing syntax for QAPI modules is available with
 ``:qapi:mod:`foo```, here's a link to :qapi:mod:`bar-module` and one to
 :qapi:mod:`block-core`.
+
+
+.. qapi:command:: example-command
+
+   This directive creates a QAPI command named `example-command` that
+   appears in both the `genindex` and the `qapi-index`. As of this
+   commit, there aren't any special arguments or options you can give to
+   this directive, it merely parses its content block and handles the
+   TOC/index/xref book-keeping.
+
+   Unlike the QAPI module directive, this directive *does* add a TOC
+   entry by default.
+
+   This object can be referenced in *quite a few ways*:
+
+   * ```example-command``` => `example-command`
+   * ```block-core.example-command``` => `block-core.example-command`
+   * ``:qapi:cmd:`example-command``` => :qapi:cmd:`example-command`
+   * ``:qapi:cmd:`block-core.example-command``` => 
:qapi:cmd:`block-core.example-command`
+   * ``:qapi:cmd:`~example-command``` => :qapi:cmd:`~example-command`
+   * ``:qapi:cmd:`~block-core.example-command``` => 
:qapi:cmd:`~block-core.example-command`
+   * ``:qapi:obj:`example-command``` => :qapi:obj:`example-command`
+   * ``:qapi:obj:`block-core.example-command``` => 
:qapi:obj:`block-core.example-command`
+   * ``:qapi:obj:`~example-command``` => :qapi:obj:`~example-command`
+   * ``:qapi:obj:`~block-core.example-command``` => 
:qapi:obj:`~block-core.example-command`
+
+   As of Sphinx v7.2.6, there are a few sphinx-standard options this
+   directive has:
+
+   * ``:no-index:`` or ``:noindex:`` Don't add to the `genindex` nor
+ the `qapi-index`; do not register for cross-references.
+   * ``:no-index-entry:`` or ``:noindexentry:``
+   * ``:no-contents-entry:`` or ``:nocontentsentry:``
+   * ``:no-typesetting:``
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index d28ac1cb9d8..2c1e60290d9 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -21,7 +21,8 @@
 from docutils.parsers.rst import directives
 
 from sphinx import addnodes
-from sphinx.addnodes import pending_xref
+from sphinx.addnodes import desc_signature, pending_xref
+from sphinx.directives import ObjectDescription
 from sphinx.domains import (
 Domain,
 Index,
@@ -108,6 +109,132 @@ def _nested_parse(directive: SphinxDirective, 
content_node: Element) -> None:
 nested_parse_with_titles(directive.state, directive.content, 
content_node)
 
 
+# Alias for the return of handle_signature(), which is used in several places.
+# (In the Python domain, this is Tuple[str, str] instead.)
+Signature = str
+
+
+class QAPIObject(ObjectDescription[Signature]):
+"""
+Description of a generic QAPI object.
+
+It's not used directly, but is instead subclassed by specific directives.
+"""
+
+# Inherit some standard options from Sphinx's ObjectDescription
+option_spec: OptionSpec = ObjectDescription.option_spec.copy()  # 
type:ignore[misc]
+option_spec.update(
+{
+# Borrowed from the Python domain:
+"module": directives.unchanged,  # Override contextual module name
+}
+)
+
+def get_signature_prefix(self, sig: str) -> List[nodes.Node]:
+"""Returns a prefix to put before the object name in the signature."""
+assert self.objtype
+return [
+addnodes.desc_sig_keyword("", self.objtype.title()),
+addnodes.desc_sig_space(),
+]
+
+def get_signature_suffix(self, sig

[PATCH 16/27] docs/qapi-domain: add qapi:struct directive

[John Snow]

Adds the .. qapi:struct:: directive, object, and :qapi:struct:`name`
cross-referencing role.

As per usual, QAPI cross-referencing for types in the member field list
will be added in a forthcoming commit.

RFC Note: The "?" syntax sneaks into the example document again. Please
ignore that for now.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 16 
 docs/sphinx/qapi-domain.py |  9 +
 2 files changed, 25 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index d81bccfb06a..b07e6e9e2e3 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -174,3 +174,19 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
  "microseconds": 959568
}
  }
+
+.. qapi:struct:: BackupPerf
+   :since: 6.0
+
+   Optional parameters for backup.  These parameters don't affect
+   functionality, but may significantly affect performance.
+
+   :memb bool? use-copy-range: Use copy offloading.  Default false.
+   :memb int? max-workers: Maximum number of parallel requests for the
+  sustained background copying process.  Doesn't influence
+  copy-before-write operations.  Default 64.
+   :memb int64? max-chunk: Maximum request length for the sustained
+ background copying process.  Doesn't influence copy-before-write
+ operations.  0 means unlimited.  If max-chunk is non-zero then it
+ should not be less than job cluster size which is calculated as
+ maximum of target image cluster size and 64k.  Default 0.
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 74dc578b3c7..b46faeaceef 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -343,6 +343,12 @@ class QAPIEvent(QAPIObjectWithMembers):
 pass
 
 
+class QAPIStruct(QAPIObjectWithMembers):
+"""Description of a QAPI Struct."""
+
+pass
+
+
 class QAPIModule(SphinxDirective):
 """
 Directive to mark description of a new module.
@@ -497,6 +503,7 @@ class QAPIDomain(Domain):
 "command": ObjType(_("command"), "cmd", "obj"),
 "event": ObjType(_("event"), "event", "obj"),
 "enum": ObjType(_("enum"), "enum", "obj", "type"),
+"struct": ObjType(_("struct"), "struct", "obj", "type"),
 "alternate": ObjType(_("alternate"), "alt", "obj", "type"),
 }
 
@@ -507,6 +514,7 @@ class QAPIDomain(Domain):
 "command": QAPICommand,
 "event": QAPIEvent,
 "enum": QAPIEnum,
+"struct": QAPIStruct,
 "alternate": QAPIAlternate,
 }
 
@@ -518,6 +526,7 @@ class QAPIDomain(Domain):
 "cmd": QAPIXRefRole(),
 "event": QAPIXRefRole(),
 "enum": QAPIXRefRole(),
+"struct": QAPIXRefRole(),
 "alt": QAPIXRefRole(),
 "type": QAPIXRefRole(),  # reference any data type (excludes modules, 
commands, events)
 "obj": QAPIXRefRole(),  # reference *any* type of QAPI object.
-- 
2.44.0

[PATCH 15/27] docs/qapi-domain: add qapi:event directive

[John Snow]

Adds the .. qapi:event:: directive, object, and :qapi:event:`name`
cross-referencing role.

Adds the :memb type name: field list syntax for documenting event data
members. As this syntax and phrasing will be shared with Structs and
Unions as well, add the field list definition to a shared abstract
class.

As per usual, QAPI cross-referencing for types in the member field list
will be added in a forthcoming commit.

NOTE 1: The "str?" type annotation syntax sneaks into this commit in the
demonstration doc. It isn't processed yet, so just ignore it for
now. The non-RFC version of this series won't include the sandbox doc,
so that inconsistency will sort itself out later. (Skip ahead to the
commit that adds XRef support for TypedField and GroupedField lists to
learn what the deal is there and leave feedback on that syntax.)

NOTE 2: We already have a QMP lexer, turn it on for example blocks in this
sample demo.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 40 ++
 docs/sphinx/qapi-domain.py | 25 
 2 files changed, 65 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 9bfe4d9f454..d81bccfb06a 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -2,6 +2,12 @@
 QAPI Domain Test
 
 
+.. this sets the code-highlighting language to QMP for this *document*.
+   I wonder if I can set a domain default...?
+
+.. highlight:: QMP
+
+
 .. qapi:module:: foo-module
:no-index:
 
@@ -134,3 +140,37 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
:choice str local: name of the bitmap, attached to the same node as
   target bitmap.
:choice BlockDirtyBitmap external: bitmap with specified node
+
+.. qapi:event:: BLOCK_JOB_COMPLETED
+   :since: 1.1
+
+   Emitted when a block job has completed.
+
+   :memb JobType type: job type
+   :memb str device: The job identifier. Originally the device name but
+  other values are allowed since QEMU 2.7
+   :memb int len: maximum progress value
+   :memb int offset: current progress value. On success this is equal to
+  len. On failure this is less than len
+   :memb int speed: rate limit, bytes per second
+   :memb str? error: error message. Only present on failure. This field
+  contains a human-readable error message. There are no semantics
+  other than that streaming has failed and clients should not try to
+  interpret the error string
+
+   Example::
+
+ <- {
+   "event": "BLOCK_JOB_COMPLETED",
+   "data": {
+ "type": "stream",
+ "device": "virtio-disk0",
+ "len": 10737418240,
+ "offset": 10737418240,
+ "speed": 0
+   },
+   "timestamp": {
+ "seconds": 1267061043,
+ "microseconds": 959568
+   }
+ }
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index c6eb6324594..74dc578b3c7 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -321,6 +321,28 @@ class QAPIAlternate(QAPIObject):
 )
 
 
+class QAPIObjectWithMembers(QAPIObject):
+"""Base class for Events/Structs/Unions"""
+
+doc_field_types = QAPIObject.doc_field_types.copy()
+doc_field_types.extend(
+[
+TypedField(
+"member",
+label=_("Members"),
+names=("memb",),
+can_collapse=True,
+),
+]
+)
+
+
+class QAPIEvent(QAPIObjectWithMembers):
+"""Description of a QAPI Event."""
+
+pass
+
+
 class QAPIModule(SphinxDirective):
 """
 Directive to mark description of a new module.
@@ -473,6 +495,7 @@ class QAPIDomain(Domain):
 object_types: Dict[str, ObjType] = {
 "module": ObjType(_("module"), "mod", "obj"),
 "command": ObjType(_("command"), "cmd", "obj"),
+"event": ObjType(_("event"), "event", "obj"),
 "enum": ObjType(_("enum"), "enum", "obj", "type"),
 "alternate": ObjType(_("alternate"), "alt", "obj", "type"),
 }
@@ -482,6 +505,7 @@ class QAPIDomain(Domain):
 directives = {
 "module": QAPIModule,
 "command": QAPICommand,
+"event": QAPIEvent,
 "enum": QAPIEnum,
 "alternate": QAPIAlternate,
 }
@@ -492,6 +516,7 @@ class QAPIDomain(Domain):
 roles = {
 "mod": QAPIXRefRole(),
 "cmd": QAPIXRefRole(),
+"event": QAPIXRefRole(),
 "enum": QAPIXRefRole(),
 "alt": QAPIXRefRole(),
 "type": QAPIXRefRole(),  # reference any data type (excludes modules, 
commands, events)
-- 
2.44.0

[PATCH 19/27] docs/qapi-domain: add :unstable: directive option

[John Snow]

Although "unstable" is a feature (and *will* appear in the features
list), add a special :unstable: option to generate an eye-catch that
makes this information very hard to miss.

(The intent is to modify qapidoc.py to add this option whenever it
detects that the features list attached to a definition contains the
"unstable" entry.)

RFC: Same comments as last patch.

Signed-off-by: Harmonie Snow 
Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 4 +++-
 docs/sphinx-static/theme_overrides.css | 6 +-
 docs/sphinx/qapi-domain.py | 8 
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index b9a69f6bd17..6350791a3ed 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -96,13 +96,13 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
 .. qapi:command:: fake-command
:since: 13.37
:deprecated:
+   :unstable:
 
This is a fake command, it's not real. It can't hurt you.
 
:arg int foo: normal parameter documentation.
:arg str bar: Another normal parameter description.
:arg baz: Missing a type.
-   :feat unstable: More than unstable, this command doesn't even exist!
:arg no-descr:
:arg BitmapSyncMode discrim: How about branches in commands?
 
@@ -120,6 +120,8 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
:feat deprecated: Although this command is fake, you should know that
   it's also deprecated. That's great news! Maybe it will go away and
   stop haunting you someday.
+   :feat unstable: This command, as a figment of your imagination, is
+  highly unstable and should not be relied upon.
:error CommandNotFound: When you try to use this command, because it
   isn't real.
:error GenericError: If the system decides it doesn't like the
diff --git a/docs/sphinx-static/theme_overrides.css 
b/docs/sphinx-static/theme_overrides.css
index 97b8c1c60e6..acdf11675db 100644
--- a/docs/sphinx-static/theme_overrides.css
+++ b/docs/sphinx-static/theme_overrides.css
@@ -172,7 +172,7 @@ div[class^="highlight"] pre {
 margin: 0.25em;
 }
 
-.qapi-deprecated {
+.qapi-deprecated,.qapi-unstable {
 background-color: #fffef5;
 border: solid #fff176 6px;
 font-weight: bold;
@@ -181,6 +181,10 @@ div[class^="highlight"] pre {
 margin: 5px;
 }
 
+.qapi-unstable::before {
+content: ' ';
+}
+
 .qapi-deprecated::before {
 content: '⚠️ ';
 }
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 065ad501960..76157476e02 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -151,6 +151,7 @@ class QAPIObject(ObjectDescription[Signature]):
 # These are QAPI originals:
 "since": since_validator,
 "deprecated": directives.flag,
+"unstable": directives.flag,
 }
 )
 
@@ -269,6 +270,13 @@ def _add_pip(source: str, content: str, classname: str) -> 
None:
 "qapi-deprecated",
 )
 
+if "unstable" in self.options:
+_add_pip(
+":unstable:",
+f"This {self.objtype} is unstable/experimental.",
+"qapi-unstable",
+)
+
 if infopips.children:
 contentnode.insert(0, infopips)
 
-- 
2.44.0

[PATCH 10/27] docs/qapi-domain: add "Features:" field lists

[John Snow]

Add support for Features field lists. There is no QAPI-specific
functionality here, but this could be changed if desired (if we wanted
the feature names to link somewhere, for instance.)

This feature list doesn't have any restrictions, so it can be used to
document object-wide features or per-member features as deemed
appropriate. It's essentially free-form text.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  6 ++
 docs/sphinx/qapi-domain.py | 11 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 197587bbc81..a570c37abb2 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -95,9 +95,15 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
:arg int foo: normal parameter documentation.
:arg str bar: Another normal parameter description.
:arg baz: Missing a type.
+   :feat unstable: More than unstable, this command doesn't even exist!
:arg no-descr:
+   :feat hallucination: This command is a figment of your imagination.
 
Field lists can appear anywhere in the directive block, but any field
list entries in the same list block that are recognized as special
("arg") will be reformatted and grouped accordingly for rendered
output.
+
+   At the moment, the order of grouped sections is based on the order in
+   which each group was encountered. This example will render Arguments
+   first, and then Features; but the order can be any that you choose.
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 853bd91b7a8..c0dc6482204 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -33,7 +33,7 @@
 from sphinx.locale import _, __
 from sphinx.roles import XRefRole
 from sphinx.util import logging
-from sphinx.util.docfields import TypedField
+from sphinx.util.docfields import GroupedField, TypedField
 from sphinx.util.docutils import SphinxDirective, switch_source_input
 from sphinx.util.nodes import (
 make_id,
@@ -146,6 +146,15 @@ class QAPIObject(ObjectDescription[Signature]):
 }
 )
 
+doc_field_types = [
+GroupedField(
+"feature",
+label=_("Features"),
+names=("feat",),
+can_collapse=True,
+),
+]
+
 def get_signature_prefix(self, sig: str) -> List[nodes.Node]:
 """Returns a prefix to put before the object name in the signature."""
 assert self.objtype
-- 
2.44.0

[PATCH 06/27] docs/qapi-domain: add QAPI xref roles

[John Snow]

Add domain-specific cross-reference syntax. As of this commit, that
means new :qapi:mod:`block-core` and :qapi:obj:`block-core` referencing
syntax.

:mod: will only find modules, but :obj: will find anything registered to
the QAPI domain. (In forthcoming commits, this means commands, events,
enums, etc.)

Creating the cross-references is powered by the QAPIXRefRole class;
resolving them is handled by QAPIDomain.resolve_xref().

QAPIXrefRole is copied almost verbatim from Sphinx's own
PyXrefRole. PyXrefRole (and QAPIXrefRole) adds two features over the
base class:

(1) Creating a cross-reference with e.g. :py:class:`~class.name`
instructs sphinx to omit the fully qualified parts of the resolved name
from the actual link text. This may be useful in the future if we add
namespaces to QAPI documentation, e.g. :qapi:cmd:`~qsd.blockdev-backup`
could link to the QSD-specific documentation for blockdev-backup while
omitting that prefix from the link text.

(2) Prefixing the link target with a "." changes the search behavior to
prefer locally-scoped items first.

I think both of these are worth keeping to help manage future namespace
issues between QEMU, QSD and QGA; but it's possible it's extraneous. It
may possibly be worth keeping just to keep feature parity with Sphinx's
other domains; e.g. "principle of least surprise". Dunno.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  4 +++
 docs/sphinx/qapi-domain.py | 67 +-
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 39ad405fd93..e2223d5f363 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -47,3 +47,7 @@ cross-referencing syntax. Here's a link to `bar-module`, even 
though
 the actual output of that directive was suppressed. Here's a link to
 `block-core`. A link to ```foo-module``` won't resolve because of the
 ``:no-index:`` option we used for that directive.
+
+Explicit cross-referencing syntax for QAPI modules is available with
+``:qapi:mod:`foo```, here's a link to :qapi:mod:`bar-module` and one to
+:qapi:mod:`block-core`.
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 4758451ff0e..d28ac1cb9d8 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -29,6 +29,7 @@
 ObjType,
 )
 from sphinx.locale import _, __
+from sphinx.roles import XRefRole
 from sphinx.util import logging
 from sphinx.util.docutils import SphinxDirective, switch_source_input
 from sphinx.util.nodes import (
@@ -56,6 +57,34 @@ class ObjectEntry(NamedTuple):
 aliased: bool
 
 
+class QAPIXRefRole(XRefRole):
+def process_link(
+self,
+env: BuildEnvironment,
+refnode: Element,
+has_explicit_title: bool,
+title: str,
+target: str,
+) -> tuple[str, str]:
+refnode["qapi:module"] = env.ref_context.get("qapi:module")
+if not has_explicit_title:
+title = title.lstrip(".")  # only has a meaning for the target
+target = target.lstrip("~")  # only has a meaning for the title
+# if the first character is a tilde, don't display the module
+# parts of the contents
+if title[0:1] == "~":
+title = title[1:]
+dot = title.rfind(".")
+if dot != -1:
+title = title[dot + 1 :]
+# if the first character is a dot, search more specific namespaces 
first
+# else search builtins first
+if target[0:1] == ".":
+target = target[1:]
+refnode["refspecific"] = True
+return title, target
+
+
 def _nested_parse(directive: SphinxDirective, content_node: Element) -> None:
 """
 This helper preserves error parsing context across sphinx versions.
@@ -234,7 +263,13 @@ class QAPIDomain(Domain):
 "module": QAPIModule,
 }
 
-roles = {}
+# These are all cross-reference roles; e.g.
+# :qapi:cmd:`query-block`. The keys correlate to the names used in
+# the object_types table values above.
+roles = {
+"mod": QAPIXRefRole(),
+"obj": QAPIXRefRole(),  # reference *any* type of QAPI object.
+}
 
 # Moved into the data property at runtime;
 # this is the internal index of reference-able objects.
@@ -363,6 +398,36 @@ def find_obj(
 matches = [m for m in matches if not m[1].aliased]
 return matches
 
+def resolve_xref(
+self,
+env: BuildEnvironment,
+fromdocname: str,
+builder: Builder,
+type: str,
+target: str,
+node: pending_xref,
+contnode: Element,
+) -> Element | None:
+modname = node.get("qapi:module")
+matches = self.find_obj(modname, target, type)
+multiple_matche

[PATCH 27/27] docs/qapi-domain: add CSS styling

[John Snow]

From: Harmonie Snow 

Improve the general look and feel of generated QAPI docs.

Attempt to limit line lengths to offer a more comfortable measure on
maximized windows, and improve some margin and spacing for field lists.

Signed-off-by: Harmonie Snow 
Signed-off-by: John Snow 
---
 docs/sphinx-static/theme_overrides.css | 50 --
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/docs/sphinx-static/theme_overrides.css 
b/docs/sphinx-static/theme_overrides.css
index b239a762a9e..9a58cf86b5c 100644
--- a/docs/sphinx-static/theme_overrides.css
+++ b/docs/sphinx-static/theme_overrides.css
@@ -18,8 +18,8 @@ h1, h2, .rst-content .toctree-wrapper p.caption, h3, h4, h5, 
h6, legend {
 
 .rst-content dl:not(.docutils) dt {
 border-top: none;
-border-left: solid 3px #ccc;
-background-color: #f0f0f0;
+border-left: solid 5px #bcc6d2;
+background-color: #eaedf1;
 color: black;
 }
 
@@ -162,6 +162,18 @@ div[class^="highlight"] pre {
 
 /* QAPI domain theming */
 
+/* most content in a qapi object definition should not eclipse about
+   80ch, but nested field lists are explicitly exempt due to their
+   two-column nature */
+.qapi dd *:not(dl) {
+max-width: 80ch;
+}
+
+/* but the content column itself should still be less than ~80ch. */
+.qapi .field-list dd {
+max-width: 80ch;
+}
+
 .qapi-infopips {
 margin-bottom: 1em;
 }
@@ -201,3 +213,37 @@ div[class^="highlight"] pre {
 border-radius: 15px;
 margin: 5px;
 }
+
+/* code blocks */
+.qapi div[class^="highlight"] {
+width: fit-content;
+background-color: #fffafd;
+border: 2px solid #ffe1f3;
+}
+
+/* note, warning, etc. */
+.qapi .admonition {
+width: fit-content;
+}
+
+/* pad the top of the field-list so the text doesn't start directly at
+   the top border; primarily for the field list labels, but adjust the
+   field bodies as well for parity. */
+dl.field-list > dt:first-of-type, dl.field-list > dd:first-of-type {
+padding-top: 0.3em;
+}
+
+dl.field-list > dt:last-of-type, dl.field-list > dd:last-of-type {
+padding-bottom: 0.3em;
+}
+
+/* pad the field list labels so they don't crash into the border */
+dl.field-list > dt {
+padding-left: 0.5em;
+padding-right: 0.5em;
+}
+
+/* Add a little padding between field list sections */
+dl.field-list > dd:not(:last-child) {
+padding-bottom: 1em;
+}
-- 
2.44.0

[PATCH 23/27] docs/qapi-domain: RFC patch - delete malformed field lists

[John Snow]

Cleanup of the last patch to fix the build before closing out this RFC
series.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst | 4 
 1 file changed, 4 deletions(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index ef58dfc4bcd..8352a27d4a5 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -129,10 +129,6 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
   argument values. It's very temperamental.
:return SomeTypeName: An esoteric collection of mystical nonsense to
   both confound and delight.
-   :arg: this is malformed.
-   :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
-- 
2.44.0

[PATCH 01/27] docs/sphinx: create QAPI domain extension stub

[John Snow]

It doesn't really do anything yet, we'll get to it brick-by-brick in the
forthcoming commits to keep the series breezy and the git history
informative.

Signed-off-by: John Snow 
---
 docs/conf.py   |  3 ++-
 docs/sphinx/qapi-domain.py | 50 ++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 docs/sphinx/qapi-domain.py

diff --git a/docs/conf.py b/docs/conf.py
index aae0304ac6e..b15665d956d 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -61,7 +61,8 @@
 # Add any Sphinx extension module names here, as strings. They can be
 # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
 # ones.
-extensions = ['kerneldoc', 'qmp_lexer', 'hxtool', 'depfile', 'qapidoc']
+extensions = ['kerneldoc', 'hxtool', 'depfile',
+  'qapidoc', 'qapi-domain', 'qmp_lexer']
 
 if sphinx.version_info[:3] > (4, 0, 0):
 tags.add('sphinx4')
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
new file mode 100644
index 000..163b9ff21c3
--- /dev/null
+++ b/docs/sphinx/qapi-domain.py
@@ -0,0 +1,50 @@
+"""
+QAPI domain extension.
+"""
+
+from __future__ import annotations
+
+from typing import (
+TYPE_CHECKING,
+Any,
+Dict,
+List,
+Tuple,
+)
+
+from sphinx.domains import Domain, ObjType
+from sphinx.util import logging
+
+
+if TYPE_CHECKING:
+from sphinx.application import Sphinx
+
+logger = logging.getLogger(__name__)
+
+
+class QAPIDomain(Domain):
+"""QAPI language domain."""
+
+name = "qapi"
+label = "QAPI"
+
+object_types: Dict[str, ObjType] = {}
+directives = {}
+roles = {}
+initial_data: Dict[str, Dict[str, Tuple[Any]]] = {}
+indices = []
+
+def merge_domaindata(self, docnames: List[str], otherdata: Dict[str, Any]) 
-> None:
+pass
+
+
+def setup(app: Sphinx) -> Dict[str, Any]:
+app.setup_extension("sphinx.directives")
+app.add_domain(QAPIDomain)
+
+return {
+"version": "1.0",
+"env_version": 1,
+"parallel_read_safe": True,
+"parallel_write_safe": True,
+}
-- 
2.44.0

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

[PATCH 09/27] docs/qapi-domain: add "Arguments:" field lists

[John Snow]

This adds special rendering for Sphinx's typed field lists.

This patch does not add any QAPI-aware markup, rendering, or
cross-referencing for the type names, yet. That feature requires a
subclass to TypedField which will happen in its own commit quite a bit
later in this series; after all the basic fields and objects have been
established first.

The syntax for this field is:

:arg type name: description
   description cont'd

You can omit the type or the description, but you cannot omit the name
-- if you do so, it degenerates into a "normal field list" entry, and
probably isn't what you want.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 15 +++
 docs/sphinx/qapi-domain.py | 14 --
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 33b9349a3ee..197587bbc81 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -86,3 +86,18 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
* ``:no-index-entry:`` or ``:noindexentry:``
* ``:no-contents-entry:`` or ``:nocontentsentry:``
* ``:no-typesetting:``
+
+.. qapi:command:: fake-command
+   :since: 13.37
+
+   This is a fake command, it's not real. It can't hurt you.
+
+   :arg int foo: normal parameter documentation.
+   :arg str bar: Another normal parameter description.
+   :arg baz: Missing a type.
+   :arg no-descr:
+
+   Field lists can appear anywhere in the directive block, but any field
+   list entries in the same list block that are recognized as special
+   ("arg") will be reformatted and grouped accordingly for rendered
+   output.
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 38a50318d08..853bd91b7a8 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -33,6 +33,7 @@
 from sphinx.locale import _, __
 from sphinx.roles import XRefRole
 from sphinx.util import logging
+from sphinx.util.docfields import TypedField
 from sphinx.util.docutils import SphinxDirective, switch_source_input
 from sphinx.util.nodes import (
 make_id,
@@ -254,8 +255,17 @@ def _toc_entry_name(self, sig_node: desc_signature) -> str:
 class QAPICommand(QAPIObject):
 """Description of a QAPI Command."""
 
-# Nothing unique for now! Changed in later commits O:-)
-pass
+doc_field_types = QAPIObject.doc_field_types.copy()
+doc_field_types.extend(
+[
+TypedField(
+"argument",
+label=_("Arguments"),
+names=("arg",),
+can_collapse=True,
+),
+]
+)
 
 
 class QAPIModule(SphinxDirective):
-- 
2.44.0

[PATCH 26/27] docs/qapi-domain: RFC patch - Add one last sample command

[John Snow]

Just to have a bit more to look at in the generated doc, here's a fairly
complex command with a lot of bells and whistles.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst | 67 +
 1 file changed, 67 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 6e85ea5280d..4562e755d21 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -267,6 +267,73 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
   :memb str key-secret: ID of a QCryptoSecret object providing a
  passphrase for unlocking the encryption
 
+.. qapi:command:: blockdev-backup
+   :since: 4.2
+
+   Start a point-in-time copy of a block device to a new
+   destination. The status of ongoing blockdev-backup operations can be
+   checked with query-block-jobs where the BlockJobInfo.type field has
+   the value ‘backup’. The operation can be stopped before it has
+   completed using the block-job-cancel command.
+
+   :arg str target:
+  the device name or node-name of the backup target node.
+   :arg str? job-id:
+  identifier for the newly-created block job. If omitted, the device
+  name will be used. (Since 2.7)
+   :arg str device:
+  the device name or node-name of a root node which should be copied.
+   :arg MirrorSyncMode sync:
+  what parts of the disk image should be copied to the destination
+  (all the disk, only the sectors allocated in the topmost image,
+  from a dirty bitmap, or only new I/O).
+   :arg int? speed:
+  the maximum speed, in bytes per second. The default is 0, for unlimited.
+   :arg str? bitmap:
+  The name of a dirty bitmap to use. Must be present if sync is
+  ``bitmap`` or ``incremental``. Can be present if sync is ``full`` or
+  ``top``. Must not be present otherwise. (Since 2.4 (drive-backup),
+  3.1 (blockdev-backup))
+   :arg BitmapSyncMode? bitmap-mode:
+  Specifies the type of data the bitmap should contain after the
+  operation concludes. Must be present if a bitmap was provided,
+  Must NOT be present otherwise. (Since 4.2)
+   :arg bool? compress:
+  true to compress data, if the target format supports it. (default:
+  false) (since 2.8)
+   :arg BlockdevOnError? on-source-error:
+  the action to take on an error on the source, default
+  ``report``. ``stop`` and ``enospc`` can only be used if the block device
+  supports io-status (see BlockInfo).
+   :arg BlockdevOnError? on-target-error:
+  the action to take on an error on the target, default ``report`` (no
+  limitations, since this applies to a different block device than
+  device).
+   :arg bool? auto-finalize:
+  When false, this job will wait in a ``PENDING`` state after it has
+  finished its work, waiting for block-job-finalize before making
+  any block graph changes. When true, this job will automatically
+  perform its abort or commit actions. Defaults to true. (Since
+  2.12)
+   :arg bool? auto-dismiss:
+  When false, this job will wait in a ``CONCLUDED`` state after it has
+  completely ceased all work, and awaits block-job-dismiss. When
+  true, this job will automatically disappear from the query list
+  without user intervention. Defaults to true. (Since 2.12)
+   :arg str? filter-node-name:
+  the node name that should be assigned to the filter driver that
+  the backup job inserts into the graph above node specified by
+  drive. If this option is not given, a node name is
+  autogenerated. (Since: 4.2)
+   :arg BackupPerf? x-perf:
+  Performance options. (Since 6.0)
+   :feat unstable: Member ``x-perf`` is experimental.
+   :error DeviceNotFound: if ``device`` is not a valid block device.
+
+   .. note:: ``on-source-error`` and ``on-target-error only`` affect
+ background I/O. If an error occurs during a guest write
+ request, the device's rerror/werror actions will be used.
+
 .. qapi:command:: x-debug-query-block-graph
:since: 4.0
:unstable:
-- 
2.44.0

[PATCH 03/27] docs/qapi-module: add QAPI domain object registry

[John Snow]

This is the first step towards QAPI domain cross-references and a QAPI
reference index.

For now, just create the object registry and amend the qapi:module
directive to use that registry. Update the merge_domaindata method now
that we have actual data we may need to merge.

RFC: Much of this code is adapted directly from sphinx.domains.python;
it is unclear to me if there ever would be a collision on merge, hence
the assertion. I haven't been able to trigger it or find better code to
use as a template, so probably I'll just leave the assertion in there.

Signed-off-by: John Snow 
---
 docs/sphinx/qapi-domain.py | 76 --
 1 file changed, 73 insertions(+), 3 deletions(-)

diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 7c5e4407bc1..ab80fd5f634 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -11,6 +11,7 @@
 Dict,
 Iterable,
 List,
+NamedTuple,
 Tuple,
 cast,
 )
@@ -20,6 +21,7 @@
 
 from sphinx import addnodes
 from sphinx.domains import Domain, ObjType
+from sphinx.locale import _, __
 from sphinx.util import logging
 from sphinx.util.docutils import SphinxDirective, switch_source_input
 from sphinx.util.nodes import make_id, nested_parse_with_titles
@@ -34,6 +36,13 @@
 logger = logging.getLogger(__name__)
 
 
+class ObjectEntry(NamedTuple):
+docname: str
+node_id: str
+objtype: str
+aliased: bool
+
+
 def _nested_parse(directive: SphinxDirective, content_node: Element) -> None:
 """
 This helper preserves error parsing context across sphinx versions.
@@ -101,6 +110,7 @@ class QAPIModule(SphinxDirective):
 }
 
 def run(self) -> List[Node]:
+domain = cast(QAPIDomain, self.env.get_domain("qapi"))
 modname = self.arguments[0].strip()
 no_index = "no-index" in self.options or "noindex" in self.options
 
@@ -113,11 +123,14 @@ def run(self) -> List[Node]:
 inode = addnodes.index(entries=[])
 
 if not no_index:
+# note module to the domain
 node_id = make_id(self.env, self.state.document, "module", modname)
 target = nodes.target("", "", ids=[node_id], ismod=True)
 self.set_source_info(target)
 self.state.document.note_explicit_target(target)
 
+domain.note_object(modname, "module", node_id, location=target)
+
 indextext = f"QAPI module; {modname}"
 inode = addnodes.index(
 entries=[
@@ -148,7 +161,12 @@ class QAPIDomain(Domain):
 name = "qapi"
 label = "QAPI"
 
-object_types: Dict[str, ObjType] = {}
+# This table associates cross-reference object types (key) with an
+# ObjType instance, which defines the valid cross-reference roles
+# for each object type.
+object_types: Dict[str, ObjType] = {
+"module": ObjType(_("module"), "mod", "obj"),
+}
 
 # Each of these provides a ReST directive,
 # e.g. .. qapi:module:: block-core
@@ -157,11 +175,63 @@ class QAPIDomain(Domain):
 }
 
 roles = {}
-initial_data: Dict[str, Dict[str, Tuple[Any]]] = {}
+
+# Moved into the data property at runtime;
+# this is the internal index of reference-able objects.
+initial_data: Dict[str, Dict[str, Tuple[Any]]] = {
+"objects": {},  # fullname -> ObjectEntry
+}
+
 indices = []
 
+@property
+def objects(self) -> Dict[str, ObjectEntry]:
+return self.data.setdefault("objects", {})  # type: 
ignore[no-any-return]
+
+def note_object(
+self,
+name: str,
+objtype: str,
+node_id: str,
+aliased: bool = False,
+location: Any = None,
+) -> None:
+"""Note a QAPI object for cross reference."""
+if name in self.objects:
+other = self.objects[name]
+if other.aliased and aliased is False:
+# The original definition found. Override it!
+pass
+elif other.aliased is False and aliased:
+# The original definition is already registered.
+return
+else:
+# duplicated
+logger.warning(
+__(
+"duplicate object description of %s, "
+"other instance in %s, use :no-index: for one of them"
+),
+name,
+other.docname,
+location=location,
+)
+self.objects[name] = ObjectEntry(self.env.docname, node_id, objtype, 
aliased)
+
+def clear_doc(self, docname: str) -> None:
+for fullname, obj in list(self.objects.items()):
+if obj.docname == 

[PATCH 00/27] Add qapi-domain Sphinx extension

[John Snow]

This series adds a new qapi-domain extension for Sphinx, which adds a
series of custom directives for documenting QAPI definitions.

GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/1259566476

(Link to a demo HTML page at the end of this cover letter, but I want
you to read the cover letter first to explain what you're seeing.)

This adds a new QAPI Index page, cross-references for QMP commands,
events, and data types, and improves the aesthetics of the QAPI/QMP
documentation.

This series adds only the new ReST syntax, *not* the autogenerator. The
ReST syntax used in this series is, in general, not intended for anyone
to actually write by hand. This mimics how Sphinx's own autodoc
extension generates Python domain directives, which are then re-parsed
to produce the final result.

I have prototyped such a generator, but it isn't ready for inclusion
yet. (Rest assured: error context reporting is preserved down to the
line, even in generated ReST. There is no loss in usability for this
approach. It will likely either supplant qapidoc.py or heavily alter
it.) The generator requires only extremely minor changes to
scripts/qapi/parser.py to preserve nested indentation and provide more
accurate line information. It is less invasive than you may
fear. Relying on a secondary ReST parse phase eliminates much of the
complexity of qapidoc.py. Sleep soundly.

The purpose of sending this series in its current form is largely to
solicit feedback on general aesthetics, layout, and features. Sphinx is
a wily beast, and feedback at this stage will dictate how and where
certain features are implemented.

A goal for this syntax (and the generator) is to fully in-line all
command/event/object members, inherited or local, boxed or not, branched
or not. This should provide a boon to using these docs as a reference,
because users will not have to grep around the page looking for various
types, branches, or inherited members. Any arguments types will be
hyperlinked to their definition, further aiding usability. Commands can
be hotlinked from anywhere else in the manual, and will provide a
complete reference directly on the first screenful of information.

(Okay, maybe two screenfuls for commands with a ton of
arguments... There's only so much I can do!)

This RFC series includes a "sandbox" .rst document that showcases the
features of this domain by writing QAPI directives by hand; this
document will be removed from the series before final inclusion. It's
here to serve as a convenient test-bed for y'all to give feedback.

All that said, here's the sandbox document fully rendered:
https://jsnow.gitlab.io/qemu/qapi/index.html

And here's the new QAPI index page created by that sandbox document:
https://jsnow.gitlab.io/qemu/qapi-index.html

Known issues / points of interest:

- The formatting upsets checkpatch. The default line length for the
  "black" formatting tool is a little long. I'll fix it next spin.

- I did my best to preserve cross-version compatibility, but some
  problems have crept in here and there. This series may require
  Sphinx>= 4.0, like the dbus extension. Notably, the Ubuntu build fails
  on Gitlab CI currently. The next version will text against more Sphinx
  versions more rigorously. Sphinx version 5.3.0 and above should work
  perfectly.

- There's a bug in Sphinx itself that may manifest in your testing,
  concerning reported error locations. There's a patch in this series
  that fixes it, but it's later in the series. If you run into the bug
  while testing with this series, try applying that patch first.

- QAPI 'namespaces' aren't yet handled. i.e., there's nothing to
  distinguish entities between QMP, QGA and QSD yet. That feature will
  be added to a future version of this patchset (Likely when the
  generator is ready for inclusion: without it, references will clash
  and the index will gripe about duplicated entries.)

- Per-member features and ifcond are not yet accounted for; though
  definition-scoped features and ifconds are. Please feel free to
  suggest how you'd like to see those represented.

- Inlining all members means there is some ambiguity on what to do with
  doc block sections on inlined entities; features and members have an
  obvious home - body, since, and misc sections are not as obvious on
  how to handle. This will feature more prominently in the generator
  series.

- Some features could be implemented in either the QAPI domain syntax
  *or* the generator, or some combination of both. Depending on
  aesthetic feedback, this may influence where those features should be
  implemented.

- The formatting and aesthetics of branches are a little up in the air,
  see the qapi:union patch for more details.

- It's late, maybe other things. Happy Friday!

Hope you like it!

--js

Harmonie Snow (1):
  docs/qapi-domain: add CSS styling

John Snow (26):
  docs/sphinx: create QAPI domain extension stub
  docs/qapi-domain: add qapi:module directive
  docs/qapi-

[PATCH 05/27] docs/qapi-domain: add resolve_any_xref()

[John Snow]

Add the ability to resolve cross-references using the `any`
cross-reference syntax. Adding QAPI-specific cross-reference roles will
be added in a forthcoming commit, and will share the same find_obj()
helper.

(There's less code needed for the generic cross-reference resolver, so
it comes first in this series.)

Once again, this code is based very heavily on sphinx.domains.python.

Signed-off-by: John Snow 
---
 docs/qapi/index.rst|  7 +++
 docs/sphinx/qapi-domain.py | 95 +-
 2 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 051dc6b3a37..39ad405fd93 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -40,3 +40,10 @@ QAPI Domain Test
 
Modules will also be reported in the `qapi-index`, under the Modules
category and in the alphabetical categories that follow.
+
+
+QAPI modules can now be cross-referenced using the ```any```
+cross-referencing syntax. Here's a link to `bar-module`, even though
+the actual output of that directive was suppressed. Here's a link to
+`block-core`. A link to ```foo-module``` won't resolve because of the
+``:no-index:`` option we used for that directive.
diff --git a/docs/sphinx/qapi-domain.py b/docs/sphinx/qapi-domain.py
index 65409786119..4758451ff0e 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -21,6 +21,7 @@
 from docutils.parsers.rst import directives
 
 from sphinx import addnodes
+from sphinx.addnodes import pending_xref
 from sphinx.domains import (
 Domain,
 Index,
@@ -30,13 +31,19 @@
 from sphinx.locale import _, __
 from sphinx.util import logging
 from sphinx.util.docutils import SphinxDirective, switch_source_input
-from sphinx.util.nodes import make_id, nested_parse_with_titles
+from sphinx.util.nodes import (
+make_id,
+make_refnode,
+nested_parse_with_titles,
+)
 
 
 if TYPE_CHECKING:
 from docutils.nodes import Element, Node
 
 from sphinx.application import Sphinx
+from sphinx.builders import Builder
+from sphinx.environment import BuildEnvironment
 from sphinx.util.typing import OptionSpec
 
 logger = logging.getLogger(__name__)
@@ -289,6 +296,92 @@ def merge_domaindata(self, docnames: List[str], otherdata: 
Dict[str, Any]) -> No
 ), f"!?!? collision on merge? {fullname=} {obj=} 
{self.objects[fullname]=}"
 self.objects[fullname] = obj
 
+def find_obj(
+self, modname: str, name: str, type: Optional[str]
+) -> list[tuple[str, ObjectEntry]]:
+"""
+Find a QAPI object for "name", perhaps using the given module.
+
+Returns a list of (name, object entry) tuples.
+
+:param modname: The current module context (if any!)
+under which we are searching.
+:param name: The name of the x-ref to resolve;
+ may or may not include a leading module.
+:param type: The role name of the x-ref we're resolving, if provided.
+ (This is absent for "any" lookups.)
+"""
+if not name:
+return []
+
+names: list[str] = []
+matches: list[tuple[str, ObjectEntry]] = []
+
+fullname = name
+if "." in fullname:
+# We're searching for a fully qualified reference;
+# ignore the contextual module.
+pass
+elif modname:
+# We're searching for something from somewhere;
+# try searching the current module first.
+# e.g. :qapi:cmd:`query-block` or `query-block` is being searched.
+fullname = f"{modname}.{name}"
+
+if type is None:
+# type isn't specified, this is a generic xref.
+# search *all* qapi-specific object types.
+objtypes: Optional[List[str]] = list(self.object_types)
+else:
+# type is specified and will be a role (e.g. obj, mod, cmd)
+# convert this to eligible object types (e.g. command, module)
+# using the QAPIDomain.object_types table.
+objtypes = self.objtypes_for_role(type)
+
+# Either we should have been given no type, or the type we were
+# given should correspond to at least one real actual object
+# type.
+assert objtypes
+
+if name in self.objects and self.objects[name].objtype in objtypes:
+names = [name]
+elif fullname in self.objects and self.objects[fullname].objtype in 
objtypes:
+names = [fullname]
+else:
+# exact match wasn't found; e.g. we are searching for
+# `query-block` from a different (or no) module.
+searchname = "." + name
+names = [
+oname
+for oname in self.objects
+if oname.endswith(searchname)
+   

[PATCH 12/27] docs/qapi-domain: add "Returns:" field lists

[John Snow]

Add "Returns:" field list syntax to QAPI Commands.

Like "Arguments:" and "Errors:", the type name isn't currently processed
for cross-referencing, but this will be addressed in a forthcoming
commit.

This patch adds "errors" as a GroupedField, which means that multiple
return values can be annotated - this is only done because Sphinx does
not seemingly (Maybe I missed it?) support mandatory type arguments to
Ungrouped fields. Because we want to cross-reference this type
information later, we want to make the type argument mandatory. As a
result, you can technically add multiple :return: fields, though I'm not
aware of any circumstance in which you'd need or want
to. Recommendation: "Don't do that, then."

Since this field describes an action/event instead of describing a list
of nouns (arguments, features, errors), I added both the imperative and
indicative forms (:return: and :returns:) to allow doc writers to use
whichever mood "feels right" in the source document. The rendered output
will always use the "Returns:" label, however.

I'm sure you'll let me know how you feel about that. O:-)

Signed-off-by: John Snow 
---
 docs/qapi/index.rst| 2 ++
 docs/sphinx/qapi-domain.py | 6 ++
 2 files changed, 8 insertions(+)

diff --git a/docs/qapi/index.rst b/docs/qapi/index.rst
index 004d02e0437..39fe4dd2dae 100644
--- a/docs/qapi/index.rst
+++ b/docs/qapi/index.rst
@@ -102,6 +102,8 @@ Explicit cross-referencing syntax for QAPI modules is 
available with
   isn't real.
:error GenericError: If the system decides it doesn't like the
   argument values. It's very temperamental.
+   :return SomeTypeName: An esoteric collection of mystical nonsense to
+  both confound and delight.
 
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 1f0b168fa2c..5d44dba6cd3 100644
--- a/docs/sphinx/qapi-domain.py
+++ b/docs/sphinx/qapi-domain.py
@@ -279,6 +279,12 @@ class QAPICommand(QAPIObject):
 names=("error",),
 can_collapse=True,
 ),
+GroupedField(
+"returnvalue",
+label=_("Returns"),
+names=("return", "returns"),
+can_collapse=True,
+),
 ]
 )
 
-- 
2.44.0

Re: [PATCH 3/3] qapi: Fix bogus documentation of query-migrationthreads

[John Snow]

On Fri, Mar 22, 2024, 9:51 AM Markus Armbruster  wrote:

> The doc comment documents an argument that doesn't exist.  Would
> fail compilation if it was marked up correctly.  Delete.
>
> The Returns: section fails to refer to the data type, leaving the user
> to guess.  Fix that.
>
> The command name violates QAPI naming rules: it should be
> query-migration-threads.  Too late to fix.
>
> Reported-by: John Snow 
> Fixes: 671326201dac (migration: Introduce interface query-migrationthreads)
> Signed-off-by: Markus Armbruster 
>

Reviewed-by: John Snow 

---
>  qapi/migration.json | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index f6238b6980..e47ad7a63b 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -2419,9 +2419,7 @@
>  #
>  # Returns information of migration threads
>  #
> -# data: migration thread name
> -#
> -# Returns: information about migration threads
> +# Returns: @MigrationThreadInfo
>  #
>  # Since: 7.2
>  ##
> --
> 2.44.0
>
>

Re: [PATCH 01/12] qapi: Drop stray Arguments: line from qmp_capabilities docs

[John Snow]

On Fri, Mar 22, 2024, 10:09 AM Markus Armbruster  wrote:

> Reported-by: John Snow 
> Fixes: 119ebac1feb2 (qapi-schema: use generated marshaller for
> 'qmp_capabilities')
> Signed-off-by: Markus Armbruster 
>

Reviewed-by: John Snow 

---
>  qapi/control.json | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/qapi/control.json b/qapi/control.json
> index f404daef60..6bdbf077c2 100644
> --- a/qapi/control.json
> +++ b/qapi/control.json
> @@ -11,8 +11,6 @@
>  #
>  # Enable QMP capabilities.
>  #
> -# Arguments:
> -#
>  # @enable: An optional list of QMPCapability values to enable.  The
>  # client must not enable any capability that is not mentioned in
>  # the QMP greeting message.  If the field is not provided, it
> --
> 2.44.0
>
>

Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

[John Snow]

On Tue, Mar 19, 2024, 12:02 PM Markus Armbruster  wrote:

> John Snow  writes:
>
> > On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster 
> wrote:
> >
> >> Entities with names starting with q_obj_ are implicit object types.
> >> Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity()
> >> can only return a QAPISchemaObjectType.  Assert that.
> >>
> >> Signed-off-by: Markus Armbruster 
> >> ---
> >>  scripts/qapi/schema.py | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> index e52930a48a..a6180f93c6 100644
> >> --- a/scripts/qapi/schema.py
> >> +++ b/scripts/qapi/schema.py
> >> @@ -1297,8 +1297,9 @@ def _make_implicit_object_type(
> >>  return None
> >>  # See also QAPISchemaObjectTypeMember.describe()
> >>  name = 'q_obj_%s-%s' % (name, role)
> >> -typ = self.lookup_entity(name, QAPISchemaObjectType)
> >> +typ = self.lookup_entity(name)
> >>  if typ:
> >> +assert(isinstance(typ, QAPISchemaObjectType))
> >>  # The implicit object type has multiple users.  This can
> >>  # only be a duplicate definition, which will be flagged
> >>  # later.
> >> --
> >> 2.44.0
> >>
> >
> > Seems obviously fine, though I don't suppose this narrowing will be
> > "remembered" by the type system. Do we care?
>
> mypy passes without it.  It's for catching programming errors and
> helping the reader along.  The former are unlikely, and the latter is
> debatable, but when in doubt, assert.
>

mmhmm. I was just wondering if we could tighten the typing of typ itself,
or if it conflicted with legitimate broader uses. it happens a lot in qapi
that we're regulated by broader base types in parent classes etc.

If we CAN tighten the variable/field (without runtime code changes), we
should do so. If we can't, this patch is 100% totally fine as is.


> > Reviewed-by: John Snow 
>
> Thanks!
>
>

Re: [PATCH v5 25/25] qapi: Dumb down QAPISchema.lookup_entity()

[John Snow]

On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster  wrote:

> QAPISchema.lookup_entity() takes an optional type argument, a subtype
> of QAPISchemaDefinition, and returns that type or None.  Callers can
> use this to save themselves an isinstance() test.
>
> The only remaining user of this convenience feature is .lookup_type().
> But we don't actually save anything anymore there: we still the
> isinstance() to help mypy over the hump.
>
> Drop the .lookup_entity() argument, and adjust .lookup_type().
>
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/schema.py | 18 ++
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index a6180f93c6..5924947fc3 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -1157,20 +1157,14 @@ def _def_definition(self, defn:
> QAPISchemaDefinition) -> None:
>  defn.info, "%s is already defined" %
> other_defn.describe())
>  self._entity_dict[defn.name] = defn
>
> -def lookup_entity(
> -self,
> -name: str,
> -typ: Optional[type] = None,
> -) -> Optional[QAPISchemaDefinition]:
> -ent = self._entity_dict.get(name)
> -if typ and not isinstance(ent, typ):
> -return None
> -return ent
> +def lookup_entity(self,name: str) -> Optional[QAPISchemaEntity]:
> +return self._entity_dict.get(name)
>
>  def lookup_type(self, name: str) -> Optional[QAPISchemaType]:
> -typ = self.lookup_entity(name, QAPISchemaType)
> -assert typ is None or isinstance(typ, QAPISchemaType)
> -return typ
> +typ = self.lookup_entity(name)
> +if isinstance(typ, QAPISchemaType):
> +return typ
> +    return None
>
>  def resolve_type(
>  self,
> --
> 2.44.0
>

Sure, dealer's choice.

with your commit message fixup:

Reviewed-by: John Snow 

>

Re: [PATCH v5 24/25] qapi: Tighten check whether implicit object type already exists

[John Snow]

On Fri, Mar 15, 2024, 11:23 AM Markus Armbruster  wrote:

> Entities with names starting with q_obj_ are implicit object types.
> Therefore, QAPISchema._make_implicit_object_type()'s .lookup_entity()
> can only return a QAPISchemaObjectType.  Assert that.
>
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/schema.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index e52930a48a..a6180f93c6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -1297,8 +1297,9 @@ def _make_implicit_object_type(
>  return None
>  # See also QAPISchemaObjectTypeMember.describe()
>  name = 'q_obj_%s-%s' % (name, role)
> -typ = self.lookup_entity(name, QAPISchemaObjectType)
> +typ = self.lookup_entity(name)
>  if typ:
> +assert(isinstance(typ, QAPISchemaObjectType))
>  # The implicit object type has multiple users.  This can
>  # only be a duplicate definition, which will be flagged
>  # later.
> --
> 2.44.0
>

Seems obviously fine, though I don't suppose this narrowing will be
"remembered" by the type system. Do we care?

Reviewed-by: John Snow 

>

Re: [PATCH v5 12/25] qapi: Assert built-in types exist

[John Snow]

On Fri, Mar 15, 2024, 11:24 AM Markus Armbruster  wrote:

> QAPISchema.lookup_type('FOO') returns a QAPISchemaType when type 'FOO'
> exists, else None.  It won't return None for built-in types like
> 'int'.
>
> Since mypy can't see that, it'll complain that we assign the
> Optional[QAPISchemaType] returned by .lookup_type() to QAPISchemaType
> variables.
>
> Add assertions to help it over the hump.
>
> Signed-off-by: Markus Armbruster 
> ---
>  scripts/qapi/introspect.py | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 67c7d89aae..4679b1bc2c 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -227,10 +227,14 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>
>  # Map the various integer types to plain int
>  if typ.json_type() == 'int':
> -typ = self._schema.lookup_type('int')
> +type_int = self._schema.lookup_type('int')
> +assert type_int
> +typ = type_int
>  elif (isinstance(typ, QAPISchemaArrayType) and
>typ.element_type.json_type() == 'int'):
> -typ = self._schema.lookup_type('intList')
> +type_intList = self._schema.lookup_type('intList')
> +assert type_intList
> +typ = type_intList
>  # Add type to work queue if new
>  if typ not in self._used_types:
>  self._used_types.append(typ)
> --
> 2.44.0
>

Yeah, if you like this more, go ahead. I know it works because I did it
this way at one point!

Matter of taste and preference etc.

Reviewed-by: John Snow 

>

Re: [PATCH v4 21/23] qapi/schema: add type hints

[John Snow]

On Fri, Mar 15, 2024, 10:03 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > This patch only adds type hints, which aren't utilized at runtime and
> > don't change the behavior of this module in any way.
> >
> > In a scant few locations, type hints are removed where no longer
> > necessary due to inference power from typing all of the rest of
> > creation; and any type hints that no longer need string quotes are
> > changed.
> >
> > Signed-off-by: John Snow 
> > ---
> >  scripts/qapi/schema.py | 568 -
> >  1 file changed, 396 insertions(+), 172 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 3b8c2ebbb5f..d2faaea6eac 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
>
> [...]
>
> > @@ -1006,18 +1181,27 @@ def _def_definition(self, defn):
> >  defn.info, "%s is already defined" %
> other_defn.describe())
> >  self._entity_dict[defn.name] = defn
> >
> > -def lookup_entity(self, name, typ=None):
> > +def lookup_entity(
> > +self,
> > +name: str,
> > +typ: Optional[type] = None,
> > +) -> Optional[QAPISchemaEntity]:
>
> Optional[QAPISchemaDefinition], actually.
>

Ah! Very good catch.


> >  ent = self._entity_dict.get(name)
> >  if typ and not isinstance(ent, typ):
> >  return None
> >  return ent
>
> [...]
>
>

Re: [PATCH v4 00/23] qapi: statically type schema.py

[John Snow]

On Thu, Mar 14, 2024, 10:43 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > This is *v4*, for some definitions of "version" and "four".
>
> Series
> Reviewed-by: Markus Armbruster 
>
> I'll replace PATCH 12, not because it's wrong, just because I like my
> replacement better.  I'll post this and two follow-up patches as v5.
> I will not let the two follow-up patches delay queuing, though.
>
> Thanks!
>

Sounds good! I'll divert efforts to improving sphinx docs next unless you
have a higher priority to suggest.

Re: [PATCH v4 05/23] qapi: create QAPISchemaDefinition

[John Snow]

On Thu, Mar 14, 2024, 10:04 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > On Thu, Mar 14, 2024, 5:12 AM Markus Armbruster 
> wrote:
> >
> >> John Snow  writes:
> >>
> >> > Include entities don't have names, but we generally expect "entities"
> to
> >> > have names. Reclassify all entities with names as *definitions*,
> leaving
> >> > the nameless include entities as QAPISchemaEntity instances.
> >> >
> >> > This is primarily to help simplify typing around expectations of what
> >> > callers expect for properties of an "entity".
> >> >
> >> > Suggested-by: Markus Armbruster 
> >> > Signed-off-by: John Snow 
> >> > ---
> >> >  scripts/qapi/schema.py | 144
> +++--
> >> >  1 file changed, 82 insertions(+), 62 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index 117f0f78f0c..da273c1649d 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >>
> >> [...]
> >>
> >> > @@ -107,6 +90,46 @@ def _set_module(self, schema, info):
> >> >  def set_module(self, schema):
> >> >  self._set_module(schema, self.info)
> >> >
> >> > +def visit(self, visitor):
> >> > +# pylint: disable=unused-argument
> >> > +assert self._checked
> >> > +
> >> > +
> >> > +class QAPISchemaDefinition(QAPISchemaEntity):
> >> > +meta: Optional[str] = None
> >> > +
> >> > +def __init__(self, name: str, info, doc, ifcond=None,
> features=None):
> >> > +assert isinstance(name, str)
> >> > +super().__init__(info)
> >> > +for f in features or []:
> >> > +assert isinstance(f, QAPISchemaFeature)
> >> > +f.set_defined_in(name)
> >> > +self.name = name
> >> > +self.doc = doc
> >> > +self._ifcond = ifcond or QAPISchemaIfCond()
> >> > +self.features = features or []
> >> > +
> >> > +def __repr__(self):
> >> > +return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
> >> > +id(self))
> >> > +
> >> > +def c_name(self):
> >> > +return c_name(self.name)
> >> > +
> >> > +def check(self, schema):
> >> > +assert not self._checked
> >> > +seen = {}
> >> > +for f in self.features:
> >> > +f.check_clash(self.info, seen)
> >> > +super().check(schema)
> >>
> >> v3 called super().check() first.  Why did you move it?  I'm asking just
> >> to make sure I'm not missing something subtle.
> >>
> >
> > I don't believe you are, I think it was just ... something I didn't
> notice
> > when rebasing, since I merged this patch by hand. I recall having to
> insert
> > the super call manually, and I guess my preferences are nondeterministic.
>
> I'd like to move it back, for consistency with its subtypes' .check(),
> which all call super().check() early.  Okay?
>

Yes, please do.

(You *really* don't have to ask, but I understand you are being courteous
to a fault. It's your module, though! I argue a lot but it *is* yours. Run
it through the armbru delinter as much as you'd like.)


> >> > +
> >> > +def connect_doc(self, doc=None):
> >> > +super().connect_doc(doc)
> >> > +doc = doc or self.doc
> >> > +if doc:
> >> > +for f in self.features:
> >> > +doc.connect_feature(f)
> >> > +
> >> >  @property
> >> >  def ifcond(self):
> >> >  assert self._checked
> >>
> >> [...]
> >>
> >>
>
>

Re: [PATCH v4 16/23] qapi/schema: Don't initialize "members" with `None`

[John Snow]

On Thu, Mar 14, 2024, 9:58 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > On Thu, Mar 14, 2024, 9:01 AM Markus Armbruster 
> wrote:
> >
> >> John Snow  writes:
> >>
> >> > Declare, but don't initialize the "members" field with type
> >> > List[QAPISchemaObjectTypeMember].
> >> >
> >> > This simplifies the typing from what would otherwise be
> >> > Optional[List[T]] to merely List[T]. This removes the need to add
> >> > assertions to several callsites that this value is not None - which it
> >> > never will be after the delayed initialization in check() anyway.
> >> >
> >> > The type declaration without initialization trick will cause
> accidental
> >> > uses of this field prior to full initialization to raise an
> >> > AttributeError.
> >> >
> >> > (Note that it is valid to have an empty members list, see the internal
> >> > q_empty object as an example. For this reason, we cannot use the empty
> >> > list as a replacement test for full initialization and instead rely on
> >> > the _checked/_check_complete fields.)
> >> >
> >> > Signed-off-by: John Snow 
> >> > ---
> >> >  scripts/qapi/schema.py | 12 +++-
> >> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index 50ebc4f12de..fb30314741a 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >> > @@ -20,7 +20,7 @@
> >> >  from collections import OrderedDict
> >> >  import os
> >> >  import re
> >> > -from typing import List, Optional
> >> > +from typing import List, Optional, cast
> >> >
> >> >  from .common import (
> >> >  POINTER_SUFFIX,
> >> > @@ -449,7 +449,7 @@ def __init__(self, name, info, doc, ifcond,
> features,
> >> >  self.base = None
> >> >  self.local_members = local_members
> >> >  self.variants = variants
> >> > -self.members = None
> >> > +self.members: List[QAPISchemaObjectTypeMember]
> >> >  self._check_complete = False
> >> >
> >> >  def check(self, schema):
> >> > @@ -482,7 +482,11 @@ def check(self, schema):
> >> >  for m in self.local_members:
> >> >  m.check(schema)
> >> >  m.check_clash(self.info, seen)
> >> > -members = seen.values()
> >> > +
> >> > +# check_clash works in terms of the supertype, but
> local_members
> >> > +# is asserted to be List[QAPISchemaObjectTypeMember].
> >>
> >> Do you mean "but self.members is declared as
> >> List[QAPISchemaObjectTypeMember]"?
> >
> > Argh. I meant asserted in the linguistic sense. mypy asserts it to be;
> not
> > a runtime assertion.
> >
> > I do this a lot, apparently.
>
> Okay to adjust your comment to my version?
>

Absolutely, of course!


> [...]
>
>

Re: [PATCH v4 16/23] qapi/schema: Don't initialize "members" with `None`

[John Snow]

On Thu, Mar 14, 2024, 9:01 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > Declare, but don't initialize the "members" field with type
> > List[QAPISchemaObjectTypeMember].
> >
> > This simplifies the typing from what would otherwise be
> > Optional[List[T]] to merely List[T]. This removes the need to add
> > assertions to several callsites that this value is not None - which it
> > never will be after the delayed initialization in check() anyway.
> >
> > The type declaration without initialization trick will cause accidental
> > uses of this field prior to full initialization to raise an
> > AttributeError.
> >
> > (Note that it is valid to have an empty members list, see the internal
> > q_empty object as an example. For this reason, we cannot use the empty
> > list as a replacement test for full initialization and instead rely on
> > the _checked/_check_complete fields.)
> >
> > Signed-off-by: John Snow 
> > ---
> >  scripts/qapi/schema.py | 12 +++-
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 50ebc4f12de..fb30314741a 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -20,7 +20,7 @@
> >  from collections import OrderedDict
> >  import os
> >  import re
> > -from typing import List, Optional
> > +from typing import List, Optional, cast
> >
> >  from .common import (
> >  POINTER_SUFFIX,
> > @@ -449,7 +449,7 @@ def __init__(self, name, info, doc, ifcond, features,
> >  self.base = None
> >  self.local_members = local_members
> >  self.variants = variants
> > -self.members = None
> > +self.members: List[QAPISchemaObjectTypeMember]
> >  self._check_complete = False
> >
> >  def check(self, schema):
> > @@ -482,7 +482,11 @@ def check(self, schema):
> >  for m in self.local_members:
> >  m.check(schema)
> >  m.check_clash(self.info, seen)
> > -members = seen.values()
> > +
> > +# check_clash works in terms of the supertype, but local_members
> > +# is asserted to be List[QAPISchemaObjectTypeMember].
>
> Do you mean "but self.members is declared as
> List[QAPISchemaObjectTypeMember]"?
>

Argh. I meant asserted in the linguistic sense. mypy asserts it to be; not
a runtime assertion.

I do this a lot, apparently.


> > +# Cast down to the subtype.
> > +members = cast(List[QAPISchemaObjectTypeMember],
> list(seen.values()))
>
> Let's break the line after the comma.
>

Go for it.

After this series I may send a patchset showing what changes black would
make. I cannot be trusted with aesthetic consistency.


> >
> >  if self.variants:
> >  self.variants.check(schema, seen)
> > @@ -515,11 +519,9 @@ def is_implicit(self):
> >  return self.name.startswith('q_')
> >
> >  def is_empty(self):
> > -assert self.members is not None
> >  return not self.members and not self.variants
> >
> >  def has_conditional_members(self):
> > -assert self.members is not None
> >  return any(m.ifcond.is_present() for m in self.members)
> >
> >  def c_name(self):
>
>

Re: [PATCH v4 05/23] qapi: create QAPISchemaDefinition

[John Snow]

On Thu, Mar 14, 2024, 5:12 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > Include entities don't have names, but we generally expect "entities" to
> > have names. Reclassify all entities with names as *definitions*, leaving
> > the nameless include entities as QAPISchemaEntity instances.
> >
> > This is primarily to help simplify typing around expectations of what
> > callers expect for properties of an "entity".
> >
> > Suggested-by: Markus Armbruster 
> > Signed-off-by: John Snow 
> > ---
> >  scripts/qapi/schema.py | 144 +++--
> >  1 file changed, 82 insertions(+), 62 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 117f0f78f0c..da273c1649d 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
>
> [...]
>
> > @@ -107,6 +90,46 @@ def _set_module(self, schema, info):
> >  def set_module(self, schema):
> >  self._set_module(schema, self.info)
> >
> > +def visit(self, visitor):
> > +# pylint: disable=unused-argument
> > +assert self._checked
> > +
> > +
> > +class QAPISchemaDefinition(QAPISchemaEntity):
> > +meta: Optional[str] = None
> > +
> > +def __init__(self, name: str, info, doc, ifcond=None,
> features=None):
> > +assert isinstance(name, str)
> > +super().__init__(info)
> > +for f in features or []:
> > +assert isinstance(f, QAPISchemaFeature)
> > +f.set_defined_in(name)
> > +self.name = name
> > +self.doc = doc
> > +self._ifcond = ifcond or QAPISchemaIfCond()
> > +self.features = features or []
> > +
> > +def __repr__(self):
> > +return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
> > +id(self))
> > +
> > +def c_name(self):
> > +return c_name(self.name)
> > +
> > +def check(self, schema):
> > +assert not self._checked
> > +seen = {}
> > +for f in self.features:
> > +f.check_clash(self.info, seen)
> > +super().check(schema)
>
> v3 called super().check() first.  Why did you move it?  I'm asking just
> to make sure I'm not missing something subtle.
>

I don't believe you are, I think it was just ... something I didn't notice
when rebasing, since I merged this patch by hand. I recall having to insert
the super call manually, and I guess my preferences are nondeterministic.


> > +
> > +def connect_doc(self, doc=None):
> > +super().connect_doc(doc)
> > +doc = doc or self.doc
> > +if doc:
> > +for f in self.features:
> > +doc.connect_feature(f)
> > +
> >  @property
> >  def ifcond(self):
> >  assert self._checked
>
> [...]
>
>

Re: [PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[John Snow]

On Wed, Mar 13, 2024, 2:57 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > On Tue, Feb 20, 2024 at 10:03 AM Markus Armbruster 
> wrote:
> >>
> >> John Snow  writes:
> >>
> >> > Declare, but don't initialize the "members" field with type
> >> > List[QAPISchemaObjectTypeMember].
> >> >
> >> > This simplifies the typing from what would otherwise be
> >> > Optional[List[T]] to merely List[T]. This removes the need to add
> >> > assertions to several callsites that this value is not None - which it
> >> > never will be after the delayed initialization in check() anyway.
> >> >
> >> > The type declaration without initialization trick will cause
> accidental
> >> > uses of this field prior to full initialization to raise an
> >> > AttributeError.
> >> >
> >> > (Note that it is valid to have an empty members list, see the internal
> >> > q_empty object as an example. For this reason, we cannot use the empty
> >> > list as a replacement test for full initialization and instead rely on
> >> > the _checked/_checking fields.)
> >> >
> >> > Signed-off-by: John Snow 
> >> > ---
> >> >  scripts/qapi/schema.py | 13 +++--
> >> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index a459016e148..947e7efb1a8 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >> > @@ -20,7 +20,7 @@
> >> >  from collections import OrderedDict
> >> >  import os
> >> >  import re
> >> > -from typing import List, Optional
> >> > +from typing import List, Optional, cast
> >> >
> >> >  from .common import (
> >> >  POINTER_SUFFIX,
> >> > @@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond,
> features,
> >> >  self.base = None
> >> >  self.local_members = local_members
> >> >  self.variants = variants
> >> > -self.members = None
> >> > +self.members: List[QAPISchemaObjectTypeMember]
> >> >  self._checking = False
> >> >
> >> >  def check(self, schema):
> >> > @@ -474,7 +474,7 @@ def check(self, schema):
> >> >
> >> >  self._checking = True
> >> >  super().check(schema)
> >> > -assert self._checked and self.members is None
> >> > +assert self._checked
> >>
> >> This asserts state is "2. Being checked:.
> >>
> >> The faithful update would be
> >>
> >>assert self._checked and self._checking
> >>
> >> Or with my alternative patch
> >>
> >>assert self._checked and not self._check_complete
> >> >
> >> >  seen = OrderedDict()
> >> >  if self._base_name:
> >> > @@ -491,7 +491,10 @@ def check(self, schema):
> >> >  for m in self.local_members:
> >> >  m.check(schema)
> >> >  m.check_clash(self.info, seen)
> >> > -members = seen.values()
> >> > +
> >> > +# check_clash is abstract, but local_members is asserted to
> be
> >> > +# List[QAPISchemaObjectTypeMember]. Cast to the narrower
> type.
> >>
> >> What do you mean by "check_clash is abstract"?
> >>
> >> > +members = cast(List[QAPISchemaObjectTypeMember],
> list(seen.values()))
> >>
> >> Do we actually need this *now*, or only later when we have more type
> >> hints?
> >>
> >> >
> >> >  if self.variants:
> >> >  self.variants.check(schema, seen)
> >> > @@ -524,11 +527,9 @@ def is_implicit(self):
> >> >  return self.name.startswith('q_')
> >> >
> >> >  def is_empty(self):
> >> > -assert self.members is not None
> >>
> >> This asserts state is "3. Checked".
> >>
> >> The faithful update would be
> >>
> >>assert self._checked and not self._checking
> >>
> >> Or with my alternative patch
> >>
> >>assert self._check_complete
> >>
> >> >  

[PATCH v4 05/23] qapi: create QAPISchemaDefinition

[John Snow]

Include entities don't have names, but we generally expect "entities" to
have names. Reclassify all entities with names as *definitions*, leaving
the nameless include entities as QAPISchemaEntity instances.

This is primarily to help simplify typing around expectations of what
callers expect for properties of an "entity".

Suggested-by: Markus Armbruster 
Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 144 +++--
 1 file changed, 82 insertions(+), 62 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 117f0f78f0c..da273c1649d 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -55,14 +55,13 @@ def is_present(self):
 
 
 class QAPISchemaEntity:
-meta: Optional[str] = None
+"""
+A schema entity.
 
-def __init__(self, name: str, info, doc, ifcond=None, features=None):
-assert name is None or isinstance(name, str)
-for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
-f.set_defined_in(name)
-self.name = name
+This is either a directive, such as include, or a definition.
+The latter uses sub-class `QAPISchemaDefinition`.
+"""
+def __init__(self, info):
 self._module = None
 # For explicitly defined entities, info points to the (explicit)
 # definition.  For builtins (and their arrays), info is None.
@@ -70,33 +69,17 @@ def __init__(self, name: str, info, doc, ifcond=None, 
features=None):
 # triggered the implicit definition (there may be more than one
 # such place).
 self.info = info
-self.doc = doc
-self._ifcond = ifcond or QAPISchemaIfCond()
-self.features = features or []
 self._checked = False
 
 def __repr__(self):
-if self.name is None:
-return "<%s at 0x%x>" % (type(self).__name__, id(self))
-return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
-id(self))
-
-def c_name(self):
-return c_name(self.name)
+return "<%s at 0x%x>" % (type(self).__name__, id(self))
 
 def check(self, schema):
 # pylint: disable=unused-argument
-assert not self._checked
-seen = {}
-for f in self.features:
-f.check_clash(self.info, seen)
 self._checked = True
 
 def connect_doc(self, doc=None):
-doc = doc or self.doc
-if doc:
-for f in self.features:
-doc.connect_feature(f)
+pass
 
 def _set_module(self, schema, info):
 assert self._checked
@@ -107,6 +90,46 @@ def _set_module(self, schema, info):
 def set_module(self, schema):
 self._set_module(schema, self.info)
 
+def visit(self, visitor):
+# pylint: disable=unused-argument
+assert self._checked
+
+
+class QAPISchemaDefinition(QAPISchemaEntity):
+meta: Optional[str] = None
+
+def __init__(self, name: str, info, doc, ifcond=None, features=None):
+assert isinstance(name, str)
+super().__init__(info)
+for f in features or []:
+assert isinstance(f, QAPISchemaFeature)
+f.set_defined_in(name)
+self.name = name
+self.doc = doc
+self._ifcond = ifcond or QAPISchemaIfCond()
+self.features = features or []
+
+def __repr__(self):
+return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
+id(self))
+
+def c_name(self):
+return c_name(self.name)
+
+def check(self, schema):
+assert not self._checked
+seen = {}
+for f in self.features:
+f.check_clash(self.info, seen)
+super().check(schema)
+
+def connect_doc(self, doc=None):
+super().connect_doc(doc)
+doc = doc or self.doc
+if doc:
+for f in self.features:
+doc.connect_feature(f)
+
 @property
 def ifcond(self):
 assert self._checked
@@ -115,10 +138,6 @@ def ifcond(self):
 def is_implicit(self):
 return not self.info
 
-def visit(self, visitor):
-# pylint: disable=unused-argument
-assert self._checked
-
 def describe(self):
 assert self.meta
 return "%s '%s'" % (self.meta, self.name)
@@ -218,7 +237,7 @@ def visit(self, visitor):
 
 class QAPISchemaInclude(QAPISchemaEntity):
 def __init__(self, sub_module, info):
-super().__init__(None, info, None)
+super().__init__(info)
 self._sub_module = sub_module
 
 def visit(self, visitor):
@@ -226,7 +245,7 @@ def visit(self, visitor):
 visitor.visit_include(self._sub_module.name, self.info)
 
 
-class QAPISchemaType(QAPISchemaEntity):
+class QAPISchemaType(QAPISchemaDefinition):
 # Return the C type for common use

[PATCH v4 09/23] qapi/schema: adjust type narrowing for mypy's benefit

[John Snow]

We already take care to perform some type narrowing for arg_type and
ret_type, but not in a way where mypy can utilize the result once we add
type hints, e.g.:

qapi/schema.py:833: error: Incompatible types in assignment (expression
has type "QAPISchemaType", variable has type
"Optional[QAPISchemaObjectType]") [assignment]

qapi/schema.py:893: error: Incompatible types in assignment (expression
has type "QAPISchemaType", variable has type
"Optional[QAPISchemaObjectType]") [assignment]

A simple change to use a temporary variable helps the medicine go down.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c316660adda..eeecdda1b58 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -843,13 +843,14 @@ def __init__(self, name, info, doc, ifcond, features,
 def check(self, schema):
 super().check(schema)
 if self._arg_type_name:
-self.arg_type = schema.resolve_type(
+arg_type = schema.resolve_type(
 self._arg_type_name, self.info, "command's 'data'")
-if not isinstance(self.arg_type, QAPISchemaObjectType):
+if not isinstance(arg_type, QAPISchemaObjectType):
 raise QAPISemError(
 self.info,
 "command's 'data' cannot take %s"
-% self.arg_type.describe())
+% arg_type.describe())
+self.arg_type = arg_type
 if self.arg_type.variants and not self.boxed:
 raise QAPISemError(
 self.info,
@@ -866,8 +867,8 @@ def check(self, schema):
 if self.name not in self.info.pragma.command_returns_exceptions:
 typ = self.ret_type
 if isinstance(typ, QAPISchemaArrayType):
-typ = self.ret_type.element_type
 assert typ
+typ = typ.element_type
 if not isinstance(typ, QAPISchemaObjectType):
 raise QAPISemError(
 self.info,
@@ -903,13 +904,14 @@ def __init__(self, name, info, doc, ifcond, features, 
arg_type, boxed):
 def check(self, schema):
 super().check(schema)
 if self._arg_type_name:
-self.arg_type = schema.resolve_type(
+typ = schema.resolve_type(
 self._arg_type_name, self.info, "event's 'data'")
-if not isinstance(self.arg_type, QAPISchemaObjectType):
+if not isinstance(typ, QAPISchemaObjectType):
 raise QAPISemError(
 self.info,
 "event's 'data' cannot take %s"
-% self.arg_type.describe())
+% typ.describe())
+self.arg_type = typ
 if self.arg_type.variants and not self.boxed:
 raise QAPISemError(
 self.info,
-- 
2.44.0

[PATCH v4 21/23] qapi/schema: add type hints

[John Snow]

This patch only adds type hints, which aren't utilized at runtime and
don't change the behavior of this module in any way.

In a scant few locations, type hints are removed where no longer
necessary due to inference power from typing all of the rest of
creation; and any type hints that no longer need string quotes are
changed.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 568 -
 1 file changed, 396 insertions(+), 172 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 3b8c2ebbb5f..d2faaea6eac 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -16,11 +16,21 @@
 
 # TODO catching name collisions in generated code would be nice
 
+from __future__ import annotations
+
 from abc import ABC, abstractmethod
 from collections import OrderedDict
 import os
 import re
-from typing import List, Optional, cast
+from typing import (
+Any,
+Callable,
+Dict,
+List,
+Optional,
+Union,
+cast,
+)
 
 from .common import (
 POINTER_SUFFIX,
@@ -32,26 +42,30 @@
 )
 from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
-from .parser import QAPIExpression, QAPISchemaParser
+from .parser import QAPIDoc, QAPIExpression, QAPISchemaParser
+from .source import QAPISourceInfo
 
 
 class QAPISchemaIfCond:
-def __init__(self, ifcond=None):
+def __init__(
+self,
+ifcond: Optional[Union[str, Dict[str, object]]] = None,
+) -> None:
 self.ifcond = ifcond
 
-def _cgen(self):
+def _cgen(self) -> str:
 return cgen_ifcond(self.ifcond)
 
-def gen_if(self):
+def gen_if(self) -> str:
 return gen_if(self._cgen())
 
-def gen_endif(self):
+def gen_endif(self) -> str:
 return gen_endif(self._cgen())
 
-def docgen(self):
+def docgen(self) -> str:
 return docgen_ifcond(self.ifcond)
 
-def is_present(self):
+def is_present(self) -> bool:
 return bool(self.ifcond)
 
 
@@ -62,8 +76,8 @@ class QAPISchemaEntity:
 This is either a directive, such as include, or a definition.
 The latter uses sub-class `QAPISchemaDefinition`.
 """
-def __init__(self, info):
-self._module = None
+def __init__(self, info: Optional[QAPISourceInfo]):
+self._module: Optional[QAPISchemaModule] = None
 # For explicitly defined entities, info points to the (explicit)
 # definition.  For builtins (and their arrays), info is None.
 # For implicitly defined entities, info points to a place that
@@ -72,34 +86,43 @@ def __init__(self, info):
 self.info = info
 self._checked = False
 
-def __repr__(self):
+def __repr__(self) -> str:
 return "<%s at 0x%x>" % (type(self).__name__, id(self))
 
-def check(self, schema):
+def check(self, schema: QAPISchema) -> None:
 # pylint: disable=unused-argument
 self._checked = True
 
-def connect_doc(self, doc=None):
+def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
 pass
 
-def _set_module(self, schema, info):
+def _set_module(
+self, schema: QAPISchema, info: Optional[QAPISourceInfo]
+) -> None:
 assert self._checked
 fname = info.fname if info else QAPISchemaModule.BUILTIN_MODULE_NAME
 self._module = schema.module_by_fname(fname)
 self._module.add_entity(self)
 
-def set_module(self, schema):
+def set_module(self, schema: QAPISchema) -> None:
 self._set_module(schema, self.info)
 
-def visit(self, visitor):
+def visit(self, visitor: QAPISchemaVisitor) -> None:
 # pylint: disable=unused-argument
 assert self._checked
 
 
 class QAPISchemaDefinition(QAPISchemaEntity):
-meta: Optional[str] = None
+meta: str
 
-def __init__(self, name: str, info, doc, ifcond=None, features=None):
+def __init__(
+self,
+name: str,
+info: Optional[QAPISourceInfo],
+doc: Optional[QAPIDoc],
+ifcond: Optional[QAPISchemaIfCond] = None,
+features: Optional[List[QAPISchemaFeature]] = None,
+):
 assert isinstance(name, str)
 super().__init__(info)
 for f in features or []:
@@ -110,21 +133,21 @@ def __init__(self, name: str, info, doc, ifcond=None, 
features=None):
 self._ifcond = ifcond or QAPISchemaIfCond()
 self.features = features or []
 
-def __repr__(self):
+def __repr__(self) -> str:
 return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
 id(self))
 
-def c_name(self):
+def c_name(self) -> str:
 return c_name(self.name)
 
-def check(self, schema):
+def check(self, schema: QAPISchema) -> None:
 assert not self._checked
-seen = {}
+seen: Dict[str, QAPISc

[PATCH v4 12/23] qapi: use schema.resolve_type instead of schema.lookup_type

[John Snow]

the function lookup_type() is capable of returning None, but some
callers aren't prepared for that and assume it will always succeed. For
static type analysis purposes, this creates problems at those callsites.

Modify resolve_type() - which already cannot ever return None - to allow
'info' and 'what' parameters to be elided, which infers that the type
lookup is a built-in type lookup.

Change several callsites to use the new form of resolve_type to avoid
the more laborious strictly-typed error-checking scaffolding:

  ret = lookup_type("foo")
  assert ret is not None
  typ: QAPISchemaType = ret

which can be replaced with the simpler:

  typ = resolve_type("foo")

Signed-off-by: John Snow 
---
 scripts/qapi/introspect.py | 4 ++--
 scripts/qapi/schema.py | 5 ++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae0..c38df61a6d5 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -227,10 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str:
 
 # Map the various integer types to plain int
 if typ.json_type() == 'int':
-typ = self._schema.lookup_type('int')
+typ = self._schema.resolve_type('int')
 elif (isinstance(typ, QAPISchemaArrayType) and
   typ.element_type.json_type() == 'int'):
-typ = self._schema.lookup_type('intList')
+typ = self._schema.resolve_type('intList')
 # Add type to work queue if new
 if typ not in self._used_types:
 self._used_types.append(typ)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index f5c7789d98f..58ec3a7c41c 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -642,8 +642,7 @@ def check(self, schema, seen):
 "discriminator '%s' is not a member of %s"
 % (self._tag_name, base))
 # Here we do:
-base_type = schema.lookup_type(self.tag_member.defined_in)
-assert base_type
+base_type = schema.resolve_type(self.tag_member.defined_in)
 if not base_type.is_implicit():
 base = "base type '%s'" % self.tag_member.defined_in
 if not isinstance(self.tag_member.type, QAPISchemaEnumType):
@@ -993,7 +992,7 @@ def lookup_type(self, name):
 assert typ is None or isinstance(typ, QAPISchemaType)
 return typ
 
-def resolve_type(self, name, info, what):
+def resolve_type(self, name, info=None, what=None):
 typ = self.lookup_type(name)
 if not typ:
 assert info and what  # built-in types must not fail lookup
-- 
2.44.0

[PATCH v4 15/23] qapi/schema: add _check_complete flag

[John Snow]

Instead of using the None value for the members field, use a dedicated
flag to detect recursive misconfigurations.

This is intended to assist with subsequent patches that seek to remove
the "None" value from the members field (which can never hold that value
after the final call to check()) in order to simplify the static typing
of that field; avoiding the need of assertions littered at many
callsites to eliminate the possibility of the None value.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d514b3c28f6..50ebc4f12de 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -450,12 +450,13 @@ def __init__(self, name, info, doc, ifcond, features,
 self.local_members = local_members
 self.variants = variants
 self.members = None
+self._check_complete = False
 
 def check(self, schema):
 # This calls another type T's .check() exactly when the C
 # struct emitted by gen_object() contains that T's C struct
 # (pointers don't count).
-if self.members is not None:
+if self._check_complete:
 # A previous .check() completed: nothing to do
 return
 if self._checked:
@@ -464,7 +465,7 @@ def check(self, schema):
"object %s contains itself" % self.name)
 
 super().check(schema)
-assert self._checked and self.members is None
+assert self._checked and not self._check_complete
 
 seen = OrderedDict()
 if self._base_name:
@@ -487,7 +488,8 @@ def check(self, schema):
 self.variants.check(schema, seen)
 self.variants.check_clash(self.info, seen)
 
-self.members = members  # mark completed
+self.members = members
+self._check_complete = True  # mark completed
 
 # Check that the members of this type do not cause duplicate JSON members,
 # and update seen to track the members seen so far. Report any errors
-- 
2.44.0

[PATCH v4 03/23] qapi: sort pylint suppressions

[John Snow]

Suggested-by: Markus Armbruster 
Signed-off-by: John Snow 
---
 scripts/qapi/pylintrc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index 90546df5345..1342412c3cf 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -16,13 +16,13 @@ ignore-patterns=schema.py,
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use "--disable=all --enable=classes
 # --disable=W".
-disable=fixme,
+disable=consider-using-f-string,
+fixme,
 missing-docstring,
 too-many-arguments,
 too-many-branches,
-too-many-statements,
 too-many-instance-attributes,
-consider-using-f-string,
+too-many-statements,
 useless-option-value,
 
 [REPORTS]
-- 
2.44.0

[PATCH v4 07/23] qapi/schema: declare type for QAPISchemaArrayType.element_type

[John Snow]

A QAPISchemaArrayType's element type gets resolved only during .check().
We have QAPISchemaArrayType.__init__() initialize self.element_type =
None, and .check() assign the actual type.  Using .element_type before
.check() is wrong, and hopefully crashes due to the value being None.
Works.

However, it makes for awkward typing.  With .element_type:
Optional[QAPISchemaType], mypy is of course unable to see that it's None
before .check(), and a QAPISchemaType after.  To help it over the hump,
we'd have to assert self.element_type is not None before all the (valid)
uses.  The assertion catches invalid uses, but only at run time; mypy
can't flag them.

Instead, declare .element_type in .__init__() as QAPISchemaType
*without* initializing it.  Using .element_type before .check() now
certainly crashes, which is an improvement.  Mypy still can't flag
invalid uses, but that's okay.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 8440a7243d8..c549a4e3bd0 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -381,7 +381,7 @@ def __init__(self, name, info, element_type):
 super().__init__(name, info, None)
 assert isinstance(element_type, str)
 self._element_type_name = element_type
-self.element_type = None
+self.element_type: QAPISchemaType
 
 def need_has_if_optional(self):
 # When FOO is an array, we still need has_FOO to distinguish
-- 
2.44.0

[PATCH v4 22/23] qapi/schema: turn on mypy strictness

[John Snow]

This patch can be rolled in with the previous one once the series is
ready for merge, but for work-in-progress' sake, it's separate here.

Signed-off-by: John Snow 
---
 scripts/qapi/mypy.ini | 5 -
 1 file changed, 5 deletions(-)

diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 56e0dfb1327..8109470a031 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -2,8 +2,3 @@
 strict = True
 disallow_untyped_calls = False
 python_version = 3.8
-
-[mypy-qapi.schema]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-- 
2.44.0

[PATCH v4 14/23] qapi/schema: assert info is present when necessary

[John Snow]

QAPISchemaInfo arguments can often be None because built-in definitions
don't have such information.  The type hint can only be
Optional[QAPISchemaInfo] then.  But, mypy gets upset about all the
places where we exploit that it can't actually be None there.  Add
assertions that will help mypy over the hump, to enable adding type
hints in a forthcoming commit.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 22e065fc13d..d514b3c28f6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -750,6 +750,7 @@ def describe(self, info):
 else:
 assert False
 
+assert info is not None
 if defined_in != info.defn_name:
 return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in)
 return "%s '%s'" % (role, self.name)
@@ -840,6 +841,7 @@ def __init__(self, name, info, doc, ifcond, features,
 self.coroutine = coroutine
 
 def check(self, schema):
+assert self.info is not None
 super().check(schema)
 if self._arg_type_name:
 arg_type = schema.resolve_type(
-- 
2.44.0

[PATCH v4 13/23] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type

[John Snow]

Adjust the expression at the callsite to work around mypy's weak type
introspection that believes this expression can resolve to
QAPISourceInfo; it cannot.

(Fundamentally: self.info only resolves to false in a boolean expression
when it is None; therefore this expression may only ever produce
Optional[str]. mypy does not know that 'info', when it is a
QAPISourceInfo object, cannot ever be false.)

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 58ec3a7c41c..22e065fc13d 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -395,7 +395,7 @@ def check(self, schema):
 super().check(schema)
 self.element_type = schema.resolve_type(
 self._element_type_name, self.info,
-self.info and self.info.defn_meta)
+self.info.defn_meta if self.info else None)
 assert not isinstance(self.element_type, QAPISchemaArrayType)
 
 def set_module(self, schema):
-- 
2.44.0

[PATCH v4 18/23] qapi/schema: assert inner type of QAPISchemaVariants in check_clash()

[John Snow]

QAPISchemaVariant's "variants" field is typed as
List[QAPISchemaVariant], where the typing for QAPISchemaVariant allows
its type field to be any QAPISchemaType.

However, QAPISchemaVariant expects that all of its variants contain the
narrower QAPISchemaObjectType. This relationship is enforced at runtime
in QAPISchemaVariants.check(). This relationship is not embedded in the
type system though, so QAPISchemaVariants.check_clash() needs to
re-assert this property in order to call
QAPISchemaVariant.type.check_clash().

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 39f653f13fd..3b8c2ebbb5f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -715,7 +715,10 @@ def check(self, schema, seen):
 def check_clash(self, info, seen):
 for v in self.variants:
 # Reset seen map for each variant, since qapi names from one
-# branch do not affect another branch
+# branch do not affect another branch.
+#
+# v.type's typing is enforced in check() above.
+assert isinstance(v.type, QAPISchemaObjectType)
 v.type.check_clash(info, dict(seen))
 
 
-- 
2.44.0

[PATCH v4 02/23] qapi/parser: shush up pylint

[John Snow]

Shhh!

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index fed88e9074d..ec4ebef4e33 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -607,6 +607,7 @@ class QAPIDoc:
 """
 
 class Section:
+# pylint: disable=too-few-public-methods
 def __init__(self, info: QAPISourceInfo,
  tag: Optional[str] = None):
 # section source info, i.e. where it begins
-- 
2.44.0

[PATCH v4 23/23] qapi/schema: remove unnecessary asserts

[John Snow]

With strict typing enabled, these runtime statements aren't necessary
anymore; we can prove them statically.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 25 -
 1 file changed, 25 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d2faaea6eac..ffd9170f577 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -123,10 +123,8 @@ def __init__(
 ifcond: Optional[QAPISchemaIfCond] = None,
 features: Optional[List[QAPISchemaFeature]] = None,
 ):
-assert isinstance(name, str)
 super().__init__(info)
 for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
 f.set_defined_in(name)
 self.name = name
 self.doc = doc
@@ -163,7 +161,6 @@ def is_implicit(self) -> bool:
 return not self.info
 
 def describe(self) -> str:
-assert self.meta
 return "%s '%s'" % (self.meta, self.name)
 
 
@@ -377,7 +374,6 @@ def check(self, schema: QAPISchema) -> None:
 f"feature '{feat.name}' is not supported for types")
 
 def describe(self) -> str:
-assert self.meta
 return "%s type '%s'" % (self.meta, self.name)
 
 
@@ -386,7 +382,6 @@ class QAPISchemaBuiltinType(QAPISchemaType):
 
 def __init__(self, name: str, json_type: str, c_type: str):
 super().__init__(name, None, None)
-assert not c_type or isinstance(c_type, str)
 assert json_type in ('string', 'number', 'int', 'boolean', 'null',
  'value')
 self._json_type_name = json_type
@@ -429,9 +424,7 @@ def __init__(
 ):
 super().__init__(name, info, doc, ifcond, features)
 for m in members:
-assert isinstance(m, QAPISchemaEnumMember)
 m.set_defined_in(name)
-assert prefix is None or isinstance(prefix, str)
 self.members = members
 self.prefix = prefix
 
@@ -474,7 +467,6 @@ def __init__(
 self, name: str, info: Optional[QAPISourceInfo], element_type: str
 ):
 super().__init__(name, info, None)
-assert isinstance(element_type, str)
 self._element_type_name = element_type
 self.element_type: QAPISchemaType
 
@@ -519,7 +511,6 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
  self.element_type)
 
 def describe(self) -> str:
-assert self.meta
 return "%s type ['%s']" % (self.meta, self._element_type_name)
 
 
@@ -539,12 +530,9 @@ def __init__(
 # union has base, variants, and no local_members
 super().__init__(name, info, doc, ifcond, features)
 self.meta = 'union' if variants else 'struct'
-assert base is None or isinstance(base, str)
 for m in local_members:
-assert isinstance(m, QAPISchemaObjectTypeMember)
 m.set_defined_in(name)
 if variants is not None:
-assert isinstance(variants, QAPISchemaVariants)
 variants.set_defined_in(name)
 self._base_name = base
 self.base = None
@@ -666,7 +654,6 @@ def __init__(
 variants: QAPISchemaVariants,
 ):
 super().__init__(name, info, doc, ifcond, features)
-assert isinstance(variants, QAPISchemaVariants)
 assert variants.tag_member
 variants.set_defined_in(name)
 variants.tag_member.set_defined_in(self.name)
@@ -742,8 +729,6 @@ def __init__(
 assert bool(tag_member) != bool(tag_name)
 assert (isinstance(tag_name, str) or
 isinstance(tag_member, QAPISchemaObjectTypeMember))
-for v in variants:
-assert isinstance(v, QAPISchemaVariant)
 self._tag_name = tag_name
 self.info = info
 self._tag_member = tag_member
@@ -857,7 +842,6 @@ def __init__(
 info: Optional[QAPISourceInfo],
 ifcond: Optional[QAPISchemaIfCond] = None,
 ):
-assert isinstance(name, str)
 self.name = name
 self.info = info
 self.ifcond = ifcond or QAPISchemaIfCond()
@@ -925,7 +909,6 @@ def __init__(
 ):
 super().__init__(name, info, ifcond)
 for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
 f.set_defined_in(name)
 self.features = features or []
 
@@ -954,10 +937,7 @@ def __init__(
 features: Optional[List[QAPISchemaFeature]] = None,
 ):
 super().__init__(name, info, ifcond)
-assert isinstance(typ, str)
-assert isinstance(optional, bool)
 for f in features or []:
-assert isinstance(f, QAPISchemaFeature)
 f.set_defined_in(name)
 self._type_name = typ
 self.type: QAPISchemaType  # set during check()
@@ -965,7 +945,6 @@ def __init__(
 self.features = features or []
 
 def need_has(self) -> 

[PATCH v4 10/23] qapi/schema: add type narrowing to lookup_type()

[John Snow]

This function is a bit hard to type as-is; mypy needs some assertions to
assist with the type narrowing.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index eeecdda1b58..b157a3b2bd8 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -989,7 +989,9 @@ def lookup_entity(self, name, typ=None):
 return ent
 
 def lookup_type(self, name):
-return self.lookup_entity(name, QAPISchemaType)
+typ = self.lookup_entity(name, QAPISchemaType)
+assert typ is None or isinstance(typ, QAPISchemaType)
+return typ
 
 def resolve_type(self, name, info, what):
 typ = self.lookup_type(name)
-- 
2.44.0

[PATCH v4 08/23] qapi/schema: make c_type() and json_type() abstract methods

[John Snow]

These methods should always return a str, it's only the default abstract
implementation that doesn't. They can be marked "abstract", which
requires subclasses to override the method with the proper return type.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c549a4e3bd0..c316660adda 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -16,6 +16,7 @@
 
 # TODO catching name collisions in generated code would be nice
 
+from abc import ABC, abstractmethod
 from collections import OrderedDict
 import os
 import re
@@ -245,9 +246,10 @@ def visit(self, visitor):
 visitor.visit_include(self._sub_module.name, self.info)
 
 
-class QAPISchemaType(QAPISchemaDefinition):
+class QAPISchemaType(QAPISchemaDefinition, ABC):
 # Return the C type for common use.
 # For the types we commonly box, this is a pointer type.
+@abstractmethod
 def c_type(self):
 pass
 
@@ -259,6 +261,7 @@ def c_param_type(self):
 def c_unboxed_type(self):
 return self.c_type()
 
+@abstractmethod
 def json_type(self):
 pass
 
-- 
2.44.0

[PATCH v4 19/23] qapi/parser: demote QAPIExpression to Dict[str, Any]

[John Snow]

Dict[str, object] is a stricter type, but with the way that code is
currently arranged, it is infeasible to enforce this strictness.

In particular, although expr.py's entire raison d'être is normalization
and type-checking of QAPI Expressions, that type information is not
"remembered" in any meaningful way by mypy because each individual
expression is not downcast to a specific expression type that holds all
the details of each expression's unique form.

As a result, all of the code in schema.py that deals with actually
creating type-safe specialized structures has no guarantee (myopically)
that the data it is being passed is correct.

There are two ways to solve this:

(1) Re-assert that the incoming data is in the shape we expect it to be, or
(2) Disable type checking for this data.

(1) is appealing to my sense of strictness, but I gotta concede that it
is asinine to re-check the shape of a QAPIExpression in schema.py when
expr.py has just completed that work at length. The duplication of code
and the nightmare thought of needing to update both locations if and
when we change the shape of these structures makes me extremely
reluctant to go down this route.

(2) allows us the chance to miss updating types in the case that types
are updated in expr.py, but it *is* an awful lot simpler and,
importantly, gets us closer to type checking schema.py *at
all*. Something is better than nothing, I'd argue.

So, do the simpler dumber thing and worry about future strictness
improvements later.

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ec4ebef4e33..2f3c704fa24 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -19,6 +19,7 @@
 import re
 from typing import (
 TYPE_CHECKING,
+Any,
 Dict,
 List,
 Mapping,
@@ -43,7 +44,7 @@
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
 
-class QAPIExpression(Dict[str, object]):
+class QAPIExpression(Dict[str, Any]):
 # pylint: disable=too-few-public-methods
 def __init__(self,
  data: Mapping[str, object],
-- 
2.44.0

[PATCH v4 16/23] qapi/schema: Don't initialize "members" with `None`

[John Snow]

Declare, but don't initialize the "members" field with type
List[QAPISchemaObjectTypeMember].

This simplifies the typing from what would otherwise be
Optional[List[T]] to merely List[T]. This removes the need to add
assertions to several callsites that this value is not None - which it
never will be after the delayed initialization in check() anyway.

The type declaration without initialization trick will cause accidental
uses of this field prior to full initialization to raise an
AttributeError.

(Note that it is valid to have an empty members list, see the internal
q_empty object as an example. For this reason, we cannot use the empty
list as a replacement test for full initialization and instead rely on
the _checked/_check_complete fields.)

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 50ebc4f12de..fb30314741a 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
 from collections import OrderedDict
 import os
 import re
-from typing import List, Optional
+from typing import List, Optional, cast
 
 from .common import (
 POINTER_SUFFIX,
@@ -449,7 +449,7 @@ def __init__(self, name, info, doc, ifcond, features,
 self.base = None
 self.local_members = local_members
 self.variants = variants
-self.members = None
+self.members: List[QAPISchemaObjectTypeMember]
 self._check_complete = False
 
 def check(self, schema):
@@ -482,7 +482,11 @@ def check(self, schema):
 for m in self.local_members:
 m.check(schema)
 m.check_clash(self.info, seen)
-members = seen.values()
+
+# check_clash works in terms of the supertype, but local_members
+# is asserted to be List[QAPISchemaObjectTypeMember].
+# Cast down to the subtype.
+members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
 
 if self.variants:
 self.variants.check(schema, seen)
@@ -515,11 +519,9 @@ def is_implicit(self):
 return self.name.startswith('q_')
 
 def is_empty(self):
-assert self.members is not None
 return not self.members and not self.variants
 
 def has_conditional_members(self):
-assert self.members is not None
 return any(m.ifcond.is_present() for m in self.members)
 
 def c_name(self):
-- 
2.44.0

[PATCH v4 17/23] qapi/schema: fix typing for QAPISchemaVariants.tag_member

[John Snow]

There are two related changes here:

(1) We need to perform type narrowing for resolving the type of
tag_member during check(), and

(2) tag_member is a delayed initialization field, but we can hide it
behind a property that raises an Exception if it's called too
early. This simplifies the typing in quite a few places and avoids
needing to assert that the "tag_member is not None" at a dozen
callsites, which can be confusing and suggest the wrong thing to a
drive-by contributor.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index fb30314741a..39f653f13fd 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -627,25 +627,39 @@ def __init__(self, tag_name, info, tag_member, variants):
 assert isinstance(v, QAPISchemaVariant)
 self._tag_name = tag_name
 self.info = info
-self.tag_member = tag_member
+self._tag_member: Optional[QAPISchemaObjectTypeMember] = tag_member
 self.variants = variants
 
+@property
+def tag_member(self) -> 'QAPISchemaObjectTypeMember':
+if self._tag_member is None:
+raise RuntimeError(
+"QAPISchemaVariants has no tag_member property until "
+"after check() has been run."
+)
+return self._tag_member
+
 def set_defined_in(self, name):
 for v in self.variants:
 v.set_defined_in(name)
 
 def check(self, schema, seen):
 if self._tag_name:  # union
-self.tag_member = seen.get(c_name(self._tag_name))
+# We need to narrow the member type:
+tmp = seen.get(c_name(self._tag_name))
+assert tmp is None or isinstance(tmp, QAPISchemaObjectTypeMember)
+self._tag_member = tmp
+
 base = "'base'"
 # Pointing to the base type when not implicit would be
 # nice, but we don't know it here
-if not self.tag_member or self._tag_name != self.tag_member.name:
+if not self._tag_member or self._tag_name != self._tag_member.name:
 raise QAPISemError(
 self.info,
 "discriminator '%s' is not a member of %s"
 % (self._tag_name, base))
 # Here we do:
+assert self.tag_member.defined_in
 base_type = schema.resolve_type(self.tag_member.defined_in)
 if not base_type.is_implicit():
 base = "base type '%s'" % self.tag_member.defined_in
@@ -665,11 +679,13 @@ def check(self, schema, seen):
 "discriminator member '%s' of %s must not be conditional"
 % (self._tag_name, base))
 else:   # alternate
+assert self._tag_member
 assert isinstance(self.tag_member.type, QAPISchemaEnumType)
 assert not self.tag_member.optional
 assert not self.tag_member.ifcond.is_present()
 if self._tag_name:  # union
 # branches that are not explicitly covered get an empty type
+assert self.tag_member.defined_in
 cases = {v.name for v in self.variants}
 for m in self.tag_member.type.members:
 if m.name not in cases:
-- 
2.44.0

[PATCH v4 00/23] qapi: statically type schema.py

[John Snow]

This is *v4*, for some definitions of "version" and "four".

v4:
 - Rebased on top of latest QAPIDoc patches.
 - A couple of hotfixes on top of the QAPIDoc patches.
 - Adjusted commit message phrasing
 - Changed the "split checked into checked and checking" patch and follow-up.

v3:
 - 01: fixed alphabetization (... sigh)
 - 03: updated docstring, added super calls,
   renamed argument to @defn, reflowed long lines
 - 04: updated commit message
 - 05: updated commit message
 - 06: pushed type hints into later commit
 - 09: removed default argument and stuffed into next patch
 - 10: Added the parts Markus doesn't like (Sorry)
 - 11: updated commit message
 - 12: updated commit message
 - 13: Split into two patches.
   Kept stuff Markus doesn't like (Sorry)
 - 14: New, split from 13.

v2:
 - dropped the resolve_type refactoring patch
 - added QAPISchemaDefinition
 - misc bits and pieces.

001/19:[down] 'qapi: sort pylint suppressions'
  New, Markus's suggestion.

002/19:[0001] [FC] 'qapi/schema: add pylint suppressions'
  Added newline, Markus's RB

003/19:[down] 'qapi: create QAPISchemaDefinition'
  New, Markus's suggestion.

004/19:[0002] [FC] 'qapi/schema: declare type for 
QAPISchemaObjectTypeMember.type'
  Adjusted commit message and comment.

005/19:[down] 'qapi/schema: declare type for QAPISchemaArrayType.element_type'
  Patch renamed; removed @property trick in favor of static type declaration

006/19:[0009] [FC] 'qapi/schema: make c_type() and json_type() abstract methods'
  Use abc.ABC and @abstractmethod

007/19:[0001] [FC] 'qapi/schema: adjust type narrowing for mypy's benefit'
  Adjusted commit message; left in an assertion that is removed later instead.

008/19:[down] 'qapi/schema: add type narrowing to lookup_type()'
  Renamed
  removed type hints which get added later in the series instead
  simplified logic.

009/19:[down] 'qapi/schema: allow resolve_type to be used for built-in types'
  New patch. (Types are added later.)

010/19:[down] 'qapi: use schema.resolve_type instead of schema.lookup_type'
  New patch, replaces old 07/19 "assert schema.lookup_type did not fail"

011/19:[0011] [FC] 'qapi/schema: fix QAPISchemaArrayType.check's call to 
resolve_type'
  Dramatically simplified.

012/19:[] [--] 'qapi/schema: assert info is present when necessary'
013/19:[] [--] 'qapi/schema: split "checked" field into "checking" and 
"checked"'
  Changed the commit message, but actually struggled finding anything simpler
  than what I already had, owing to the fact that q_empty is a valid construct
  and I can't seem to avoid adding a new state variable here.

014/19:[] [-C] 'qapi/schema: fix typing for QAPISchemaVariants.tag_member'
  Also unchanged from review, I think this is simplest still...

015/19:[down] 'qapi/schema: assert inner type of QAPISchemaVariants in 
check_clash()'
  Renamed, changed commit message and comment.

016/19:[] [--] 'qapi/parser: demote QAPIExpression to Dict[str, Any]'
017/19:[0042] [FC] 'qapi/schema: add type hints'
  Mostly contextual changes.

018/19:[] [--] 'qapi/schema: turn on mypy strictness'
019/19:[0006] [FC] 'qapi/schema: remove unnecessary asserts'
  Zapped a few more.

John Snow (23):
  qapi/parser: fix typo - self.returns.info => self.errors.info
  qapi/parser: shush up pylint
  qapi: sort pylint suppressions
  qapi/schema: add pylint suppressions
  qapi: create QAPISchemaDefinition
  qapi/schema: declare type for QAPISchemaObjectTypeMember.type
  qapi/schema: declare type for QAPISchemaArrayType.element_type
  qapi/schema: make c_type() and json_type() abstract methods
  qapi/schema: adjust type narrowing for mypy's benefit
  qapi/schema: add type narrowing to lookup_type()
  qapi/schema: assert resolve_type has 'info' and 'what' args on error
  qapi: use schema.resolve_type instead of schema.lookup_type
  qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type
  qapi/schema: assert info is present when necessary
  qapi/schema: add _check_complete flag
  qapi/schema: Don't initialize "members" with `None`
  qapi/schema: fix typing for QAPISchemaVariants.tag_member
  qapi/schema: assert inner type of QAPISchemaVariants in check_clash()
  qapi/parser: demote QAPIExpression to Dict[str, Any]
  qapi/parser.py: assert member.info is present in connect_member
  qapi/schema: add type hints
  qapi/schema: turn on mypy strictness
  qapi/schema: remove unnecessary asserts

 scripts/qapi/introspect.py |   4 +-
 scripts/qapi/mypy.ini  |   5 -
 scripts/qapi/parser.py |   7 +-
 scripts/qapi/pylintrc  |  11 +-
 scripts/qapi/schema.py | 792 -
 5 files changed, 534 insertions(+), 285 deletions(-)

-- 
2.44.0

[PATCH v4 20/23] qapi/parser.py: assert member.info is present in connect_member

[John Snow]

Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 2f3c704fa24..7b13a583ac1 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -707,6 +707,7 @@ def append_line(self, line: str) -> None:
 
 def connect_member(self, member: 'QAPISchemaMember') -> None:
 if member.name not in self.args:
+assert member.info
 if self.symbol not in member.info.pragma.documentation_exceptions:
 raise QAPISemError(member.info,
"%s '%s' lacks documentation"
-- 
2.44.0

[PATCH v4 11/23] qapi/schema: assert resolve_type has 'info' and 'what' args on error

[John Snow]

resolve_type() is generally used to resolve configuration-provided type
names into type objects, and generally requires valid 'info' and 'what'
parameters.

In some cases, such as with QAPISchemaArrayType.check(), resolve_type
may be used to resolve built-in types and as such will not have an
'info' argument, but also must not fail in this scenario.

Use an assertion to sate mypy that we will indeed have 'info' and 'what'
parameters for the error pathway in resolve_type.

Note: there are only three callsites to resolve_type at present where
"info" is perceived by mypy to be possibly None:

1) QAPISchemaArrayType.check()
2) QAPISchemaObjectTypeMember.check()
3) QAPISchemaEvent.check()

Of those three, only the first actually ever passes None; the other two
are limited by their base class initializers which accept info=None, but
neither subclass actually use a None value in practice, currently.

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b157a3b2bd8..f5c7789d98f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -996,6 +996,7 @@ def lookup_type(self, name):
 def resolve_type(self, name, info, what):
 typ = self.lookup_type(name)
 if not typ:
+assert info and what  # built-in types must not fail lookup
 if callable(what):
 what = what(info)
 raise QAPISemError(
-- 
2.44.0

[PATCH v4 01/23] qapi/parser: fix typo - self.returns.info => self.errors.info

[John Snow]

Small copy-pasto. The correct info field to use in this conditional
block is self.errors.info.

Fixes: 3a025d3d1ffa
Signed-off-by: John Snow 
---
 scripts/qapi/parser.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index d8f76060b8c..fed88e9074d 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -733,7 +733,7 @@ def check_expr(self, expr: QAPIExpression) -> None:
 "'Returns' section is only valid for commands")
 if self.errors:
 raise QAPISemError(
-self.returns.info,
+self.errors.info,
 "'Errors' section is only valid for commands")
 
 def check(self) -> None:
-- 
2.44.0

[PATCH v4 06/23] qapi/schema: declare type for QAPISchemaObjectTypeMember.type

[John Snow]

A QAPISchemaObjectTypeMember's type gets resolved only during .check().
We have QAPISchemaObjectTypeMember.__init__() initialize self.type =
None, and .check() assign the actual type.  Using .type before .check()
is wrong, and hopefully crashes due to the value being None.  Works.

However, it makes for awkward typing.  With .type:
Optional[QAPISchemaType], mypy is of course unable to see that it's None
before .check(), and a QAPISchemaType after.  To help it over the hump,
we'd have to assert self.type is not None before all the (valid) uses.
The assertion catches invalid uses, but only at run time; mypy can't
flag them.

Instead, declare .type in .__init__() as QAPISchemaType *without*
initializing it.  Using .type before .check() now certainly crashes,
which is an improvement.  Mypy still can't flag invalid uses, but that's
okay.

Addresses typing errors such as these:

qapi/schema.py:657: error: "None" has no attribute "alternate_qtype"  
[attr-defined]
qapi/schema.py:662: error: "None" has no attribute "describe"  [attr-defined]

Signed-off-by: John Snow 
---
 scripts/qapi/schema.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index da273c1649d..8440a7243d8 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -786,7 +786,7 @@ def __init__(self, name, info, typ, optional, ifcond=None, 
features=None):
 assert isinstance(f, QAPISchemaFeature)
 f.set_defined_in(name)
 self._type_name = typ
-self.type = None
+self.type: QAPISchemaType  # set during check()
 self.optional = optional
 self.features = features or []
 
-- 
2.44.0