Instead of using the info object for the doc block as a whole (which
always points to the very first line of the block), update the info
pointer for each call to ensure_untagged_section when the existing
section is otherwise empty. This way, Sphinx error information will
match precisely to where the text actually starts.

For example, this patch will move the info pointer for the "Hello!"
untagged section ...

> ##       <-- from here ...
> # Hello! <-- ... to here.
> ##

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index adc85b5b394..36cb64a677a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -687,7 +687,11 @@ def end(self) -> None:
     def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
         if self.all_sections and not self.all_sections[-1].tag:
             # extend current section
-            self.all_sections[-1].text += '\n'
+            section = self.all_sections[-1]
+            if not section.text:
+                # Section is empty so far; update info to start *here*.
+                section.info = info
+            section.text += '\n'
             return
         # start new section
         section = self.Section(info)
-- 
2.48.1

John Snow <jsnow@redhat.com> writes:

> Instead of using the info object for the doc block as a whole (which
> always points to the very first line of the block), update the info
> pointer for each call to ensure_untagged_section when the existing
> section is otherwise empty. This way, Sphinx error information will
> match precisely to where the text actually starts.
>
> For example, this patch will move the info pointer for the "Hello!"
> untagged section ...
>
>> ##       <-- from here ...
>> # Hello! <-- ... to here.
>> ##
>
> Signed-off-by: John Snow <jsnow@redhat.com>

This patch would be easier to accept with a test case where it improves
the error location.  I tried to construct one quickly, but failed.  Can
you help?

Possible substitute: point to a patch later in this series where things
become worse without this patch.

On Wed, Mar 5, 2025 at 5:10 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Instead of using the info object for the doc block as a whole (which
> > always points to the very first line of the block), update the info
> > pointer for each call to ensure_untagged_section when the existing
> > section is otherwise empty. This way, Sphinx error information will
> > match precisely to where the text actually starts.
> >
> > For example, this patch will move the info pointer for the "Hello!"
> > untagged section ...
> >
> >> ##       <-- from here ...
> >> # Hello! <-- ... to here.
> >> ##
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> This patch would be easier to accept with a test case where it improves
> the error location.  I tried to construct one quickly, but failed.  Can
> you help?
>
> Possible substitute: point to a patch later in this series where things
> become worse without this patch.


Maybe we can use the "if build_docs" section of the qapi schema testing to
run things through Sphinx and harvest the error messages for negative
cases...? I gotta sit down and figure out how.

In the meantime, if I unapply my series, then edit block-core to look like
this:

##
# @SnapshotInfo:
#
# rST syntax error: *ahh!
#

(Lines 13-17, error is on line 16)

Building, I get this error:

/home/jsnow/src/qemu/docs/../qapi/block-core.json:14: WARNING: Inline
emphasis start-string without end-string. [docutils]
/home/jsnow/src/qemu/docs/../storage-daemon/qapi/../../qapi/block-core.json:14:
WARNING: Inline emphasis start-string without end-string. [docutils]

Mmm, nope. Not quite.

If I re-push my series and try again with the same edit ...

/home/jsnow/src/qemu/docs/../qapi/block-core.json:14: WARNING: Inline
emphasis start-string without end-string. [docutils]
/home/jsnow/src/qemu/docs/../storage-daemon/qapi/../../qapi/block-core.json:14:
WARNING: Inline emphasis start-string without end-string. [docutils]
/home/jsnow/src/qemu/docs/../qapi/block-core.json:16: WARNING: Inline
emphasis start-string without end-string. [docutils]

The two inclusions from the old qapidoc are still wrong, but the inclusion
through the new transmogrifier is correct.

(I'm going to be honest with you, I don't know why the error location
didn't change at all for the old qapidoc. I think one of the many error
location bugs I fixed when writing the new transmogrifier that just never
got applied to the old system...)

If I undo this fix but keep the rest of my series, I get these errors:

/home/jsnow/src/qemu/docs/../qapi/block-core.json:14: WARNING: Inline
emphasis start-string without end-string. [docutils]
/home/jsnow/src/qemu/docs/../storage-daemon/qapi/../../qapi/block-core.json:14:
WARNING: Inline emphasis start-string without end-string. [docutils]
/home/jsnow/src/qemu/docs/../qapi/block-core.json:13: WARNING: Inline
emphasis start-string without end-string. [docutils]

Two are from the old qapidoc, one is from the new one. They're all wrong.

--js

John Snow <jsnow@redhat.com> writes:

> On Wed, Mar 5, 2025 at 5:10 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Instead of using the info object for the doc block as a whole (which
>> > always points to the very first line of the block), update the info
>> > pointer for each call to ensure_untagged_section when the existing
>> > section is otherwise empty. This way, Sphinx error information will
>> > match precisely to where the text actually starts.
>> >
>> > For example, this patch will move the info pointer for the "Hello!"
>> > untagged section ...
>> >
>> >> ##       <-- from here ...
>> >> # Hello! <-- ... to here.
>> >> ##
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> This patch would be easier to accept with a test case where it improves
>> the error location.  I tried to construct one quickly, but failed.  Can
>> you help?
>>
>> Possible substitute: point to a patch later in this series where things
>> become worse without this patch.
>
>
> Maybe we can use the "if build_docs" section of the qapi schema testing to
> run things through Sphinx and harvest the error messages for negative
> cases...? I gotta sit down and figure out how.

Rule of thumb for compilers: every error needs a negative test.  So,
test coverage for transmogrifier errors is certainly wanted, but it's
not a blocker for an initial merge.  We need to make progress, and to
make progress, we need to limit the size of the steps.

> In the meantime, if I unapply my series, then edit block-core to look like
> this:
>
> ##
> # @SnapshotInfo:
> #
> # rST syntax error: *ahh!
> #
>
> (Lines 13-17, error is on line 16)
>
> Building, I get this error:
>
> /home/jsnow/src/qemu/docs/../qapi/block-core.json:14: WARNING: Inline
> emphasis start-string without end-string. [docutils]
> /home/jsnow/src/qemu/docs/../storage-daemon/qapi/../../qapi/block-core.json:14:
> WARNING: Inline emphasis start-string without end-string. [docutils]
>
> Mmm, nope. Not quite.
>
> If I re-push my series and try again with the same edit ...
>
> /home/jsnow/src/qemu/docs/../qapi/block-core.json:14: WARNING: Inline
> emphasis start-string without end-string. [docutils]
> /home/jsnow/src/qemu/docs/../storage-daemon/qapi/../../qapi/block-core.json:14:
> WARNING: Inline emphasis start-string without end-string. [docutils]
> /home/jsnow/src/qemu/docs/../qapi/block-core.json:16: WARNING: Inline
> emphasis start-string without end-string. [docutils]
>
> The two inclusions from the old qapidoc are still wrong, but the inclusion
> through the new transmogrifier is correct.

Reproduced.  To get non-first warnings, you have to configure
--disable-werror.  Observation, not a complaint.

> (I'm going to be honest with you, I don't know why the error location
> didn't change at all for the old qapidoc. I think one of the many error
> location bugs I fixed when writing the new transmogrifier that just never
> got applied to the old system...)

I'm definitely not asking you to find out more :)

> If I undo this fix but keep the rest of my series, I get these errors:
>
> /home/jsnow/src/qemu/docs/../qapi/block-core.json:14: WARNING: Inline
> emphasis start-string without end-string. [docutils]
> /home/jsnow/src/qemu/docs/../storage-daemon/qapi/../../qapi/block-core.json:14:
> WARNING: Inline emphasis start-string without end-string. [docutils]
> /home/jsnow/src/qemu/docs/../qapi/block-core.json:13: WARNING: Inline
> emphasis start-string without end-string. [docutils]
>
> Two are from the old qapidoc, one is from the new one. They're all wrong.

Also reproduced.  Thanks!

Perhaps the patch could be moved closer to where it's needed, or even be
squashed into one the first one that depends it.  Bah, good enough as
is.

Here's my attempt to improve the commit message:

    qapi/parser: adjust info location for doc body section

    Instead of using the info object for the doc block as a whole (which
    always points to the very first line of the block), update the info
    pointer for each call to ensure_untagged_section when the existing
    section is otherwise empty. This way, Sphinx error information will
    match precisely to where the text actually starts.

    For example, this patch will move the info pointer for the "Hello!"
    untagged section ...

        ##       <-- from here ...
        # Hello! <-- ... to here.
        ##

    This doesn't seem to improve error reporting now.  It will with the
    QAPI doc transmogrifier I'm about to add.

    If I stick bad rST into qapi/block-core.json like this

         ##
         # @SnapshotInfo:
         #
        +# rST syntax error: *ahh!
        +#
         # @id: unique snapshot id
         #
         # @name: user chosen name

    the existing code's error message will point to the beginning of the
    doc comment, which is less than helpful.  The transmogrifier's
    message will point to the erroneous line, but to accomplish this, it
    needs this patch.

What do you think?