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

[John Snow <jsnow@redhat.com>]

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

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

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

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

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


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

> What is inlining?
>

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

Yes, this is all correct so far.


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

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

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

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

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

It's only in this case that it whines:

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

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

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


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


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

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