Re: [PATCH v3 09/16] qapi/docs: adjust stub member insertion algorithm

John Snow <[email protected]> writes:

> On Wed, Jun 3, 2026 at 7:27 AM Markus Armbruster <[email protected]> wrote:
>>
>> Quick first pass...
>>
>> John Snow <[email protected]> writes:
>>
>> > A forthcoming patch removes the implicit PLAIN section that always
>> > starts a QAPIDoc section list. Further future changes begin converting
>> > "PLAIN" sections to "INTRO" sections. To accommodate this, the
>> > insertion algorithm that places stub and dummy members must be
>> > adjusted to cope with not only the finished state, but temporary
>> > intermediate states while the series is merged.
>>
>> Perhaps:
>>
>>   A forthcoming patch removes the implicit PLAIN section that always
>>   starts a QAPIDoc section list. Further future changes begin converting
>>   "PLAIN" sections to "INTRO" sections.
>>
>>   This will affect the code that inserts "Not documented" descriptions
>>   for undocumented members ("stub sections") and the dummy section that
>>   marks the spot for "The members of ..." references.
>>
>>   Adjust the algorithm to cope with not only the finished state, but
>>   temporary intermediate states while the series is merged.
>
> Sure.
>
>>
>> > This algorithm can handle zero-or-more PLAIN *or* INTRO sections at
>> > the beginning of a QAPIDoc object, in contrast to the previous
>> > algorithm which assumed and relied upon there being always one PLAIN
>> > section at the beginning of every QAPIDoc section list.
>> >
>> > In other words: (PLAIN | INTRO)* <EverythingElse>
>> >
>> > This does not impact what the parser itself will actually produce. As
>> > of this patch, the parser will still always generate QAPIDoc section
>> > lists that start with precisely one PLAIN section (whether or not it
>> > is empty), followed by the remaining sections. Those remaining
>> > sections may or may not include additional PLAIN sections, but never
>> > two such sections contiguously as the parser will always treat that
>> > layout as one PLAIN section consisting of multiple paragraph(s).
>> >
>> > In other other words: This insertion algorithm is more lenient than
>> > the parser, but this is on purpose for flexibility mid-stream as we
>> > convert QAPI to using explicit introductory sections. The allowed
>> > order of sections will eventually become strictly enforced in the
>> > parser, which will in turn allow dramatic simplifications to the
>> > insertion algorithm. This only exists as transitory code until we are
>> > able to enforce that order.
>> >
>> > Fear not: the intermediate rest output before and after this patch
>>
>> "ReST output"?
>
> Sure
>
>>
>> > are byte identical, so failing all else, we at least know it doesn't
>> > make anything worse.
>> >
>> > Lastly, because we have three places in the code that need to insert
>> > stub/dummy sections, we take the opportunity to consolidate this code
>> > to handle all three cases with one function. This winds up
>> > necessitating the qapidoc.py generator actually modify the section
>> > list to insert a "dummy" member that acts as a placeholder for "The
>> > members of ..." text. While it looks like a code smell to modify the
>> > caller's argument, it is ultimately safe because the QAPI Schema
>> > object is re-parsed and re-constructed in memory for each individual
>> > process that needs to operate on it. In other words, the Sphinx
>> > document generator already does have "its own copy" of the section
>> > lists, so it is "safe" to modify here without regards to other
>> > consumers of the QAPIDoc objects. It only *looks* like it smells
>> > bad. Ultimately, this code will also be removed once the inliner is
>> > merged, so it is only a temporary aesthetic issue regardless.
>>
>> Such trickery, even when safe, risks making attentive readers go
>> "WAT?!?"  I find it tolerable only because we plan to replace it fairly
>> soon.
>>
>> The code finding the spot to insert stub/dummy sections is more
>> complicated than it has any right to be, both before and after your
>> patch.  It reconstructs information the doc parser has, but doesn't pass
>> on in usable form.  Passing that info on would likely be simpler and
>> cleaner.  However, you tell me your inliner patches also simplify
>> things, and they already exist.  So let's go with those.
>
> Right, the basic idea is this:
>
> - Differentiate INTRO with new syntax
> - Convert "PLAIN" into "DETAILS"
> - Enforce strict section ordering: INTRO only at the beginning,
> DETAILS only at or quite near to the end
>
> Once that is cemented, the insertion algorithm can become much simpler
> and dumber; e.g. using a map of regions where we can append to the end
> of a region without having to iterate through to find our sweet spot.
>
> Since that's the plan for the inliner anyway, some temporary ugliness
> here in order to ensure that we do not regress even one byte seems
> acceptable.
>
> Additionally, once the inliner is merged, we won't need the "dummy"
> sections at all, so those go away entirely, and all of the ugliness of
> that particular part of the hack with it - i.e. no more modifying the
> caller's argument to insert a bookmark.
>
> So, it's a little gross, but it's just a band-aid to get to where we
> want to be; and it isn't as gross as it looks.
>
> With your suggested commentary changes, do I have your blessing to
> push forward? In the interim, please review the rest of the series so
> I can send you a new version.

Yes, you do.

In addition to the commit message changes, I'd like to have a comment in
the code.

Perhaps as separate patch before your series:

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index c3cf33904e..844afc6e0e 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -852,6 +852,12 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
 
     def ensure_returns(self, info: QAPISourceInfo) -> None:
 
+        # This is more complicated than it ought to be.  The doc
+        # parser should already know where a generated RETURNS section
+        # should go.  It currently doesn't, mostly because it accepts
+        # tagged sections in any order.
+        # TODO Tighten doc syntax and simplify.
+
         def _insert_near_kind(
             kind: QAPIDoc.Kind,
             new_sect: QAPIDoc.Section,
-- 
2.54.0

We could adjust the comment within your series to also cover the new
uses of _insert_near_kind(), but I don't think that's necessary.  It
should serve as a reminder as is.

If that works for you, I'll post the patch.

>
> --js
>
>>
>> > That's my story and I'm sticking to it.
>> >
>> > Signed-off-by: John Snow <[email protected]>
>>